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

x/tools/cmd/goimports: inconsistency with go fmt #37584

Closed
ghost opened this issue Feb 29, 2020 · 2 comments
Closed

x/tools/cmd/goimports: inconsistency with go fmt #37584

ghost opened this issue Feb 29, 2020 · 2 comments
Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@ghost
Copy link

ghost commented Feb 29, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14 freebsd/amd64

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ryan/.cache/go-build"
GOENV="/home/ryan/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="freebsd"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="freebsd"
GOPATH="/home/ryan/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/freebsd_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build464992684=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Used the tools go fmt and goimports on the following file (a minimal test case):

package main

func f() {
	var d = struct {
		Shorty int
		Short int
		VeryMuchLongerFieldName int
	}{
		Shorty: 5,
		Short: 5,
		VeryMuchLongerFieldName: 5,
	}
}

Go fmt converts it to:

package main

func f() {
	var d = struct {
		Shorty                  int
		Short                   int
		VeryMuchLongerFieldName int
	}{
		Shorty:                  5,
		Short:                   5,
		VeryMuchLongerFieldName: 5,
	}
}

Goimports converts it to:

package main

func f() {
	var d = struct {
		Shorty                  int
		Short                   int
		VeryMuchLongerFieldName int
	}{
		Shorty: 5,
		Short:  5,
		VeryMuchLongerFieldName: 5,
	}
}

The two tools disagree. This results in them always trying to reformat each others' code.

Goimports starts to agree with gofmt if Shorty and Short are in the opposite order, so that the field length progresses upward; or if VeryMuchLongerFieldName is reduced to MuchLongerFieldName.

@gopherbot gopherbot added this to the Unreleased milestone Feb 29, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 29, 2020
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 29, 2020
@dmitshur
Copy link
Contributor

Did you build goimports command with Go 1.14, or is it an older binary? What does go version -m $(which goimports) print?

I can't reproduce this. I suspect your goimports binary was built with an older version of Go, where gofmt behavior was slightly different, and so it's producing formatting output consistent with that old version of Go.

Edit: Actually, I can reproduce this if I build goimports with Go 1.10. There was a change to gofmt behavior in Go 1.11 (see https://golang.org/doc/go1.11#gofmt). So your goimports binary is probably built with Go 1.10 or older. Can you confirm?

@dmitshur dmitshur added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 29, 2020
@ghost
Copy link
Author

ghost commented Feb 29, 2020

You were right, that was it. It works after updating goimports. Thanks for such a prompt response.

@ghost ghost closed this as completed Feb 29, 2020
@golang golang locked and limited conversation to collaborators Feb 28, 2021
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

2 participants