x/tools/internal/imports: do not detach comments above a given import when merging

[jpap]

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

go version go1.17 darwin/arm64

Does this issue reproduce with the latest release?

Yes

What did you do?

• When formatting imports, gopls will merge multiple imports into a single declaration.
• Any comments attached to an import get detached.
• This breaks tools which parse go source files to act on directives associated with an import comment: think cgo preamble, but for other packages.
• In some cases (as given below) it can also break the cgo preamble, even though the "C" import is not merged.

For example,

// Package A
import "my.package/a"

// Package B
import "my.package/b"

// #cgo CFLAGS: -Wall
import "C"

// Package D
import "my.package/d"

import "fmt"

gets combined into:

// Package A
import (
	// Package B
	// #cgo CFLAGS: -Wall
	// Package D
	"fmt"

	"my.package/a"
	"my.package/b"
	"my.package/d"
)

import "C"

Here you can see that the comments associated with each of the packages have been detached. This is especially problematic for the "C" package, which has lost its preamble entirely.

What did you expect to see?

• Comments attached to an import should remain attached, whether they are grouped or not.

For the above example:

import (
  "fmt"

  // Package A
  "my.package/a"

  // Package B
  "my.package/b"

  // Package D
  "my.package/d"
)

// #cgo CFLAGS: -Wall
import "C"

where "C" remains detached, as given in func mergeImports, file internal/imports/sortimports.go. In general, I don't see why it needs to remain detached, but I'm not suggesting that convention be changed.

You could also write this without the line separations between "same host" packages, as below, though it doesn't read as clearly:

import (
  "fmt"

  // Package A
  "my.package/a"
  // Package B
  "my.package/b"
  // Package D
  "my.package/d"
)

// #cgo CFLAGS: -Wall
import "C"

I would prefer the previous style where, in general, any package having an attached comment has one blank line of padding on top and bottom even if it is part of a "same host" package group.

[mknyszek]

CC @heschi via https://dev.golang.org/owners

[heschi]

I agree that the behavior you're proposing would be better. I personally would go for the second one.

AFAIK the go/ast package isn't able to reliably move these comments around, which is why goimports avoids messing with groups as much as possible. Happy to see a clean, well-tested CL that proves otherwise.

[gopherbot]

Change https://golang.org/cl/358334 mentions this issue: internal/imports: do not detach comments when merging imports.