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

Comments

Projects
None yet
4 participants
@pja
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

@gopherbot gopherbot added the Proposal label Mar 27, 2017

@rsc rsc changed the title from Proposal: cmd/vet: missing error assignment check to proposal: cmd/vet: missing error assignment check Mar 27, 2017

@rsc

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

acme: add missing err assignment check
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

This comment has been minimized.

Contributor

ianlancetaylor commented Apr 28, 2017

See #20148.

@rsc

This comment has been minimized.

Contributor

rsc commented Jun 12, 2017

On hold for #20148.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment