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 <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

gopls/internal/regtest/marker/testdata/codeaction/removeparam_imports.txt

@@ -85,7 +85,7 @@ package a
 
 import (
 	"mod.test/b"
-	c "mod.test/c"
+	"mod.test/c"
 )
 
 func _() {
@@ -97,7 +97,7 @@ package a
 
 import (
 	"mod.test/b"
-	c "mod.test/c"
+	"mod.test/c"
 )
 
 func _() {
@@ -111,7 +111,7 @@ package a
 
 import (
 	"mod.test/b"
-	c "mod.test/c"
+	"mod.test/c"
 )
 
 func _() {
@@ -129,9 +129,9 @@ package a
 // 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 _() {

internal/refactor/inline/callee.go

@@ -59,9 +59,12 @@ type freeRef struct {
 
 // 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 @@ func AnalyzeCallee(logf func(string, ...any), fset *token.FileSet, pkg *types.Pa
 					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(),
 						})

internal/refactor/inline/inline.go

@@ -222,13 +222,13 @@ func Inline(logf func(string, ...any), caller *Caller, callee *Callee) ([]byte,
 			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 @@ func Inline(logf func(string, ...any), caller *Caller, callee *Callee) ([]byte,
 	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 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 	}
 
 	// 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 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 
 		newlyAdded := func(name string) bool {
 			for _, new := range newImports {
-				if new.Name.Name == name {
+				if new.pkgName == name {
 					return true
 				}
 			}
@@ -419,29 +424,32 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 		//
 		// "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 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 		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 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 
 			// 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),

internal/refactor/inline/testdata/crosspkg.txtar

@@ -22,22 +22,30 @@ func A() {
 	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 @@ package a
 import (
 	"fmt"
 	"testdata/b"
-	c "testdata/c"
+	"testdata/c"
 )
 
 // Nor this one.
@@ -55,6 +63,7 @@ func A() {
 	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 @@ func A() {
 	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)
 }

internal/refactor/inline/testdata/dotimport.txtar

@@ -29,7 +29,7 @@ func _() {
 package c
 
 import (
-	a "testdata/a"
+	"testdata/a"
 )
 
 func _() {

internal/refactor/inline/testdata/issue63298.txtar

@@ -38,7 +38,7 @@ func B() {}
 package a
 
 import (
-	b "testdata/b"
+	"testdata/b"
 	b0 "testdata/another/b"
 
 	//@ inline(re"a2", result)

internal/refactor/inline/testdata/revdotimport.txtar

@@ -32,8 +32,8 @@ func _() {
 package c
 
 import (
+	"testdata/a"
 	. "testdata/a"
-	a "testdata/a"
 )
 
 func _() {