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

gofumpt's behaviour depends on the version of Go used to build it #244

Closed
adamroyjones opened this issue Sep 1, 2022 · 10 comments · Fixed by #255
Closed

gofumpt's behaviour depends on the version of Go used to build it #244

adamroyjones opened this issue Sep 1, 2022 · 10 comments · Fixed by #255

Comments

@adamroyjones
Copy link
Contributor

adamroyjones commented Sep 1, 2022

I'm finding that the behaviour of gofumpt is sensitive to the version of Go used to build the binary. It also looks to not fully respect the -lang flag (ed.: this was a misreading on my part; see the discussion below). A reproducing example follows.

This was first raised here; I misattributed things.

(I'll try and fix the problem rather than only whinge like an ingrate, but I think it's a good idea to raise the issue here first.)

Reproducing example

Preparing

Prepare an empty directory with the following files:

// main.go
package main

// Hello, world.
//
// - This will be modified by `gofumpt` if the version of Go is 1.19, but not
//   if the version is 1.18.
func main() {
}
# docker-compose.yml
services:
  gofumpt18:
    image: golang:1.18-bullseye
    stdin_open: true
    tty: true
    volumes:
      - .:/go/src/

  gofumpt19:
    image: golang:1.19-bullseye
    stdin_open: true
    tty: true
    volumes:
      - .:/go/src/

Then run:

docker compose up -d

Installing via go install

docker compose exec gofumpt18 bash

go install mvdan.cc/gofumpt@latest
cd /go/src
gofumpt -d main.go # No differences.
gofumpt -lang "1.19" -d main.go # No differences.
gofumpt -lang "1.18" -d main.go # No differences.
exit
docker compose exec gofumpt19 bash

go install mvdan.cc/gofumpt@latest
cd /go/src
gofumpt -d main.go # Differences.
gofumpt -lang "1.19" -d main.go # Differences.
gofumpt -lang "1.18" -d main.go # Differences.
exit

Building with Go 1.18, running in a Go 1.19 context

docker compose exec gofumpt18 bash

cp $(which gofumpt) /go/src
exit
docker compose exec gofumpt19 bash

cd /go/src
./gofumpt -d main.go # No differences.
./gofumpt -lang "1.19" -d main.go # No differences.
./gofumpt -lang "1.18" -d main.go # No differences.
exit

Building with Go 1.19, running in a Go 1.18 context

docker compose exec gofumpt19 bash

cp $(which gofumpt) /go/src
exit
docker compose exec gofumpt18 bash

cd /go/src
./gofumpt -d main.go # Differences.
./gofumpt -lang "1.19" -d main.go # Differences.
./gofumpt -lang "1.18" -d main.go # Differences.
exit

Cleaning up

docker compose down
@mvdan
Copy link
Owner

mvdan commented Sep 1, 2022

I'm finding that the behaviour of gofumpt is sensitive to the version of Go used to build the binary.

For better or worse, that will always be the case, and the same applies to gofmt - both rely on go/printer, which implements the "canonical" gofmt formatting. If that changes between 1.18 and 1.19, then gofumpt will change depending on what Go version you built it with.

In particular, what you are seeing is https://go-review.googlesource.com/c/go/+/384264/, a go/printer change for golang/go#51082 which is documented in the 1.19 release notes.

There is literally no way to work around this without having one fork of go/printer per major Go version, and that's not something I am contemplating to do. Beyond being an ever-increasing amount of work for me forever, and making gofumpt larger and larger in code and binary size, it's just a very heavyweight approach to how -lang should work. Put simply, if someone really wants old Go formatting behavior, they can always use that older Go version to build the formatting tool. But I also don't expect this scenario to be very common.

I know that for reformatting godoc comments it may seem obvious to just disable that bit of code if -lang is older than 1.19, but in many cases it's not nearly that simple. A few releases ago, go/printer changed its heuristic for how it aligns inline comments vertically. Without keeping two copies of go/printer, the only alternative I would have is to manually add a -lang conditional that implements both heuristics at once.

I think perhaps -lang is not very well documented if you got confused by this situation. It is meant to be a language version, i.e. what Go language spec features it can rely on for formatting. For example, it will only rewrite 0777 to 0o777 if your -lang version (by default derived from go 1.X in go.mod) is new enough to support octal literals with the 0o prefix, since that was added in Go 1.13. I can imagine other rules or simplifications which would rely on newer syntax changes in the future.

So the flag was never intended to mean "format exactly like Go version 1.x" - hopefully that's clear. I'd be interested to hear where you got that impression, so I can improve the docs :)

@mvdan
Copy link
Owner

mvdan commented Sep 1, 2022

It could always be possible for gofumpt to give an error if its -lang version is not the same as the Go version that built the binary, but that would feel overly restrictive in my opinion. Right now, I can use a recent gofmt on any Go project, even if it was written with Go 1.12. I should be able to do the same with gofumpt - it is a drop-in replacement for gofmt, after all. And gofmt will still apply those godoc comment rewrites to older projects as well.

@adamroyjones
Copy link
Contributor Author

Thanks for the quick, detailed, and illuminating reply—I appreciate it. Thanks for your work on gofumpt, as well. It's a wonderful tool.


Put simply, if someone really wants old Go formatting behavior, they can always use that older Go version to build the formatting tool. But I also don't expect this scenario to be very common.

The triggering scenario for me was the following; I believe you're right about its rarity.

My employer has a repository that requires the use of Go 1.18. This repository has a CI pipeline that uses golangci-lint. The binaries that are distributed with golangci-lint are built using Go 1.19:

go version -v $(which golangci-lint)

This inconsistency can cause CI pipelines to fail.

There are obvious solutions to that immediate problem, of course; I'll build gofumpt locally with Go 1.19 and use that.


I think perhaps -lang is not very well documented if you got confused by this situation. It is meant to be a language version, i.e. what Go language spec features it can rely on for formatting [...] the flag was never intended to mean "format exactly like Go version 1.x" - hopefully that's clear. I'd be interested to hear where you got that impression, so I can improve the docs :)

It was a breezy, optimistic assumption on my part. The documentation I saw was from the struct definition of format.Options here, which says that:

LangVersion corresponds to the Go language version a piece of code is written in. The version is used to decide whether to apply formatting rules which require new language features.

I didn't realise that this meant that the version is only used for that purpose; I assumed more than was written. It's a behaviour that resists a pithy explanation, though.


You're, of course, right that it'd be an unsustainable and unreasonable burden on you to maintain forks of go/printer. It'd also bloat and introduce fragility to the (lean!) codebase to start jamming in logic that tries to handle all possible versions of lang, all changing behaviours of go/printer, and so on.

One possibility would be for releases of gofumpt to be built and distributed on GitHub. This'd dampen down the variation in behaviour, but that likewise introduces a new burden—one that I'd be happy to share, if you'd benefit from that.

@mvdan
Copy link
Owner

mvdan commented Sep 1, 2022

Thanks for being understanding! This is a tricky situation so I understand where you were coming from. I actually made a proposal to upstream to provide versioned gofmt downloads for a very similar reason, so that at a previous company we could all use exactly the same Go formatting even if some of the developers were on slightly newer Go versions: golang/go#26397

My employer has a repository that requires the use of Go 1.18. This repository has a CI pipeline that uses golangci-lint. The binaries that are distributed with golangci-lint are built using Go 1.19:

Arguably, that may be a problem for more than just formatting. If your work project builds and tests with Go 1.18, you should also use your tools (formatting, static analysis, editors with LSP, etc) with 1.18 as well. Using 1.19 may lead to subtle differences in behavior beyond just formatting.

I didn't realise that this meant that the version is only used for that purpose; I assumed more than was written.

I'll think about making the docs clearer - I'll ping you for a PR review if that's OK.

One possibility would be for releases of gofumpt to be built and distributed on GitHub.

Personally, the only reason I thought the project didn't need release binaries is because it is a tool for Go developers. For them, I imagine that running go install mvdan.cc/gofumpt@latest is easier than running a long curl command and placing the binary in the right place. go install also has the ability of automatically figuring out what the latest version is, for example.

Perhaps you're right that each gofumpt release should advocate what Go version it mainly targets. https://github.com/mvdan/gofumpt/releases/tag/v0.3.0 did say it was based on Go 1.18's gofmt, and that relates to code in cmd/gofmt like its flags, but not to go/printer. Perhaps the release notes should clarify that each version recommends to be built with a certain Go version, and provide binaries to make that a bit easier for some use cases like CI. Other Go versions will still most likely work, but with varying degrees of different formatting results.

@adamroyjones
Copy link
Contributor Author

Arguably, that may be a problem for more than just formatting. If your work project builds and tests with Go 1.18, you should also use your tools (formatting, static analysis, editors with LSP, etc) with 1.18 as well. Using 1.19 may lead to subtle differences in behavior beyond just formatting.

Yes: as good as Go is with its backward-compatibility guarantees, it'd be better if everything were on the same version. I'll have a look into what I can do.

I'll think about making the docs clearer - I'll ping you for a PR review if that's OK.

By all means!

Perhaps you're right that each gofumpt release should advocate what Go version it mainly targets.

I think that'd definitely help. An additional possibility would be for gofumpt to print a warning if the version used to build it differs from the recommended version. Would you welcome a PR that adds that functionality?

@mvdan
Copy link
Owner

mvdan commented Sep 1, 2022

I'm not sure about the warning. Without any flags, when running gofumpt on its own codebase today with Go 1.19, it would default to -lang=1.18, derived from go.mod. Note that it's 1.18 even though I develop and build on Go 1.19, because I only need language features from 1.18 onwards. In other words, 1.18 is the minimum version of Go I support (language-wise), but then I currently target 1.19. And the target constantly moves too; when 1.20 is out, I may start using that, but I might not rush to drop support for 1.18 right away.

@adamroyjones
Copy link
Contributor Author

adamroyjones commented Sep 1, 2022

Yes, quite right—sorry. It's all a bit tricky! Perhaps the least bad solution would be:

  • for each gofumpt tag, provide a range of supported Go versions (which looks to be the case now);
  • for each gofumpt tag, each corresponding supported Go version, and each supported platform, build a corresponding binary with that version of Go and distribute it on GitHub;
  • add a bit of documentation in the readme that highlights that the behaviour of gofumpt depends on the version of Go it was built with; and
  • add a small clarifying note in the documentation that emphasises that the lang option doesn't try to fully "impersonate" that version of Go.

I'll defer to you and your judgement, of course. I'm happy to try and help however I can, whether it's writing documentation, reviewing things, or working on a pipeline.

@mvdan
Copy link
Owner

mvdan commented Sep 8, 2022

Your points about the docs are certainly a good idea - happy to review a PR :)

I'm less convinced that providing so many gofumpt binaries will be a net benefit. My guess is that it would confuse users more than anything. We should recommend using a gofumpt version with one major version of Go, and that's easy enough. If a project absolutely cannot use that Go version just yet, they can either use our prebuilt binary, or they can choose to stay on a slightly older version for a bit.

I also just realised that having gofumpt's formatting be independent of the Go version isn't such a difficult task. Yes, we would have to keep a vendored copy of go/printer, and potentially other packages which affect its behavior like go/doc/comment, but we only need to keep one copy as long as -lang keeps its current behavior to only affect the use of Go language features.

Part of me still thinks that's nasty, as it would lead to some downstreams like gopls shipping multiple copies of the same packages - often for no benefit at all, if the versions are almost exactly the same.

mvdan added a commit that referenced this issue Sep 27, 2022
Note that we're starting to recommend a particular Go version for
building gofumpt, as that is the go/printer version we've tested the
most with, and a consistent version will also bring consistent behavior.

See #244.
@adamroyjones
Copy link
Contributor Author

I'm less convinced that providing so many gofumpt binaries will be a net benefit. My guess is that it would confuse users more than anything. We should recommend using a gofumpt version with one major version of Go, and that's easy enough. If a project absolutely cannot use that Go version just yet, they can either use our prebuilt binary, or they can choose to stay on a slightly older version for a bit.

I think you're right, having reflected on it; I think providing a single simple recommendation makes the most sense. Opinionated tools are, after all, opinionated.

@mvdan
Copy link
Owner

mvdan commented Oct 17, 2022

I think I do want to go in the direction of gofumpt versioning its use of go/printer. It will make using it as a library slightly more bloated, but so be it. If users can't trust on a pinned gofumpt version to behave consistently, then the tool isn't particularly useful. gofmt does not have this problem as it ships with Go itself.

mvdan added a commit that referenced this issue Jan 1, 2023
These two packages affect the formatting of gofmt, and by extension
gofumpt as well. For example, most changes in formatting to gofmt are
done in go/printer, as it implements most of the formatting logic.

This tight coupling between gofmt and std packages like go/printer
is not a problem for gofmt, as gofmt is released with Go itself.
It is very unlikely that any user would build a version of gofmt with a
different version of those std packages.

gofumpt is a separate Go module, so when users `go install` it, they may
currently be using Go 1.18.x or 1.19.x. This can cause problems, as
choosing a different major version can easily lead to different behavior.

To get the same tight coupling that gofmt has, vendor the two libraries
which affect formatting. go/doc/comment is also in this bag since it
affects how gofmt reformats godoc comments.

Note that this vendoring is not ideal, as it adds code duplication.
This shouldn't bloat the size of gofumpt binaries, as they simply swap
out the original packages for the vendored ones.

For users of the mvdan.cc/gofumpt/format library such as gopls,
they may get duplication if they use packages like go/printer elsewhere.
This is a small amount of unfortunate bloat, but I don't see a better
way to ensure that each version of gofumpt behaves in a consistent way.

Note that we're not vendoring std packages for parsing Go code like
go/parser or go/token, and neither are we vendoring go/ast.
If a project uses new Go syntax, such as generics, they will still need
to build gofumpt with a new enough version of Go.

The copied code comes from Go 1.19.4 and will be updated regularly.
The copying was done manually this time, skipping test files and
rewriting imports as necessary. In the future, we an automate it.
Also note that we ran gofumpt on the copied code.

Fixes #244.
@mvdan mvdan closed this as completed in #255 Jan 7, 2023
mvdan added a commit that referenced this issue Jan 7, 2023
These two packages affect the formatting of gofmt, and by extension
gofumpt as well. For example, most changes in formatting to gofmt are
done in go/printer, as it implements most of the formatting logic.

This tight coupling between gofmt and std packages like go/printer
is not a problem for gofmt, as gofmt is released with Go itself.
It is very unlikely that any user would build a version of gofmt with a
different version of those std packages.

gofumpt is a separate Go module, so when users `go install` it, they may
currently be using Go 1.18.x or 1.19.x. This can cause problems, as
choosing a different major version can easily lead to different behavior.

To get the same tight coupling that gofmt has, vendor the two libraries
which affect formatting. go/doc/comment is also in this bag since it
affects how gofmt reformats godoc comments.

Note that this vendoring is not ideal, as it adds code duplication.
This shouldn't bloat the size of gofumpt binaries, as they simply swap
out the original packages for the vendored ones.

For users of the mvdan.cc/gofumpt/format library such as gopls,
they may get duplication if they use packages like go/printer elsewhere.
This is a small amount of unfortunate bloat, but I don't see a better
way to ensure that each version of gofumpt behaves in a consistent way.

Note that we're not vendoring std packages for parsing Go code like
go/parser or go/token, and neither are we vendoring go/ast.
If a project uses new Go syntax, such as generics, they will still need
to build gofumpt with a new enough version of Go.

The copied code comes from Go 1.19.4 and will be updated regularly.
The copying was done manually this time, skipping test files and
rewriting imports as necessary. In the future, we an automate it.
Also note that we ran gofumpt on the copied code.

Fixes #244.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants