Add nonamedreturns linter

[firefart]

This adds a new linter nonamedreturns

https://github.com/FireFart/nonamedreturns

This linter simply checks for the use of named returns

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

Hello,
in order for a PR adding a linter to be reviewed, the linter and the PR should follow these constraints:

Requirements:

• the PR description should have a link to the linter repository
• the PR description should provide a short explanation about the linter
• the linter should have a valid license (the file must contain the required information by the license, ex: author, year, etc.)
• the linter should not be a duplicate of another linter (the team will verify that)
• the linter should use go/analysis (https://golangci-lint.run/contributing/new-linters/)
• the linter should not be added to the .golangci.yml of golangci-lint itself (the .golangci.yml should not be edited)
• .golangci.example.yml:
  • add the configuration if the linter has a configuration (alphabetical order case-insensitive)
  • add the linter to the list of available linters (enable and disable) (alphabetical order case-insensitive)
• the linter should have a valid tag (ex: v1.0.0)
• the linter should not have false positives/negatives (the team will verify that)
• the linter should have tests inside golangci-lint
• the linter tests should work with T=<name of the linter test file>.go make test_linters
• the linter tests should have at least one std lib import
• the load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager
• the linters should be sorted in the alphabetical order (case-insensitive) in the Manager and .golangci.example.yml
• the version in WithSince(...) must be the next minor version of golangci-lint (v1.X.0)
• the linter should not contain init
• the linter should not contain panic, log.fatal, os.exit
• the PR should pass the linter.

Recommended:

• no SSA
• the linter repository has a CI, tests, a readme, and linting
• the linter is published as a binary

The golangci-lint team will edit this comment to check the boxes before and during the review.

[ldez]

The license is still invalid https://github.com/FireFart/namedreturnlint/blob/aaa436feb6710f65388a6c3e5d7a185efedb119b/LICENSE#L655

And some other points need to be fixed.

[firefart]

The license is still invalid https://github.com/FireFart/namedreturnlint/blob/aaa436feb6710f65388a6c3e5d7a185efedb119b/LICENSE#L655

And some other points need to be fixed.

This text in the license file is not meant to be changed (take a look at your license for example)

Everything else is addressed except the linter tests should have at least one std lib import because it makes no sense in this linter

[ldez]

yes, I read the license a bit too quickly.

Everything else is addressed except the linter tests should have at least one std lib import because it makes no sense in this linter

You have to add an std lib import it's a requirement, it's a bit complex to explain in detail but this is required to be sure that the linter works well in several situations.

The other points (except those for the team) still need to be fixed.

[firefart]

ok thanks I hope everything is now addressed correctly

[ldez]

This was the first time I tested this checklist, so it's not perfect.
Do you have some feedback about this checklist? was this helpful to you?

[Antonboom]

@firefart interesting linter

I hope u know that you cannot do without named values when changing results via defer?

Moreover, nothing prevents you from using both a named results value and a no-nacked return. And in some cases using named results is useful:

func GetList(ctx context.Context) (page []feed.Object, higher, lower *Cursor, err error) { /* ... */ }

func (c *cache) Get(userID string) (u *User, missed bool, err error) {
    // ...
        return nil, false, fmt.Errorf("save user in cache: %w", err)
    // ...
}

• See CodeReviewComments#named-result-parameters
• See nakedret linter

---

It seems to me that we need to come up with a number of checks from the linter more complex than the binary logic "used / don't". Also the name of the linter is confusing now.

[bombsimon]

FWIW I would consider usung this linter as is. I’ve worked at companies that did not allow named returnes (unconditionally like this linter) so we basically never changed returnes values in defer and almost never returned more than two types.

We don’t have a hard rule for that where I’m at now but I know some people (like me) think omitting variables at return or overwriting what gets returned makes the code harder to understand and it’s pretty unusual that we do that.

Like you said though @Antonboom you can name them and still return named variables. A check for empty returns instead of checking the signature would be nice but I still think there’s a use-case for the linter and I’m not sure we should start deciding what linters to accept based on how many will find it useful. We already have a bunch of linters used by very few people that’s opinionated. Maybe something to discuss outside this PR?

[Antonboom]

@bombsimon

I have nothing against the linter, because previously received your support for my linters (dubious for other people).
I'm just highlighting some cases, suddenly it will be useful to the author and he decides to change the approach.

[firefart]

Hi,
jeah I know about the defer but I tend to rather not check the return than using named returns (it does not fit my coding style I would say). They caused to many weird errors in the past for me, especially if you have an err error named return and than produce some errors that don‘t make sense if you don‘t reset an error value sometimes during the code. For example when a function returns an error and you choose to ignore it, but the return value is still set.
I use this linter personally as a coding style check to warn me about them if I accidentally added one to not have production problems afterwards

[ldez]

Also the name of the linter is confusing now.

I agree with this point, maybe a better name can be nonamedreturn because the meaning of this linter is to forbid named return.

The name namedreturn feels like it will handle all possibilities around named/naked returns.

Personally, I am also a "no named returns" guy (at least by default), because the readability is bad in the majority of the cases and they are sources of bugs.
For me, in general, if a function returns more than 3 values, the function has an algorithmic problem.

[leonklingele]

I've opened #2526 a couple of months ago which offers the same (and more) functionality but was closed immediately. Can it be reconsidered?

[ldez]

The problem with your linter was that half of the linter was already provided by another linter.
But yes I could say the same as here but this linter provides only a non-existing rule and go-critic doesn't seem to be interested in.

Note: I have not checked "the linter should not be a duplicate of another linter" and it's not a mistake.

[firefart]

I agree with this point, maybe a better name can be nonamedreturn because the meaning of this linter is to forbid named return.

Jeah I agree I renamed the linter to reflect the purpose more cleanly

[firefart]

This was the first time I tested this checklist, so it's not perfect. Do you have some feedback about this checklist? was this helpful to you?

Jeah I really like a strict rulebook to check against

Some feedback:
I missed a few points because I did not read them clearly enough like where you need to put the linter in the enabled AND the disabled section (I tought it was an or). So maybe it's better to mark the and is this case bold or make it two points. Also I missed the version point because I changed the revision instead of the minor version.
I think it might also be better to split the points in two sections: 1) Things you need to do in your linter repository (like license and valid tag) and 2) Things that need to be done in this PR. So 1) need to be completed before doing the integration PR.
Once the list is finished it should also be put on https://golangci-lint.run/contributing/new-linters/

[leonklingele]

@firefart thanks for providing such a linter! Please make it pass the additional test cases described in https://github.com/leonklingele/funcresult/blob/9bcd76fcada71b380ab86f0f8b5ac8bb56ecee5c/pkg/analyzer/testdata/namedresult/src/named-require-unnamed/testcast.go

[firefart]

@leonklingele thanks for the testcases! I missed a few in my implementation and updated the linter to match those too

[ldez]

LGTM

[firefart]

@ldez thanks!

[joewreschnig]

This has resulted in being unable to build golangci-lint without using the official module proxy, since the upstream checksum of [email protected] has changed.

firefart/nonamedreturns#2