internal/lsp/source: improve completion in type assertions

In cases like:

var foo *someType = bar.(some<>)

We will now complete "some" to "*someType". This involved two changes:

1. Properly detect expected type as *someType in above example. To do
   this I just removed *ast.TypeAssertExpr from
   breaksExpectedTypeInference() so we continue searching up the AST for
   the expected type.

2. If the given type name T doesn't match, also try *T. If *T does
   match, we mark the candidate as "makePointer=true" so we know to
   prepend the "*" when formatting the candidate.

Change-Id: I05859c68082a798141755b614673a1483d864e3e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212717
Run-TryBot: Muir Manders <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>

Author: muirdm

internal/lsp/completion.go

@@ -127,9 +127,9 @@ func toProtocolCompletionItems(candidates []source.CompletionItem, rng protocol.
 			// https://github.com/Microsoft/language-server-protocol/issues/348.
 			SortText: fmt.Sprintf("%05d", i),
 
-			// Trim address operator (VSCode doesn't like weird characters
-			// in filterText).
-			FilterText: strings.TrimLeft(candidate.InsertText, "&"),
+			// Trim operators (VSCode doesn't like weird characters in
+			// filterText).
+			FilterText: strings.TrimLeft(candidate.InsertText, "&*"),
 
 			Preselect:     i == 0,
 			Documentation: candidate.Documentation,

internal/lsp/source/completion.go

@@ -379,6 +379,9 @@ type candidate struct {
 	// addressable is true if a pointer can be taken to the candidate.
 	addressable bool
 
+	// makePointer is true if the candidate type name T should be made into *T.
+	makePointer bool
+
 	// imp is the import that needs to be added to this package in order
 	// for this candidate to be valid. nil if no import needed.
 	imp *importInfo
@@ -1535,7 +1538,7 @@ func findSwitchStmt(path []ast.Node, pos token.Pos, c *ast.CaseClause) ast.Stmt
 // expect a function argument, not a composite literal value.
 func breaksExpectedTypeInference(n ast.Node) bool {
 	switch n.(type) {
-	case *ast.FuncLit, *ast.CallExpr, *ast.TypeAssertExpr, *ast.IndexExpr, *ast.SliceExpr, *ast.CompositeLit:
+	case *ast.FuncLit, *ast.CallExpr, *ast.IndexExpr, *ast.SliceExpr, *ast.CompositeLit:
 		return true
 	default:
 		return false
@@ -1758,39 +1761,52 @@ func (c *completer) matchingTypeName(cand *candidate) bool {
 		return false
 	}
 
-	// Take into account any type name modifier prefixes.
-	actual := c.expectedType.applyTypeNameModifiers(cand.obj.Type())
+	typeMatches := func(candType types.Type) bool {
+		// Take into account any type name modifier prefixes.
+		candType = c.expectedType.applyTypeNameModifiers(candType)
+
+		if from := c.expectedType.typeName.assertableFrom; from != nil {
+			// Don't suggest the starting type in type assertions. For example,
+			// if "foo" is an io.Writer, don't suggest "foo.(io.Writer)".
+			if types.Identical(from, candType) {
+				return false
+			}
+
+			if intf, ok := from.Underlying().(*types.Interface); ok {
+				if !types.AssertableTo(intf, candType) {
+					return false
+				}
+			}
+		}
 
-	if c.expectedType.typeName.assertableFrom != nil {
-		// Don't suggest the starting type in type assertions. For example,
-		// if "foo" is an io.Writer, don't suggest "foo.(io.Writer)".
-		if types.Identical(c.expectedType.typeName.assertableFrom, actual) {
+		if c.expectedType.typeName.wantComparable && !types.Comparable(candType) {
 			return false
 		}
 
-		if intf, ok := c.expectedType.typeName.assertableFrom.Underlying().(*types.Interface); ok {
-			if !types.AssertableTo(intf, actual) {
-				return false
-			}
+		// We can expect a type name and have an expected type in cases like:
+		//
+		//   var foo []int
+		//   foo = []i<>
+		//
+		// Where our expected type is "[]int", and we expect a type name.
+		if c.expectedType.objType != nil {
+			return types.AssignableTo(candType, c.expectedType.objType)
 		}
+
+		// Default to saying any type name is a match.
+		return true
 	}
 
-	if c.expectedType.typeName.wantComparable && !types.Comparable(actual) {
-		return false
+	if typeMatches(cand.obj.Type()) {
+		return true
 	}
 
-	// We can expect a type name and have an expected type in cases like:
-	//
-	//   var foo []int
-	//   foo = []i<>
-	//
-	// Where our expected type is "[]int", and we expect a type name.
-	if c.expectedType.objType != nil {
-		return types.AssignableTo(actual, c.expectedType.objType)
+	if typeMatches(types.NewPointer(cand.obj.Type())) {
+		cand.makePointer = true
+		return true
 	}
 
-	// Default to saying any type name is a match.
-	return true
+	return false
 }
 
 // candKind returns the objKind of candType, if any.

internal/lsp/source/completion_format.go

@@ -130,30 +130,29 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
 		}
 	}
 
-	// Prepend "&" operator if our candidate needs address taken.
+	// Prepend "&" or "*" operator as appropriate.
+	var prefixOp string
 	if cand.takeAddress {
-		var (
-			sel *ast.SelectorExpr
-			ok  bool
-		)
-		if sel, ok = c.path[0].(*ast.SelectorExpr); !ok && len(c.path) > 1 {
-			sel, _ = c.path[1].(*ast.SelectorExpr)
-		}
+		prefixOp = "&"
+	} else if cand.makePointer {
+		prefixOp = "*"
+	}
 
-		// If we are in a selector, add an edit to place "&" before selector node.
-		if sel != nil {
-			edits, err := referenceEdit(c.snapshot.View().Session().Cache().FileSet(), c.mapper, sel)
+	if prefixOp != "" {
+		// If we are in a selector, add an edit to place prefix before selector.
+		if sel := enclosingSelector(c.path, c.pos); sel != nil {
+			edits, err := prependEdit(c.snapshot.View().Session().Cache().FileSet(), c.mapper, sel, prefixOp)
 			if err != nil {
-				log.Error(c.ctx, "error generating reference edit", err)
+				log.Error(c.ctx, "error generating prefix edit", err)
 			} else {
 				protocolEdits = append(protocolEdits, edits...)
 			}
 		} else {
-			// If there is no selector, just stick the "&" at the start.
-			insert = "&" + insert
+			// If there is no selector, just stick the prefix at the start.
+			insert = prefixOp + insert
 		}
 
-		label = "&" + label
+		label = prefixOp + label
 	}
 
 	detail = strings.TrimPrefix(detail, "untyped ")

internal/lsp/source/completion_literal.go

@@ -116,7 +116,7 @@ func (c *completer) literal(literalType types.Type, imp *importInfo) {
 				// If we are in a selector we must place the "&" before the selector.
 				// For example, "foo.B<>" must complete to "&foo.Bar{}", not
 				// "foo.&Bar{}".
-				edits, err := referenceEdit(c.snapshot.View().Session().Cache().FileSet(), c.mapper, sel)
+				edits, err := prependEdit(c.snapshot.View().Session().Cache().FileSet(), c.mapper, sel, "&")
 				if err != nil {
 					log.Error(c.ctx, "error making edit for literal pointer completion", err)
 					return
@@ -173,17 +173,17 @@ func (c *completer) literal(literalType types.Type, imp *importInfo) {
 	}
 }
 
-// referenceEdit produces text edits that prepend a "&" operator to the
-// specified node.
-func referenceEdit(fset *token.FileSet, m *protocol.ColumnMapper, node ast.Node) ([]protocol.TextEdit, error) {
+// prependEdit produces text edits that preprend the specified prefix
+// to the specified node.
+func prependEdit(fset *token.FileSet, m *protocol.ColumnMapper, node ast.Node, prefix string) ([]protocol.TextEdit, error) {
 	rng := newMappedRange(fset, m, node.Pos(), node.Pos())
 	spn, err := rng.Span()
 	if err != nil {
 		return nil, err
 	}
 	return ToProtocolEdits(m, []diff.TextEdit{{
 		Span:    spn,
-		NewText: "&",
+		NewText: prefix,
 	}})
 }
 

internal/lsp/source/source_test.go

@@ -102,20 +102,13 @@ func (r *runner) Completion(t *testing.T, src span.Span, test tests.Completion,
 	for _, pos := range test.CompletionItems {
 		want = append(want, tests.ToProtocolCompletionItem(*items[pos]))
 	}
-	prefix, list := r.callCompletion(t, src, func(opts *source.Options) {
-		opts.Matcher = source.Fuzzy
+	_, got := r.callCompletion(t, src, func(opts *source.Options) {
+		opts.Matcher = source.CaseInsensitive
 		opts.Literal = strings.Contains(string(src.URI()), "literal")
 		opts.DeepCompletion = false
 	})
 	if !strings.Contains(string(src.URI()), "builtins") {
-		list = tests.FilterBuiltins(list)
-	}
-	var got []protocol.CompletionItem
-	for _, item := range list {
-		if !strings.HasPrefix(strings.ToLower(item.Label), prefix) {
-			continue
-		}
-		got = append(got, item)
+		got = tests.FilterBuiltins(got)
 	}
 	if diff := tests.DiffCompletionItems(want, got); diff != "" {
 		t.Errorf("%s: %s", src, diff)

internal/lsp/testdata/func_rank/func_rank.go.in

@@ -29,7 +29,7 @@ func _() {
 	fnInt([]int{}[s.A])       //@complete("])", rankAA, rankAC, rankAB)
 	fnInt([]int{}[:s.A])      //@complete("])", rankAA, rankAC, rankAB)
 
-	fnInt(s.A.(int)) //@complete(".(", rankAA, rankAB, rankAC)
+	fnInt(s.A.(int)) //@complete(".(", rankAA, rankAC, rankAB)
 
 	fnPtr := func(*string) {}
 	fnPtr(&s.A) //@complete(")", rankAB, rankAA, rankAC)

internal/lsp/testdata/maps/maps.go.in

@@ -6,13 +6,13 @@ func _() {
 	// not comparabale
 	type aSlice []int     //@item(mapSliceType, "aSlice", "[]int", "type")
 
+	*aSlice     //@item(mapSliceTypePtr, "*aSlice", "[]int", "type")
+
 	// comparable
 	type aStruct struct{} //@item(mapStructType, "aStruct", "struct{...}", "struct")
 
-	map[]a{} //@complete("]", mapStructType, mapSliceType, mapVar)
+	map[]a{} //@complete("]", mapSliceTypePtr, mapStructType, mapVar)
 
-	map[a]a{} //@complete("]", mapStructType, mapSliceType, mapVar)
+	map[a]a{} //@complete("]", mapSliceTypePtr, mapStructType, mapVar)
 	map[a]a{} //@complete("{", mapSliceType, mapStructType, mapVar)
-
-	map[]a{} //@rank("]", int, mapSliceType)
 }

internal/lsp/testdata/summary.txt.golden

@@ -4,7 +4,7 @@ CompletionSnippetCount = 63
 UnimportedCompletionsCount = 9
 DeepCompletionsCount = 5
 FuzzyCompletionsCount = 8
-RankedCompletionsCount = 56
+RankedCompletionsCount = 55
 CaseSensitiveCompletionsCount = 4
 DiagnosticsCount = 35
 FoldingRangesCount = 2

internal/lsp/testdata/typeassert/type_assert.go

@@ -13,12 +13,14 @@ func (*abcPtrImpl) abc()
 type abcNotImpl struct{} //@item(abcNotImpl, "abcNotImpl", "struct{...}", "struct")
 
 func _() {
+	*abcPtrImpl //@item(abcPtrImplPtr, "*abcPtrImpl", "struct{...}", "struct")
+
 	var a abc
 	switch a.(type) {
-	case ab: //@complete(":", abcImpl, abcIntf, abcNotImpl, abcPtrImpl)
+	case ab: //@complete(":", abcPtrImplPtr, abcImpl, abcIntf, abcNotImpl)
 	case *ab: //@complete(":", abcImpl, abcPtrImpl, abcIntf, abcNotImpl)
 	}
 
-	a.(ab)  //@complete(")", abcImpl, abcIntf, abcNotImpl, abcPtrImpl)
+	a.(ab)  //@complete(")", abcPtrImplPtr, abcImpl, abcIntf, abcNotImpl)
 	a.(*ab) //@complete(")", abcImpl, abcPtrImpl, abcIntf, abcNotImpl)
 }