Add ST1024: Avoid named error return values named 'err'

[thaJeztah]

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.

[thaJeztah]

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.

[dominikh]

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.

[thaJeztah]

@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 (err for named outputs), and I didn't know of any linter that had a check for this, so thought I'd give it a try, taking parts of https://github.com/dominikh/go-tools/tree/master/stylecheck/st1008 as an example.

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]

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

[dominikh]

qqq?

[dominikh]

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?

[dominikh]

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]

This shouldn't be part of the docstring. We will (should?) generate the correct output from the value of NonDefault.

[dominikh]

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.