Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/gofmt: formatting improvements may break existing presubmit checks #22607

Closed
griesemer opened this issue Nov 7, 2017 · 13 comments
Closed

cmd/gofmt: formatting improvements may break existing presubmit checks #22607

griesemer opened this issue Nov 7, 2017 · 13 comments

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 7, 2017

See e.g. #18264 (comment) .

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?

@griesemer griesemer self-assigned this Nov 7, 2017
@griesemer griesemer added this to the Go1.10 milestone Nov 7, 2017
@ianlancetaylor ianlancetaylor changed the title gofmt: formatting improvements may break existing presubmit checks cmd/gofmt: formatting improvements may break existing presubmit checks Nov 7, 2017
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 7, 2017

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
Copy link
Contributor Author

@griesemer griesemer commented Nov 7, 2017

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 7, 2017

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
Copy link
Contributor Author

@griesemer griesemer commented Nov 7, 2017

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
Copy link
Member

@mvdan mvdan commented Nov 7, 2017

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 7, 2017

@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
Copy link
Member

@mvdan mvdan commented Nov 7, 2017

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

@dlsniper
Copy link
Contributor

@dlsniper dlsniper commented Nov 7, 2017

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
Copy link
Contributor Author

@griesemer griesemer commented Nov 7, 2017

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

@dsnet
Copy link
Member

@dsnet dsnet commented Nov 7, 2017

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
Copy link
Member

@dsnet dsnet commented Nov 7, 2017

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
Copy link
Contributor

@dgryski dgryski commented Nov 9, 2017

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
Copy link
Contributor

@rsc rsc commented Nov 13, 2017

Closed in favor of #22695.

@rsc rsc closed this Nov 13, 2017
@golang golang locked and limited conversation to collaborators Nov 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.