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: inconsistent formatting of empty methods #28082

Closed
neild opened this issue Oct 8, 2018 · 7 comments
Closed

cmd/gofmt: inconsistent formatting of empty methods #28082

neild opened this issue Oct 8, 2018 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@neild
Copy link
Contributor

neild commented Oct 8, 2018

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

agnivade commented Oct 9, 2018

@griesemer

@agnivade agnivade added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 9, 2018
@agnivade agnivade added this to the Unplanned milestone Oct 9, 2018
@dmitshur
Copy link
Contributor

dmitshur commented Oct 9, 2018

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

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.

@griesemer griesemer self-assigned this Oct 9, 2018
@agnivade agnivade added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 10, 2018
@eliben
Copy link
Member

eliben commented Aug 1, 2019

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

eliben commented Aug 1, 2019

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

@eliben Worth a try.

@gopherbot
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants