search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.04k stars 17.54k forks source link

cmd/gofmt: struct fields not aligned when interrupted by a multi-line expression #31431

Closed hollowaykeanho closed 5 years ago

hollowaykeanho commented 5 years ago

I'm getting a werid go fmt -w -s output. The line before a multi-line is not aligned properly in a struct list. In my case, my inID is way off the column alignments, consistently with the same patterns.

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

$ go version
go version go1.12.1 linux/amd64

Does this issue reproduce with the latest release?

Yet to explore 1.12.3 just release

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

go env Output
$ go env
GOARCH="amd64"
GOBIN="/home/u0/bin"
GOCACHE="/home/u0/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/u0"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/u0/Documents/gosandbox/go.mod"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build565099213=/tmp/go-build -gno-record-gcc-switches"

What did you do?

When applying $ gofmt -w -s . to the source code, the line before a multi-line was formatted into a weird, not aligning to the correct column. It is very consistent for all the lines right before their respective next multi-lines code.

It happens when I was working on a large struct list for table-driven testing. No issue with execution.

What did you expect to see?

                {
                        inID:          0,
                        inAction: (AsymmetricEncryptAction |
                                AsymmetricDecryptAction),
                        inBadRand:     false,
                        outError:      false,
                        outPublicKey:  true,
                        outPrivateKey: true,
                }, {
                        inID:          1,
                        inAction:      AsymmetricEncryptAction,
                        inBadRand:     false,
                        outError:      false,
                        outPublicKey:  true,
                        outPrivateKey: true,
                }, {
                        inID:          2,
                        inAction:      AsymmetricDecryptAction,
                        inBadRand:     false,
                        outError:      false,
                        outPublicKey:  true,
                        outPrivateKey: true,
                }, {
                        inID:          3,
                        inAction:      SignAction | VerifyAction,
                        inBadRand:     false,
                        outError:      false,
                        outPublicKey:  true,
                        outPrivateKey: true,

What did you see instead?

                {
                        inID: 0,
                        inAction: (AsymmetricEncryptAction |
                                AsymmetricDecryptAction),
                        inBadRand:     false,
                        outError:      false,
                        outPublicKey:  true,
                        outPrivateKey: true,
                }, {
                        inID:          1,
                        inAction:      AsymmetricEncryptAction,
                        inBadRand:     false,
                        outError:      false,
                        outPublicKey:  true,
                        outPrivateKey: true,
                }, {
                        inID:          2,
                        inAction:      AsymmetricDecryptAction,
                        inBadRand:     false,
                        outError:      false,
                        outPublicKey:  true,
                        outPrivateKey: true,
                }, {
                        inID:          3,
                        inAction:      SignAction | VerifyAction,
                        inBadRand:     false,
                        outError:      false,
                        outPublicKey:  true,
                        outPrivateKey: true,
bcmills commented 5 years ago

I don't see any difference between those two snippets. What am I missing?

For gofmt issues especially, please post a link from https://play.golang.org: the Playground has gofmt built in, so that makes it easy to confirm the formatting for at least one version of gofmt.

crvv commented 5 years ago

The difference is in the second line

                        inID:          0,
                        inID: 0,

This looks like an intended change happened not long ago.

bcmills commented 5 years ago

See previously #22852 (CC @griesemer).

hollowaykeanho commented 5 years ago

Currently I have the source code (subject to change when I commit Argon2 cipher) here: https://gitlab.com/ZORALab/cerigo/blob/next/crypto/internal/ciphers/nacl_test.go#L2883

Reconfirmed on Playground: https://play.golang.org/p/5cOTMOiihGx

Let me know what can I contribute back. I'm currently working on a cryptography manager for my toolbox.

p/s: Sorry for the late reply, I was hiking 2 mountains this morning.

griesemer commented 5 years ago

This is working as expected. The line length differences between 2nd and 3rd line are large enough for alignment to be (on purpose) broken. As an aside, what you were expecting to see seems also inconsistent since the 3rd line would have to be aligned as well; so your expectation is inconsistent anyway.

Closing.

hollowaykeanho commented 5 years ago

@griesemer ,

The 3rd line is expected since we are breaking a long line unto multiple line and I'm currently using tab, not space. After all, the slightly indent and the OR bar indicates a multi-line. 3rd line doesn't need any amendment.

The 1st line (id) is way outside of the expectation when the list is long (new link: https://gitlab.com/ZORALab/cerigo/blob/next/crypto/manager/internal/ciphers/nacl_test.go#L2299).

I'm okay if this is accepted expectation given the weight of the issue is very small (annoyance over malfunction).

griesemer commented 5 years ago

@hollowaykeanho Thanks for the longer example. Again, the algorithm looks at the current line (actually a statistical average of line lengths of lines formatted so far) and the next line length and then decides whether to maintain alignment or not. At a certain threshold, alignment is not maintained. The thinking behind this is that a single overlong map key in a long map shouldn't force every other line to be aligned the same, and then causing all keys to be moved far to the right (and possible out of sight, depending on how your editor is configured).

It's a heuristic that sometimes fails. Your specific example could probably be fixed trivially, by simply turning the threshold knob (a hardwired constant) a bit. But then it will just fail elsewhere. Note that this happens every time your next entry is much longer than the one that's not aligned (inID = 0, 6, 9).

Maybe the threshold should be dynamically determined, but that would require some look-ahead or analysis of the table which will slow things down.

This is working fine most of the time; I'm not sure it's worth the complexity to make it work in all cases.

You can look at that specific decision making code here.

hollowaykeanho commented 5 years ago

After briefly went through the source codes, I agreed with you: it's an expensive cosmetic amendment.

I suggest when this issues got itself a bunch of +1 then only worth looking at it. For now, let's keep the issue closed.

On Sat, Apr 20, 2019, 01:47 Robert Griesemer notifications@github.com wrote:

@hollowaykeanho https://github.com/hollowaykeanho Thanks for the longer example. Again, the algorithm looks at the current line (actually a statistical average of line lengths of lines formatted so far) and the next line length and then decides whether to maintain alignment or not. At a certain threshold, alignment is not maintained. The thinking behind this is that a single overlong map key in a long map shouldn't force every other line to be aligned the same, and then causing all keys to be moved far to the right (and possible out of sight, depending on how your editor is configured).

It's a heuristic that sometimes fails. Your specific example could probably be fixed trivially, by simply turning the threshold knob (a hardwired constant) a bit. But then it will just fail elsewhere. Note that this happens every time your next entry is much longer than the one that's not aligned (inID = 0, 6, 9).

Maybe the threshold should be dynamically determined, but that would require some look-ahead or analysis of the table which will slow things down.

This is working fine most of the time; I'm not sure it's worth the complexity to make it work in all cases.

You can look at that specific decision making code here https://golang.org/src/go/printer/nodes.go?#L224.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/31431#issuecomment-484968553, or mute the thread https://github.com/notifications/unsubscribe-auth/ABXMLNDRG2VESHNPMH7NZ23PRIATTANCNFSM4HFMT52A .