Skip to content

cmd/gofmt: formatting change between go1.13.8 and go1.14, unknown if intentional or not #37639

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.

Sign up for GitHub

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

Jump to bottom
Closed
kevinconaway opened this issue Mar 3, 2020 · 5 comments
Closed

cmd/gofmt: formatting change between go1.13.8 and go1.14, unknown if intentional or not #37639

kevinconaway opened this issue Mar 3, 2020 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.15

Comments

@kevinconaway
Copy link

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
Contributor

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 3, 2020
@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
Contributor

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

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
Contributor

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
Contributor

dmitshur commented Mar 4, 2020 •
edited
Loading

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 as completed Mar 4, 2020
@golang golang locked and limited conversation to collaborators Mar 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
3 participants
@dmitshur @kevinconaway @gopherbot