internal/refactor/inline: avoid redundant import names added by inlining
When analyzing callee objects, keep track of their package names so that
we don't have to assign explicit names to new imports when there is no
conflict with their implicit name.
Fixes golang/go#63613
Change-Id: I5b8dc9c514e434fb4262cab321933a28729bbd76
Reviewed-on: https://go-review.googlesource.com/c/tools/+/536755
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/regtest/marker/testdata/codeaction/removeparam_imports.txt b/gopls/internal/regtest/marker/testdata/codeaction/removeparam_imports.txt
index d616fe2..1a483d2 100644
--- a/gopls/internal/regtest/marker/testdata/codeaction/removeparam_imports.txt
+++ b/gopls/internal/regtest/marker/testdata/codeaction/removeparam_imports.txt
@@ -85,7 +85,7 @@
import (
"mod.test/b"
- c "mod.test/c"
+ "mod.test/c"
)
func _() {
@@ -97,7 +97,7 @@
import (
"mod.test/b"
- c "mod.test/c"
+ "mod.test/c"
)
func _() {
@@ -111,7 +111,7 @@
import (
"mod.test/b"
- c "mod.test/c"
+ "mod.test/c"
)
func _() {
@@ -129,9 +129,9 @@
// TODO(rfindley/adonovan): inlining here adds an additional import of
// mod.test/b. Can we do better?
import (
+ "mod.test/b"
. "mod.test/b"
- b "mod.test/b"
- c "mod.test/c"
+ "mod.test/c"
)
func _() {
diff --git a/internal/refactor/inline/callee.go b/internal/refactor/inline/callee.go
index dc74eab..c9a7ea0 100644
--- a/internal/refactor/inline/callee.go
+++ b/internal/refactor/inline/callee.go
@@ -59,9 +59,12 @@
// An object abstracts a free types.Object referenced by the callee. Gob-serializable.
type object struct {
- Name string // Object.Name()
- Kind string // one of {var,func,const,type,pkgname,nil,builtin}
- PkgPath string // pkgpath of object (or of imported package if kind="pkgname")
+ Name string // Object.Name()
+ Kind string // one of {var,func,const,type,pkgname,nil,builtin}
+ PkgPath string // path of object's package (or imported package if kind="pkgname")
+ PkgName string // name of object's package (or imported package if kind="pkgname")
+ // TODO(rfindley): should we also track LocalPkgName here? Do we want to
+ // preserve the local package name?
ValidPos bool // Object.Pos().IsValid()
Shadow map[string]bool // names shadowed at one of the object's refs
}
@@ -192,15 +195,18 @@
objidx, ok := freeObjIndex[obj]
if !ok {
objidx = len(freeObjIndex)
- var pkgpath string
- if pkgname, ok := obj.(*types.PkgName); ok {
- pkgpath = pkgname.Imported().Path()
+ var pkgpath, pkgname string
+ if pn, ok := obj.(*types.PkgName); ok {
+ pkgpath = pn.Imported().Path()
+ pkgname = pn.Imported().Name()
} else if obj.Pkg() != nil {
pkgpath = obj.Pkg().Path()
+ pkgname = obj.Pkg().Name()
}
freeObjs = append(freeObjs, object{
Name: obj.Name(),
Kind: objectKind(obj),
+ PkgName: pkgname,
PkgPath: pkgpath,
ValidPos: obj.Pos().IsValid(),
})
diff --git a/internal/refactor/inline/inline.go b/internal/refactor/inline/inline.go
index be16161..06f6401 100644
--- a/internal/refactor/inline/inline.go
+++ b/internal/refactor/inline/inline.go
@@ -222,13 +222,13 @@
importDecl = &ast.GenDecl{Tok: token.IMPORT}
f.Decls = prepend[ast.Decl](importDecl, f.Decls...)
}
- for _, spec := range res.newImports {
+ for _, imp := range res.newImports {
// Check that the new imports are accessible.
- path, _ := strconv.Unquote(spec.Path.Value)
+ path, _ := strconv.Unquote(imp.spec.Path.Value)
if !canImport(caller.Types.Path(), path) {
return nil, fmt.Errorf("can't inline function %v as its body refers to inaccessible package %q", callee, path)
}
- importDecl.Specs = append(importDecl.Specs, spec)
+ importDecl.Specs = append(importDecl.Specs, imp.spec)
}
}
@@ -300,8 +300,13 @@
return newSrc, nil
}
+type newImport struct {
+ pkgName string
+ spec *ast.ImportSpec
+}
+
type result struct {
- newImports []*ast.ImportSpec
+ newImports []newImport
old, new ast.Node // e.g. replace call expr by callee function body expression
}
@@ -387,14 +392,14 @@
}
// localImportName returns the local name for a given imported package path.
- var newImports []*ast.ImportSpec
- localImportName := func(path string, shadows map[string]bool) string {
+ var newImports []newImport
+ localImportName := func(obj *object) string {
// Does an import exist?
- for _, name := range importMap[path] {
+ for _, name := range importMap[obj.PkgPath] {
// Check that either the import preexisted,
// or that it was newly added (no PkgName) but is not shadowed,
// either in the callee (shadows) or caller (caller.lookup).
- if !shadows[name] {
+ if !obj.Shadow[name] {
found := caller.lookup(name)
if is[*types.PkgName](found) || found == nil {
return name
@@ -404,7 +409,7 @@
newlyAdded := func(name string) bool {
for _, new := range newImports {
- if new.Name.Name == name {
+ if new.pkgName == name {
return true
}
}
@@ -419,29 +424,32 @@
//
// "init" is not a legal PkgName.
//
- // TODO(adonovan): preserve the PkgName used
- // in the original source, or, for a dot import,
- // use the package's declared name.
- base := pathpkg.Base(path)
+ // TODO(rfindley): is it worth preserving local package names for callee
+ // imports? Are they likely to be better or worse than the name we choose
+ // here?
+ base := obj.PkgName
name := base
- for n := 0; shadows[name] || caller.lookup(name) != nil || newlyAdded(name) || name == "init"; n++ {
+ for n := 0; obj.Shadow[name] || caller.lookup(name) != nil || newlyAdded(name) || name == "init"; n++ {
name = fmt.Sprintf("%s%d", base, n)
}
- // TODO(adonovan): don't use a renaming import
- // unless the local name differs from either
- // the package name or the last segment of path.
- // This requires that we tabulate (path, declared name, local name)
- // triples for each package referenced by the callee.
- logf("adding import %s %q", name, path)
- newImports = append(newImports, &ast.ImportSpec{
- Name: makeIdent(name),
+ logf("adding import %s %q", name, obj.PkgPath)
+ spec := &ast.ImportSpec{
Path: &ast.BasicLit{
Kind: token.STRING,
- Value: strconv.Quote(path),
+ Value: strconv.Quote(obj.PkgPath),
},
+ }
+ // Use explicit pkgname (out of necessity) when it differs from the declared name,
+ // or (for good style) when it differs from base(pkgpath).
+ if name != obj.PkgName || name != pathpkg.Base(obj.PkgPath) {
+ spec.Name = makeIdent(name)
+ }
+ newImports = append(newImports, newImport{
+ pkgName: name,
+ spec: spec,
})
- importMap[path] = append(importMap[path], name)
+ importMap[obj.PkgPath] = append(importMap[obj.PkgPath], name)
return name
}
@@ -471,8 +479,7 @@
var newName ast.Expr
if obj.Kind == "pkgname" {
// Use locally appropriate import, creating as needed.
- newName = makeIdent(localImportName(obj.PkgPath, obj.Shadow)) // imported package
-
+ newName = makeIdent(localImportName(&obj)) // imported package
} else if !obj.ValidPos {
// Built-in function, type, or value (e.g. nil, zero):
// check not shadowed at caller.
@@ -515,7 +522,7 @@
// Form a qualified identifier, pkg.Name.
if qualify {
- pkgName := localImportName(obj.PkgPath, obj.Shadow)
+ pkgName := localImportName(&obj)
newName = &ast.SelectorExpr{
X: makeIdent(pkgName),
Sel: makeIdent(obj.Name),
diff --git a/internal/refactor/inline/testdata/crosspkg.txtar b/internal/refactor/inline/testdata/crosspkg.txtar
index 7c0704b..e0744f9 100644
--- a/internal/refactor/inline/testdata/crosspkg.txtar
+++ b/internal/refactor/inline/testdata/crosspkg.txtar
@@ -22,22 +22,30 @@
fmt.Println()
b.B1() //@ inline(re"B1", b1result)
b.B2() //@ inline(re"B2", b2result)
+ b.B3() //@ inline(re"B3", b3result)
}
-- b/b.go --
package b
import "testdata/c"
+import "testdata/d"
import "fmt"
func B1() { c.C() }
func B2() { fmt.Println() }
+func B3() { e.E() } // (note that "testdata/d" points to package e)
-- c/c.go --
package c
func C() {}
+-- d/d.go --
+package e // <- this package name intentionally mismatches the path
+
+func E() {}
+
-- b1result --
package a
@@ -46,7 +54,7 @@
import (
"fmt"
"testdata/b"
- c "testdata/c"
+ "testdata/c"
)
// Nor this one.
@@ -55,6 +63,7 @@
fmt.Println()
c.C() //@ inline(re"B1", b1result)
b.B2() //@ inline(re"B2", b2result)
+ b.B3() //@ inline(re"B3", b3result)
}
-- b2result --
@@ -73,4 +82,24 @@
fmt.Println()
b.B1() //@ inline(re"B1", b1result)
fmt.Println() //@ inline(re"B2", b2result)
+ b.B3() //@ inline(re"B3", b3result)
+}
+-- b3result --
+package a
+
+// This comment does not migrate.
+
+import (
+ "fmt"
+ "testdata/b"
+ e "testdata/d"
+)
+
+// Nor this one.
+
+func A() {
+ fmt.Println()
+ b.B1() //@ inline(re"B1", b1result)
+ b.B2() //@ inline(re"B2", b2result)
+ e.E() //@ inline(re"B3", b3result)
}
diff --git a/internal/refactor/inline/testdata/dotimport.txtar b/internal/refactor/inline/testdata/dotimport.txtar
index 8ca5f05..644398b 100644
--- a/internal/refactor/inline/testdata/dotimport.txtar
+++ b/internal/refactor/inline/testdata/dotimport.txtar
@@ -29,7 +29,7 @@
package c
import (
- a "testdata/a"
+ "testdata/a"
)
func _() {
diff --git a/internal/refactor/inline/testdata/issue63298.txtar b/internal/refactor/inline/testdata/issue63298.txtar
index 990ebcd..cc556c9 100644
--- a/internal/refactor/inline/testdata/issue63298.txtar
+++ b/internal/refactor/inline/testdata/issue63298.txtar
@@ -38,7 +38,7 @@
package a
import (
- b "testdata/b"
+ "testdata/b"
b0 "testdata/another/b"
//@ inline(re"a2", result)
diff --git a/internal/refactor/inline/testdata/revdotimport.txtar b/internal/refactor/inline/testdata/revdotimport.txtar
index 3838793..f33304f 100644
--- a/internal/refactor/inline/testdata/revdotimport.txtar
+++ b/internal/refactor/inline/testdata/revdotimport.txtar
@@ -32,8 +32,8 @@
package c
import (
+ "testdata/a"
. "testdata/a"
- a "testdata/a"
)
func _() {
