simple: unnecessary "if err != nil" before returning error or nil

[hchargois]

I often see code ending with an if and two returns like this:

func foo() error {
    var err error = bar()
    if err != nil {
        return err
    }
    return nil
}

But (unless I'm mistaken) as long as err is already an error interface (or actually any interface), the if and the two different returns are not needed, and this can always be safely simplified into:

func foo() error {
    var err error = bar()
    return err
}

I think the unnecessary if err != nil may happen just from muscle memory, or due to refactoring...

Or it may come from a confused understanding of the infamous case of "Why is my nil error value not equal to nil?" https://go.dev/doc/faq#nil_error But most often, err is already an error, i.e. an interface, and in that case if err is nil, returning err is the same as returning a verbatim nil.

So I think adding a "simple" check for that case would be interesting, not only for simpler code and a few lines removed, but also for "educational" value, basically saying "yes, it's OK to do this".

Of course all of the above is true for any kind of interface, not just error, so the check could be generic. But errors is probably the most common case.

[dgryski]

I have a check for this in my semgrep rules. It had a very high false positive rate, which is to say it's a very noisy pattern.

https://github.com/dgryski/semgrep-go/blob/master/errnilcheck.yml

[dominikh]

This could likely be an extension of S1008.

[hchargois]

I have a check for this in my semgrep rules. It had a very high false positive rate, which is to say it's a very noisy pattern.

https://github.com/dgryski/semgrep-go/blob/master/errnilcheck.yml

Thanks for your input. I don't know semgrep but in the yml you linked, I don't see any condition checking whether err is an interface or not. In that case, of course we can expect to flag code where replacing with return err is not possible, as described in the Go FAQ I posted above.

Is this what you call "false positive"?

I agree these cases shouldn't be flagged. The check needs to check that err is an interface. This should remove all of these false positives, unless I'm completely mistaken on the whole topic.

Or are you calling "false positives" cases where you may want to keep the if instead of simplifying (e.g. for readability reasons) even though the code would work the same?

This could likely be an extension of S1008.

I agree, to me it's the exact same kind of thing. But others may disagree, it's not as "obvious" as the boolean case. Some people may want to simplify boolean returns but not error returns? IDK