go/format: provide convenience function Gofmt to invoke installed gofmt

[rsc]

We've had reports of skew where people are using one tool that links against go/printer (or go/format, which wraps it) and writes code, and then some other tool runs gofmt and sees that the formatting is different, because gofmt has a newer/older/different version of go/printer.

We should document that tools that write code files that will be checked in should run gofmt (the program) over their output, not assume that their copy of go/printer matches. One such tool is goimports.

The alternative is that we somehow never ever again change the output of gofmt on existing code, which seems too restrictive.

See:

• cmd/gofmt: comment lines broken at new position #18782 (cmd/gofmt: comment lines broken at new position)
• proposal: cmd/gofmt: write official spec for gofmt #18790 (proposal: cmd/gofmt: write official spec for gofmt)
• cmd/gofmt: formatting improvements may break existing presubmit checks #22607 (cmd/gofmt: formatting improvements may break existing presubmit checks)

[dsnet]

Maybe I'm missing something, but I'm not sure how just using gofmt fixes the issue for people.

Suppose I have a repo with a presubmit check that uses Go1.9's version of gofmt. Some user submits code formatted with Go1.10's version of gofmt and then the presubmit complains since Go1.10's gofmt permits something that Go1.9's gofmt forbids.

In my opinion, the important property that gofmt must have is that you can run both gofmt-19 and gofmt-110 on some Go1.9 source code and obtain some idempotent output that neither gofmt-19 nor gofmt-110 will change again. This property ensures that it is possible to check in source code that both the current and next version of gofmt won't complain about.

[griesemer]

@dsnet The presubmit check must exec the one and only gofmt on your system. Problems arise if different gofmt's are mixed.

[dmitshur]

We should document that tools that write code files that will be checked in should run gofmt (the program) over their output, not assume that their copy of go/printer matches.

A downside of that is increased execution time. It's slower to fork and execute a new gofmt process (which has to re-read and parse the input) rather than to output .go code that is gofmt-ed.

The alternative is that we somehow never ever again change the output of gofmt on existing code, which seems too restrictive.

This implies there are only 2 alternatives to choose from: never changing gofmt behavior, or having slower formatting tools. I hope there's a better solution, one that we can still find...

In my opinion, the important property that gofmt must have is that you can run both gofmt-19 and gofmt-110 on some Go1.9 source code and obtain some idempotent output that neither gofmt-19 nor gofmt-110 will change again. This property ensures that it is possible to check in source code that both the current and next version of gofmt won't complain about.

But that makes a certain class of gofmt changes not possible. You said in #22607 (comment):

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.

So CL 71990 would not be possible if that property is followed. In other words, gofmt couldn't become more opinionated about certain things, since that wouldn't be "backwards compatible" with previous gofmt.

Edit: Actually, what I said isn't completely accurate. It would be possible, but it would require some valid gofmt-19 Go code to change in a way that gofmt-110 wouldn't complain about it. But gofmt-110 may still end up inadvertently generating valid gofmt-110 code that isn't compatible with gofmt-19, if it doesn't know what gofmt-19 is.

[dsnet]

The presubmit check must exec the one and only gofmt on your system. Problems arise if different gofmt's are mixed.

gofmt is run by many people's Travis CI scripts, which does not run locally and may differ from the gofmt used locally by a user.

So CL 71990 would not be possible if that property is followed.

For users with gofmt as a presubmit check, it would only be forbidden while that repo continues to require source code to be gofmt-19 and gofmt-110 compliant. Once they are are on gofmt-111 and later, then they can start writing code the new way.

[bradfitz]

cmd/eg is also affected, I believe. (@alandonovan)

This implies there are only 2 alternatives to choose from: never changing gofmt behavior, or having slower formatting tools. I hope there's a better solution, one that we can still find...

I share your general sadness but I also can't think of anything. Suggestions welcome.

[cespare]

Couldn't we just as well document that tools that output Go source code should be rebuilt whenever Go is updated? I think people are already used to needing to do this for tools that take Go source as input (for instance, adding aliases in 1.9 broke a bunch of tools that were built with older versions of Go.)

At our company we use a few different tools that output gofmted Go source and I don't think we'll want them to run gofmt. I think these tools should even be able to run on systems that don't have Go (and hence gofmt) installed.

[rsc]

I suggest that along with this documentation we add a new function to go/format:

func Gofmt(src []byte) ([]byte, error)

It's exactly like format.Source except that it just runs gofmt.

[dmitshur]

I also can't think of anything. Suggestions welcome.

Like @dsnet, I don't quite understand how the proposed solution ("We should document that tools that write code files that will be checked in should run gofmt (the program)") fixes the stated problem ("We've had reports of skew"). But mostly, I don't have enough information about the problem to evaluate solutions or suggest better ones. I would like to understand this issue better, because the downsides of the suggested solution worry me.

As @cespare said, I don't see how behavior of gofmt differs compared to a go/format-using tool that was built using the same version of Go as gofmt comes from.

Is this skew between different binaries and versions of Go on a local computer? Or across different people/computers/teams?

We've had reports of skew

@rsc I think it would be very helpful to point to more details about the specific skew. There are 3 linked issues, that contain various comments, but it's really hard to find specific information there.

[rsc]

It is completely untenable to require that all future versions of gofmt must preserve the output of all past versions of gofmt. I think it is also deeply problematic to have gofmt -go1.9, gofmt -go1.10, and so on.

Maybe I'm missing something, but I'm not sure how just using gofmt fixes the issue for people.

Yes, there is a second half to this, which is that local presubmit checks are fine (and should use the installed gofmt) but post-submit CI failures are typically not. It should not be the case that updating the gofmt on the CI machine starts making some kind of automated "gofmt-ed" test fail. Our position is don't write tests like that. Treat running gofmt over your source as best-effort during submit but not something to be reverified automatically after submit.

Inside Google we arrange that the presubmit check is always using exactly the same gofmt binary as used by engineers on their workstations, so that there is one single point of truth. And there is no post-submit failure if you somehow get past the presubmit checks. Similarly, in the open source Go repo, git codereview shells out to gofmt for its check, and go fmt shells out to gofmt for its reformatting. Neither assumes that they have the right go/printer linked in. This makes the gofmt binary the single point of truth in that situation as well.

To reply to @cespare, it's fine for your tools to use go/printer as long as they do not have to produce exactly gofmt-ed source code. (Whether they do depends on how fussy your presubmits are, whether you have (not recommended) post-submit checks, and whether you can run gofmt before checking any generated code in.)

I'm working on the release notes, and immediately following the text about gofmt changes, my current draft text is:

Note that these kinds of minor changes like these are expected from time to time. In general, we recommend against building systems that are sensitive to the exact version of gofmt or go/printer being used. For example, a continuous integration test that fails if any code already checked into a repository is not “properly formatted” is inherently fragile and not recommended.

When multiple programs must agree about the exact details of go formatting, they should arrange to share a single point of truth by all invoking the same gofmt binary. For example, in the Go open source repository, we arrange for goimports and our Git pre-commit hook to agree about source code formatting by having both invoke the gofmt binary found in the current path.

(Of course we have to make that last sentence true.)

[rsc]

The short version is this: Gofmt output must be expected to change in minor ways from time to time. The implication is that people must either (1) not build systems that care which version of gofmt is being used, or (2) make sure that systems that do care all agree which gofmt is being used. The point of the examples is to share what we've been using for these past many years that has been working well for us.

[rsc]

It is completely untenable to require that all future versions of gofmt must preserve the output of all past versions of gofmt.

It's actually worse than I thought. Since skew can in theory go in either direction depending on which thing in the skew updates first, all past versions of gofmt also have to accept/preserve the output of all future versions of gofmt. So basically no changes anymore ever. I understand the appeal of that, but that's not practical.

For example one of the changes in Go 1.10 is to format x[i+1:j:k] as x[i+1 : j : k] instead of what Go 1.9 did: the obviously wrong x[i+1 : j:k]. Instead of accepting that badly formatted expression forever because some people have set up fragile post-submit formatting checks (as in #18782), my hope is that we can explain best practices for setting up non-fragile checks.

[dmitshur]

Gofmt output must be expected to change in minor ways from time to time. The implication is that people must either (1) not build systems that care which version of gofmt is being used, or (2) make sure that systems that do care all agree which gofmt is being used.

I am in full agreement with that.

my hope is that we can explain best practices for setting up non-fragile checks.

Yes, that's a good resolution that I hope to see here. I don't want "no changes anymore ever".

I personally will accomplish this without necessarily relying on shelling out to gofmt, but that's implementation details. I think the documentation you wrote above explains the situation clearly, and gives justified rationale for the actions people must take. They should be able to figure out the exact implementation details themselves that match their preferences.

[gopherbot]

Change https://golang.org/cl/80415 mentions this issue: go/format: add Gofmt and document use

[gopherbot]

Change https://golang.org/cl/80696 mentions this issue: go/format: document use of Source better

[griesemer]

The documentation issue was addressed with CL 80696. Need to decide if we want to provide an explicit format.Gofmt call (see CL 80415). Leaving for 1.11.

[jimmyfrasche]

Piping through gofmt works if you're creating a new file, like stringer and co., or instrumenting a file where few will ever inspect the modified file directly like code coverage.

It doesn't help if you're making a change to an existing file on behalf of the user like with an editor/IDE plugin or a refactoring tool.

You could pipe the output of go/printer through gofmt, but, when there's skew, go/printer can make a change that gofmt would not have and that gofmt will not undo.

It seems like it would also be necessary to have a mode on go/printer (or a new package) that tries to use the position information in the FileSet as much as possible while doing as little pretty printing as possible so that the modified AST could either be given back to the editor/IDE more or less as it was or then piped through gofmt for the actual pretty printing.

[dlsniper]

As a small side comment / reply to the previous comment, IDEs, and in general any tool that's not capable of understanding Go's AST for Go code, will not really benefit from this, especially given the cost (and complexity) of invoking external tools to manipulate code.

[jimmyfrasche]

@dlsniper https://github.com/fatih/gomodifytags for an example.

[jimmyfrasche]

@dlsniper I see you reacted with 😕 I should have provided more context, sorry.

gomodifytags is a program used in multiple editors. https://github.com/fatih/gomodifytags#supported-editors

Its input is the contents of the file currently being edited, it manipulates the ast to add/change/remove struct tags as requested by the user, and then it outputs the modified ast via go/format, which the editor will then use as the contents of the file.

It has the issue with go/format vs gofmt version skew. However, since it has to print the ast, running the output of that through gofmt wouldn't help in general.

For example, if someone were using go1.10beta2 but hadn't recompiled gomodifytags, they could create a file containing if _, ok := v.(interface{ x() }); ok { and gofmt would let it pass. But, if they then ran gomodifytags on a struct in the same file, the 1.9 go/format would silently change it to

if _, ok := v.(interface {
    x()
}); ok {

and running that through the 1.10beta2 gofmt wouldn't change it back.

I posted because I'm working on another tool that will have the same issue and couldn't find any good options for dealing with it. (Other than copying the unmodified bits of the file verbatim while manually splicing in the changes).

[rsc]

I retract this suggestion now that we have the documentation about being careful. I worry about people calling format.Gofmt without understanding it's a subprocess call and being surprised by the cost or failure mode when gofmt isn't there.