Removing nakedret golint check

[scallister]

I mentioned this in #12 but figured I'd just make a PR.

Naked returns with named variables are valid and useful, and can lend to much cleaner code when used correctly. I don't think we should block this feature of the language with a CI check ❤️.

[jmatt]

[jmatt]

I don't have a strong opinion on this. But it looks good, otherwise.

[cep21]

Can we revert this? Naked returns are almost always the wrong thing to do.

Lots of reading on why to avoid them: https://levelup.gitconnected.com/go-naked-returns-4e2094b598e6
Avoiding naked returns: https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_avoid_naked_returns
Even named returns, which are a prerequisite, are avoided unless there's no way not to or for documentation reasons.

Since the cases for naked returns are so few, it's best to keep the check and let people explicitly disable it for lines of code they want to. https://golangci-lint.run/usage/false-positives/

[scallister]

@cep21 I took a look at the links you shared. There are great points in there. There is also an open PR where someone is advocating for the removal of Naked Returns, but it has not been accepted, and I don't think it will be 🤔 golang/go#21291

I hope it is okay that at this point I don't agree that they should be completely dropped, especially given they're used in core libraries themselves. They do have their place, and I don't think they should be gated behind a //nolint 😞 .

The dave.cheney.net page has some great suggestions in there, but also advocates for not using named returned values as well. That doesn't sit quite right with me either. Some functions are better without named return values in the function declaration, but imo most benefit from it, especially when using autocompletion functionality. It makes it easy for users of APIs to see what is getting returned.

The arguments against naked returns and named returned values mention shadowing. I think that is a problem easily solved by always running go vet against our code, this is what I've done in the past. In general I'm not a fan of ever redeclaring variables like err multiple times in the same function 😬.

Part of the reason I'm drawn to naked returns is because I don't see value in functions that return the same variables list those same variables multiple times. I think it clutters the code with extra redundant text. Of course if there are varied returns in a function, then I agree mixing naked returns with non-naked returns is problematic and harms readability in that case.

I was curious so I checked the main Go repository and there are quite a few naked returns:

git clone --depth=1 [email protected]:golang/go.git
cd go
grep -riP 'return$' . | wc -l
17205

Maybe we can revisit this again in the future? It seems like we both have strong opinions on it, but hoping in the meantime we can default to allowing by default since it is part of the language intentionally and also used in the core code. Thanks Jack.

(Not the hill I want to die on, but those are my thoughts ❤️)

[cep21]

open PR where someone is advocating for the removal of Naked Returns, but it has not been accepted

Wouldn't make sense to remove backwards compatibility with existing code, so I agree the feature is here to stay.

also advocates for not using named returned values as well

Good advice there too.

The arguments against naked returns and named returned values mention shadowing

And complexity and obfuscation

checked the main Go repository and there are quite a few naked returns

I don't believe you :)

17205

That number seems suspect. Did you inspect those? Here is the first page.

./doc/asm.html:Do not insert instructions to allocate a stack frame and save/restore the return
./doc/go_spec.html:\r   U+000D carriage return
./doc/go_spec.html:     return
./doc/go_spec.html:     return
./doc/go_spec.html:     return
./doc/go_spec.html:     return
./doc/go_spec.html:sent values have been received, receive operations will return

That's the spec. Here are the first go files

./misc/cgo/errors/argposition_test.go:          return
./misc/cgo/gmp/gmp.go:          return
./misc/cgo/test/callback.go:    return
./misc/cgo/test/cgo_thread_lock.go:                             return

Here's the first of those files

func TestArgumentsPositions(t *testing.T) {
....
        if err != nil {
                fmt.Println(err)
                return
        }

Seems you're counting returns without parameters. Can you link me to a few naked and named returns in go or kubernetes?

They do have their place, and I don't think they should be gated behind a //nolint 😞

Gating makes sense when it's rare.