go/printer, gofmt: tuned table alignment for better results

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 #644.
For #7335.
For #10392.
(and probably more related issues)

Fixes #22852.

Change-Id: I5e48b3d3b157a5cf2d649833b7297b33f43a6f6e

src/cmd/go/testdata/src/dupload/p2/p2.go

@@ -1,2 +1,3 @@
 package p2
+
 import _ "dupload/vendor/p"

src/cmd/go/testdata/src/exclude/x_linux.go

@@ -1,4 +1,3 @@
 // +build windows
 
 package x
-

src/cmd/go/testdata/src/my.pkg/main/main.go

@@ -1,5 +1,7 @@
 package main
+
 import "my.pkg"
+
 func main() {
 	println(pkg.Text)
 }

src/cmd/go/testdata/src/vetcycle/p.go

@@ -1,11 +1,12 @@
 package p
 
-
 type (
 	_  interface{ m(B1) }
 	A1 interface{ a(D1) }
 	B1 interface{ A1 }
-	C1 interface{ B1 /* ERROR issue #18395 */ }
+	C1 interface {
+		B1 /* ERROR issue #18395 */
+	}
 	D1 interface{ C1 }
 )
 

src/cmd/internal/obj/arm64/asm7.go

@@ -4014,7 +4014,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
 		rt := int((p.To.Reg) & 31)
 		r := int((p.Reg) & 31)
 
-		o1 |= ((Q&1) << 30) | ((size&3) << 22) | (uint32(rf&31) << 16) | (uint32(r&31) << 5) | uint32(rt&31)
+		o1 |= ((Q & 1) << 30) | ((size & 3) << 22) | (uint32(rf&31) << 16) | (uint32(r&31) << 5) | uint32(rt&31)
 
 	case 94: /* vext $imm4, Vm.<T>, Vn.<T>, Vd.<T> */
 		if p.From3Type() != obj.TYPE_REG {
@@ -4053,7 +4053,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
 		rt := int((p.To.Reg) & 31)
 		r := int((p.Reg) & 31)
 
-		o1 |= ((Q&1) << 30) | (uint32(r&31) << 16) | (uint32(index&15) << 11) | (uint32(rf&31) << 5) | uint32(rt&31)
+		o1 |= ((Q & 1) << 30) | (uint32(r&31) << 16) | (uint32(index&15) << 11) | (uint32(rf&31) << 5) | uint32(rt&31)
 
 	case 95: /* vushr $shift, Vn.<T>, Vd.<T> */
 		at := int((p.To.Reg >> 5) & 15)
@@ -4111,7 +4111,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
 		rt := int((p.To.Reg) & 31)
 		rf := int((p.Reg) & 31)
 
-		o1 |= ((Q&1) << 30) | (uint32(imm&127) << 16) | (uint32(rf&31) << 5) | uint32(rt&31)
+		o1 |= ((Q & 1) << 30) | (uint32(imm&127) << 16) | (uint32(rf&31) << 5) | uint32(rt&31)
 
 	case 96: /* vst1 Vt1.<T>[index], offset(Rn) */
 		af := int((p.From.Reg >> 5) & 15)
@@ -4183,7 +4183,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
 			o1 |= 26 << 23
 		}
 
-		o1 |= (uint32(Q&1) << 30) | (uint32(r&31) << 16) | ((opcode&7) << 13) | (uint32(S&1) << 12) | (uint32(size&3) << 10) | (uint32(rf&31) << 5) | uint32(rt&31)
+		o1 |= (uint32(Q&1) << 30) | (uint32(r&31) << 16) | ((opcode & 7) << 13) | (uint32(S&1) << 12) | (uint32(size&3) << 10) | (uint32(rf&31) << 5) | uint32(rt&31)
 
 	case 97: /* vld1 offset(Rn), vt.<T>[index] */
 		at := int((p.To.Reg >> 5) & 15)
@@ -4257,7 +4257,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
 			o1 |= 106 << 21
 		}
 
-		o1 |= (uint32(Q&1) << 30) | (uint32(r&31) << 16) | ((opcode&7) << 13) | (uint32(S&1) << 12) | (uint32(size&3) << 10) | (uint32(rf&31) << 5) | uint32(rt&31)
+		o1 |= (uint32(Q&1) << 30) | (uint32(r&31) << 16) | ((opcode & 7) << 13) | (uint32(S&1) << 12) | (uint32(size&3) << 10) | (uint32(rf&31) << 5) | uint32(rt&31)
 	}
 	out[0] = o1
 	out[1] = o2

src/go/printer/nodes.go

@@ -12,6 +12,7 @@ import (
 	"bytes"
 	"go/ast"
 	"go/token"
+	"math"
 	"strconv"
 	"strings"
 	"unicode"
@@ -147,15 +148,15 @@ func (p *printer) exprList(prev0 token.Pos, list []ast.Expr, depth int, mode exp
 	// list entries span multiple lines;
 	// use source code positions to guide line breaks
 
-	// don't add extra indentation if noIndent is set;
-	// i.e., pretend that the first line is already indented
+	// Don't add extra indentation if noIndent is set;
+	// i.e., pretend that the first line is already indented.
 	ws := ignore
 	if mode&noIndent == 0 {
 		ws = indent
 	}
 
-	// the first linebreak is always a formfeed since this section must not
-	// depend on any previous formatting
+	// The first linebreak is always a formfeed since this section must not
+	// depend on any previous formatting.
 	prevBreak := -1 // index of last expression that was followed by a linebreak
 	if prev.IsValid() && prev.Line < line && p.linebreak(line, 0, ws, true) {
 		ws = ignore
@@ -165,22 +166,29 @@ func (p *printer) exprList(prev0 token.Pos, list []ast.Expr, depth int, mode exp
 	// initialize expression/key size: a zero value indicates expr/key doesn't fit on a single line
 	size := 0
 
+	// We use the ratio between the geometric mean of the previous key sizes and
+	// the current size to determine if there should be a break in the alignment.
+	// To compute the geometric mean we accumulate the ln(size) values (lnsum)
+	// and the number of sizes included (count).
+	lnsum := 0.0
+	count := 0
+
 	// print all list elements
 	prevLine := prev.Line
 	for i, x := range list {
 		line = p.lineFor(x.Pos())
 
-		// determine if the next linebreak, if any, needs to use formfeed:
+		// Determine if the next linebreak, if any, needs to use formfeed:
 		// in general, use the entire node size to make the decision; for
-		// key:value expressions, use the key size
+		// key:value expressions, use the key size.
 		// TODO(gri) for a better result, should probably incorporate both
 		//           the key and the node size into the decision process
 		useFF := true
 
-		// determine element size: all bets are off if we don't have
+		// Determine element size: All bets are off if we don't have
 		// position information for the previous and next token (likely
 		// generated code - simply ignore the size in this case by setting
-		// it to 0)
+		// it to 0).
 		prevSize := size
 		const infinity = 1e6 // larger than any source line
 		size = p.nodeSize(x, infinity)
@@ -195,35 +203,45 @@ func (p *printer) exprList(prev0 token.Pos, list []ast.Expr, depth int, mode exp
 			size = 0
 		}
 
-		// if the previous line and the current line had single-
+		// If the previous line and the current line had single-
 		// line-expressions and the key sizes are small or the
-		// ratio between the key sizes does not exceed a
-		// threshold, align columns and do not use formfeed
+		// ratio between the current key and the geometric mean
+		// if the previous key sizes does not exceed a threshold,
+		// align columns and do not use formfeed.
 		if prevSize > 0 && size > 0 {
-			const smallSize = 20
-			if prevSize <= smallSize && size <= smallSize {
+			const smallSize = 40
+			if count == 0 || prevSize <= smallSize && size <= smallSize {
 				useFF = false
 			} else {
-				const r = 4 // threshold
-				ratio := float64(size) / float64(prevSize)
-				useFF = ratio <= 1.0/r || r <= ratio
+				const r = 2.5                               // threshold
+				geomean := math.Exp(lnsum / float64(count)) // count > 0
+				ratio := float64(size) / geomean
+				useFF = r*ratio <= 1 || r <= ratio
 			}
 		}
+		if useFF {
+			lnsum = 0
+			count = 0
+		}
+		if size > 0 {
+			lnsum += math.Log(float64(size))
+			count++
+		}
 
 		needsLinebreak := 0 < prevLine && prevLine < line
 		if i > 0 {
-			// use position of expression following the comma as
+			// Use position of expression following the comma as
 			// comma position for correct comment placement, but
-			// only if the expression is on the same line
+			// only if the expression is on the same line.
 			if !needsLinebreak {
 				p.print(x.Pos())
 			}
 			p.print(token.COMMA)
 			needsBlank := true
 			if needsLinebreak {
-				// lines are broken using newlines so comments remain aligned
-				// unless forceFF is set or there are multiple expressions on
-				// the same line in which case formfeed is used
+				// Lines are broken using newlines so comments remain aligned
+				// unless useFF is set or there are multiple expressions on
+				// the same line in which case formfeed is used.
 				if p.linebreak(line, 0, ws, useFF || prevBreak+1 < i) {
 					ws = ignore
 					prevBreak = i
@@ -236,10 +254,10 @@ func (p *printer) exprList(prev0 token.Pos, list []ast.Expr, depth int, mode exp
 		}
 
 		if len(list) > 1 && isPair && size > 0 && needsLinebreak {
-			// we have a key:value expression that fits onto one line
+			// We have a key:value expression that fits onto one line
 			// and it's not on the same line as the prior expression:
-			// use a column for the key such that consecutive entries
-			// can align if possible
+			// Use a column for the key such that consecutive entries
+			// can align if possible.
 			// (needsLinebreak is set if we started a new line before)
 			p.expr(pair.Key)
 			p.print(pair.Colon, token.COLON, vtab)
@@ -252,7 +270,7 @@ func (p *printer) exprList(prev0 token.Pos, list []ast.Expr, depth int, mode exp
 	}
 
 	if mode&commaTerm != 0 && next.IsValid() && p.pos.Line < next.Line {
-		// print a terminating comma if the next token is on a new line
+		// Print a terminating comma if the next token is on a new line.
 		p.print(token.COMMA)
 		if ws == ignore && mode&noIndent == 0 {
 			// unindent if we indented

src/go/printer/printer_test.go

@@ -188,6 +188,7 @@ var data = []entry{
 	{"comments.input", "comments.golden", 0},
 	{"comments.input", "comments.x", export},
 	{"comments2.input", "comments2.golden", idempotent},
+	{"alignment.input", "alignment.golden", idempotent},
 	{"linebreaks.input", "linebreaks.golden", idempotent},
 	{"expressions.input", "expressions.golden", idempotent},
 	{"expressions.input", "expressions.raw", rawFormat | idempotent},

src/go/printer/testdata/alignment.golden

@@ -0,0 +1,130 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package alignment
+
+// ----------------------------------------------------------------------------
+// Examples from issue #7335.
+
+func main() {
+	z := MyStruct{
+		Foo:		"foo",
+		Bar:		"bar",
+		Name:		"name",
+		LongName:	"longname",
+		Baz:		"baz",
+	}
+	y := MyStruct{
+		Foo:			"foo",
+		Bar:			"bar",
+		NameXX:			"name",
+		LongNameXXXXXXXXXXXXX:	"longname",
+		Baz:			"baz",
+	}
+	z := MyStruct{
+		Foo:			"foo",
+		Bar:			"bar",
+		Name:			"name",
+		LongNameXXXXXXXXXXXXX:	"longname",
+		Baz:			"baz",
+	}
+}
+
+// ----------------------------------------------------------------------------
+// Examples from issue #10392.
+
+var kcfg = KubeletConfig{
+	Address:			s.Address,
+	AllowPrivileged:		s.AllowPrivileged,
+	HostNetworkSources:		hostNetworkSources,
+	HostnameOverride:		s.HostnameOverride,
+	RootDirectory:			s.RootDirectory,
+	ConfigFile:			s.Config,
+	ManifestURL:			s.ManifestURL,
+	FileCheckFrequency:		s.FileCheckFrequency,
+	HTTPCheckFrequency:		s.HTTPCheckFrequency,
+	PodInfraContainerImage:		s.PodInfraContainerImage,
+	SyncFrequency:			s.SyncFrequency,
+	RegistryPullQPS:		s.RegistryPullQPS,
+	RegistryBurst:			s.RegistryBurst,
+	MinimumGCAge:			s.MinimumGCAge,
+	MaxPerPodContainerCount:	s.MaxPerPodContainerCount,
+	MaxContainerCount:		s.MaxContainerCount,
+	ClusterDomain:			s.ClusterDomain,
+	ClusterDNS:			s.ClusterDNS,
+	Runonce:			s.RunOnce,
+	Port:				s.Port,
+	ReadOnlyPort:			s.ReadOnlyPort,
+	CadvisorInterface:		cadvisorInterface,
+	EnableServer:			s.EnableServer,
+	EnableDebuggingHandlers:	s.EnableDebuggingHandlers,
+	DockerClient:			dockertools.ConnectToDockerOrDie(s.DockerEndpoint),
+	KubeClient:			client,
+	MasterServiceNamespace:		s.MasterServiceNamespace,
+	VolumePlugins:			ProbeVolumePlugins(),
+	NetworkPlugins:			ProbeNetworkPlugins(),
+	NetworkPluginName:		s.NetworkPluginName,
+	StreamingConnectionIdleTimeout:	s.StreamingConnectionIdleTimeout,
+	TLSOptions:			tlsOptions,
+	ImageGCPolicy:			imageGCPolicy, imageGCPolicy,
+	Cloud:				cloud,
+	NodeStatusUpdateFrequency:	s.NodeStatusUpdateFrequency,
+}
+
+var a = A{
+	Long:					1,
+	LongLong:				1,
+	LongLongLong:				1,
+	LongLongLongLong:			1,
+	LongLongLongLongLong:			1,
+	LongLongLongLongLongLong:		1,
+	LongLongLongLongLongLongLong:		1,
+	LongLongLongLongLongLongLongLong:	1,
+	Short:					1,
+	LongLongLongLongLongLongLongLongLong:	3,
+}
+
+// ----------------------------------------------------------------------------
+// Examples from issue #22852.
+
+var fmtMap = map[string]string{
+	"1":					"123",
+	"12":					"123",
+	"123":					"123",
+	"1234":					"123",
+	"12345":				"123",
+	"123456":				"123",
+	"12345678901234567890123456789":	"123",
+	"abcde":				"123",
+	"123456789012345678901234567890":	"123",
+	"1234567":				"123",
+	"abcdefghijklmnopqrstuvwxyzabcd":	"123",
+	"abcd":					"123",
+}
+
+type Fmt struct {
+	abcdefghijklmnopqrstuvwx	string
+	abcdefghijklmnopqrstuvwxy	string
+	abcdefghijklmnopqrstuvwxyz	string
+	abcdefghijklmnopqrstuvwxyza	string
+	abcdefghijklmnopqrstuvwxyzab	string
+	abcdefghijklmnopqrstuvwxyzabc	string
+	abcde				string
+	abcdefghijklmnopqrstuvwxyzabcde	string
+	abcdefg				string
+}
+
+func main() {
+	_ := Fmt{
+		abcdefghijklmnopqrstuvwx:		"foo",
+		abcdefghijklmnopqrstuvwxyza:		"foo",
+		abcdefghijklmnopqrstuvwxyzab:		"foo",
+		abcdefghijklmnopqrstuvwxyzabc:		"foo",
+		abcde:					"foo",
+		abcdefghijklmnopqrstuvwxyzabcde:	"foo",
+		abcdefg:				"foo",
+		abcdefghijklmnopqrstuvwxy:		"foo",
+		abcdefghijklmnopqrstuvwxyz:		"foo",
+	}
+}

src/go/printer/testdata/alignment.input

@@ -0,0 +1,130 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package alignment
+
+// ----------------------------------------------------------------------------
+// Examples from issue #7335.
+
+func main() {
+    z := MyStruct{
+        Foo:      "foo",
+        Bar:      "bar",
+        Name:     "name",
+        LongName: "longname",
+        Baz:      "baz",
+    }
+    y := MyStruct{
+        Foo:                   "foo",
+        Bar:                   "bar",
+        NameXX:                "name",
+        LongNameXXXXXXXXXXXXX: "longname",
+        Baz: "baz",
+    }
+    z := MyStruct{
+        Foo:  "foo",
+        Bar:  "bar",
+        Name: "name",
+        LongNameXXXXXXXXXXXXX: "longname",
+        Baz: "baz",
+    }
+}
+
+// ----------------------------------------------------------------------------
+// Examples from issue #10392.
+
+var kcfg = KubeletConfig{
+    Address:                        s.Address,
+    AllowPrivileged:                s.AllowPrivileged,
+    HostNetworkSources:             hostNetworkSources,
+    HostnameOverride:               s.HostnameOverride,
+    RootDirectory:                  s.RootDirectory,
+    ConfigFile:                     s.Config,
+    ManifestURL:                    s.ManifestURL,
+    FileCheckFrequency:             s.FileCheckFrequency,
+    HTTPCheckFrequency:             s.HTTPCheckFrequency,
+    PodInfraContainerImage:         s.PodInfraContainerImage,
+    SyncFrequency:                  s.SyncFrequency,
+    RegistryPullQPS:                s.RegistryPullQPS,
+    RegistryBurst:                  s.RegistryBurst,
+    MinimumGCAge:                   s.MinimumGCAge,
+    MaxPerPodContainerCount:        s.MaxPerPodContainerCount,
+    MaxContainerCount:              s.MaxContainerCount,
+    ClusterDomain:                  s.ClusterDomain,
+    ClusterDNS:                     s.ClusterDNS,
+    Runonce:                        s.RunOnce,
+    Port:                           s.Port,
+    ReadOnlyPort:                   s.ReadOnlyPort,
+    CadvisorInterface:              cadvisorInterface,
+    EnableServer:                   s.EnableServer,
+    EnableDebuggingHandlers:        s.EnableDebuggingHandlers,
+    DockerClient:                   dockertools.ConnectToDockerOrDie(s.DockerEndpoint),
+    KubeClient:                     client,
+    MasterServiceNamespace:         s.MasterServiceNamespace,
+    VolumePlugins:                  ProbeVolumePlugins(),
+    NetworkPlugins:                 ProbeNetworkPlugins(),
+    NetworkPluginName:              s.NetworkPluginName,
+    StreamingConnectionIdleTimeout: s.StreamingConnectionIdleTimeout,
+    TLSOptions:                     tlsOptions,
+    ImageGCPolicy:                  imageGCPolicy,imageGCPolicy,
+    Cloud:                          cloud,
+    NodeStatusUpdateFrequency: s.NodeStatusUpdateFrequency,
+}
+
+var a = A{
+    Long:                             1,
+    LongLong:                         1,
+    LongLongLong:                     1,
+    LongLongLongLong:                 1,
+    LongLongLongLongLong:             1,
+    LongLongLongLongLongLong:         1,
+    LongLongLongLongLongLongLong:     1,
+    LongLongLongLongLongLongLongLong: 1,
+    Short: 1,
+    LongLongLongLongLongLongLongLongLong: 3,
+}
+
+// ----------------------------------------------------------------------------
+// Examples from issue #22852.
+
+var fmtMap = map[string]string{
+	"1": "123",
+	"12": "123",
+	"123": "123",
+	"1234": "123",
+	"12345": "123",
+	"123456": "123",
+	"12345678901234567890123456789": "123",
+	"abcde": "123",
+	"123456789012345678901234567890": "123",
+	"1234567": "123",
+	"abcdefghijklmnopqrstuvwxyzabcd": "123",
+	"abcd": "123",
+}
+
+type Fmt struct {
+	abcdefghijklmnopqrstuvwx string
+	abcdefghijklmnopqrstuvwxy string
+	abcdefghijklmnopqrstuvwxyz string
+	abcdefghijklmnopqrstuvwxyza string
+	abcdefghijklmnopqrstuvwxyzab string
+	abcdefghijklmnopqrstuvwxyzabc string
+	abcde string
+	abcdefghijklmnopqrstuvwxyzabcde string
+	abcdefg string
+}
+
+func main() {
+	_ := Fmt{
+		abcdefghijklmnopqrstuvwx: "foo",
+		abcdefghijklmnopqrstuvwxyza: "foo",
+		abcdefghijklmnopqrstuvwxyzab: "foo",
+		abcdefghijklmnopqrstuvwxyzabc: "foo",
+		abcde: "foo",
+		abcdefghijklmnopqrstuvwxyzabcde: "foo",
+		abcdefg: "foo",
+		abcdefghijklmnopqrstuvwxy: "foo",
+		abcdefghijklmnopqrstuvwxyz: "foo",
+	}
+}