proposal: gofmt should add named return variables to empty return statements

[kf6nux]

The Go community's adoption of gofmt demonstrates the community cares about code being well formatted and appearing the same.

The Go spec allows for two different (but equivalent) return statements. Either explicitly declaring what one is returning, or implicitly returning the function's result parameters.

Using an example func from the spec:

func complexF3() (re float64, im float64) {
	re = 7.0
	im = 4.0
	return
}
// vs
func complexF3() (re float64, im float64) {
	re = 7.0
	im = 4.0
	return re, im
}

This proposal is to have gofmt add result parameters to empty return statements for added clarity and unified appearance.

What version of Go are you using (go version)?
go version go1.11.1 darwin/amd64

Does this issue reproduce with the latest release?
Yes

[earthboundkid]

This strikes me as giving the benefits of #21291 without the compatibility drawbacks. It also means you can just write return in your editor and have it fill in the values for you, which is convenient.

[jimmyfrasche]

This could easily be done by a third-party tool. The idea is similar to https://github.com/sqs/goreturns which appears to support this with a -b flag (I have never used it, personally).

[DeedleFake]

If this did happen, it might be good to have it give an error, or optionally give an error, if the return variables have been shadowed at the point that the return is being modified by it.

[earthboundkid]

1. A third party tool defeats the purpose of the change, which is to standardize on not using bare returns.

2. Warnings should be (and are) part of go vet.

[DeedleFake]

I'm not saying a warning; I'm saying an error. Otherwise it could change what code actually does. Consider the following:

func Example(v int) (n int) {
  if v > 3 {
    n := 5
    return // If this gets modified, it'll return the shadow instead.
  }
  return
}

[magical]

Given that the compiler already emits an error for shadowed bare returns, the tool could just leave the return statement unmodified in such cases.

[kf6nux]

Shadowed return variables are probably always bugs or something that'll confuse future readers.

Not adding the variables defeats the purpose of this proposal.

I'd propose gofmt could exit with error (not formatting the file) if it detects a shadowed return variable when it attempts to add explicit return variables.

[cznic]

I'd propose gofmt could exit with error (not formatting the file) if it detects a shadowed return variable when it attempts to add explicit return variables.

I think gofmt should not, at least not by default, perform any type checking and/or semantic analysis. What the parser accepts should always produce formatted unless explicitly asked for more (like eg. -s).

Otherwise it's not possible to properly format for example code demonstrating bad practices, invalid code (but syntactically valid), code for which its imports are not available, etc.

[DeedleFake]

Given that the compiler already emits an error for shadowed bare returns, the tool could just leave the return statement unmodified in such cases.

I keep forgetting that that's an error. I've been using Go since it was doing weekly releases and I only found out about this when I saw the Go says WAT? talk from GopherCon. Given that's an error, I'd say that gofmt should probably also give an error if it was going to modify that line. It definitely shouldn't be silently fixing errors, potentially incorrectly, for the user.

I think gofmt should not, at least not by default, perform any type checking and/or semantic analysis. What the parser accepts should always produce formatted unless explicitly asked for more (like eg. -s).

Maybe this should wait on #21291. If that gets accepted it would probably make more sense to put this in gofix. That's a big 'if', though.

[kf6nux]

@cznic , would you please help me understand the use case for having gofmt format invalid code?

I agree (from a single responsibility principle standpoint) that having gofmt look beyond syntax to variable declaration/shadowing feels like a bit of an overreach. Is there another place you'd like to see this change happen, or are you opposed to it altogether?

[josharian]

@kf6nux I have gofmt-on-save set up, and I frequently save in the middle of writing (temporarily) semantically invalid code, just to make my code more readable.

[kf6nux]

Thanks @josharian . That makes a lot of sense. Hopefully nearly everyone has gofmt on save.

If we broadened this change to include a change to go build to error on both implicit and explicit returns that have been shadowed (instead of just implicit), gofmt could safely always add the return variables.

An example of implicit vs explicit shadowed return behavior (attempt to run this in the playground):
https://play.golang.org/p/50GWX_RrKsx

I'm not sure if prohibiting explicit shadowed returns would break Go's Promise of Compatibility. Does anyone have context on the precedent of when Go added an error for implicit shadowed returns?

[DeedleFake]

It would very much break the compatability promise, as well as tons of code. Anytime someone used named returns they wouldn't be able to return any variable with the same name as those returns. Right now the way you fix the <variable> is shadowed on return error is by not using a bare return; that would suddenly cause any code that did that to break, and the only way around it would be to either rename the variable or assign it to another one with a different name. Especially for errors this seems annoyingly tedious, as it's common to just always call the variable err if you only need to deal with one at a time.

Bottom line: Making explicit returns of shadowed named returns an error would break a lot of code.

[bcmills]

As a milder variation, you could imagine rewriting return statements in -s (“simplify”) mode only.

[kf6nux]

Yes, gofmt -s is a better fit for this type of feature which would actually be re-writing code instead of merely adjusting formatting. Thanks for suggesting that @bcmills

One possible implementation:

Given a bare return in a function with named result parameters
When gofmt -s is invoked
Then the bare return is populated with the named result parameters

Given a bare return with named result parameter(s) that have been shadowed
When gofmt -s is invoked
Then the shadowed variable is rewritten to <originalname>Shadowed and the bare return is populated with the named result parameters

Given a function return with named result parameter(s) that have been shadowed
When gofmt -s is invoked
Then the shadowed variable is rewritten to <originalname>Shadowed

A downside of that implementation is that it would cause the compiler to no longer error with shadowed result parameters + bare returns. This would be mitigated by gofmt -s re-writing the shadowed variable name which should cause the application logic to come up in code review if the author's original code wasn't their intent.

Thoughts?

[ianlancetaylor]

I'm not sure I like this idea, but if we were to pursue it, gofmt should not be renaming variables. A naked return with shadowed result parameter, a case that will not compile anyhow, should be left alone.

[kf6nux]

To be clear, we're talking about gofmt -s which rewrites code to be functionally equivalent but easier to read. Personally, I find not having to reason about shadowed return parameters easier to read.

That said, I'm not convinced my last comment has the best implementation. I'm just trying to enumerate options in this discussion thread to move the conversation forward. Perhaps re-writing shadowed return variables could be better discussed in a separate thread?

It's good feedback that this feature wouldn't necessarily need to handle bare returns with shadowed variables as the compiler will err. That'd leave us with a single change/requirement:

Given a bare return in a function with named result parameters that haven't been shadowed
When `gofmt -s` is invoked
Then the bare return is populated with the named result parameters

A think a lot of the value of this proposal is reduced by requiring -s, but that seems to conform to the spirit of how the tool is intended to be used.

[magical]

I like this idea, but I think i agree with cznic: if gofmt doesn't currently do any scope analysis, then it's not worth adding just for this.

And per jimmyfrasche's comment, it sounds like there's already a tool that can do this. Maybe that's enough.

[kf6nux]

goreturns feels like a different use case to me.

This proposal is regarding making returns explicit instead of implicit without changing functionality.

goreturns adds zero-value returns to broken return statements (re-writing the original intent).

[rsc]

Sorry, but this is not gofmt's job. Furthermore, as long as the input is a valid program, gofmt should not be coercing it into some other form.

Even if #21291 were accepted, this would be something for go fix, not gofmt.