consider flagging errors.Wrap(err) where err is known to be nil

[jkowalski]

I found several cases in my codebase (kopia/kopia#747) that follow the following buggy pattern:

err := doSomething()
if err != nil {
  return result, errors.Wrap(err, "something failed")
}

if somethingElse {
  return result, errors.Wrap(err, "something else failed")
}

The second errors.Wrap() is incorrect, because it's not wrapping any error (but it's easy to copy/paste it as such).

This is quite hard to spot in code reviews because lines that return errors will always have errors.Wrap() and it looks ok at first glance, until you notice that in this case is err is always nil. Because errors.Wrap(nil, "something") always returns nil this one returns success, which is unintended.

Based on flow analysis, it should be possible to determine that err == nil in this case, and it would be really amazing if the linter could flag this pattern is as misuse.

[tomarrell]

Hmm interesting, definitely seems doable. I can take a look when I get some time.

Cheers for bringing it up.

[jtwatson]

There is another version of this that I find a lot in code reviews that would be handy to catch with this linter...

Common sql error checking:

if rows.Err() != nil {
  return result, errors.Wrap(err, "something failed")
}

Because err is declared above this statement, the compiler doesn't raise any issues.

Maybe this is out of scope for this linter and should be a dedicated linter?

[kepelletier]

Hello @tomarrell, any news on this ? That would be tremendously helpful to have such functionality

[tomarrell]

G'day, I think both of these cases could be implemented neatly within this linter as it already has the definition checking there.

As far as timeline, I can't give anything concrete, however I will start looking into this next as I get some time. If you want it ASAP, I'm quite responsive to PR's.

Thanks!

[kepelletier]

Thanks a lot for your answer. I'm looking forward to this new feature!