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: missing error assignment check #19727

Open
pja opened this issue Mar 27, 2017 · 4 comments
Open

proposal: cmd/vet: missing error assignment check #19727

pja opened this issue Mar 27, 2017 · 4 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal Proposal-Hold
Milestone

Comments

@pja
Copy link
Contributor

pja commented Mar 27, 2017

I noticed a mistake during a code review that I think could be automatically detected. The idea is to detect code which looks like:

if f(); err != nil {
    ...
}

which should instead assign the return value of f()

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

For the code to compile at all, err must be declared in the outer scope.

Correctness:

This check detects code that the programmer didn't intend to write. In some instances the mistake is caused by what I personally might call a readability issue, such as when f() is a multi-line func literal that obscures the if statement. Nevertheless, this reports on the missing assignment, not the style.

Frequency:

A number of these have been detected at Google. The check is lightweight. I did not do a large review of open-source Go, but examples can be seen at:
https://github.com/golang/crypto/blob/459e26527287adbc2adcc5d0d49abff9a5f315a7/acme/acme.go#L157
https://github.com/tensorflow/tensorflow/blob/c44601f5411408c324c81dd38bc8686cbd2568aa/tensorflow/go/tensor.go#L101

Precision:

It is difficult to define a false negative for this test, but for my particular implementation perhaps errors that are not called "err", and are not checked against nil. Others implementations may be able to increase the scope.

False positives seem rare, though it is possible to write code like:

var err error
f := func() {
   mu.Lock()
   defer mu.Unlock()
   err = g()
}
if f(); err != nil {

I do not think it would any less clear for f() to return err in that case though.

A false positive of my initial implementation is

https://github.com/ziutek/mymysql/blob/0582bcf675f52c0c2045c027fd135bd726048f45/autorc/autorecon.go#L124

this could be eliminated by looking for err (or &err) being passed as a parameter, though that function could avoid modifying its input parameter and instead return an error.

A more thorough check for if conditions that are always true/false would detect some of these situations, especially if a previous check for nil caused a return. But it would miss some, so I think a simple check is worth introducing. Some mistakes occur in tests:

if err := f(); err != nil {
    t.Error(err)
}
if g(); err != nil {
    t.Error(err)
}

whilst others aggregate errors rather than returning:


for x := range xs {
    if f(x); err != nil {
        errs = append(errs, err)
    }
}

An initial implementation is here: https://go-review.googlesource.com/c/38631/

@gopherbot gopherbot added this to the Proposal milestone Mar 27, 2017
@rsc rsc changed the title Proposal: cmd/vet: missing error assignment check proposal: cmd/vet: missing error assignment check Mar 27, 2017
@rsc
Copy link
Contributor

rsc commented Apr 3, 2017

In this example:

if f(); err != nil {
    ...
}

there are two arguable problems:

  1. The err != nil test does not obviously depend on the setup statement f().
  2. The function f returns an error that is discarded by the call f() outside an assignment.

The discussion on this issue focuses on detecting (1), which is pretty tricky. An alternative would be to detect (2), namely code that throws errors away without making that clear (without using _ = f()). @robpike has been experimenting to see if this is possible to do with a low enough false positive rate.

@pja
Copy link
Contributor Author

pja commented Apr 10, 2017

It's definitely tricky to be certain, but I have seen very few instances where code like the example was intended. Perhaps a solution to (2) would find a superset of these problems, in which case is it worth adding a simpler check in the meantime?

gopherbot pushed a commit to golang/crypto that referenced this issue Apr 25, 2017
Spotted it thanks to a proposal in
golang/go#19727.

Change-Id: I389a3fc0db3cf64fba41c3ecd70a236917ea8fa3
Reviewed-on: https://go-review.googlesource.com/41698
Run-TryBot: Sam Whited <sam@samwhited.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Sam Whited <sam@samwhited.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@ianlancetaylor
Copy link
Contributor

See #20148.

@rsc
Copy link
Contributor

rsc commented Jun 12, 2017

On hold for #20148.

c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
Spotted it thanks to a proposal in
golang/go#19727.

Change-Id: I389a3fc0db3cf64fba41c3ecd70a236917ea8fa3
Reviewed-on: https://go-review.googlesource.com/41698
Run-TryBot: Sam Whited <sam@samwhited.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Sam Whited <sam@samwhited.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
Spotted it thanks to a proposal in
golang/go#19727.

Change-Id: I389a3fc0db3cf64fba41c3ecd70a236917ea8fa3
Reviewed-on: https://go-review.googlesource.com/41698
Run-TryBot: Sam Whited <sam@samwhited.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Sam Whited <sam@samwhited.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
Spotted it thanks to a proposal in
golang/go#19727.

Change-Id: I389a3fc0db3cf64fba41c3ecd70a236917ea8fa3
Reviewed-on: https://go-review.googlesource.com/41698
Run-TryBot: Sam Whited <sam@samwhited.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Sam Whited <sam@samwhited.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
Spotted it thanks to a proposal in
golang/go#19727.

Change-Id: I389a3fc0db3cf64fba41c3ecd70a236917ea8fa3
Reviewed-on: https://go-review.googlesource.com/41698
Run-TryBot: Sam Whited <sam@samwhited.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Sam Whited <sam@samwhited.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal Proposal-Hold
Projects
None yet
Development

No branches or pull requests

5 participants