internal/lsp: handle different package names in signature help

There were a few cases where we were not properly qualifying package
names, particularly if the original package had a named import. Now,
we map between these names correctly - handling the case of multiple
packages that need to be qualified. This requires applying edits to
*ast.SelectorExprs, as well as *ast.Idents.

We still do not fully qualify unimported packages, and likely won't,
unless that's an issue for many users.

Updates golang/go#38591

Change-Id: I966a4d1f936f37ede89362d03da3ff98d8952a06
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229979
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/source/completion_format.go

@@ -44,7 +44,7 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e
 	// expandFuncCall mutates the completion label, detail, and snippet
 	// to that of an invocation of sig.
 	expandFuncCall := func(sig *types.Signature) error {
-		s, err := newSignature(ctx, c.snapshot, c.pkg, "", sig, nil, c.qf)
+		s, err := newSignature(ctx, c.snapshot, c.pkg, c.file, "", sig, nil, c.qf)
 		if err != nil {
 			return err
 		}
@@ -67,7 +67,7 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e
 		if _, ok := obj.Type().(*types.Struct); ok {
 			detail = "struct{...}" // for anonymous structs
 		} else if obj.IsField() {
-			detail = formatVarType(ctx, c.snapshot, c.pkg, obj, c.qf)
+			detail = formatVarType(ctx, c.snapshot, c.pkg, c.file, obj, c.qf)
 		}
 		if obj.IsField() {
 			kind = protocol.FieldCompletion

internal/lsp/source/signature_help.go

@@ -121,7 +121,7 @@ FindCall:
 	} else {
 		name = "func"
 	}
-	s, err := newSignature(ctx, snapshot, pkg, name, sig, comment, qf)
+	s, err := newSignature(ctx, snapshot, pkg, file, name, sig, comment, qf)
 	if err != nil {
 		return nil, 0, err
 	}

internal/lsp/source/types_format.go

@@ -10,9 +10,9 @@ import (
 	"fmt"
 	"go/ast"
 	"go/printer"
+	"go/token"
 	"go/types"
 	"strings"
-	"unicode"
 
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/internal/event"
@@ -150,11 +150,11 @@ func formatFieldList(ctx context.Context, view View, list *ast.FieldList, variad
 	return result, writeResultParens
 }
 
-func newSignature(ctx context.Context, s Snapshot, pkg Package, name string, sig *types.Signature, comment *ast.CommentGroup, qf types.Qualifier) (*signature, error) {
+func newSignature(ctx context.Context, s Snapshot, pkg Package, file *ast.File, name string, sig *types.Signature, comment *ast.CommentGroup, qf types.Qualifier) (*signature, error) {
 	params := make([]string, 0, sig.Params().Len())
 	for i := 0; i < sig.Params().Len(); i++ {
 		el := sig.Params().At(i)
-		typ := formatVarType(ctx, s, pkg, el, qf)
+		typ := formatVarType(ctx, s, pkg, file, el, qf)
 		p := typ
 		if el.Name() != "" {
 			p = el.Name() + " " + typ
@@ -168,7 +168,7 @@ func newSignature(ctx context.Context, s Snapshot, pkg Package, name string, sig
 			needResultParens = true
 		}
 		el := sig.Results().At(i)
-		typ := formatVarType(ctx, s, pkg, el, qf)
+		typ := formatVarType(ctx, s, pkg, file, el, qf)
 		if el.Name() == "" {
 			results = append(results, typ)
 		} else {
@@ -194,7 +194,7 @@ func newSignature(ctx context.Context, s Snapshot, pkg Package, name string, sig
 // formatVarType formats a *types.Var, accounting for type aliases.
 // To do this, it looks in the AST of the file in which the object is declared.
 // On any errors, it always fallbacks back to types.TypeString.
-func formatVarType(ctx context.Context, s Snapshot, srcpkg Package, obj *types.Var, qf types.Qualifier) string {
+func formatVarType(ctx context.Context, s Snapshot, srcpkg Package, srcfile *ast.File, obj *types.Var, qf types.Qualifier) string {
 	file, pkg, err := findPosInPackage(s.View(), srcpkg, obj.Pos())
 	if err != nil {
 		return types.TypeString(obj.Type(), qf)
@@ -214,9 +214,12 @@ func formatVarType(ctx context.Context, s Snapshot, srcpkg Package, obj *types.V
 	// Determine the package name to use based on the package that originated
 	// the query and the package in which the type is declared.
 	// We then qualify the value by cloning the AST node and editing it.
-	pkgName := importedPkgName(s, srcpkg, pkg, file)
-	cloned := cloneExpr(expr)
-	qualified := qualifyExpr(cloned, pkgName)
+	clonedInfo := make(map[token.Pos]*types.PkgName)
+	qualified := cloneExpr(expr, pkg.GetTypesInfo(), clonedInfo)
+
+	// If the request came from a different package than the one in which the
+	// types are defined, we may need to modify the qualifiers.
+	qualified = qualifyExpr(s.View().Session().Cache().FileSet(), qualified, srcpkg, pkg, srcfile, clonedInfo)
 	fmted := formatNode(s.View().Session().Cache().FileSet(), qualified)
 	return fmted
 }
@@ -262,29 +265,8 @@ func namedVarType(ctx context.Context, s Snapshot, pkg Package, file *ast.File,
 	return typ, nil
 }
 
-// importedPkgName returns the package name used for pkg in srcpkg.
-func importedPkgName(s Snapshot, srcpkg, pkg Package, file *ast.File) string {
-	if srcpkg == pkg {
-		return ""
-	}
-	// If the file already imports the package under another name, use that.
-	for _, group := range astutil.Imports(s.View().Session().Cache().FileSet(), file) {
-		for _, cand := range group {
-			if strings.Trim(cand.Path.Value, `"`) == pkg.PkgPath() {
-				if cand.Name != nil && cand.Name.Name != "" {
-					return cand.Name.Name
-				}
-			}
-		}
-	}
-	return pkg.GetTypes().Name()
-}
-
 // qualifyExpr applies the "pkgName." prefix to any *ast.Ident in the expr.
-func qualifyExpr(expr ast.Expr, pkgName string) ast.Expr {
-	if pkgName == "" {
-		return expr
-	}
+func qualifyExpr(fset *token.FileSet, expr ast.Expr, srcpkg, pkg Package, file *ast.File, clonedInfo map[token.Pos]*types.PkgName) ast.Expr {
 	ast.Inspect(expr, func(n ast.Node) bool {
 		switch n := n.(type) {
 		case *ast.ArrayType, *ast.ChanType, *ast.Ellipsis,
@@ -295,11 +277,28 @@ func qualifyExpr(expr ast.Expr, pkgName string) ast.Expr {
 			// modify. This is not an ideal approach, but it works for now.
 			return true
 		case *ast.SelectorExpr:
-			// Don't add qualifiers to selectors.
+			// We may need to change any selectors in which the X is a package
+			// name and the Sel is exported.
+			x, ok := n.X.(*ast.Ident)
+			if !ok {
+				return false
+			}
+			obj, ok := clonedInfo[x.Pos()]
+			if !ok {
+				return false
+			}
+			pkgName := importedPkgName(fset, srcpkg.GetTypes(), obj.Imported(), file)
+			if pkgName != "" {
+				x.Name = pkgName
+			}
 			return false
 		case *ast.Ident:
+			if srcpkg == pkg {
+				return false
+			}
 			// Only add the qualifier if the identifier is exported.
-			if unicode.IsUpper(rune(n.Name[0])) {
+			if ast.IsExported(n.Name) {
+				pkgName := importedPkgName(fset, srcpkg.GetTypes(), pkg.GetTypes(), file)
 				n.Name = pkgName + "." + n.Name
 			}
 		}
@@ -308,20 +307,42 @@ func qualifyExpr(expr ast.Expr, pkgName string) ast.Expr {
 	return expr
 }
 
+// importedPkgName returns the package name used for pkg in srcpkg.
+func importedPkgName(fset *token.FileSet, srcpkg, pkg *types.Package, file *ast.File) string {
+	if srcpkg == pkg {
+		return ""
+	}
+	// If the file already imports the package under another name, use that.
+	for _, group := range astutil.Imports(fset, file) {
+		for _, cand := range group {
+			if strings.Trim(cand.Path.Value, `"`) == pkg.Path() {
+				if cand.Name != nil && cand.Name.Name != "" {
+					return cand.Name.Name
+				}
+			}
+		}
+	}
+	return pkg.Name()
+}
+
 // cloneExpr only clones expressions that appear in the parameters or return
 // values of a function declaration. The original expression may be returned
 // to the caller in 2 cases:
 //    (1) The expression has no pointer fields.
 //    (2) The expression cannot appear in an *ast.FuncType, making it
 //        unnecessary to clone.
+// This function also keeps track of selector expressions in which the X is a
+// package name and marks them in a map along with their type information, so
+// that this information can be used when rewriting the expression.
+//
 // NOTE: This function is tailored to the use case of qualifyExpr, and should
 // be used with caution.
-func cloneExpr(expr ast.Expr) ast.Expr {
+func cloneExpr(expr ast.Expr, info *types.Info, clonedInfo map[token.Pos]*types.PkgName) ast.Expr {
 	switch expr := expr.(type) {
 	case *ast.ArrayType:
 		return &ast.ArrayType{
 			Lbrack: expr.Lbrack,
-			Elt:    cloneExpr(expr.Elt),
+			Elt:    cloneExpr(expr.Elt, info, clonedInfo),
 			Len:    expr.Len,
 		}
 	case *ast.ChanType:
@@ -329,56 +350,60 @@ func cloneExpr(expr ast.Expr) ast.Expr {
 			Arrow: expr.Arrow,
 			Begin: expr.Begin,
 			Dir:   expr.Dir,
-			Value: cloneExpr(expr.Value),
+			Value: cloneExpr(expr.Value, info, clonedInfo),
 		}
 	case *ast.Ellipsis:
 		return &ast.Ellipsis{
 			Ellipsis: expr.Ellipsis,
-			Elt:      cloneExpr(expr.Elt),
+			Elt:      cloneExpr(expr.Elt, info, clonedInfo),
 		}
 	case *ast.FuncType:
 		return &ast.FuncType{
 			Func:    expr.Func,
-			Params:  cloneFieldList(expr.Params),
-			Results: cloneFieldList(expr.Results),
+			Params:  cloneFieldList(expr.Params, info, clonedInfo),
+			Results: cloneFieldList(expr.Results, info, clonedInfo),
 		}
 	case *ast.Ident:
 		return cloneIdent(expr)
 	case *ast.MapType:
 		return &ast.MapType{
 			Map:   expr.Map,
-			Key:   cloneExpr(expr.Key),
-			Value: cloneExpr(expr.Value),
+			Key:   cloneExpr(expr.Key, info, clonedInfo),
+			Value: cloneExpr(expr.Value, info, clonedInfo),
 		}
 	case *ast.ParenExpr:
 		return &ast.ParenExpr{
 			Lparen: expr.Lparen,
 			Rparen: expr.Rparen,
-			X:      cloneExpr(expr.X),
+			X:      cloneExpr(expr.X, info, clonedInfo),
 		}
 	case *ast.SelectorExpr:
-		return &ast.SelectorExpr{
+		s := &ast.SelectorExpr{
 			Sel: cloneIdent(expr.Sel),
-			X:   cloneExpr(expr.X),
+			X:   cloneExpr(expr.X, info, clonedInfo),
+		}
+		if x, ok := expr.X.(*ast.Ident); ok && ast.IsExported(expr.Sel.Name) {
+			if obj, ok := info.ObjectOf(x).(*types.PkgName); ok {
+				clonedInfo[s.X.Pos()] = obj
+			}
+
 		}
 	case *ast.StarExpr:
 		return &ast.StarExpr{
 			Star: expr.Star,
-			X:    cloneExpr(expr.X),
+			X:    cloneExpr(expr.X, info, clonedInfo),
 		}
 	case *ast.StructType:
 		return &ast.StructType{
 			Struct:     expr.Struct,
-			Fields:     cloneFieldList(expr.Fields),
+			Fields:     cloneFieldList(expr.Fields, info, clonedInfo),
 			Incomplete: expr.Incomplete,
 		}
-	default:
-		// Not in function literals or don't need to be cloned.
-		return expr
 	}
+	return expr
 }
 
-func cloneFieldList(fl *ast.FieldList) *ast.FieldList {
+func cloneFieldList(fl *ast.FieldList, info *types.Info, clonedInfo map[token.Pos]*types.PkgName) *ast.FieldList {
 	if fl == nil {
 		return nil
 	}
@@ -399,7 +424,7 @@ func cloneFieldList(fl *ast.FieldList) *ast.FieldList {
 			Doc:     f.Doc,
 			Names:   names,
 			Tag:     f.Tag,
-			Type:    cloneExpr(f.Type),
+			Type:    cloneExpr(f.Type, info, clonedInfo),
 		})
 	}
 	return &ast.FieldList{

internal/lsp/testdata/lsp/primarymod/signature/signature_test.go

@@ -3,11 +3,11 @@ package signature_test
 import (
 	"testing"
 
-	"golang.org/x/tools/internal/lsp/signature"
+	sig "golang.org/x/tools/internal/lsp/signature"
 )
 
 func TestSignature(t *testing.T) {
-	signature.AliasSlice()    //@signature(")", "AliasSlice(a []*signature.Alias) (b signature.Alias)", 0)
-	signature.AliasMap()      //@signature(")", "AliasMap(a map[*signature.Alias]signature.StringAlias) (b map[*signature.Alias]signature.StringAlias, c map[*signature.Alias]signature.StringAlias)", 0)
-	signature.OtherAliasMap() //@signature(")", "OtherAliasMap(a map[signature.Alias]signature.OtherAlias, b map[signature.Alias]signature.OtherAlias) map[signature.Alias]signature.OtherAlias", 0)
+	sig.AliasSlice()    //@signature(")", "AliasSlice(a []*sig.Alias) (b sig.Alias)", 0)
+	sig.AliasMap()      //@signature(")", "AliasMap(a map[*sig.Alias]sig.StringAlias) (b map[*sig.Alias]sig.StringAlias, c map[*sig.Alias]sig.StringAlias)", 0)
+	sig.OtherAliasMap() //@signature(")", "OtherAliasMap(a map[sig.Alias]sig.OtherAlias, b map[sig.Alias]sig.OtherAlias) map[sig.Alias]sig.OtherAlias", 0)
 }

internal/lsp/testdata/lsp/primarymod/signature/signature_test.go.golden

@@ -1,6 +1,12 @@
+-- AliasMap(a map[*sig.Alias]sig.StringAlias) (b map[*sig.Alias]sig.StringAlias, c map[*sig.Alias]sig.StringAlias)-signature --
+AliasMap(a map[*sig.Alias]sig.StringAlias) (b map[*sig.Alias]sig.StringAlias, c map[*sig.Alias]sig.StringAlias)
+
 -- AliasMap(a map[*signature.Alias]signature.StringAlias) (b map[*signature.Alias]signature.StringAlias, c map[*signature.Alias]signature.StringAlias)-signature --
 AliasMap(a map[*signature.Alias]signature.StringAlias) (b map[*signature.Alias]signature.StringAlias, c map[*signature.Alias]signature.StringAlias)
 
+-- AliasSlice(a []*sig.Alias) (b sig.Alias)-signature --
+AliasSlice(a []*sig.Alias) (b sig.Alias)
+
 -- AliasSlice(a []*signature.Alias) (b signature.Alias)-signature --
 AliasSlice(a []*signature.Alias) (b signature.Alias)
 
@@ -10,6 +16,9 @@ GetAlias() signature.Alias
 -- GetAliasPtr() *signature.Alias-signature --
 GetAliasPtr() *signature.Alias
 
+-- OtherAliasMap(a map[sig.Alias]sig.OtherAlias, b map[sig.Alias]sig.OtherAlias) map[sig.Alias]sig.OtherAlias-signature --
+OtherAliasMap(a map[sig.Alias]sig.OtherAlias, b map[sig.Alias]sig.OtherAlias) map[sig.Alias]sig.OtherAlias
+
 -- OtherAliasMap(a map[signature.Alias]signature.OtherAlias, b map[signature.Alias]signature.OtherAlias) map[signature.Alias]signature.OtherAlias-signature --
 OtherAliasMap(a map[signature.Alias]signature.OtherAlias, b map[signature.Alias]signature.OtherAlias) map[signature.Alias]signature.OtherAlias
 

internal/lsp/testdata/lsp/primarymod/snippets/literal.go

@@ -2,19 +2,19 @@ package snippets
 
 import (
 	"golang.org/x/tools/internal/lsp/signature"
-	"golang.org/x/tools/internal/lsp/types"
+	t "golang.org/x/tools/internal/lsp/types"
 )
 
 type structy struct {
 	x signature.MyType
 }
 
-func X(_ map[signatures.Alias]types.CoolAlias) (map[signatures.Alias]types.CoolAlias) {
+func X(_ map[signature.Alias]t.CoolAlias) (map[signature.Alias]t.CoolAlias) {
 	return nil
 }
 
 func _() {
-	X() //@signature(")", "X(_ map[signatures.Alias]types.CoolAlias) map[signatures.Alias]types.CoolAlias", 0)
+	X() //@signature(")", "X(_ map[signature.Alias]t.CoolAlias) map[signature.Alias]t.CoolAlias", 0)
 	_ = signature.MyType{} //@item(literalMyType, "signature.MyType{}", "", "var")
 	s := structy{
 		x: //@snippet(" //", literalMyType, "signature.MyType{\\}", "signature.MyType{\\}")

internal/lsp/testdata/lsp/primarymod/snippets/literal.go.golden

@@ -1,3 +1,6 @@
+-- X(_ map[signature.Alias]t.CoolAlias) map[signature.Alias]t.CoolAlias-signature --
+X(_ map[signature.Alias]t.CoolAlias) map[signature.Alias]t.CoolAlias
+
 -- X(_ map[signatures.Alias]types.CoolAlias) map[signatures.Alias]types.CoolAlias-signature --
 X(_ map[signatures.Alias]types.CoolAlias) map[signatures.Alias]types.CoolAlias
 

internal/lsp/testdata/lsp/primarymod/testy/testy_test.go

@@ -1,8 +1,18 @@
 package testy
 
-import "testing"
+import (
+	"testing"
+
+	sig "golang.org/x/tools/internal/lsp/signature"
+	"golang.org/x/tools/internal/lsp/snippets"
+)
 
 func TestSomething(t *testing.T) { //@item(TestSomething, "TestSomething(t *testing.T)", "", "func")
 	var x int //@mark(testyX, "x"),diag("x", "compiler", "x declared but not used", "error"),refs("x", testyX)
 	a()       //@mark(testyA, "a")
 }
+
+func _() {
+	_ = snippets.X(nil) //@signature("nil", "X(_ map[sig.Alias]types.CoolAlias) map[sig.Alias]types.CoolAlias", 0)
+	var _ sig.Alias
+}

internal/lsp/testdata/lsp/primarymod/testy/testy_test.go.golden

@@ -0,0 +1,3 @@
+-- X(_ map[sig.Alias]types.CoolAlias) map[sig.Alias]types.CoolAlias-signature --
+X(_ map[sig.Alias]types.CoolAlias) map[sig.Alias]types.CoolAlias
+

internal/lsp/testdata/lsp/summary.txt.golden

@@ -22,7 +22,7 @@ SymbolsCount = 3
 WorkspaceSymbolsCount = 2
 FuzzyWorkspaceSymbolsCount = 3
 CaseSensitiveWorkspaceSymbolsCount = 2
-SignaturesCount = 31
+SignaturesCount = 32
 LinksCount = 8
 ImplementationsCount = 14
 

internal/lsp/tests/util.go

@@ -283,35 +283,30 @@ func DiffSignatures(spn span.Span, want, got *protocol.SignatureHelp) string {
 	decorate := func(f string, args ...interface{}) string {
 		return fmt.Sprintf("invalid signature at %s: %s", spn, fmt.Sprintf(f, args...))
 	}
-
 	if len(got.Signatures) != 1 {
 		return decorate("wanted 1 signature, got %d", len(got.Signatures))
 	}
-
 	if got.ActiveSignature != 0 {
 		return decorate("wanted active signature of 0, got %d", int(got.ActiveSignature))
 	}
-
 	if want.ActiveParameter != got.ActiveParameter {
 		return decorate("wanted active parameter of %d, got %d", want.ActiveParameter, int(got.ActiveParameter))
 	}
-
-	gotSig := got.Signatures[int(got.ActiveSignature)]
-
-	if want.Signatures[0].Label != got.Signatures[0].Label {
-		d := myers.ComputeEdits("", want.Signatures[0].Label, got.Signatures[0].Label)
-		return decorate("mismatched labels:\n%q", diff.ToUnified("want", "got", want.Signatures[0].Label, d))
+	g := got.Signatures[0]
+	w := want.Signatures[0]
+	if w.Label != g.Label {
+		wLabel := w.Label + "\n"
+		d := myers.ComputeEdits("", wLabel, g.Label+"\n")
+		return decorate("mismatched labels:\n%q", diff.ToUnified("want", "got", wLabel, d))
 	}
-
 	var paramParts []string
-	for _, p := range gotSig.Parameters {
+	for _, p := range g.Parameters {
 		paramParts = append(paramParts, p.Label)
 	}
 	paramsStr := strings.Join(paramParts, ", ")
-	if !strings.Contains(gotSig.Label, paramsStr) {
-		return decorate("expected signature %q to contain params %q", gotSig.Label, paramsStr)
+	if !strings.Contains(g.Label, paramsStr) {
+		return decorate("expected signature %q to contain params %q", g.Label, paramsStr)
 	}
-
 	return ""
 }