go: proposal: cmd/vet: add check for common error mishandling pattern

People have proposed adding a test to vet to catch certain unused errors. This can be very noisy, and there are many cases where the error is genuinely known to be nil and can be ignored, such as in a call to bufio.Writer.Write. Therefore a general “unused error” check will be too noisy and fail the precision criterion described in cmd/vet/README.

However, my project has been badly bitten twice recently by a particular problem of unused errors that should be caught. The two looked schematically like these snippets:

func e() error { ... }
var err error

# Error value not caught in if init:
if e(); err != nil {...}

# Error value not caught before if:
e()
if err != nil { ... }

I did some experiments to catch calls to functions with the type of e that did not catch the error, and found too may false positives for things like os.Setenv.

However, since the real problem isn’t that the error is discarded, but that it is discarded immediately before a check, I think it’s worth trying a more targeted approach, one that would catch both of the problems in the snippets above.

The -unusedresult flag, on by default, already checks for things like calls to fmt.Sprintf where the value is not used, which is clearly a bug - there is no other reason to call fmt.Sprintf. That check works with a whitelist, but the problems in the examples above involved local functions that would never appear on a whitelist unless explicitly added when vet is called, which is error-prone (and not done in general).

I therefore propose to add to the -unusedresult check a test for the specific case with these three properties:

a call to a function that returns an error as one of its possibly several arguments;
return values from the function are dropped (no explicit _);
the next statement reads a variable of type error.

It’s point 3 that makes this work, I think, eliminating almost all the noise from calls to things like os.Setenv while catching the real bugs. If you just called an error function and then ask about the error, if there’s no flow from the call to the check, you almost certainly made a mistake.

Regarding the criteria:

Correctness: This is a real bug, and it’s bitten our project twice with severe security problems as a result. This is a problem that needs fixing.

Frequency: It’s hit our project twice, and our project isn’t big. I believe the severity is high enough that even a modest frequency of hits justifies the fix. If I implement this, I predict it will find more.

Precision: I believe that rule 3 in this proposal should make it precise enough.

About this issue

Original URL
State: open
Created 7 years ago
Reactions: 23
Comments: 16 (11 by maintainers)

Most upvoted comments

One downside to introducing this into go get, is that while it would introduce less noise than if go vet mimicked errcheck, it would require us to write code that is not consistent. In some cases we may need to explicitly discard an error, in others we won’t. Inconsistencies degrade readability because each inconsistency adds noise or calls attention to something that’s different for a real reason. In this case the inconsistency would not aid the reader and may raise questions for the reader.

Example: If we need to call a function that returns an unimportant error, in between calling a function with an error and handling that error, we’ll be required to insert _ = to signal an intent to discard the error.

fmt.Println("Welcome") // not required to signal intent to discard
n, err := doThing()
_ = metrics.Track("action", n, err) // required to signal intent to discard
if err != nil {
	handleError(err)
	return
}

But this is one downside and it seems like the benefits would out way it.

leighmcculloch on Nov 19, 2017