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: address formatting oddities of composite literals with irregularly sized entries #22852

Closed
matthewhartstonge opened this issue Nov 22, 2017 · 6 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@matthewhartstonge
Copy link

matthewhartstonge commented Nov 22, 2017

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

1.9.2

Does this issue reproduce with the latest release?

Yes

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

Windows 10 (1709) 64-bit

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\matt\go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\matt\AppData\Local\Temp\go-build832937591=/tmp/go-build -gno-record-gcc-switches
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config

What did you do?

Created maps with long keys.
^ Hit the playground's format button to reproduce the issue

What did you expect to see?

Blocks formatted consistently

What did you see instead?

Broken Indentation blocks when running Go Fmt.
^ Hit the playground's format button to reproduce the issue

@bradfitz
Copy link
Contributor

This is by design. Search the issue tracker where @griesemer has explained it a few times.

In general, the best strategy to being happy about gofmt's choices is to not worry about it.

Maybe this needs a FAQ entry. I'll let @griesemer decide whether to keep this bug open as a documentation bug.

@griesemer
Copy link
Contributor

Fine to leave this open as documentation issue.

@griesemer griesemer modified the milestones: Unplanned, Go1.11 Jan 3, 2018
@griesemer
Copy link
Contributor

Looking at this again, I admit that the formatting looks pretty ugly in this case. Also, given that this very issue has come up so many times, it may be worthwhile re-investigating the formatting of literals of this form. There's a few solutions I see:

  • don't do anything (doesn't address the issue) [trivial]
  • improve the heuristics (e.g., don't change alignment if there's only a few shorter entries) [hard]
  • align all entries, and leave it to programmer to introduce empty lines to break alignment [easy]

@griesemer griesemer changed the title cmd/gofmt: Indentation oddities with long keys cmd/gofmt: address formatting oddities of composite literals with irregularly sized entries Jan 3, 2018
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. early-in-cycle A change that should be done early in the 3 month dev cycle. and removed Documentation labels Jan 3, 2018
@matthewhartstonge
Copy link
Author

matthewhartstonge commented Jan 4, 2018

In my opinion, the align all option would be easier for programmers to reason with and understand. 👍

@gopherbot
Copy link

Change https://golang.org/cl/104755 mentions this issue: go/printer, gofmt: tuned table alignment for better results

@gopherbot
Copy link

Change https://golang.org/cl/104836 mentions this issue: cmd/coordinator: remove gofmt inconsistency

gopherbot pushed a commit to golang/build that referenced this issue Apr 5, 2018
Issue golang/go#22852 modifies formatting for Go tables. If the
code stays as is, users running Go 1.9 will format the file one way
and users running tip will format the file a different way. This is
problematic when git-codereview attempts to check the file is gofmt'ed
correctly, and does not let you mail commits that aren't.

Adding another newline makes the file compatible with tip and the
stable release branch and avoids this problem.

Change-Id: I0dbfe01db6e07578993e793e3710702ff2518436
Reviewed-on: https://go-review.googlesource.com/104836
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
The go/printer (and thus gofmt) uses a heuristic to determine
whether to break alignment between elements of an expression
list which is spread across multiple lines. The heuristic only
kicked in if the entry sizes (character length) was above a
certain threshold (20) and the ratio between the previous and
current entry size was above a certain value (4).

This heuristic worked reasonably most of the time, but also
led to unfortunate breaks in many cases where a single entry
was suddenly much smaller (or larger) then the previous one.

The behavior of gofmt was sufficiently mysterious in some of
these situations that many issues were filed against it.

The simplest solution to address this problem is to remove
the heuristic altogether and have a programmer introduce
empty lines to force different alignments if it improves
readability. The problem with that approach is that the
places where it really matters, very long tables with many
(hundreds, or more) entries, may be machine-generated and
not "post-processed" by a human (e.g., unicode/utf8/tables.go).

If a single one of those entries is overlong, the result
would be that the alignment would force all comments or
values in key:value pairs to be adjusted to that overlong
value, making the table hard to read (e.g., that entry may
not even be visible on screen and all other entries seem
spaced out too wide).

Instead, we opted for a slightly improved heuristic that
behaves much better for "normal", human-written code.

1) The threshold is increased from 20 to 40. This disables
the heuristic for many common cases yet even if the alignment
is not "ideal", 40 is not that many characters per line with
todays screens, making it very likely that the entire line
remains "visible" in an editor.

2) 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.
This emphasizes the "overall picture" of the previous lines,
rather than a single one (which might be an outlier).

3) Changed the ratio from 4 to 2.5. Now that we ignore sizes
below 40, a ratio of 4 would mean that a new entry would have
to be 4 times bigger (160) or smaller (10) before alignment
would be broken. A ratio of 2.5 seems more sensible.

Applied updated gofmt to all of src and misc. Also tested
against several former issues that complained about this
and verified that the output for the given examples is
satisfactory (added respective test cases).

Some of the files changed because they were not gofmt-ed
in the first place.

For golang#644.
For golang#7335.
For golang#10392.
(and probably more related issues)

Fixes golang#22852.

Change-Id: I5e48b3d3b157a5cf2d649833b7297b33f43a6f6e
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
The go/printer (and thus gofmt) uses a heuristic to determine
whether to break alignment between elements of an expression
list which is spread across multiple lines. The heuristic only
kicked in if the entry sizes (character length) was above a
certain threshold (20) and the ratio between the previous and
current entry size was above a certain value (4).

This heuristic worked reasonably most of the time, but also
led to unfortunate breaks in many cases where a single entry
was suddenly much smaller (or larger) then the previous one.

The behavior of gofmt was sufficiently mysterious in some of
these situations that many issues were filed against it.

The simplest solution to address this problem is to remove
the heuristic altogether and have a programmer introduce
empty lines to force different alignments if it improves
readability. The problem with that approach is that the
places where it really matters, very long tables with many
(hundreds, or more) entries, may be machine-generated and
not "post-processed" by a human (e.g., unicode/utf8/tables.go).

If a single one of those entries is overlong, the result
would be that the alignment would force all comments or
values in key:value pairs to be adjusted to that overlong
value, making the table hard to read (e.g., that entry may
not even be visible on screen and all other entries seem
spaced out too wide).

Instead, we opted for a slightly improved heuristic that
behaves much better for "normal", human-written code.

1) The threshold is increased from 20 to 40. This disables
the heuristic for many common cases yet even if the alignment
is not "ideal", 40 is not that many characters per line with
todays screens, making it very likely that the entire line
remains "visible" in an editor.

2) 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.
This emphasizes the "overall picture" of the previous lines,
rather than a single one (which might be an outlier).

3) Changed the ratio from 4 to 2.5. Now that we ignore sizes
below 40, a ratio of 4 would mean that a new entry would have
to be 4 times bigger (160) or smaller (10) before alignment
would be broken. A ratio of 2.5 seems more sensible.

Applied updated gofmt to all of src and misc. Also tested
against several former issues that complained about this
and verified that the output for the given examples is
satisfactory (added respective test cases).

Some of the files changed because they were not gofmt-ed
in the first place.

For golang#644.
For golang#7335.
For golang#10392.
(and probably more related issues)

Fixes golang#22852.

Change-Id: I5e48b3d3b157a5cf2d649833b7297b33f43a6f6e
@golang golang locked and limited conversation to collaborators Apr 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants