-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
\cc @griesemer |
This is due to a bug in |
The function Any fix will be subtle. Leaving for 1.10. |
Even more interesting is if you have -
which becomes
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 Another thing to note is that this does not happen if a single CommentGroup has multiple comments- like We will have to match line nos. along with token position and also add some additional position information to the |
Change https://golang.org/cl/162337 mentions this issue: |
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. |
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. |
Great. We should be sure to duplicate any fixes to goimports, then.
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. |
@bradfitz @josharian - I have a WIP CL which ports this change to goimports. However, goimports calls What is the recommended approach here ? |
I have no advice, but I do note that this issue is closed, so .... maybe there's nothing to do? :) |
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 |
Ah 1.13 was added. Great. |
Change https://golang.org/cl/170917 mentions this issue: |
Change https://golang.org/cl/189379 mentions this issue: |
Re-opening as the fix has been reverted. A future attempt at re-fixing this needs to address #33538 |
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>
Change https://golang.org/cl/190523 mentions this issue: |
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>
Change https://golang.org/cl/190480 mentions this issue: |
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>
I was running
fmt
on this file: https://github.com/seriesoftubes/lint/blob/cd36bee163f7e24809372f9635ae1ec4c956bbd5/testdata/duplicate-imports3.goSmaller reproduction is: https://play.golang.org/p/dyVWMr-vma
Formatting the following:
Results in the following:
The text was updated successfully, but these errors were encountered: