Proposal: Clothe naked returns about gofumpt HOT 10 OPEN

"clothing" returns is such a neat little name for this rewrite :)

I'm fully behind your rationale. I avoid naked returns practically everywhere.

I'm not sure this belongs in a formatter, though, even though it's meant to be more aggressive and strict than gofmt. You could squint a bit and say that such a rewrite would increase consistency, but the rewrite itself adds new identifiers to the syntax tree, which is not something that gofumpt has ever done before.

Another way to look at this is that such a rewrite is only safe to do with type information. gofumpt works off of the syntax tree alone, so it would have to start keeping track of what function body it's currently in, whether it's got any named result parameters, and what their names are. Not terribly expensive to do this, but it would be the first instance where we'd need to make gofumpt aware of types, further indicating it's not a formatting concept.

All that said, if this were added in another tool, I'd use it. Which tool that would be is a good question, and I'm not sure. If that's how you ended up suggesting it here, I completely understand.

from gofumpt.

I think you're right that most people would see this as a style issue, and the line between formatting and styling is a fuzzy one.

I also agree that golangci-lint wouldn't be a good home for such a rewriter. Personally I'm not a fan of that tool, so I'd avoid using the rewriter even if it did exist there.

I'm open to accepting a contribution for this. Would you be able to do a brief study on how prevalent naked returns are across popular Go codebases? My personal impression is that they are very rare, so adding this to gofumpt would not be controversial. But I have no idea if other codebases use it often or perhaps even consistently. It would be particularly interesting to see if any Go repos use gofumpt and use naked returns with some frequency.

from gofumpt.

Thanks @mvdan. I appreciate your arguments against including this in this tool. They're in line with some of my skepticism about including it here as well. Let me offer a small response, and some additional thoughts:

• From a useability standpoint, I think most would consider naked-vs-clothed returns to be a formatting issue (or a "style" issue, which is often considered synonymous, IME). I've never seen a code review discussion discussing this in terms of the AST or type information etc. While these are important implementation details, I'm not sure if they enter into the minds of the users of such tools.
• Indeed, what other tool would this belong in? It wouldn't be hard to create a new, stand-alone tool. But it would be hard to get meaningful adoption of such another tool.
• Selfish motivation: Now that Go 2 is "off the table", my dreams of a Go 2 without naked returns have been dashed. The next best thing I can imagine is a world in which gofmt itself employs the proposed rule. I figured the best way to explore that possibility would be by starting here. 😉

Probably the next best option for another tool would be a golangci-lint linter that not only flags, but rewrites naked returns. This would be better than another stand-alone tool, for adoption, but does pose a number of additional challenges. In particular, it's often problematic to enable rewriting rules in golangci-lint for various reasons. Most important, it's currently not possible to enable rewriting on a per-linter basis, and some rewrite rules aren't always safe, so it's dangerous to enable rewriting across all linters (this is why I've virtually never used golangci-lint's rewriting capabilities).

from gofumpt.

One option we didn't talk about is an analyzer (https://pkg.go.dev/golang.org/x/tools/go/analysis#hdr-Analyzer) which could then be added to gopls or other "meta linters" easily. However, the bar for adding these to gopls is somewhat high, since they need to be bundled at compile time. So far I think they've only added staticcheck and gofumpt, and gofumpt isn't even an analyzer (yet).

from gofumpt.

golangci-lint also uses the analyzer interface, so creating a linter for that project would kill that second bird with the same stone.

from gofumpt.

Here are some very high-level, preliminary results from my investigation. I've counted total number of clothed and naked returns, as well as the number of functions that mix naked and non-naked returns. The discrepancy between total and clothed+naked represent returns in functions that have no return values. I've also excluded generated code where it's easy to identify, and the testdata and vendor directories.

I've compared three code corpora:

1. The Go standard library (1.21.0)
2. The repos from the top 100 modules listed at grank.io
3. The subset of 2 that mention gofumpt within the repo, as determined by a simple naïve grep

I used this very unsophisticated, thrown-together tool to do the analysis.

Update: The majority of the naked returns in the grank list come from a single repo: https://github.com/tencentcloud/tencentcloud-sdk-go

If I pull that one out, the results look like:

from gofumpt.

I used this very unsophisticated, thrown-together tool to do the analysis.

Did you mean to make that open source, or is the link perhaps incorrect?

Ignoring that SDK module seems fine; the examples in the README don't even follow gofmt :) Though note that grank appears to heavily skew towards libraries. The top ten are all libraries, and out of them six are primarily for testing, for example. I would personally think that large non-library projects are significantly more relevant than testing libraries. For example, here's a rather simple ranking based on stars and forks on github: https://github.com/mvdan/corpus/blob/master/top-100.tsv

~1% across popular repos and ~0.8% across those which are aware of gofumpt sounds like what I would expect, and that doesn't worry me in terms of giving this a try. ~2.7% in std is definitely more worrying, though. I think we'd have to implement a first version of this, run it across some good codebases like std, and see whether the result looks better or not. If we're not sure, then we shouldn't release it.

from gofumpt.

Did you mean to make that open source, or is the link perhaps incorrect?

I did. It's public now.

here's a rather simple ranking based on stars and forks on github

I'll follow up with an analysis of some of these.

from gofumpt.

related items:

• https://github.com/alexkohler/nakedret
• https://github.com/gavv/returnstyles

from gofumpt.

Took me a while to get back to this, but I've now run the naked return analysis on the collection of repos at https://github.com/mvdan/corpus/blob/master/top-100.tsv:

from gofumpt.