Go: cmd/gofmt: formatting improvements may break existing presubmit checks

[griesemer]

See e.g. https://github.com/golang/go/issues/18264#issuecomment-342338179 .

The change in https://golang.org/cl/71990 changes permissible formats. Existing presubmit checks that use the new gofmt may complain about existing code formatted differently.

Do we need to do anything about this besides documentation? Do we need to roll back the (positive) change? Do we need a flag?

[ianlancetaylor]

Any change to gofmt, even a positive one, is indeed problematic for many users. We have also seen several failures in internal google testing.

[griesemer]

In this specific case (CL 71990) we could provide a (temporary) flag or perhaps use -s to enable the change. Once made, the code is ok. This leaves it to each team to decide whether they want to change or not.

[ianlancetaylor]

Using a flag for gofmt seems to me to make the problem worse. If it's a problem today, then it's a problem when we remove the flag, so I don't see how we can ever remove the flag. And people have to know to add the flag. So we have the same problem, plus we have a flag.

[griesemer]

I'm thinking that perhaps we don't remove the flag. It would be the same flag for all future changes. If people chose to set it, the code will go through a one-time reformat, after which the code remains changed (in this case, empty lines will remain removed) even if the flag is not used. This would users give a chance to "opt in" when they want to. Similarly to the option -s which allows a one-time simplification that remains applied even if future gofmt invocations don't use -s.

[mvdan]

How about making gofmt allow both old and new formats in these cases? Without changing the old to the new one. Then, at some point in the future (2.0?), support for the old formats is dropped.

[ianlancetaylor]

@griesemer I see. What would the flag name be? What should people who use the go/format package do? Some tests generate code, run it through go/format, and compare that to a golden file.

@mvdan If we don't change the format, but simply accept either variant, then at least with regard to https://golang.org/cl/71990 I'm not clear on how that differs from what we did before that CL.

[mvdan]

You're right, my proposal doesn't work at all with changes that simply made gofmt stricter.

[dlsniper]

This is a bit unfortunate that it happened and yes, a lot of issues will be caused by this.
I've seen a lot of people, both on Gophers Slack or while helping them with various issues for the GoLand IDE, that are running gofmt ./... or similar commands. Having this change will generate a lot of needless commit diffs and upset a lot of people.
I've asked many times about the gofmt status and actually documenting it, see #18790, but I've always got the same response as in the linked issue. Maybe in the light of this issue, this will be reconsidered?
Thank you.

[griesemer]

@dlsniper The proposal #18790 has been declined for good reasons. There's no need to revisit this for now.

[dsnet]

Several thoughts:

• For the class of gofmt changes where the tool is more strict, gofmt can always be run across the code-base today, and have the (probably small) set of changes checked in. This will avoid future breakages for the upcoming release.
• If gofmt is being run as part of a CI or presubmit step, then perhaps it should only be run on the files that changed in the last commit. This has many benefits:
  
  
  
  • It scales better as the repository scales.
  
  
  • The issue of version skew for gofmt fixes itself naturally over time. If you change foo.go and the CI complains because of gofmt, then just submit it with the updated gofmt change as well (which will almost always be irrelevant whitespace changes).
  
  
• At least for Google, most of the breakages have been because of golden test data hard-coding gofmt'd output and expect the output to match byte-for-byte.
  
  
  
  • This is just a bad test, and the test should run both the golden output and test output through gofmt again before comparing it.
  
  
  • From gofmt's perspective, the only way to avoid these failures is to ensure gofmt's output is byte-for-byte identical from here on out, which is just not reasonable.
  
  

[dsnet]

Also, note there are two classes of gofmt changes:

• A relaxation on what is allowed. CL/66130 is an example where gofmt permits something that was formerly forbidden.
• A restriction on what is allowed. CL/71990 is an example where gofmt forbids something that was formerly permitted.

For the upcoming release, we have both.

[dgryski]

For migration of "breaking" gofmt changes, could we use something to how vendoring was introduced? Behaviour defaults to off, enable with environment variable, then switch to behaviour defaults to on, disable with environment variable, then remove environment variable?

[rsc]

Closed in favor of #22695.