Please remove nakedret from the list of linters

[jech]

Hi @joeturki,

Could you please consider removing the nakedret linter from the list? This linter has forced me to write some rather unidiomatic code in #3329.

Thanks.

[JoTurk]

Hmm, in the next lint change I'll try to change its settings so it matches gocognit,gocyclo and cyclop. it's super helpful for big functions. And I do think that Go named returns is a troll feature.

---

Welcome back. We missed you. Still don't want to join the discord? :)

too many changes, have fun with the review :)

[Sean-Der]

I like your change in #3329!

I was reading what people think about this and found golang/go#21291 (comment)

I like naked returns for small functions. However for big functions it sure is nice to not keep 'what is being returned' in my mental model.

We don't really have a 'voting' process so not sure how to resolve. How strongly do you feel @jech about nakedret causing bad code?

[jech]

Still don't want to join the discord?

They don't seem to have improved their usage conditions:

by using our services, you grant us a license [...] to do the following with your content:

• Use, copy, store, distribute, and communicate your content [...]
• Publish, publicly perform, or publicly display your content [...]
• Monitor, modify, translate, and reformat your content [...]
• Sublicense your content [...]

That's in addition to all the problems with Slack, where we created a proprietary, non-searchable database of Pion knowledge that we no longer can access.

[jech]

We don't really have a 'voting' process

Please don't, voting is not a good way to solve technical disagreements, it dilutes responsibility. We have a technical discussion until we reach consensus (not necessarily unanimity). Anyone who uses a non-technical argument gets thumped on the head. In the rare cases when we don't reach consensus, Sean or Joe take a decision (and take responsibility for the consequences). (That, of course, assumes that Sean and Joe have the time and energy to understand the various positions.)

How strongly do you feel @jech about nakedret causing bad code?

Consider the code in #3329:

	if err = rp.Unmarshal(buf); err != nil {
		return mid, rid, rsid, false, err
	}

	if rp.Padding && len(rp.Payload) == 0 {
		return mid, rid, rsid, true, nil
	}

	if !rp.Header.Extension {
		return mid, rid, rsid, false, nil
	}
	...

In order to review this code, one needs to carefully check that the ordering of the return values is consistent with the header; if one were to write return mid, rsid, rid, false, nil in an obscure case, the compiler wouldn't catch it, and I don't think our current unit tests would. The reviewer must painstakingly check that all the return statements have their parameters in the right order.

The issue doesn't occur with named returns:

	if err = rp.Unmarshal(buf); err != nil {
		return
	}

	if rp.Padding && len(rp.Payload) == 0 {
		paddingOnly = true
		return
	}

	if !rp.Header.Extension {
		return
	}
	...

A workaround is to use a struct, see for example e710dae, whose fields are named, and therefore don't suffer from the ordering problem. Named returns are a less verbose and more idiomatic way to obtain the same results

[JoTurk]

In order to review this code, one needs to carefully check that the ordering of the return values is consistent with the header; if one were to write return mid, rsid, rid, false, nil in an obscure case, the compiler wouldn't catch it, and I don't think our current unit tests would. The reviewer must painstakingly check that all the return statements have their parameters in the right order.

For me named returns are much harder to review, especially If i'm doing the review in the Gaithub web interface, if all I see is return.

also i see a possible future bug in your example.

consider

if rp.Padding && len(rp.Payload) == 0 {
    paddingOnly = true
    return
}

logically this is correct now, but if we mutate mid, rid, etc in the future; before this condition, we'll be returning incorrect value. and future reviewers have to review all the paths in this function and prove that the behavior didn't change for any return.

[jech]

Sure, there are up- and downsides to both approaches. Which is why we should be able to choose on a case-by-case basis, and not ban the feature using a linter. (It's not like I'm proposing to add a linter to ban unnamed returns, that would be absurd.)

[JoTurk]

the linter doesn't ban the future, it bans it for big functions, what we consider a big function. should align with other linter like gocognit, gocyclo and cyclop.
I would argue that the function in this example is a big function, if it can't fit the github web review UI it's a big function :)

• I already showed you an example for why it's bad in this very best case scenario example.

[jech]

I'll assume that the patronising tone was not intended.

[JoTurk]

I'll assume that the patronising tone was not intended.

English isn't my first language :) I'll keep this open, I agree that we need to update its settings but not sure about full removal :)

[JoTurk]

@jech really appreciate you and we missed your reviews when you weren't around :) sorry if you miss-understood my tone as patronising. I was trying to go with suggestion for "technical discussion" even tho I did agree to change the linter settings in my first message. and again English isn't my first language :)