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

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

Open
jpap opened this issue Sep 29, 2021 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@jpap
Copy link
Contributor

jpap commented Sep 29, 2021

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.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 29, 2021
@gopherbot gopherbot added this to the Unreleased milestone Sep 29, 2021
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 4, 2021
@mknyszek
Copy link
Contributor

mknyszek commented Oct 4, 2021

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

@heschi
Copy link
Contributor

heschi commented Oct 4, 2021

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants