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

proposal: cmd/vet: check for unused return value from testing/quick.Check #12834

Closed
minux opened this issue Oct 4, 2015 · 8 comments
Closed

Comments

@minux
Copy link
Member

minux commented Oct 4, 2015

https://golang.org/cl/15263 fixed a place where quick.Check is
used without its return value being checked for non-nil.

I did a quick search, and found that unfortunately, there are 6 more
similar cases in standard library alone. (see https://golang.org/cl/15381)

I'm wondering if cmd/vet should check for this case as not checking
the result from quick.Check means the automated checks are useless.

And perhaps cmd/vet should also check for other functions whose
returned value must not be ignored.

I imagine that we don't want a source code annotation is similar to
gcc's __attribute__((warn_unused_result))?
(How about project specific cmd/vet rules? If there are enough
interest, we might be able to start a discussion on this topic too.
I'm thinking about an example based approach, aka eg.)

@minux minux added the Proposal label Oct 4, 2015
@gopherbot
Copy link

CL https://golang.org/cl/15381 mentions this issue.

@cespare
Copy link
Contributor

cespare commented Oct 4, 2015

Just to note prior art, the functionality you're proposing sounds similar to github.com/kisielk/errcheck.

@minux
Copy link
Member Author

minux commented Oct 4, 2015 via email

@minux
Copy link
Member Author

minux commented Oct 4, 2015 via email

@adg
Copy link
Contributor

adg commented Oct 6, 2015

Couldn't this proposal take that into account?

On 5 October 2015 at 10:58, Minux Ma notifications@github.com wrote:

One complication is that not checking the return value
from quick.Check is ok if the func value actually calls
t.Error/t.Fatal on failure. I guess the value of this proposal
is greatly reduced by this.


Reply to this email directly or view it on GitHub
#12834 (comment).

minux added a commit that referenced this issue Oct 6, 2015
Update #12834.

Change-Id: If7bbcc249517f2f2d8a7dcbba6411ede92331abe
Reviewed-on: https://go-review.googlesource.com/15381
Reviewed-by: Damian Gryski <dgryski@gmail.com>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@adg
Copy link
Contributor

adg commented Oct 21, 2015

Is there still interest in this? Seems low impact to me.

@rsc rsc added this to the Proposal milestone Oct 24, 2015
@rsc
Copy link
Contributor

rsc commented Oct 24, 2015

No thanks.

One of the criteria for vet is that the check being added must be for something that happens often, so that the benefit pays for the maintenance cost. Approximately no one uses quick.Check (we probably should have killed it before Go 1). There isn't enough benefit here.

@rsc
Copy link
Contributor

rsc commented Oct 24, 2015

/cc @robpike

@rsc rsc closed this as completed Oct 30, 2015
@golang golang locked and limited conversation to collaborators Nov 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants