I've done this intentionally a few times. It's usually in code that wants to execute several statements but report the first error:

err := foo.X()
if e := foo.Close(); err == nil {
	err = e
}
return err

(Not saying this is definitely the best way to write this code, but the fact that the pattern exists intentionally out in the wild probably means that this is a no-go for a vet check.)

@cespare If someone can come up with a sane use case for this, then that's why I suggested golint as well. But I've not found any. Either way, a warning should come out somewhere!

The specific snippet above already produces a compiler error (e defined and not used). As others pointed out, there are reasonable code snippets that would be caught by any attempt to catch this.

@rsc Not true... Here's a longer example where the compiler doesn't catch this:

var err error
if err != nil {
        printf("blah")
}

if e := foo.Init(); err != nil {
        return errwrap.Wrapf(e, "some error")
}

Please re-open, thanks.

What @rsc meant was: any attempt to report this error in vet is likely to cause false positives for correct code.

@ianlancetaylor Can you provide one scenario where that sort of thing happens or run your fancy scripts on the stdlib and see if that's the case?

If so, please re-open and move to golint, otherwise re-open and leave here.

Thanks

@purpleidea See the comment by @cespare , above.

@ianlancetaylor if you believe this is the case:

please re-open and move to golint

I know you folks probably have a lot of issues to wade through, but closing issues quickly without too much discussion and consensus is not awesome for a community.

please re-open and move to golint

golint has its own repository with its own issue tracker, and GitHub has no notion of moving issues across repositories. You're free to file the issue there yourself, but it will probably be similarly rejected there. Golint is primarily concerned with style, not correctness. And there is nothing inherently wrong with the code, as demonstrated by @cespare.

but closing issues quickly without too much discussion and consensus

There is consensus among the people responsible for vet, in that the proposed check would have false positives in valid code, and is therefore not suitable. This issue tracker isn't concerned with golint, which is maintained by other people, so any golint-specific discussion should be had on golint's issue tracker, see my note above regarding filing an issue there.

@dominikh

1. Didn't realize golint was elsewhere

2. Since so much golang code consists of if err != ???, you'd think this common bug would be caught or warned about in some tool. I guess not!

AFAICT, this is literally one of the ways to get software in golang to "accidentally" miss a critical safety check, and as a result, I would have thought more folks would be interested in preventing this sort of potential security bug.

Thanks

One pattern that could be caught is the following:

if err != nil {
  return ...
}

if e := fn(); err != nil {
  ...
}

Because the second condition is known to be always false. It may still be out of scope for vet due to the increased complexity of that check (as it requires tracking path conditions and assignment to variables), but if it is of any consolation, a check like that will eventually make it into staticcheck.

@purpleidea I'm sorry if it seems like we are closing issues too quickly. These issues are not new to us. We've seen and thought about them before. One of vet's guiding rules, as listed in cmd/vet/README, is that any tests must be precise. Any change in vet comes to close to being a language change. The question is not "do we want to warn about e := fn(); err != nil"; the question is whether we want to prohibit that construct. There is clearly no consensus about that.

I'm less familiar with golint.