internal/refactor: AddImport: remove unnecessary result

adonovan · gopherbot · commit 122c93afe118 · 2025-10-08T13:48:54.000-07:00
And improve doc comment, and simplify the test. Change-Id: I8ea0e3e71152245d2b3e675585175a84dc18b750 Reviewed-on: https://go-review.googlesource.com/c/tools/+/710315 Reviewed-by: Robert Findley <rfindley@google.com> Auto-Submit: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/go/analysis/passes/inline/gofix.go b/go/analysis/passes/inline/gofix.go
@@ -288,7 +288,7 @@ func (a *analyzer) inlineAlias(tn *types.TypeName, curId inspector.Cursor) {
 		} else if _, ok := importPrefixes[pkgPath]; !ok {
 			// Use AddImport to add pkgPath if it's not there already. Associate the prefix it assigns
 			// with the package path for use by the TypeString qualifier below.
-			_, prefix, eds := refactor.AddImport(
+			prefix, eds := refactor.AddImport(
 				a.pass.TypesInfo, curFile, pkgName, pkgPath, tn.Name(), id.Pos())
 			importPrefixes[pkgPath] = strings.TrimSuffix(prefix, ".")
 			edits = append(edits, eds...)
@@ -459,7 +459,7 @@ func (a *analyzer) inlineConst(con *types.Const, cur inspector.Cursor) {
 		edits        []analysis.TextEdit
 	)
 	if incon.RHSPkgPath != a.pass.Pkg.Path() {
-		_, importPrefix, edits = refactor.AddImport(
+		importPrefix, edits = refactor.AddImport(
 			a.pass.TypesInfo, curFile, incon.RHSPkgName, incon.RHSPkgPath, incon.RHSName, n.Pos())
 	}
 	// If n is qualified by a package identifier, we'll need the full selector expression.
diff --git a/go/analysis/passes/modernize/maps.go b/go/analysis/passes/modernize/maps.go
@@ -166,7 +166,7 @@ func mapsloop(pass *analysis.Pass) (any, error) {
 
 		// Report diagnostic, and suggest fix.
 		rng := curRange.Node()
-		_, prefix, importEdits := refactor.AddImport(info, file, "maps", "maps", funcName, rng.Pos())
+		prefix, importEdits := refactor.AddImport(info, file, "maps", "maps", funcName, rng.Pos())
 		var (
 			newText    []byte
 			start, end token.Pos
diff --git a/go/analysis/passes/modernize/slices.go b/go/analysis/passes/modernize/slices.go
@@ -164,7 +164,7 @@ func appendclipped(pass *analysis.Pass) (any, error) {
 			//
 			// This is unsound if s is empty and its nilness
 			// differs from zerocap (#73557).
-			_, prefix, importEdits := refactor.AddImport(info, file, clonepkg, clonepkg, "Clone", call.Pos())
+			prefix, importEdits := refactor.AddImport(info, file, clonepkg, clonepkg, "Clone", call.Pos())
 			message := fmt.Sprintf("Replace append with %s.Clone", clonepkg)
 			pass.Report(analysis.Diagnostic{
 				Pos:     call.Pos(),
@@ -185,7 +185,7 @@ func appendclipped(pass *analysis.Pass) (any, error) {
 		// append(append(append(base, a...), b..., c...) -> slices.Concat(base, a, b, c)
 		//
 		// This is unsound if all slices are empty and base is non-nil (#73557).
-		_, prefix, importEdits := refactor.AddImport(info, file, "slices", "slices", "Concat", call.Pos())
+		prefix, importEdits := refactor.AddImport(info, file, "slices", "slices", "Concat", call.Pos())
 		pass.Report(analysis.Diagnostic{
 			Pos:     call.Pos(),
 			End:     call.End(),
diff --git a/go/analysis/passes/modernize/slicescontains.go b/go/analysis/passes/modernize/slicescontains.go
@@ -202,7 +202,7 @@ func slicescontains(pass *analysis.Pass) (any, error) {
 		}
 
 		// Prepare slices.Contains{,Func} call.
-		_, prefix, importEdits := refactor.AddImport(info, file, "slices", "slices", funcName, rng.Pos())
+		prefix, importEdits := refactor.AddImport(info, file, "slices", "slices", funcName, rng.Pos())
 		contains := fmt.Sprintf("%s%s(%s, %s)",
 			prefix,
 			funcName,
diff --git a/go/analysis/passes/modernize/slicesdelete.go b/go/analysis/passes/modernize/slicesdelete.go
@@ -62,7 +62,7 @@ func slicesdelete(pass *analysis.Pass) (any, error) {
 			return false
 		}
 
-		_, prefix, edits := refactor.AddImport(info, file, "slices", "slices", "Delete", call.Pos())
+		prefix, edits := refactor.AddImport(info, file, "slices", "slices", "Delete", call.Pos())
 		// append's indices may be any integer type; slices.Delete requires int.
 		// Insert int conversions as needed (and if possible).
 		if isIntShadowed() && (!isIntExpr(slice1.High) || !isIntExpr(slice2.Low)) {
diff --git a/go/analysis/passes/modernize/sortslice.go b/go/analysis/passes/modernize/sortslice.go
@@ -90,7 +90,7 @@ func slicessort(pass *analysis.Pass) (any, error) {
 						fileUses(info, file, "go1.21") {
 						// Have: sort.Slice(s, func(i, j int) bool { return s[i] < s[j] })
 
-						_, prefix, importEdits := refactor.AddImport(
+						prefix, importEdits := refactor.AddImport(
 							info, file, "slices", "slices", "Sort", call.Pos())
 
 						pass.Report(analysis.Diagnostic{
diff --git a/go/analysis/passes/modernize/stringsbuilder.go b/go/analysis/passes/modernize/stringsbuilder.go
@@ -102,7 +102,7 @@ nextcand:
 			}
 
 			// Add strings import.
-			_, prefix, importEdits := refactor.AddImport(
+			prefix, importEdits := refactor.AddImport(
 				pass.TypesInfo, astutil.EnclosingFile(def), "strings", "strings", "Builder", v.Pos())
 			edits = append(edits, importEdits...)
 
@@ -141,7 +141,7 @@ nextcand:
 			// => var s strings.Builder; s.WriteString(expr)
 
 			// Add strings import.
-			_, prefix, importEdits := refactor.AddImport(
+			prefix, importEdits := refactor.AddImport(
 				pass.TypesInfo, astutil.EnclosingFile(def), "strings", "strings", "Builder", v.Pos())
 			edits = append(edits, importEdits...)
 
diff --git a/go/analysis/passes/modernize/stringscutprefix.go b/go/analysis/passes/modernize/stringscutprefix.go
@@ -128,7 +128,7 @@ func stringscutprefix(pass *analysis.Pass) (any, error) {
 					// shadow variables won't be valid because we only access the first statement (ditto Suffix).
 					if astutil.EqualSyntax(s0, s) && astutil.EqualSyntax(pre0, pre) {
 						after := refactor.FreshName(info.Scopes[ifStmt], ifStmt.Pos(), varName)
-						_, prefix, importEdits := refactor.AddImport(
+						prefix, importEdits := refactor.AddImport(
 							info,
 							curFile.Node().(*ast.File),
 							obj1.Pkg().Name(),
@@ -213,7 +213,7 @@ func stringscutprefix(pass *analysis.Pass) (any, error) {
 
 						// We use AddImport not to add an import (since it exists already)
 						// but to compute the correct prefix in the dot-import case.
-						_, prefix, importEdits := refactor.AddImport(
+						prefix, importEdits := refactor.AddImport(
 							info,
 							curFile.Node().(*ast.File),
 							obj.Pkg().Name(),
diff --git a/go/analysis/passes/stringintconv/string.go b/go/analysis/passes/stringintconv/string.go
@@ -198,7 +198,7 @@ func run(pass *analysis.Pass) (any, error) {
 		// the type has methods, as some {String,GoString,Format}
 		// may change the behavior of fmt.Sprint.
 		if len(ttypes) == 1 && len(vtypes) == 1 && types.NewMethodSet(V0).Len() == 0 {
-			_, prefix, importEdits := refactor.AddImport(pass.TypesInfo, file, "fmt", "fmt", "Sprint", arg.Pos())
+			prefix, importEdits := refactor.AddImport(pass.TypesInfo, file, "fmt", "fmt", "Sprint", arg.Pos())
 			if types.Identical(T0, types.Typ[types.String]) {
 				// string(x) -> fmt.Sprint(x)
 				addFix("Format the number as a decimal", append(importEdits,
diff --git a/gopls/internal/analysis/embeddirective/embeddirective.go b/gopls/internal/analysis/embeddirective/embeddirective.go
@@ -47,7 +47,7 @@ func run(pass *analysis.Pass) (any, error) {
 
 			if !hasEmbedImport {
 				// Add blank import of "embed".
-				_, _, edits := refactor.AddImport(pass.TypesInfo, f, "_", "embed", "", c.Pos())
+				_, edits := refactor.AddImport(pass.TypesInfo, f, "_", "embed", "", c.Pos())
 				if len(edits) > 0 {
 					pass.Report(analysis.Diagnostic{
 						Pos:     pos,
diff --git a/internal/refactor/imports.go b/internal/refactor/imports.go
@@ -17,28 +17,25 @@ import (
 	"golang.org/x/tools/internal/analysisinternal"
 )
 
-// AddImport checks whether this file already imports pkgpath and that
-// the import is in scope at pos. If so, it returns the name under
-// which it was imported and no edits. Otherwise, it adds a new import
-// of pkgpath, using a name derived from the preferred name, and
-// returns the chosen name, a prefix to be concatenated with member to
-// form a qualified name, and the edit for the new import.
+// AddImport returns the prefix (either "pkg." or "") that should be
+// used to qualify references to the desired symbol (member) imported
+// from the specified package, plus any necessary edits to the file's
+// import declaration to add a new import.
 //
-// The member argument indicates the name of the desired symbol within
-// the imported package. This is needed in the case when the existing
-// import is a dot import, because then it is possible that the
-// desired symbol is shadowed by other declarations in the current
-// package. If member is not shadowed at pos, AddImport returns (".",
-// "", nil). (AddImport accepts the caller's implicit claim that the
-// imported package declares member.)
+// If the import already exists, and is accessible at pos, AddImport
+// returns the existing name and no edits. (If the existing import is
+// a dot import, the prefix is "".)
 //
-// Use a preferredName of "_" to request a blank import;
-// member is ignored in this case.
+// Otherwise, it adds a new import, using a local name derived from
+// the preferred name. To request a blank import, use a preferredName
+// of "_", and discard the prefix result; member is ignored in this
+// case.
 //
-// It does not mutate its arguments.
+// AddImport accepts the caller's implicit claim that the imported
+// package declares member.
 //
-// TODO(adonovan): needs dedicated tests.
-func AddImport(info *types.Info, file *ast.File, preferredName, pkgpath, member string, pos token.Pos) (name, prefix string, newImport []analysis.TextEdit) {
+// AddImport does not mutate its arguments.
+func AddImport(info *types.Info, file *ast.File, preferredName, pkgpath, member string, pos token.Pos) (prefix string, edits []analysis.TextEdit) {
 	// Find innermost enclosing lexical block.
 	scope := info.Scopes[file].Innermost(pos)
 	if scope == nil {
@@ -50,18 +47,18 @@ func AddImport(info *types.Info, file *ast.File, preferredName, pkgpath, member
 	for _, spec := range file.Imports {
 		pkgname := info.PkgNameOf(spec)
 		if pkgname != nil && pkgname.Imported().Path() == pkgpath {
-			name = pkgname.Name()
+			name := pkgname.Name()
 			if preferredName == "_" {
 				// Request for blank import; any existing import will do.
-				return name, "", nil
+				return "", nil
 			}
 			if name == "." {
 				// The scope of ident must be the file scope.
 				if s, _ := scope.LookupParent(member, pos); s == info.Scopes[file] {
-					return name, "", nil
+					return "", nil
 				}
 			} else if _, obj := scope.LookupParent(name, pos); obj == pkgname {
-				return name, name + ".", nil
+				return name + ".", nil
 			}
 		}
 	}
@@ -122,7 +119,7 @@ func AddImport(info *types.Info, file *ast.File, preferredName, pkgpath, member
 		pos = before.Pos()
 		newText = "import " + newText + "\n\n"
 	}
-	return newName, newName + ".", []analysis.TextEdit{{
+	return newName + ".", []analysis.TextEdit{{
 		Pos:     pos,
 		End:     pos,
 		NewText: []byte(newText),
diff --git a/internal/refactor/imports_test.go b/internal/refactor/imports_test.go

Original file line number	Diff line number	Diff line change
`@@ -202,7 +202,7 @@ func slicescontains(pass *analysis.Pass) (any, error) {`
`202`	`202`	`}`
`203`	`203`
`204`	`204`	`// Prepare slices.Contains{,Func} call.`
`205`		`- _, prefix, importEdits := refactor.AddImport(info, file, "slices", "slices", funcName, rng.Pos())`
	`205`	`+ prefix, importEdits := refactor.AddImport(info, file, "slices", "slices", funcName, rng.Pos())`
`206`	`206`	`contains := fmt.Sprintf("%s%s(%s, %s)",`
`207`	`207`	`prefix,`
`208`	`208`	`funcName,`
Original file line number	Diff line number	Diff line change
`@@ -62,7 +62,7 @@ func slicesdelete(pass *analysis.Pass) (any, error) {`
`62`	`62`	`return false`
`63`	`63`	`}`
`64`	`64`
`65`		`- _, prefix, edits := refactor.AddImport(info, file, "slices", "slices", "Delete", call.Pos())`
	`65`	`+ prefix, edits := refactor.AddImport(info, file, "slices", "slices", "Delete", call.Pos())`
`66`	`66`	`// append's indices may be any integer type; slices.Delete requires int.`
`67`	`67`	`// Insert int conversions as needed (and if possible).`
`68`	`68`	`if isIntShadowed() && (!isIntExpr(slice1.High) \|\| !isIntExpr(slice2.Low)) {`