False positive on implementing interface

[robinknaapen]

golangci-lint version
# golangci-lint has version 1.39.0 built from 9aea4aee on 2021-03-26T08:02:53Z

// .golangci.yml
linters:
  disable-all: true
  enable:
    - unparam

// lower implements encoding.TextUnmarshaler to force lowercase
type lower string

// unparam: (*lower).UnmarshalText - result 0 (error) is always nil
func (l *lower) UnmarshalText(b []byte) error {
    s := strings.ToLower(string(b))

    *l = lower(s)
    return nil
}

[mvdan]

Thanks for flagging; I think most of these don't show up in practice because the package implementing the interface imports the package defining the interface, so we know to skip those messages on e.g. UnmarshalText.

The easiest fix on your side is to add an "implements assertion", which ensures that you properly implement the interface where the type is declared, but also allows unparam to see the interface you're implementing when the package is built:

var _ encoding.TextUnmarshaler = (*lower)(nil)

It would be best if unparam didn't give a warning here anyway, but it's very hard to fix in general. We could hard-code some standard library interfaces into unparam to solve some of these false positives, but you'd still be able to run into the issue with third party libraries.

Any other ideas?

[robinknaapen]

I suppose a hard coded list would be nice... But probably unmaintainable in the sense of distinction between common interface implementations. Plus maybe overlapping interface definitions in third party libraries

A comment wouldn't be sustainable since in my opinion it forces the developer in a pattern

At this time I don't have a particular idea on how to solve this in a sensible/developer friendly way...

Maybe nolint might just be the answer.

[mvdan]

If we're going to be annotating the source code, I'd personally favour an "implements assertion" in the form of the Go code I showed above, instead of a comment. A comment can be prone to typos, and the assertion also serves the purpose of making the build fail if the interface implementation isn't right.

I still agree it's just a form of workaround, though.

[alexrudd]

Just hit this in a major way with some code that uses the command pattern. We have a bunch of request types that all implement an interface that includes an unexported method that could error. All the implementations that don't error triggered the linter:

type request interface {
  execute(*state) (response, error)
}

// Create implements request
type Create struct {
  Thing string
}

func (c *Create) execute(s *state) (response, error) {
  s.things = append(s.things, c.Thing)

  return nil, nil
}

I can either make all these types exported or add nolint comments to them all. Adding implement assertions for each of these types (there are hundreds) is not ideal.

Otherwise this linter did catch a bunch of actual bugs! :D