cmd/gofmt: inconsistent formatting of empty methods

[neild]

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

go version go1.11rc2 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

Format https://play.golang.org/p/VW-uvEb5Mro

package main

func main(                                                                                                      ) {}

What did you expect to see?

https://play.golang.org/p/34L1Aw0cZ1D

package main

func main() {}

What did you see instead?

package main

func main() {
}

Looks like gofmt is deciding whether to turn {} into {\n} based on the original length of the line, rather than the post-formatting length. This is harmless, but surprising.

[agnivade]

@griesemer

[dmitshur]

An interesting behavior.

For anyone confused why it formats instead of printing an error like main.go:3:12: expected type, found 'EOF', scroll to the right. The ) {} characters are just off-screen.

[griesemer]

This patch solves this specific issue:

diff --git a/src/go/printer/nodes.go b/src/go/printer/nodes.go
index 1de7cd81b2..38b50f2596 100644
--- a/src/go/printer/nodes.go
+++ b/src/go/printer/nodes.go
@@ -1693,7 +1693,9 @@ func (p *printer) funcBody(headerSize int, sep whiteSpace, b *ast.BlockStmt) {
 func (p *printer) distanceFrom(from token.Pos) int {
        if from.IsValid() && p.pos.IsValid() {
                if f := p.posFor(from); f.Line == p.pos.Line {
-                       return p.pos.Column - f.Column
+                       // Use actual (formatted, i.e., p.out) column information
+                       // to get an accurate column distance. See issue #28082.
+                       return p.out.Column - f.Column
                }
        }
        return infinity

But it leads to cmd/gofmt test failures. Haven't investigated. Not urgent. If somebody wants to analyze this a bit more, please go ahead and keep me in the loop, thanks.

[eliben]

I looked into why @griesemer 's proposed patch causes test failures. The failures are in tests that format fragments of code that don't have a function wrapping them. For example testdata/stdin1.input:

	if x {
		y
	}

In this case, gofmt wraps the code in a dummy package and function to make it parsable, turning it into something like:

package p; func _() {	if x {
		y
	}

}

When the printer sees this code, it starts by splitting off package p into a separate line, and then emits the func body header: func _() {, and at this point p.out.Column is 9. However, from points further forward (in the original line after the fake package), and the result of distanceFrom becomes negative.

funcBody checks if headerSize+p.BodySize(...) <= maxSize. p.BodySize returns maxSize+1 since the body has multiple lines, but as headerSize is now -3, funcBody tries to fold the entire sample onto the same line with func _() {, which results in incorrect formatting.

I found a workaround that makes the whole test suite pass. It basically assumes that this discrepancy (p.out preceding from) is due to the package p being removed and adds its size back when calculating.

Even though all tests pass and the original snippets by @neild are formatted correctly, this doesn’t feel robust. While figuring out the workaround I encountered several other failing tests that are sensitive to changes in p.out, so this could have unexpected side effects unless we have a fully robust fix.

A more robust fix could be to remember the last emitted func header in the state, and use that to measure its size.

[eliben]

The most accurate measure of the actual header length would be to count how many bytes were emitted by the header printer. distanceFrom is only called from funcDecl and expr (for *ast.FuncLit). In these two places we could measure p.out.Column before and after the header itself is emitted and pass the difference as headerSize to funcBody. Then we could just delete distanceFrom.

[griesemer]

@eliben Worth a try.

[gopherbot]

Change https://golang.org/cl/188818 mentions this issue: cmd/gofmt: fix computation of function header size