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: check for logically incorrect return based on scope and error #21848

Closed
brenol opened this issue Sep 12, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@brenol
Copy link

commented Sep 12, 2017

Please answer these questions before submitting your issue. Thanks!

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

1.9 linux/amd 64

Does this issue reproduce with the latest release?

yes, using go vet -all main.go

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

linux/amd64

What did you do?

Ran the following program:
https://play.golang.org/p/Hu3qX6aSr-

What did you expect to see?

I (think) vet should warn about that return err in r.StatusCode != r.StatusOK as it will return nil.

What I actually think should happen is that vet perhaps could be able to see if the error is being checked before and check if it's being returned after, where in the happy code path it will surely be nil.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2017

The playground link doesn't work, and I'm not sure what you are suggesting.

@martisch

This comment has been minimized.

Copy link
Member

commented Sep 12, 2017

Github does not add the _ at the end of the play link to the actual link:
https://play.golang.org/p/4R-IQsF3p_

@brenol

This comment has been minimized.

Copy link
Author

commented Sep 12, 2017

Sorry, terrible playground I created over there. Did a correct one over here:
https://play.golang.org/p/Hu3qX6aSr-

Basically I am returning a nil error that was already checked and it's known that it's nil. I think vet will be able to catch this but I am not sure.

Thanks

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2017

Thanks. I think that you are asking for vet to look for cases where we return a value of type error but where vet can prove that the value is always nil. For vet tests we look for three characteristics; see cmd/vet/README. This seems correct, and I imagine that we can make it precise, but how frequent is it?

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Sep 12, 2017

@brenol

This comment has been minimized.

Copy link
Author

commented Sep 12, 2017

Yes, exactly. I think it's rare as well but when it happens its pretty hard to see it in the middle of all that code.

Perhaps it could also be used for other types that aren't errors - making it way more useful but then it might have a performance penalty.

By the way, thank you for that cmd/vet/README, hadn't seen it before :)

Perhaps this can be closed as it doesn't match the "frequency" rule.

Thank you

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2017

What about:

val, err := ret()
if err != nil {
	panic("DEBUG")
}
return val, err

where the panic is just for debugging? Does vet now complain that err is statically nil in the return? It seems like a very specific check. So in addition to frequency above I'd raise some questions about precision. But it seems like the consensus is already that it's too special, so closing.

@rsc rsc closed this Oct 23, 2017

@golang golang locked and limited conversation to collaborators Oct 23, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.