Skip to content

Fix gofmt group separator placement in Go import add/remove - #8549

Merged
knutwannheden merged 4 commits into
mainfrom
fix-stray-blank-line-when-removing-first-grouped-import
Aug 19, 2026
Merged

Fix gofmt group separator placement in Go import add/remove#8549
knutwannheden merged 4 commits into
mainfrom
fix-stray-blank-line-when-removing-first-grouped-import

Conversation

@knutwannheden

Copy link
Copy Markdown
Contributor

Motivation

Removing the first import of a grouped block leaves a stray blank line immediately after the opening paren. A gofmt import group separator is spelled as a blank line on the prefix of the import that opens the group, so when the import ahead of a group boundary is deleted, that blank line is orphaned onto the front of the block. RemoveUnusedImports is invoked automatically by templateRecipeVisitor whenever an after-template declares SourceImports, so the artifact lands in a large share of the diffs these recipes produce.

// in
import (
	"io/ioutil"

	"example.com/plainpkg"
)

// out, before this change
import (

	"example.com/plainpkg"
)

Pulling on that thread showed the same root cause on the insertion side: insertGrouped copies a neighbouring import's prefix onto the new import, which drags that neighbour's group separator along with the indent. The separator then lands wherever the donor happened to sit rather than on the group boundary, and all four insertion positions get it wrong — joining an existing group gains a spurious blank line, opening a new group at the head or the tail gets none. This was originally reported as "adding an import appends it as its own third group", which is the tail case. It is a whitespace bug, not a grouping-policy choice: placement is unchanged, an import still goes at the end of its own group.

Note for anyone bisecting: a reproduction that uses a versioned import path (gopkg.in/yaml.v3 and friends) will appear to start failing only once the companion PackageName fix lands. On today's main such an import is deleted outright as unreferenced, the block collapses, and there is no separator left to orphan. The defect is pre-existing and independent of that fix; the tests here use plain paths so the two are decoupled.

Summary

  • RemoveFromBlock collapses the group separator on the element it promotes to the head of the block. Removal elsewhere in the block is untouched.
  • insertGrouped composes the new import's prefix from the block's indent plus a separator when the preceding import belongs to another group, instead of copying a neighbour's prefix wholesale. Inserting ahead of every existing import moves the separator onto the import displaced from the head.
  • SortByGroup shares the separator helper, so the spelling of a separator lives in one place.
  • Landed as two commits so the insertion change is separately attributable. It turns out to cause no downstream churn: the moderneinc/recipes-go expectation that pins this behaviour ("encoding/json/v2", "os", "encoding/json/jsontext") is a single stdlib group with no separator in play, and produces byte-identical output before and after — verified against this branch.

Test plan

  • TestRemoveUnusedImports_DropsFirstOfGroupedBlock — removing the first element of a grouped block leaves no leading blank line (fails before, passes after).
  • TestRemoveUnusedImports_KeepsGroupSeparatorWhenDroppingMiddle — pins that a mid-block removal keeps the separator on the following group.
  • TestAddImport_OpensNewLeadingGroup / TestAddImport_OpensNewTrailingGroup — a new group at either end gets a separator.
  • TestAddImport_JoinsExistingLeadingGroup / TestAddImport_JoinsExistingTrailingGroup — joining an existing group gets none.
  • go build ./... && go vet ./... && go test ./... green from rewrite-go/ (16 packages, no failures).

PackageName read the trailing path element, so an import path carrying a
major version came out as the version: encoding/json/v2 as "v2",
gopkg.in/yaml.v3 as "yaml.v3". The qualifier is what RemoveUnusedImports
falls back on for a reference attribution did not resolve, so any such
import in a file it walks read as unreferenced and was removed, leaving
the code that used it behind.

Under semantic import versioning the version lives in the path rather
than the package name, so the name comes from the segment before a
trailing /vN element, and gopkg.in's .vN suffix is stripped. A module may
declare a package name matching neither; this is the better guess, not a
guarantee.
A template is type-checked as a file belonging to no module, against
whatever importer.Default() can load, so anything outside the stdlib
resolves to nothing and the emitted tree carries no types.

A recipe module can now carry the export data for the packages its
templates import. cmd/goexportdata generates it, keeping only the
__.PKGDEF member so a blob is a few percent of the archive go build
leaves behind (encoding/json/v2: 4.7 MB -> 170 KB), and emits an
embed.FS shim. pkg/exportdata resolves from those blobs first and falls
back to the toolchain, so a blob a newer toolchain cannot read costs
attribution and nothing else.

Each template builds one importer and reuses it across every Apply.
Decoding dominated the call before: Apply against a 99 KB blob was
788,804 ns/op, of which 768,596 was the decode, and is now 20,642.

RemoveUnusedImports kept a superseded import alive whenever another
import bound the same qualifier, which is the shape a major-version
package move takes once both sides are attributed. The qualifier
fallback now yields to attribution where attribution has accounted for
that name, and still keeps everything where nothing is attributed.
A gofmt import group separator is spelled as a blank line on the prefix of
the import that opens the group. Removing the import ahead of one left that
blank line in place, so the survivor opened the block with a stray blank
line after the `(`:

    import (

    	"example.com/plainpkg"
    )

RemoveFromBlock now collapses the separator on the element it promotes to
the head of the block. Removal elsewhere in the block is untouched, and a
regression test pins both positions.
insertGrouped copies a neighbouring import's prefix onto the new import,
which carries that neighbour's group separator along with the indent. The
separator then lands wherever the donor happened to sit rather than on the
group boundary, so all four insertion positions misplace it: joining an
existing group gets a spurious blank line, opening a new group at the head
or tail gets none.

The prefix is now composed from the block's indent plus a separator when
the preceding import belongs to another group. Inserting ahead of every
existing import moves the separator onto the import displaced from the
head. Placement within the block is unchanged — an import still goes at the
end of its own group.

SortByGroup shares the separator helper so the spelling lives in one place.
@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Aug 19, 2026
@knutwannheden knutwannheden changed the title Fix gofmt group separator placement in Go import add/remove Fix gofmt group separator placement in Go import add/remove Aug 19, 2026
@knutwannheden
knutwannheden merged commit 1f58f0f into main Aug 19, 2026
1 check passed
@knutwannheden
knutwannheden deleted the fix-stray-blank-line-when-removing-first-grouped-import branch August 19, 2026 09:40
@github-project-automation github-project-automation Bot moved this from In Progress to Done in OpenRewrite Aug 19, 2026
knutwannheden added a commit that referenced this pull request Aug 19, 2026
The feature landed here as a side effect of #8549, whose branch was cut
from the feature branch rather than from main, so an unreviewed ~1400
line change merged under the title of an unrelated import-formatting
fix. It continues on its own branch and will land through its own PR.

The two fixes #8549 and #8548 exist for stay: the gofmt group separator
placement in import add/remove, and deriving package names from
semantically versioned import paths. RemoveUnusedImports goes back to
deciding on refs and quals alone, since the qualifier rule that yields
to attribution is part of the feature and not of either fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant