internal/refactor: AddImport: remove unnecessary result
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
index 00d87b0..fe1032c 100644
--- a/go/analysis/passes/inline/gofix.go
+++ b/go/analysis/passes/inline/gofix.go
@@ -288,7 +288,7 @@
} 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 @@
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
index d8d9b6e..3072cf6 100644
--- a/go/analysis/passes/modernize/maps.go
+++ b/go/analysis/passes/modernize/maps.go
@@ -166,7 +166,7 @@
// 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
index 52e58f2..032f874 100644
--- a/go/analysis/passes/modernize/slices.go
+++ b/go/analysis/passes/modernize/slices.go
@@ -164,7 +164,7 @@
//
// 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 @@
// 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
index 2d521dd..b3c2e56 100644
--- a/go/analysis/passes/modernize/slicescontains.go
+++ b/go/analysis/passes/modernize/slicescontains.go
@@ -202,7 +202,7 @@
}
// 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
index 305000f..b3e063d 100644
--- a/go/analysis/passes/modernize/slicesdelete.go
+++ b/go/analysis/passes/modernize/slicesdelete.go
@@ -62,7 +62,7 @@
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
index b2d04e1..66af16d 100644
--- a/go/analysis/passes/modernize/sortslice.go
+++ b/go/analysis/passes/modernize/sortslice.go
@@ -90,7 +90,7 @@
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
index ef128d5..56d0ba7 100644
--- a/go/analysis/passes/modernize/stringsbuilder.go
+++ b/go/analysis/passes/modernize/stringsbuilder.go
@@ -102,7 +102,7 @@
}
// 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 @@
// => 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
index b2a5ae7..9e76f95 100644
--- a/go/analysis/passes/modernize/stringscutprefix.go
+++ b/go/analysis/passes/modernize/stringscutprefix.go
@@ -128,7 +128,7 @@
// 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 @@
// 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
index 164fb27..7a02d85 100644
--- a/go/analysis/passes/stringintconv/string.go
+++ b/go/analysis/passes/stringintconv/string.go
@@ -198,7 +198,7 @@
// 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
index b5218ce..2284055 100644
--- a/gopls/internal/analysis/embeddirective/embeddirective.go
+++ b/gopls/internal/analysis/embeddirective/embeddirective.go
@@ -47,7 +47,7 @@
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
index e52567c..1ba3a96 100644
--- a/internal/refactor/imports.go
+++ b/internal/refactor/imports.go
@@ -17,28 +17,25 @@
"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 @@
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 @@
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
index db40c25..072a9a9 100644
--- a/internal/refactor/imports_test.go
+++ b/internal/refactor/imports_test.go
@@ -31,10 +31,9 @@
panic("runtime.Caller failed")
}
- // Each test case contains a «name pkgpath»
- // section to be replaced with a reference
- // to a valid import of pkgpath,
- // ideally of the specified name.
+ // Each test case contains a «name pkgpath member»
+ // triple to be replaced with a valid qualified identifier
+ // to pkgpath.member, ideally of the specified name.
for _, test := range []struct {
descr, src, want string
}{
@@ -42,13 +41,13 @@
descr: descr("simple add import"),
src: `package a
func _() {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
import "fmt"
func _() {
- fmt
+ fmt.Print
}`,
},
{
@@ -58,14 +57,14 @@
import "fmt"
func _(fmt.Stringer) {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
import "fmt"
func _(fmt.Stringer) {
- fmt
+ fmt.Print
}`,
},
{
@@ -75,7 +74,7 @@
import _ "fmt"
func _() {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -84,7 +83,7 @@
import _ "fmt"
func _() {
- fmt
+ fmt.Print
}`,
},
{
@@ -96,7 +95,7 @@
var fmt int
func _(fmtpkg.Stringer) {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -105,7 +104,7 @@
var fmt int
func _(fmtpkg.Stringer) {
- fmtpkg
+ fmtpkg.Print
}`,
},
{
@@ -117,7 +116,7 @@
var _ fmt.Stringer
func _(fmt int) {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -128,7 +127,7 @@
var _ fmt.Stringer
func _(fmt int) {
- fmt0
+ fmt0.Print
}`,
},
{
@@ -138,7 +137,7 @@
import "fmt"
func _(fmt fmt.Stringer) {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -147,7 +146,7 @@
import "fmt"
func _(fmt fmt.Stringer) {
- fmt0
+ fmt0.Print
}`,
},
{
@@ -159,7 +158,7 @@
// world
func _() {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -170,7 +169,7 @@
// world
func _() {
- fmt
+ fmt.Print
}`,
},
{
@@ -178,14 +177,14 @@
src: `package a
func _() {
- «foo encoding/json»
+ «foo encoding/json Marshal»
}`,
want: `package a
import foo "encoding/json"
func _() {
- foo
+ foo.Marshal
}`,
},
{
@@ -195,14 +194,14 @@
import . "fmt"
func _() {
- «. fmt»
+ «. fmt Print»
}`,
want: `package a
import . "fmt"
func _() {
- .
+ Print
}`,
},
{
@@ -212,7 +211,7 @@
import . "fmt"
func _(Print fmt.Stringer) {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -221,7 +220,7 @@
import . "fmt"
func _(Print fmt.Stringer) {
- fmt
+ fmt.Print
}`,
},
{
@@ -233,7 +232,7 @@
)
func _(io.Reader) {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -243,7 +242,7 @@
)
func _(io.Reader) {
- fmt
+ fmt.Print
}`,
},
{
@@ -257,7 +256,7 @@
)
func _(io.Reader) {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -269,7 +268,7 @@
)
func _(io.Reader) {
- fmt
+ fmt.Print
}`,
},
{
@@ -281,7 +280,7 @@
import "vendor/golang.org/x/net/dns/dnsmessage"
func _(io.Reader) {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -292,7 +291,7 @@
import "vendor/golang.org/x/net/dns/dnsmessage"
func _(io.Reader) {
- fmt
+ fmt.Print
}`,
},
{
@@ -305,7 +304,7 @@
)
func _(io.Reader) {
- «fmt fmt»
+ «fmt fmt Print»
}`,
want: `package a
@@ -317,19 +316,23 @@
)
func _(io.Reader) {
- fmt
+ fmt.Print
}`,
},
} {
t.Run(test.descr, func(t *testing.T) {
- // splice marker
+ // splice marker (name pkgpath member)
before, mid, ok1 := strings.Cut(test.src, "«")
mid, after, ok2 := strings.Cut(mid, "»")
if !ok1 || !ok2 {
- t.Fatal("no «name path» marker")
+ t.Fatal("no «name path member» marker")
}
src := before + "/*!*/" + after
- name, path, _ := strings.Cut(mid, " ")
+ fields := strings.Fields(mid)
+ if len(fields) != 3 {
+ t.Fatalf("splice marker needs 3 fields (got %q)", mid)
+ }
+ name, path, member := fields[0], fields[1], fields[2]
// parse
fset := token.NewFileSet()
@@ -355,9 +358,7 @@
}
conf.Check(f.Name.Name, fset, []*ast.File{f}, info)
- // add import
- // The "Print" argument is only relevant for dot-import tests.
- name, prefix, edits := refactor.AddImport(info, f, name, path, "Print", pos)
+ prefix, edits := refactor.AddImport(info, f, name, path, member, pos)
var edit analysis.TextEdit
switch len(edits) {
@@ -368,20 +369,11 @@
t.Fatalf("expected at most one edit, got %d", len(edits))
}
- // prefix is a simple function of name.
- wantPrefix := name + "."
- if name == "." {
- wantPrefix = ""
- }
- if prefix != wantPrefix {
- t.Errorf("got prefix %q, want %q", prefix, wantPrefix)
- }
-
// apply patch
start := fset.Position(edit.Pos)
end := fset.Position(edit.End)
output := src[:start.Offset] + string(edit.NewText) + src[end.Offset:]
- output = strings.ReplaceAll(output, "/*!*/", name)
+ output = strings.ReplaceAll(output, "/*!*/", prefix+member)
if output != test.want {
t.Errorf("\n--got--\n%s\n--want--\n%s\n--diff--\n%s",
output, test.want, cmp.Diff(test.want, output))
