Open jkowalski opened 3 years ago
Hmm interesting, definitely seems doable. I can take a look when I get some time.
Cheers for bringing it up.
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?
Hello @tomarrell, any news on this ? That would be tremendously helpful to have such functionality
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!
Thanks a lot for your answer. I'm looking forward to this new feature!
I found several cases in my codebase (https://github.com/kopia/kopia/pull/747) that follow the following buggy pattern:
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 iserr
is alwaysnil
. Becauseerrors.Wrap(nil, "something")
always returnsnil
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.