Add ST1024: Avoid named error return values named 'err'#1667
Add ST1024: Avoid named error return values named 'err'#1667thaJeztah wants to merge 1 commit intodominikh:masterfrom
Conversation
Named return values called 'err' are discouraged because they can lead
to confusion and subtle mistakes.
In most Go code, 'err' is used for short-lived local variables. Naming a
return parameter 'err' can induce cognitive load.
For example:
func fn() (err error) {
if err := doSomething(); err != nil { // shadows the return
return err
}
defer func() {qqq
// This 'err' is the named return, not the one above
if err != nil {
log.Println("deferred error:", err)
}
}()
return nil
}
Using a distinct name for the named return makes the return value’s
role explicit and avoids confusion:
func fn() (retErr error) {
if err := doSomething(); err != nil {
return err
}
defer func() {
if retErr != nil {
log.Println("deferred error:", retErr)
}
}()
return nil
}
This check is opt-in and not enabled by default.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
16fd734 to
2a9f7ad
Compare
| Since: "2025.2", | ||
| NonDefault: true, | ||
| MergeIf: lint.MergeIfAll, |
There was a problem hiding this comment.
Hope I did this correct; I wasn't sure what Since to add here, and assumed that NonDefault: false meant that it's an opt-in.
There was a problem hiding this comment.
We use "Unreleased" for checks that aren't part of an existing release yet. That gets updated on release day.
You're right about the use of NonDefault.
|
@dominikh ptal; I'm really unfamiliar with this codebase, so hope I did it correct (tests passed locally, so that's a win 😅). This was a bit of a pet-peeve of mine ( I hope it's something you're willing to accept (I think I made it "opt-in"), but happy to hear your thoughts! (if not accepted, it was a fun exercise 😂) |
dominikh
left a comment
There was a problem hiding this comment.
As far as the PR itself is concerned, this is great work. I am, however, not sure this check is suitable for Staticcheck. Style checks, even non-default ones, have to be pretty much universally true, if not official. The checks that are non-default are non-default because they--while correct--can be a bit much to worry about constantly, not because people are able to disagree with them (at least that's the goal).
So when you say
Named return values called 'err' are discouraged because they can lead to confusion and subtle mistakes.
it comes down to this: discouraged by whom, and always? Because it doesn't seem all that discouraged in Go's own code base:
rg 'func.+\).+err error\)' $(go env GOROOT)/src | wc -l
5637
| if err := doSomething(); err != nil { // shadows the return | ||
| return err | ||
| } | ||
| defer func() {qqq |
| return err | ||
| } | ||
| defer func() {qqq | ||
| // This 'err' is the named return, not the one above |
There was a problem hiding this comment.
I'm not sure I understand how this is a convincing or illustrative example. If the user thought that err referred to a local variable, wouldn't the whole check be redundant, since that err had to be nil to get here?
| Since: "2025.2", | ||
| NonDefault: true, | ||
| MergeIf: lint.MergeIfAll, |
There was a problem hiding this comment.
We use "Unreleased" for checks that aren't part of an existing release yet. That gets updated on release day.
You're right about the use of NonDefault.
| return nil | ||
| } | ||
|
|
||
| This check is opt-in and not enabled by default.`, |
There was a problem hiding this comment.
This shouldn't be part of the docstring. We will (should?) generate the correct output from the value of NonDefault.
| errorType := types.Universe.Lookup("error").Type() | ||
|
|
||
| fnLoop: | ||
| for _, fn := range pass.ResultOf[buildir.Analyzer].(*buildir.IR).SrcFuncs { |
There was a problem hiding this comment.
We don't need the power of the IR here. Instead, I'd use an inspector.Inspector to look at the ast.FuncDecls directly.
This doesn't really matter for staticcheck (our command) because we have to build the IR form, anyway, but we may have clients that use a subset of our checks, such as only those that are fast, which excludes those that rely on the IR.
Named return values called 'err' are discouraged because they can lead to confusion and subtle mistakes.
In most Go code, 'err' is used for short-lived local variables. Naming a return parameter 'err' can induce cognitive load.
For example:
Using a distinct name for the named return makes the return value’s role explicit and avoids confusion:
This check is opt-in and not enabled by default.