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 change between go1.13.8 and go1.14, unknown if intentional or not #37639

Closed
kevinconaway opened this issue Mar 3, 2020 · 5 comments
Assignees
Milestone

Comments

@kevinconaway
Copy link

@kevinconaway kevinconaway commented Mar 3, 2020

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

go version go1.14 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/kevin.conaway/Library/Caches/go-build"
GOENV="/Users/kevin.conaway/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/kevin.conaway/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14/libexec/pkg/tool/darwin_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=/var/folders/ft/kd6lkcqn0jb3l0jyxkqvl2zw0000gn/T/go-build005865273=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

After upgrading to go 1.14, we noticed that go fmt was reformatting some of our files that previously had not changed under go 1.13 and earlier versions.

I didn't see anything related to fmt in the release notes, is this behavior expected?

Below is a sample file that shows the issue

Run go fmt on the following file

package main

type NullCache struct{}

func (n NullCache) SetManyWithTTL(keys []string, values [][]byte, ttl time.Duration) error { return nil }

What did you expect to see?

No change in formatting

What did you see instead?

The formatting was was reformatted to

package main

type NullCache struct{}

func (n NullCache) SetManyWithTTL(keys []string, values [][]byte, ttl time.Duration) error {
        return nil
}
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 3, 2020

Thanks for reporting. I can reproduce this.

I'll copy what's written at https://pkg.go.dev/go/format:

Note that formatting of Go source code changes over time, so tools relying on consistent formatting should execute a specific version of the gofmt binary

That said, we should indeed confirm whether this was an intentional change in Go 1.14 or not. I'll apply NeedsInvestigation label to do that work.

/cc @griesemer

@dmitshur dmitshur added this to the Go1.15 milestone Mar 3, 2020
@dmitshur dmitshur changed the title cmd/go: go fmt changes in 1.14 cmd/gofmt: formatting change between go1.13.8 and go1.14, unknown if intentional or not Mar 3, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 3, 2020

I'll point out that formatting the input file with Go 1.14 produces an output that is gofmt-compatible with previous versions of Go (1.13.x and 1.12.x). (If it hadn't, that would make it very obvious that the change is unintentional.)

@kevinconaway
Copy link
Author

@kevinconaway kevinconaway commented Mar 3, 2020

Thanks. I should clarify that this isn't a problem for us apart from having to reformat a handful of places in our code base where this pattern occurs.

I just wanted to bring this up in case it wasn't intentional.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 3, 2020

Thanks for making that clear, and for reporting this!

@dmitshur dmitshur self-assigned this Mar 4, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 4, 2020

I've investigated this.

This is caused by CL 188818, which was an intentional change in order to fix issue #28082. /cc @eliben @mvdan

The single-line SetManyWithTTL method definition in the snippet is 105 characters, which is slightly over 100, and gets formatted into a multi-line definition. That is one of the side-effects of that CL, and as described in the commit message of CL 188818, it was deemed acceptable.

Closing since cmd/gofmt is working as intended and there's nothing to do here.

Thanks again for reporting this issue @kevinconaway!

@dmitshur dmitshur closed this Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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