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: empty line doesn't reset value alignment #26352

Closed
nd opened this issue Jul 12, 2018 · 20 comments
Closed

cmd/gofmt: empty line doesn't reset value alignment #26352

nd opened this issue Jul 12, 2018 · 20 comments
Assignees
Milestone

Comments

@nd
Copy link
Contributor

@nd nd commented Jul 12, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version go1.11beta1 linux/386

Does this issue reproduce with the latest release?

yes

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

GOARCH="386"
GOBIN=""
GOCACHE="/home/nd/.cache/go-build"
GOEXE=""
GOHOSTARCH="386"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/nd/go"
GOPROXY=""
GORACE=""
GOROOT="/home/nd/p/gos/go1.10.1"
GOTMPDIR=""
GOTOOLDIR="/home/nd/p/gos/go1.10.1/pkg/tool/linux_386"
GCCGO="gccgo"
GO386="sse2"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m32 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build186038796=/tmp/go-build -gno-record-gcc-switches"
VGOMODROOT=""

What did you do?

Run gofmt on the following code (https://play.golang.org/p/BP3LQ9iKvNz):

package main

func main() {
	_ = map[int]string{
		1: "",

		12345678901234567: "",
		12345678901234567890123456789012345678901: "",
	}
}

What did you expect to see?

Empty line after 1: "" resets alignment, the first short key doesn't affect alignment of the 12345678901234567 key, values of the last 2 keys are aligned.

What did you see instead?

Empty line between elements doesn't reset alignment and values of the last 2 elements are not aligned.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Jul 12, 2018

This is an unfortunate result of other gofmt updates. It will be mentioned in the release notes. Please see #26228.

Thank you. Closing since this has been already raised before.

@agnivade agnivade closed this Jul 12, 2018
@nd
Copy link
Contributor Author

@nd nd commented Jul 12, 2018

I've seen the recent change in alignment algorithm. Its commit message states:

  1. Changed the heuristic to not simply look at the size ratio
    between current and previous line, but instead considering the
    geometric mean of the sizes of the previous (aligned) lines.

My understanding is that there should be a way to break an alignment, but current implementation doesn't provide it.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Jul 12, 2018

I will defer to @griesemer regarding this.

@nd
Copy link
Contributor Author

@nd nd commented Jul 13, 2018

Maybe a better way to put it: commit message says that the new algorithm considers a geometircal mean of the previous aligned lines. The value on the first element is not aligned with the value on the second element, but it still affects the alignment of the 3rd element. That's why I think this might be a bug.

@dominikh
Copy link
Member

@dominikh dominikh commented Jul 13, 2018

Reopening for gri's input.

@dominikh dominikh reopened this Jul 13, 2018
@agnivade agnivade added this to the Go1.11 milestone Jul 13, 2018
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 20, 2018

@griesemer is on vacation so let's bump this to Go 1.12. There's already an outstanding CL to add a note about gofmt changes in the release notes.

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jul 20, 2018
@mvdan
Copy link
Member

@mvdan mvdan commented Jul 20, 2018

This could be a regression from 1.10, since empty lines always break alignment in previous releases of Go. I've read the 542ea5a, and it doesn't seem like any of the added tests or changes in the Go tree involve empty lines, so perhaps this was simply an oversight.

On the other hand, this behavior was never documented (that I can find) and it doesn't look like there were any regression tests for it. So perhaps it was never intended as a feature. Does anyone know?

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 20, 2018

Change https://golang.org/cl/125260 mentions this issue: go/printer: make empty lines break table alignment

@mvdan
Copy link
Member

@mvdan mvdan commented Jul 21, 2018

I sent the CL above, assuming that this change in behavior was unintended. It can always be dropped if that wasn't the case.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 23, 2018

@ianlancetaylor, thoughts? (for Go 1.11?)

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 23, 2018

I don't understand what in the 1.10 code caused there to be no alignment across empty lines. Does anybody know?

I agree that I don't see anything in CL 104755 that suggests that it is intended to change the behavior of blank lines. But I don't see where the behavior of blank lines was coming from. Why do we need a blunt instrument like CL 125260?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 8, 2018

This does look like a bug to me - the intent was always that a blank line could be used to break alignment. Investigating.

@griesemer griesemer changed the title cmd/gofmt: empty line doesn't reset key alignment cmd/gofmt: empty line doesn't reset value alignment Aug 8, 2018
@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 8, 2018

@ianlancetaylor The 1.10 go/printer code doesn't have anything special that breaks alignment across empty lines - that is done by the tabwriter: It breaks alignment whenever there's an empty line. That code has not changed. The issue here is that alignment is broken between line 7 and 8 (in the example), but it wouldn't be broken if line 5 (the first map element) were commented out. And it shouldn't be broken because an empty line starts a new section of lines (which should be independent of what came before). The problem here is that go/printer doesn't "see" if the tabwriter broke the section and then doesn't reset the lnsum/count variables which are used to compute the geomean which in turn is used to decide if alignment should be broken. This was not an issue in 1.10 because there the computation only always looked at the one previous entry, not many previous entries (though there may have been issues there as well, perhaps less common).

This is clearly a bug in my mind and may need something like the "blunt" instrument mentioned above.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 8, 2018

This is actually a regression, so probably should be addressed for 1.11.

@griesemer griesemer modified the milestones: Go1.12, Go1.11 Aug 8, 2018
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 8, 2018

I'm fine with the blunt instrument and I'm fine with it being in 1.11. I just didn't understand what had happened. Thanks for the explanation.

@gopherbot gopherbot closed this in c116265 Aug 9, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 14, 2018

Change https://golang.org/cl/129255 mentions this issue: go/printer: revert "make empty lines break table alignment"

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 14, 2018

Change https://golang.org/cl/129256 mentions this issue: go/printer: consider empty lines in table layout computation

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 14, 2018

Reopened since we reverted the original fix.

@griesemer griesemer reopened this Aug 14, 2018
gopherbot pushed a commit that referenced this issue Aug 14, 2018
This reverts commit c116265.

The change, while addressing issue #26352, introduced another
regression (#26930), which is worse. Reverting this change in
favor of a better fix for the original issue.

Updates #26352.
Fixes #26930.

Change-Id: I71ad12a8212992cce5c1e73907d1f7460f98d9e8
Reviewed-on: https://go-review.googlesource.com/129255
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot gopherbot closed this in c882f4b Aug 14, 2018
@dvyukov
Copy link
Member

@dvyukov dvyukov commented Aug 15, 2018

  In general, systems that need consistent formatting of Go source code should
  use a specific version of the <code>gofmt</code> binary.

Can somebody please clarify what exactly does this mean for an open-source project on github with external contributors and travis build against several Go versions? Is there an example of handing this?

So far it all looks broken for everybody. I can make it work for myself. But all external pull requests break build and can't be merged.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 15, 2018

@dvyukov Can you please comment on #22695 rather than this issue (which has been fixed and closed, and as such doesn't change gofmt anymore, at least not as alleged in this issue).

Specifically see: #22695 (comment)

@golang golang locked and limited conversation to collaborators Aug 15, 2019
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
9 participants
You can’t perform that action at this time.