Repository navigation
Detect returning nil after err != nil check #863
Description
I would like staticcheck to detect the following:
func foo() error {
...
err := bar()
if err != nil {
return nil // BUG! Most of the time it should be `return err`
}
...
}Sometimes, of course, you want to explicitly return an error there, so this check will have quite a few false positives.
Most of the time, however, it will detect bugs.
Activity
ilinum commentedon Oct 24, 2020
I'd be happy to implement this but I would like to hear some input first with regards to whether or not this would cause too many false positives for the value that the check would provide.
dominikh commentedon Oct 24, 2020
That, unfortunately, makes it ineligible for Staticcheck. We err on the side of false negatives, and checks are expected to have no false positives in the general case. As such, we can't just flag
if err != nil { return nil }.This might be better suited for https://github.com/go-critic/go-critic
dgryski commentedon Oct 24, 2020
This pattern is very easy to match with ruleguard or semgrep.
dominikh commentedon Oct 17, 2021
Reopening under the
aggressivelabel, for a future set of checks specific to code review.dominikh commentedon Oct 19, 2021
Running this against
k8s.io/kubernetes/pkg/...yieldsDo people see this kind of bug during code review, and how often?
ainar-g commentedon Oct 19, 2021
Does the check that you run include something like an exception for branches with comments? For example, does it match code like:
To answer your question directly, I've definitely seen this kind of bug a couple of times in code reviews and at least a dozen of times in the code that I wrote but haven't committed yet.
dominikh commentedon Oct 20, 2021
That implementation did not. However, none of the results found in k8s had comments, either.
Thanks.
dgryski commentedon Oct 20, 2021
Same. For me the pattern occurs not as a direct typo, but as the result mis-think during refactoring when you change the
ifcondition and forget to change the body because your brain already sees areturnthere.ainar-g commentedon Oct 20, 2021
I mean, if we're talking code review, I'm pretty sure that even if such code is correct, it should at least have a comment there saying so, so that the readers won't have to guess if it's actually correct or not. But opinions may differ here.
seiyab commentedon Mar 16, 2024
I've scanned some projects with my toy analyzer and found some real bugs.
gh gpg-key deletelooks to discard HTTP errors mistakenly cli/cli#8026And it also causes false positives.