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: struct fields not aligned when interrupted by a multi-line expression #31431

Closed
hollowaykeanho opened this issue Apr 12, 2019 · 8 comments

Comments

@hollowaykeanho
Copy link

@hollowaykeanho hollowaykeanho commented Apr 12, 2019

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
Copy link
Member

@bcmills bcmills commented Apr 12, 2019

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.

@bcmills bcmills changed the title go fmt -w -s misaligned columns values for line before a multi-lines cmd/gofmt: go fmt -w -s misaligned columns values for line before a multi-lines Apr 12, 2019
@crvv
Copy link
Contributor

@crvv crvv commented Apr 13, 2019

The difference is in the second line

                        inID:          0,
                        inID: 0,

This looks like an intended change happened not long ago.

@bcmills bcmills removed the WaitingForInfo label Apr 13, 2019
@bcmills
Copy link
Member

@bcmills bcmills commented Apr 13, 2019

See previously #22852 (CC @griesemer).

@bcmills bcmills added this to the Unplanned milestone Apr 13, 2019
@bcmills bcmills changed the title cmd/gofmt: go fmt -w -s misaligned columns values for line before a multi-lines cmd/gofmt: struct fields not aligned when interrupted by a multi-line expression Apr 13, 2019
@hollowaykeanho
Copy link
Author

@hollowaykeanho hollowaykeanho commented Apr 13, 2019

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
Copy link
Contributor

@griesemer griesemer commented Apr 15, 2019

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.

@griesemer griesemer closed this Apr 15, 2019
@hollowaykeanho
Copy link
Author

@hollowaykeanho hollowaykeanho commented Apr 19, 2019

@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
Copy link
Contributor

@griesemer griesemer commented Apr 19, 2019

@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
Copy link
Author

@hollowaykeanho hollowaykeanho commented Apr 20, 2019

@golang golang locked and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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