Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive on implementing interface #52

Open
robinknaapen opened this issue May 27, 2021 · 4 comments
Open

False positive on implementing interface #52

robinknaapen opened this issue May 27, 2021 · 4 comments

Comments

@robinknaapen
Copy link

robinknaapen commented May 27, 2021

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
Copy link
Owner

mvdan commented Dec 9, 2021

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
Copy link
Author

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
Copy link
Owner

mvdan commented Dec 9, 2021

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.

atc0005 added a commit to atc0005/check-statuspage that referenced this issue Apr 29, 2022
Fail the build if the json.Unmarshaler implementation isn't correct.

This resolves the unparam linter error:
(*NullString).UnmarshalJSON - result 0 (error) is always nil (unparam)

refs mvdan/unparam#52
atc0005 added a commit to atc0005/check-statuspage that referenced this issue Apr 29, 2022
Fail the build if the json.Unmarshaler implementation isn't correct.

This resolves the unparam linter error:
(*NullString).UnmarshalJSON - result 0 (error) is always nil (unparam)

refs mvdan/unparam#52
@alexrudd
Copy link

alexrudd commented Jul 19, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants