Skip to content

Go: honor onlyIfReferenced and add imports into the right declaration and group - #8963

Merged
jkschneider merged 3 commits into
mainfrom
golang-add-import-only-if-referenced
Sep 27, 2026
Merged

jkschneider merged 3 commits into
mainfrom
golang-add-import-only-if-referenced

Conversation

@jkschneider

@jkschneider jkschneider commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

Why

GolangAddImport ignored onlyIfReferenced and always added the import. Java recipes run over Go compilation units, which are JavaSourceFiles, so any unconditional maybeAddImport(fqn) spliced a Java package into every Go file. The Java security recipes' CsrfProtection did exactly that, producing invalid Go:

import "fmt"
	"org.springframework.security.web.csrf"
  • In a Moderne DevCenter run this changed 5,908 Go files, and printing each diff kept a rewrite-go-rpc engine alive per Go repository (the recipe itself is fixed separately in moderneinc/rewrite-java-security#463).

What

GolangAddImport now matches the Go-side AddImport, and both handle files with several import declarations. It stays in-process: delegating to the Go-side visitor over RPC would start a Go engine for every candidate file.

  • onlyIfReferenced: add only when the parser's type attribution names the package outside the import declarations. This is the Go side's ReferencedPackages / pkgPathOf, ported to Java.
  • Existing imports: a blank or dot import no longer satisfies a request for a regular one, and an aliased request needs that alias.
  • Grouping: the import lands at the end of its stdlib / third-party / local group, with gofmt's blank line before a new group.
  • Which declaration: the import joins the last declaration holding its group, else the last declaration. An ungrouped declaration there is promoted to import ( ... ), and the declaration after it closes the parens. When the new import opens a later declaration, it takes over the ImportBlock marker that prints import (. The Go-side AddToBlock had the same multi-declaration bug and is fixed the same way.
  • First import: a file with no imports gets import "x", as gofmt, goimports and the Go side write a lone import. It previously got import (\n\t"x"\n).
  • Spacing: an aliased import is now spaced from its path. It previously printed as yy"github.com/x/y".
  • maybeAddImport(fqn): this splits at the last dot, so "net/http" arrived with no package (a no-op) and "github.com/x/y" / "gopkg.in/yaml.v3" as fragments. GolangImportService rejoins them. It can't tell a type name from a dotted last path element (a bare example.com module root, or a directory named bar.baz). Go type FQNs have the same ambiguity.

Tests

  • Java: GolangAddImportTest (integTest, 13 cases) takes over the add-import cases, and addImportForCrossPackageType now expects the ungrouped form.
  • Go: three multi-declaration cases in test/import_recipes_test.go.
  • Fail without the fix: every new gap case: the two onlyIfReferenced Java tests against main, the other eight Java gap tests against the first commit, and the three Go tests against the old AddToBlock.
  • Full suites: rewrite-go test (36), integTest (185, one pre-existing skip), and go test ./... all pass.

GolangAddImport ignored onlyIfReferenced and always added the import. Java
recipes run over Go compilation units (they are JavaSourceFiles), so any
unconditional maybeAddImport(fqn) spliced a Java package into every Go file,
e.g. `import "org.springframework.security.web.csrf"` from the Java security
recipes' CsrfProtection. Gate it on the same type-attribution check the
Go-side AddImport uses (ReferencedPackages/pkgPathOf).

Appending to a lone ungrouped `import "fmt"` also printed the new path after
it with no parens, which is invalid Go. Promote it to `import ( ... )` first,
as the Go-side promoteToGrouped does.
GolangAddImport now matches the Go-side AddImport, and both handle files
with several import declarations:

- A blank or dot import no longer satisfies a request for a regular one,
  and an aliased request needs that alias. An aliased import is spaced
  from its path.
- The import lands at the end of its stdlib / third-party / local group,
  with gofmt's blank line before a new group.
- It joins the last declaration holding its group (else the last one).
  An ungrouped declaration there is promoted to `import ( ... )`, and the
  declaration after it closes the parens, instead of appending a path
  after `import "x"`. When the import opens a later declaration it takes
  over the ImportBlock marker that prints `import (`.
- maybeAddImport(fqn) splits at the last dot, so "net/http" arrived with
  no package (a no-op) and "github.com/x/y" or "gopkg.in/yaml.v3" as a
  fragment. GolangImportService rejoins them.

GolangAddImport's tests move into their own GolangAddImportTest.
GolangAddImport gave an import-less file `import (\n\t"x"\n)`, while the
Go-side AddImport, gofmt, and goimports all write a lone import as
`import "x"`. Match them; adding a second import already promotes it to
the grouped form.
@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Sep 27, 2026
@jkschneider
jkschneider merged commit 8ca2449 into main Sep 27, 2026
1 check passed
@jkschneider
jkschneider deleted the golang-add-import-only-if-referenced branch September 27, 2026 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant