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.

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:

• https://github.com/dominikh/go-tools/tree/master/cmd/staticcheck
• https://github.com/kisielk/errcheck

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.

This is a refinement of #19727.

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.

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.

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.)

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

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?

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.

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.

A solution via Go 2 Error Handling: #20803 (comment)

I've found that nilness and ineffassign end up catching these cases.

I think these two cases are fairly distinct.

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

It seems like the checker Rob described should have a low false positive rate already for this case. I would be curious to see a prototype explore this.

e()
if err != nil { ... }

Due to things like fmt.Println, more evidence seems needed here to infer intent. Knowing that err != nil is always false seems sufficiently precise. One can pick this back up from the control flow and conditions. I am not 100% on how the nilness pass works, but I am not surprised this would also report these. Example:

val, err := doSomething()
if err != nil {
    return err
}
e(val) // where e has an error return value.
if err != nil {...} // err must be nil due to the previous if

Weaker ways of inferring intent I can think of include:

• statistical modeling, i.e. in X% of e() calls this condition is checked (this will have false positives, but they will probably seem reasonable), or
• err != nil has been checked without err changing value. This can have false positives if say a programmer wanted to respond to just log when there is an error, continue for a while and then exit. This sounds rare to me but it would need to be tested.

nilness does indeed catch the second case in your example. The data flow analysis notices that err must be nil (otherwise the function would have returned) and so warns the second err != nil check is redundant. Note that simply checking err != nil twice without the control flow change is not enough to trigger the check, unless the two checks are nested (again, causing the second check to be reundant).

Unless I'm misunderstanding something, this proposal wouldn't catch an issue I had recently:

func (c Client) Close() {
  c.Close() // returns type error
}

Not realizing that Close() returns an error, newcomers like myself may easily drop it without checking at all.

Your problem there @yawaramin is that your outer function should be returning an error if you want to implement io.Closer. If it had, the compiler would have complained about a missing return, which would probably tip you off that you'd missed it.