-
Notifications
You must be signed in to change notification settings - Fork 28
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
consider flagging errors.Wrap(err) where err is known to be nil #5
Comments
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:
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 (kopia/kopia#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.The text was updated successfully, but these errors were encountered: