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

cmd/vet: error when returning `nil` inside an `if err != nil` clause #24141

Closed
cgrushko opened this issue Feb 26, 2018 · 7 comments
Closed

cmd/vet: error when returning `nil` inside an `if err != nil` clause #24141

cgrushko opened this issue Feb 26, 2018 · 7 comments

Comments

@cgrushko
Copy link

@cgrushko cgrushko commented Feb 26, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

version 1.10

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

amd64, linux

What did you do?

I mistakenly return nil inside a if err != nil clause, which hid a bug in my code. Since it almost never makes sense to return nil in such a case, I thought it'd be a nice go vet error.

func f() error {
err := ....
if err != nil {
return nil
}
return nil
}

@cgrushko cgrushko changed the title cmd/vet: return `err` inside an `if err != nil` clause cmd/vet: error when returning `err` inside an `if err != nil` clause Feb 26, 2018
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Feb 27, 2018

This seems like it will bring quite a few false positives. Have you prototyped such a change, and seen what it finds in the Go tree?

@mvdan mvdan added the Suggested label Feb 27, 2018
@dgryski

This comment has been minimized.

Copy link
Contributor

@dgryski dgryski commented Feb 27, 2018

Seems like the title should be "returning nil inside if err != nil" instead of "returning err inside if err != nil".

@adamdecaf

This comment has been minimized.

Copy link
Contributor

@adamdecaf adamdecaf commented Feb 27, 2018

Here's a valid return nil from the stdlib. I agree this will bring a lot of false positives.

https://github.com/golang/go/blob/release-branch.go1.10/src/crypto/x509/root_darwin.go#L213-L225

@cgrushko

This comment has been minimized.

Copy link
Author

@cgrushko cgrushko commented Feb 27, 2018

This example is not what I suggest catching, because the if clause contains things other than return nil.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Feb 27, 2018

Other reasonable scenarios come to mind, for example:

  • Getting io.EOF but returning nil, because it was expected
  • Funcs whose return type isn't error (perhaps obvious, but this issue does not clarify that)
  • Expecting an operation to fail, thus returning a nil error when an obtained error is non-nil

And there's also the can of worms that is errors that are always nil. This kind of check probably belongs in a tool that gives a human suggestions, and not a linter that gives warnings that must be accurate.

@titanous titanous changed the title cmd/vet: error when returning `err` inside an `if err != nil` clause cmd/vet: error when returning `nil` inside an `if err != nil` clause Feb 27, 2018
@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Mar 13, 2018

cc @robpike for a final decision, but I think the likely false positives from this make it not a good fit for vet.

cc also @dominikh in general for all things that don’t make it into vet. :)

@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Mar 13, 2018

Fails the precision criterion of cmd/vet/README.

@robpike robpike closed this Mar 13, 2018
@golang golang locked and limited conversation to collaborators Mar 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.