internal/refactor/inline: fix pkgname shadowing bug

In cross-package inlining, when replacing free names with
qualified identifiers, we failed to check for intervening
names defined in the callee that might cast a shadow over the
reference. This change re-uses the logic for shadow
detection when replacing parameters with arguments.

Plus tests.

Fixes golang/go#62667

Change-Id: Ib6a620ed2cde313bf51d5fb8a0cd9363f9eadf6e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/531455
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

internal/refactor/inline/callee.go

@@ -58,10 +58,11 @@ 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")
-	ValidPos bool   // Object.Pos().IsValid()
+	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")
+	ValidPos bool            // Object.Pos().IsValid()
+	Shadow   map[string]bool // names shadowed at one of the object's refs
 }
 
 // AnalyzeCallee analyzes a function that is a candidate for inlining
@@ -119,7 +120,14 @@ func AnalyzeCallee(logf func(string, ...any), fset *token.FileSet, pkg *types.Pa
 	)
 	var f func(n ast.Node) bool
 	visit := func(n ast.Node) { ast.Inspect(n, f) }
+	var stack []ast.Node
+	stack = append(stack, decl.Type) // for scope of function itself
 	f = func(n ast.Node) bool {
+		if n != nil {
+			stack = append(stack, n) // push
+		} else {
+			stack = stack[:len(stack)-1] // pop
+		}
 		switch n := n.(type) {
 		case *ast.SelectorExpr:
 			// Check selections of free fields/methods.
@@ -198,6 +206,8 @@ func AnalyzeCallee(logf func(string, ...any), fset *token.FileSet, pkg *types.Pa
 						freeObjIndex[obj] = objidx
 					}
 
+					freeObjs[objidx].Shadow = addShadows(freeObjs[objidx].Shadow, info, obj.Name(), stack)
+
 					freeRefs = append(freeRefs, freeRef{
 						Offset: int(n.Pos() - decl.Pos()),
 						Object: objidx,
@@ -417,6 +427,9 @@ func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.I
 
 	// Record locations of all references to parameters.
 	// And record the set of intervening definitions for each parameter.
+	//
+	// TODO(adonovan): combine this traversal with the one that computes
+	// FreeRefs. The tricky part is that calleefx needs this one first.
 	var stack []ast.Node
 	stack = append(stack, decl.Type) // for scope of function itself
 	ast.Inspect(decl.Body, func(n ast.Node) bool {
@@ -429,26 +442,11 @@ func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.I
 		if id, ok := n.(*ast.Ident); ok {
 			if v, ok := info.Uses[id].(*types.Var); ok {
 				if pinfo, ok := paramInfos[v]; ok {
-					// Record location of ref to parameter/result.
+					// Record location of ref to parameter/result
+					// and any intervening (shadowing) names.
 					offset := int(n.Pos() - decl.Pos())
 					pinfo.Refs = append(pinfo.Refs, offset)
-
-					// Find set of names shadowed within body
-					// (excluding the parameter itself).
-					// If these names are free in the arg expression,
-					// we can't substitute the parameter.
-					for _, n := range stack {
-						if scope, ok := info.Scopes[n]; ok {
-							for _, name := range scope.Names() {
-								if name != pinfo.Name {
-									if pinfo.Shadow == nil {
-										pinfo.Shadow = make(map[string]bool)
-									}
-									pinfo.Shadow[name] = true
-								}
-							}
-						}
-					}
+					pinfo.Shadow = addShadows(pinfo.Shadow, info, pinfo.Name, stack)
 				}
 			}
 		}
@@ -465,6 +463,29 @@ func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.I
 
 // -- callee helpers --
 
+// addShadows returns the shadows set augmented by the set of names
+// locally shadowed at the location of the reference in the callee
+// (identified by the stack). The name of the reference itself is
+// excluded.
+//
+// These shadowed names may not be used in a replacement expression
+// for the reference.
+func addShadows(shadows map[string]bool, info *types.Info, exclude string, stack []ast.Node) map[string]bool {
+	for _, n := range stack {
+		if scope, ok := info.Scopes[n]; ok {
+			for _, name := range scope.Names() {
+				if name != exclude {
+					if shadows == nil {
+						shadows = make(map[string]bool)
+					}
+					shadows[name] = true
+				}
+			}
+		}
+	}
+	return shadows
+}
+
 // deref removes a pointer type constructor from the core type of t.
 func deref(t types.Type) types.Type {
 	if ptr, ok := typeparams.CoreType(t).(*types.Pointer); ok {

internal/refactor/inline/inline.go

@@ -291,14 +291,17 @@ 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) string {
+	localImportName := func(path string, shadows map[string]bool) string {
 		// Does an import exist?
 		for _, name := range importMap[path] {
 			// Check that either the import preexisted,
-			// or that it was newly added (no PkgName) but is not shadowed.
-			found := caller.lookup(name)
-			if is[*types.PkgName](found) || found == nil {
-				return name
+			// or that it was newly added (no PkgName) but is not shadowed,
+			// either in the callee (shadows) or caller (caller.lookup).
+			if !shadows[name] {
+				found := caller.lookup(name)
+				if is[*types.PkgName](found) || found == nil {
+					return name
+				}
 			}
 		}
 
@@ -313,7 +316,7 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 		// use the package's declared name.
 		base := pathpkg.Base(path)
 		name := base
-		for n := 0; caller.lookup(name) != nil; n++ {
+		for n := 0; shadows[name] || caller.lookup(name) != nil; n++ {
 			name = fmt.Sprintf("%s%d", base, n)
 		}
 
@@ -344,7 +347,7 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 		//   => check not shadowed in caller.
 		// - package-level var/func/const/types
 		//   => same package: check not shadowed in caller.
-		//   => otherwise: import other package form a qualified identifier.
+		//   => otherwise: import other package, form a qualified identifier.
 		//      (Unexported cross-package references were rejected already.)
 		// - type parameter
 		//   => not yet supported
@@ -353,10 +356,14 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 		//
 		// There can be no free references to labels, fields, or methods.
 
+		// Note that we must consider potential shadowing both
+		// at the caller side (caller.lookup) and, when
+		// choosing new PkgNames, within the callee (obj.shadow).
+
 		var newName ast.Expr
 		if obj.Kind == "pkgname" {
 			// Use locally appropriate import, creating as needed.
-			newName = makeIdent(localImportName(obj.PkgPath)) // imported package
+			newName = makeIdent(localImportName(obj.PkgPath, obj.Shadow)) // imported package
 
 		} else if !obj.ValidPos {
 			// Built-in function, type, or value (e.g. nil, zero):
@@ -396,7 +403,7 @@ func inline(logf func(string, ...any), caller *Caller, callee *gobCallee) (*resu
 
 			// Form a qualified identifier, pkg.Name.
 			if qualify {
-				pkgName := localImportName(obj.PkgPath)
+				pkgName := localImportName(obj.PkgPath, obj.Shadow)
 				newName = &ast.SelectorExpr{
 					X:   makeIdent(pkgName),
 					Sel: makeIdent(obj.Name),

internal/refactor/inline/testdata/import-shadow.txtar

@@ -2,13 +2,10 @@ Just because a package (e.g. log) is imported by the caller,
 and the name log is in scope, doesn't mean the name in scope
 refers to the package: it could be locally shadowed.
 
-Two scenarios below:
-
-1. a second (renaming) import is added because the first import is
-   locally shadowed.
-
-2. a new import is added with a fresh name because the default
-   name is locally shadowed.
+In all three scenarios below, renaming import with a fresh name is
+added because the usual name is locally shadowed: in cases 1, 2 an
+existing import is shadowed by (respectively) a local constant,
+parameter; in case 3 there is no existing import.
 
 -- go.mod --
 module testdata
@@ -95,3 +92,34 @@ func A(b int) {
 		b0.Two()
 	} //@ inline(re"F", fresult)
 }
+
+-- d/d.go --
+package d
+
+import "testdata/e"
+
+func D() {
+	const log = "shadow"
+	e.E() //@ inline(re"E", eresult)
+}
+
+-- e/e.go --
+package e
+
+import "log"
+
+func E() {
+	log.Printf("")
+}
+
+-- eresult --
+package d
+
+import (
+	log0 "log"
+)
+
+func D() {
+	const log = "shadow"
+	log0.Printf("") //@ inline(re"E", eresult)
+}

internal/refactor/inline/testdata/issue62667.txtar

@@ -0,0 +1,44 @@
+Regression test for #62667: the callee's reference to Split
+was blindly qualified to path.Split even though the imported
+PkgName path is shadowed by the parameter of the same name.
+
+The defer is to defeat reduction of the call and
+substitution of the path parameter by g().
+
+-- go.mod --
+module testdata
+go 1.12
+
+-- a/a.go --
+package a
+
+import "testdata/path"
+
+func A() {
+	path.Dir(g()) //@ inline(re"Dir", result)
+}
+
+func g() string
+
+-- path/path.go --
+package path
+
+func Dir(path string) {
+	defer func(){}()
+	Split(path)
+}
+
+func Split(string) {}
+
+-- result --
+package a
+
+import (
+	path0 "testdata/path"
+)
+
+func A() {
+	func(path string) { defer func() {}(); path0.Split(path) }(g()) //@ inline(re"Dir", result)
+}
+
+func g() string