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

go/ast: Incorrect SortImports leads to strange multiline formatting with block comments #18929

Open
dsnet opened this issue Feb 3, 2017 · 17 comments

Comments

@dsnet
Copy link
Member

commented Feb 3, 2017

I was running fmt on this file: https://github.com/seriesoftubes/lint/blob/cd36bee163f7e24809372f9635ae1ec4c956bbd5/testdata/duplicate-imports3.go

Smaller reproduction is: https://play.golang.org/p/dyVWMr-vma

Formatting the following:

import (
	/* comment */ io1 "io"
	/* comment */ io2 "io"
)

Results in the following:

import (
	/* comment */ io1 "io" /* comment */
	io2 "io"
)
@dsnet

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2017

@griesemer griesemer self-assigned this Feb 3, 2017

@griesemer griesemer added this to the Go1.9 milestone Feb 3, 2017

@griesemer griesemer modified the milestones: Go1.9Maybe, Go1.9 May 9, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2017

This is due to a bug in go/ast.SortImports: If we comment out the call to SortImports in gofmt.go, the result looks correct.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2017

The function sortSpecs in go/ast/import.go appears to assume that comments are always following (not preceding) an ImportSpec. Consequently, if a comments is before an ImportSpec, it is associated with the previous one instead of the next one.

Any fix will be subtle. Leaving for 1.10.

@griesemer griesemer closed this Jun 20, 2017

@griesemer griesemer modified the milestones: Go1.10, Go1.9Maybe Jun 20, 2017

@griesemer griesemer reopened this Jun 20, 2017

@griesemer griesemer changed the title go/fmt: strange multiline formatting with block comments go/ast: Incorrect SortImports leads to strange multiline formatting with block comments Jun 20, 2017

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 23, 2018

@agnivade

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Even more interesting is if you have -

package main


import (
	/* comment */ io1 "io" // hello
	/* comment */ io2 "io"
)

which becomes

package main

import (
	/* comment */ io1 "io" // hello
	/* comment */
	io2 "io"
)

The culprit is these lines in go/ast/import.go

for _, g := range importComments[s] {
		for _, c := range g.List {
			c.Slash = pos[i].End
		}
	}

This means all comments mapped to a single import spec will always be positioned at the end of the spec. So then both // hello and /* comment */ gets mapped, and there is a clash. And most probably down the gofmt pipeline, the AST auto-fixes itself to bring the /* comment */ to the next line.

Another thing to note is that this does not happen if a single CommentGroup has multiple comments- like /* comment1 */ /* comment2 */. In this case, even if the .Slash position of both the comments are mapped to the same position, which is end of the import spec, they are kept in the same line.

We will have to match line nos. along with token position and also add some additional position information to the importComments map. Investigating.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 13, 2019

Change https://golang.org/cl/162337 mentions this issue: go/ast: fix SortImports to handle block comments

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

You might want to look at how goimports handles comments, and some of the test cases there. This is exceedingly difficult to get right with go/ast, and there are known failing corner cases still. But a lot of the quirks have already been dealt with in goimports, with extensive tests.

@agnivade

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Thanks ! From what I am seeing, it is a near copy of go/ast/import.go with some very minor modifications. And there is nothing special in it for handling comments.

And indeed, by running goimports on the testcase mentioned in the first comment leads to the same result.

package main

import (
	/* comment */ io1 "io"
	/* comment */ io2 "io"
)

func main() {
	_ = io1.ByteReader
	_ = io2.ByteReader
}

goimports imp.go

package main

import (
	/* comment */ io1 "io" /* comment */
	io2 "io"
)

func main() {
	_ = io1.ByteReader
	_ = io2.ByteReader
}

I did see the comment related test cases in tools/imports/fix_test.go. There seem to be a few with blank imports which are missing from gofmt tests, but they are not failing when I run them through gofmt. I will dig further and see if I get any interesting tests to add to gofmt.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

From what I am seeing, it is a near copy of go/ast/import.go with some very minor modifications.

Great. We should be sure to duplicate any fixes to goimports, then.

And there is nothing special in it for handling comments.

This usually manifests as position munging, without direct reference to comments.

I think much of the heavy duty comment handling is actually in the golang.org/x/tools/go/ast/astutil import modification methods. Glad you found those tests.

@gopherbot gopherbot closed this in 5b68cb6 Mar 28, 2019

@agnivade

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@bradfitz @josharian - I have a WIP CL which ports this change to goimports. However, goimports calls format.Source on its way out, which again calls ast.SortImports. So, unless the code is running on tip, everything just goes haywire again. And build tags only support till 1.12 and above, so I cannot guard the new tests by that.

What is the recommended approach here ?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

I have no advice, but I do note that this issue is closed, so .... maybe there's nothing to do? :)

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Oh, for goimports. I'm fine relying on tip for the fix. You can use the +build go1.13 build tag for tests. It was added in 889aa5e

@agnivade

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Ah 1.13 was added. Great.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 5, 2019

Change https://golang.org/cl/170917 mentions this issue: imports: sync sortImports with the new changes to go/ast.SortImports

@gopherbot

This comment has been minimized.

Copy link

commented Aug 8, 2019

Change https://golang.org/cl/189379 mentions this issue: Revert "go/ast: fix SortImports to handle block comments"

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Re-opening as the fix has been reverted. A future attempt at re-fixing this needs to address #33538

@dsnet dsnet reopened this Aug 8, 2019

gopherbot pushed a commit that referenced this issue Aug 8, 2019
Revert "go/ast: fix SortImports to handle block comments"
This reverts CL 162337.

Reason for revert: this introduces a regression

Fixes #33538
Updates #18929

Change-Id: Ib2320a840c6d3ec7912e8f414e933d04fbf11ab4
Reviewed-on: https://go-review.googlesource.com/c/go/+/189379
Reviewed-by: Robert Griesemer <gri@golang.org>

@agnivade agnivade assigned agnivade and unassigned griesemer Aug 9, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Aug 16, 2019

Change https://golang.org/cl/190523 mentions this issue: cmd/gofmt: update TestRewrite to avoid future regressions

gopherbot pushed a commit that referenced this issue Aug 16, 2019
cmd/gofmt: update TestRewrite to avoid future regressions
CL 162337 changed go/ast to better handle block comments,
but was reverted because it introduced an off-by-one bug.
This CL adds a test case to enforce the correct behavior
so that future changes do not break this again.

Updates #18929
Updates #33538

Change-Id: I2d25c139d007f8db1091b7a48b1dd20c584e2699
Reviewed-on: https://go-review.googlesource.com/c/go/+/190523
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Aug 17, 2019

Change https://golang.org/cl/190480 mentions this issue: go/ast: fix SortImports to handle block comments (take 2)

t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
cmd/gofmt: update TestRewrite to avoid future regressions
CL 162337 changed go/ast to better handle block comments,
but was reverted because it introduced an off-by-one bug.
This CL adds a test case to enforce the correct behavior
so that future changes do not break this again.

Updates golang#18929
Updates golang#33538

Change-Id: I2d25c139d007f8db1091b7a48b1dd20c584e2699
Reviewed-on: https://go-review.googlesource.com/c/go/+/190523
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.