\cc @griesemer

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.

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.

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.

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

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.

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.

@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 ?

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: imports: sync sortImports with the new changes to go/ast.SortImports

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

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

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

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