Go: honor onlyIfReferenced and add imports into the right declaration and group - #8963
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
GolangAddImportignoredonlyIfReferencedand always added the import. Java recipes run over Go compilation units, which areJavaSourceFiles, so any unconditionalmaybeAddImport(fqn)spliced a Java package into every Go file. The Java security recipes'CsrfProtectiondid exactly that, producing invalid Go:rewrite-go-rpcengine alive per Go repository (the recipe itself is fixed separately in moderneinc/rewrite-java-security#463).What
GolangAddImportnow matches the Go-sideAddImport, 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'sReferencedPackages/pkgPathOf, ported to Java.import ( ... ), and the declaration after it closes the parens. When the new import opens a later declaration, it takes over theImportBlockmarker that printsimport (. The Go-sideAddToBlockhad the same multi-declaration bug and is fixed the same way.import "x", as gofmt, goimports and the Go side write a lone import. It previously gotimport (\n\t"x"\n).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.GolangImportServicerejoins them. It can't tell a type name from a dotted last path element (a bareexample.commodule root, or a directory namedbar.baz). Go type FQNs have the same ambiguity.Tests
GolangAddImportTest(integTest, 13 cases) takes over the add-import cases, andaddImportForCrossPackageTypenow expects the ungrouped form.test/import_recipes_test.go.onlyIfReferencedJava tests againstmain, the other eight Java gap tests against the first commit, and the three Go tests against the oldAddToBlock.test(36),integTest(185, one pre-existing skip), andgo test ./...all pass.