Add iface linter

[uudashr]

Add new linter "iface" to help discover and avoid interface pollution

Linter link: https://github.com/uudashr/iface

[ldez]

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

• The CLA must be signed

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description of the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter (the team will help to verify that).
• It must have a valid license (AGPL is not allowed), and the file must contain the required information by the license, ex: author, year, etc.
• It must use Go version <= 1.21
• The linter repository must have a CI and tests.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic().
• It must not contain log.Fatal(), os.Exit(), or similar.
• It must not modify the AST.
• It must not have false positives/negatives (the team will help to verify that).
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must have integration tests without configuration (default).
• They must have integration tests with configuration (if the linter has a configuration).

.golangci.next.reference.yml

• The file .golangci.next.reference.yml must be updated.
• The file .golangci.reference.yml must NOT be edited.
• The linter must be added to the lists of available linters (alphabetical case-insensitive order).
  • enable and disable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in the lintersdb/builder_linter.go and .golangci.next.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter uses goanalysis.LoadModeSyntax -> no WithLoadForGoAnalysis() in lintersdb/builder_linter.go
  • if the linter uses goanalysis.LoadModeTypesInfo, it requires WithLoadForGoAnalysis() in lintersdb/builder_linter.go
• The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.
• WithURL() must contain the URL of the repository.

Recommendations

• The file jsonschema/golangci.next.jsonschema.json should be updated.
• The file jsonschema/golangci.jsonschema.json must NOT be edited.
• The linter repository should have a readme and linting.
• The linter should be published as a binary (useful to diagnose bug origins).
• The linter repository should have a .gitignore (IDE files, binaries, OS files, etc. should not be committed)
• A tag should never be recreated.

---

The golangci-lint team will edit this comment to check the boxes before and during the review.

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

The reviews should be addressed as commits (no squash).

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

[ldez]

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

@uudashr please don't edit the message.

[ldez]

@uudashr stop editing the message or you will be kicked off the organization.

[uudashr]

Sorry, reverting the message

[ldez]

You edited the message once again and reverted all my changes to the message...

[ldez]

As long as the elements required by the checklist (except those related to maintainer actions) are not handled, the review will not start.

Don't edit the checklist!

[ldez]

• empty: Finds empty interfaces.

This could lead to false positive because an empty interface can be used on purpose.

• unused: Finds unused interfaces within the package.

This leads to false positive because you can provide interface shared by other packages to avoid duplication.

Note: it's not "the package" but "a package"

• duplicate: Finds duplicate interfaces within the package.

This leads to false positive with extended interfaces.

type a interface {
	// ...
}

type b interface {
	a
}

Note: it's not "the package" but "a package"

• opaque: Find the interfaces that is used to abstract a single concrete implementation only.

This leads to false positives because a library can provide an interface without implementation to extend the behavior.

[uudashr]

empty: Finds empty interfaces.

This could lead to false positive because an empty interface can be used on purpose.

Correct. Thanks for pointing this out.

I've seen some library use interface{}, this is allowed.
But still empty "interface{} says nothing", you need to have careful consideration to use it.

If we define type Payload interface {}, it still says nothing unless the name tells that it is Payload, but any value can be assumed as Payload.

We can use type Payload any as alternative which is allowed if still needed. Or we can make the empty linter works only for Go1.18 or above?

Any suggestion?

The linter still able to help us raising awareness so we can make an extra consideration when using empty interface.

---

unused: Finds unused interfaces within the package.

This leads to false positive because you can provide interface shared by other packages to avoid duplication.

I'm taking below statements as consideration:

1. "Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values. The implementing package should return concrete (usually pointer or struct) types: that way, new methods can be added to implementations without requiring extensive refactoring." Go Code Review Comments, go.dev
2. "Don’t export any interfaces until you have to." Interface pollution in Go, rakyll.org

I would love to have Logger interface in built-in Go library, but they didn't take this direction, It might because of above statements.

---

duplicate: Finds duplicate interfaces within the package.

This leads to false positive with extended interfaces.

type a interface {
	// ...
}

type b interface {
	a
}

Why you want to do that?

Technically Go allow us to pollute our code with interfaces, but again the questions is why you want to do that?

If we have very large package that host many concepts, having duplicate package is possible. But then you might want to consider breakdown the package into more specific concept, the duplication is less likely.

---

opaque: Find the interfaces that is used to abstract a single concrete implementation only.

This leads to false positives because a library can provide an interface without implementation to extend the behavior.

This is related to function that hides the concrete value, for example

type Server interface {
    Server() error
}
func NewServer(addr string) Server { // will report warning
    return &serverImpl{addr: addr}
}

So we suggest to just return the concrete exported struct.

But below code is valid, no warning will be reported. It has possibility to return more than one implementation.

type Server interface {
    Server() error
}


func NewServer(addr string) Server { // will not report warning
  if strings.HasPrefix(addr, "secure:") {
    return &secureServer{addr: addr}
  }

  return &server{addr: addr}
}

[ldez]

My remarks were based on real projects with legitimate usages.

The theory is great but it should not be turned into a dogma 😉

---

About empty:

For me, this rule is the only one in which false positives are niche.

About unused:

When you are designing a complex library, providing an interface in a separate package can be required for several reasons:

• avoiding cyclic imports
• have a clear public API
• etc.

log has no interface, but slog has one because it's a requirement to use a logger.
Not a lot of projects use the log because, in the real world, you need to interact with other loggers so you need an interface.

About duplicate:

Why do you want to do that?

Because of readability: complex things require to be named to be easy to maintain.

About opaque:

I will repeat but a library can provide an interface without implementation to extend the behavior.
A, not so real, example, but I think it will be clearer: if you are building a SQL-neutral library then you don't want to have real implementations of the SQL part.

[uudashr]

About empty:

For me, this rule is the only one in which false positives are niche.

This looks valid. Made me think to take out this one. Thanks man. 👍

Not many people will use empty interface unless they might really need it. If this linter really insist to be exist, will not much useful either.

About unused:

• avoiding cyclic imports
• have a clear public API
• etc.

If you have reference, a project or real example on this, I would love to continue my research on what should we do to avoid interface pollution.

log has no interface, but slog has one because it's a requirement to use a logger.
Yeah... I didn't use log either.

FYI the linter won't yield warning to slog.

About duplicate:

Because of readability: complex things require to be named to be easy to maintain.

Been thinking of this before to justify whether the linter is not necessary to be exists. But hardly find real example on this.

If you have some, please drop me a the repo link.

About opaque:

I will repeat but a library can provide an interface without implementation to extend the behavior.

The linter would not yield warning due to this.

A, not so real, example, but I think it will be clearer: if you are building a SQL-neutral library then you don't want to have real implementations of the SQL part.

I'm not sure this is relate to opaque analyzer. I would appreciate if we have this SQL-neutral library for me to learn more about it.

Well I might need to rephrase the description of the analyzer, here it is: "identifies functions returning interfaces when the actual return value is always a single concrete implementation".

It is related to "factory function" mentioned Avoid Interface Pollution

[ldez]

I updated your PR to ease the detection of false-positives.

[ldez]

If you want an example of a lib with an interface but no implementation (except for tests): https://github.com/kvtools/valkeyrie/blob/017bb9979d23d57c439c6f24b724ed5625c40f1e/store/store.go#L12

An example of false-positive with unused:
https://github.com/go-acme/lego/blob/04864ff13b01445c65cee924c790f3ef4d60a37f/challenge/provider.go#L25

You can take a look of the "Details" section here: ldez/golangci-lint-bench#14 (comment)

[ldez]

You have reverted all my changes and did a bad rebase.

I fixed what I could but please avoid that.

[uudashr]

You have reverted all my changes and did a bad rebase.

Sorry of that, my bad.

I fixed what I could but please avoid that.

Thanks for the fix. Sure do

I need to catch up with huge master diff, I do confuse myself having this big rebase.
If you need fresh PR or anything else.. please let me know.

I updated your PR to ease the detection of false-positives.

Thanks for this, I use this and made some changes with same goal.

[ldez]

I need to catch up with huge master diff, I do confuse myself having this big rebase.

The problem was not a "huge master diff/big rebase" but the fact that you didn't pull my changes before modifying the code locally despite my message notifying you about this change.

I fixed the rebase but my previous code has been deleted.

[ldez]

I fixed:

• configuration management
• JSONSchema
• tests
• implementation
• etc.

But this is only to illustrate my opinion with real world examples.

My opinion is still the same:

• unused -> more than 90 % of false positives
• opaque -> at least 80 % of false positives
• identical (previously duplicate) -> ~20 % of false positives and non-clear report messages.

For real-world examples, see the "Details" section here.

[uudashr]

thanks, let me check for those each projects

[ldez]

You will not be able to fix something because it's related to the nature of your linters.

The best and only way is to change the default to disable unused and opaque.

[uudashr]

I see. Let me change those

[ldez]

LGTM