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: add check for common error mishandling pattern #20148

Open
robpike opened this Issue Apr 27, 2017 · 10 comments

Comments

Projects
None yet
@robpike
Contributor

robpike commented Apr 27, 2017

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:

  1. a call to a function that returns an error as one of its possibly several arguments;
  2. return values from the function are dropped (no explicit _);
  3. 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.

@gopherbot gopherbot added this to the Proposal milestone Apr 27, 2017

@gopherbot gopherbot added the Proposal label Apr 27, 2017

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Apr 27, 2017

Contributor

This seems worthwhile doing. I think the proof is in the pudding - let's implement it and see if it can be made to work as advertised.

Contributor

griesemer commented Apr 27, 2017

This seems worthwhile doing. I think the proof is in the pudding - let's implement it and see if it can be made to work as advertised.

@dlsniper

This comment has been minimized.

Show comment
Hide comment
@dlsniper

dlsniper Apr 27, 2017

Contributor

Hi,

I'm not sure about the full context of those issues, but both these tools would catch the error, with this code https://play.golang.org/p/pkaWAqDUY0 as an example:

Both of them are included in this other tool: https://github.com/alecthomas/gometalinter

I wonder if rather than having the effort duplicate, more gophers should be made aware by these tools via the https://golang.org website by adding a (third-party) tooling section (cc @rakyll). Or do you think having this check in the vet tool would bring any advantage over the existing tools in the ecosystem?

Thank you.

Contributor

dlsniper commented Apr 27, 2017

Hi,

I'm not sure about the full context of those issues, but both these tools would catch the error, with this code https://play.golang.org/p/pkaWAqDUY0 as an example:

Both of them are included in this other tool: https://github.com/alecthomas/gometalinter

I wonder if rather than having the effort duplicate, more gophers should be made aware by these tools via the https://golang.org website by adding a (third-party) tooling section (cc @rakyll). Or do you think having this check in the vet tool would bring any advantage over the existing tools in the ecosystem?

Thank you.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Apr 28, 2017

Contributor

This is a refinement of #19727.

Contributor

ianlancetaylor commented Apr 28, 2017

This is a refinement of #19727.

@dominikh

This comment has been minimized.

Show comment
Hide comment
@dominikh

dominikh Apr 28, 2017

Member

It's worth pointing out that neither staticcheck nor errcheck fulfill Rob's requirements. Staticcheck doesn't actually catch this problem at all (yet), its output for https://play.golang.org/p/pkaWAqDUY0 is complaining about something unrelated. Errcheck does catch the issue – being the canonical tool for finding unchecked errors – but it's also a tool that's too noisy for a lot of people, see Rob's original message that mentions bufio.Writer.Write.

Member

dominikh commented Apr 28, 2017

It's worth pointing out that neither staticcheck nor errcheck fulfill Rob's requirements. Staticcheck doesn't actually catch this problem at all (yet), its output for https://play.golang.org/p/pkaWAqDUY0 is complaining about something unrelated. Errcheck does catch the issue – being the canonical tool for finding unchecked errors – but it's also a tool that's too noisy for a lot of people, see Rob's original message that mentions bufio.Writer.Write.

@kevinburke

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke Apr 29, 2017

Contributor

On a slightly related note: I sometimes try to make it impossible to have this problem by trying to never reuse an error; specifically to never assign to an error value with =.

Sometimes I get caught up though with code like this:

val, err := doSomething()
if err != nil {
    return err
}

val2, err2 := doSomethingElse()
if err2 != nil {
    return err
}

And then get confused why I'm returning nil from the function.

Contributor

kevinburke commented Apr 29, 2017

On a slightly related note: I sometimes try to make it impossible to have this problem by trying to never reuse an error; specifically to never assign to an error value with =.

Sometimes I get caught up though with code like this:

val, err := doSomething()
if err != nil {
    return err
}

val2, err2 := doSomethingElse()
if err2 != nil {
    return err
}

And then get confused why I'm returning nil from the function.

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Apr 29, 2017

Contributor

I sometimes try to make it impossible to have this problem by trying to never reuse an error; specifically to never assign to an error value with =.

I would go the opposite way. It's the := which causes the trouble. (While being undeniably handy in almost every other situation.)

Contributor

cznic commented Apr 29, 2017

I sometimes try to make it impossible to have this problem by trying to never reuse an error; specifically to never assign to an error value with =.

I would go the opposite way. It's the := which causes the trouble. (While being undeniably handy in almost every other situation.)

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc May 15, 2017

Contributor

Leaving on hold until @robpike has time to further explore a prototype.

Contributor

rsc commented May 15, 2017

Leaving on hold until @robpike has time to further explore a prototype.

@rsc rsc added the Proposal-Hold label May 15, 2017

@rsc rsc changed the title from proposal: add a vet check for a common pattern involving unused error value to proposal: cmd/vet: add check for common error mishandling pattern May 15, 2017

@velovix

This comment has been minimized.

Show comment
Hide comment
@velovix

velovix Jun 27, 2017

I'm not especially comfortable with the idea of ignoring errors when they are generally expected not to happen. It doesn't sound like a good idea, but perhaps I don't understand just how unlikely these errors are. It seems like the best case scenario is that you save three lines of code to panic on the error, and the worse case is that your program silently fails in a potentially subtle way.

Is this something a lot of Gophers do in practice? Has anyone run errcheck against as sizeable amount of Go code to make sure this is the case?

velovix commented Jun 27, 2017

I'm not especially comfortable with the idea of ignoring errors when they are generally expected not to happen. It doesn't sound like a good idea, but perhaps I don't understand just how unlikely these errors are. It seems like the best case scenario is that you save three lines of code to panic on the error, and the worse case is that your program silently fails in a potentially subtle way.

Is this something a lot of Gophers do in practice? Has anyone run errcheck against as sizeable amount of Go code to make sure this is the case?

@velovix

This comment has been minimized.

Show comment
Hide comment
@velovix

velovix Aug 28, 2017

I have since found out that fmt.Println returns an error, which is itself reason enough for me to be convinced that some errors should be ignored in practice.

velovix commented Aug 28, 2017

I have since found out that fmt.Println returns an error, which is itself reason enough for me to be convinced that some errors should be ignored in practice.

@leighmcculloch

This comment has been minimized.

Show comment
Hide comment
@leighmcculloch

leighmcculloch Nov 19, 2017

Contributor

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.

Contributor

leighmcculloch commented Nov 19, 2017

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.

@dominikh dominikh referenced this issue Sep 18, 2018

Open

staticcheck: incorporate go vet checks #149

4 of 22 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment