internal/lsp/source: untangle completion type comparison

The code to check if a candidate object matches our candidate
inference had become complicated, messy, and in some cases incorrect.
The main source of the complexity is the "derived" expected and
candidate types. When considering a candidate object "foo", we also
consider "&foo", "foo()", and "*foo", as appropriate. On the expected
side of things, when completing the a variadic function parameter we
expect either the variadic slice type and the scalar element type.

The code had grown organically to handle the expanding concerns, but
that resulted in confused code that didn't handle the interplay
between the various facets of candidate inference.

For example, we were inappropriately invoking func candidates when
completing variadic args:

    func foo(...func())
    func bar() {}
    foo(bar<>) // oops - expanded to "bar()"

and we weren't type matching functions properly as builtin args:

    func myMap() map[string]int { ... }
    delete(myM<>) // we weren't preferring (or invoking) "myMap()"

We also had methods like "typeMatches" which took both a "candidate"
object and a "candType" type, which doesn't make sense because the
candidate contains the type already.

Now instead we explicitly iterate over all the derived candidate and
expected types so they are treated the same. There are still some
warts left but I think this is a step in the right direction.

Change-Id: If84a84b34a8fb771a32231f7ab64ca192f611b3d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218877
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

internal/lsp/source/completion.go

@@ -333,7 +333,7 @@ func (c *completer) found(cand candidate) {
 		return
 	}
 
-	if c.matchingCandidate(&cand, nil) {
+	if c.matchingCandidate(&cand) {
 		cand.score *= highScore
 	} else if isTypeName(obj) {
 		// If obj is a *types.TypeName that didn't otherwise match, check
@@ -1299,9 +1299,10 @@ type candidateInference struct {
 	// objKind is a mask of expected kinds of types such as "map", "slice", etc.
 	objKind objKind
 
-	// variadic is true if objType is a slice type from an initial
-	// variadic param.
-	variadic bool
+	// variadicType is the scalar variadic element type. For example,
+	// when completing "append([]T{}, <>)" objType is []T and
+	// variadicType is T.
+	variadicType types.Type
 
 	// modifiers are prefixes such as "*", "&" or "<-" that influence how
 	// a candidate type relates to the expected type.
@@ -1443,13 +1444,13 @@ Nodes:
 						}
 
 						if sig.Variadic() {
+							variadicType := deslice(sig.Params().At(numParams - 1).Type())
+
 							// If we are beyond the last param or we are the last
 							// param w/ further expressions, we expect a single
 							// variadic item.
 							if beyondLastParam || isLastParam && len(node.Args) > numParams {
-								if slice, ok := sig.Params().At(numParams - 1).Type().(*types.Slice); ok {
-									inf.objType = slice.Elem()
-								}
+								inf.objType = variadicType
 								break Nodes
 							}
 
@@ -1457,7 +1458,7 @@ Nodes:
 							// completing the variadic positition (i.e. we expect a
 							// slice type []T or an individual item T).
 							if isLastParam {
-								inf.variadic = true
+								inf.variadicType = variadicType
 							}
 						}
 
@@ -1474,12 +1475,11 @@ Nodes:
 					obj := c.pkg.GetTypesInfo().ObjectOf(funIdent)
 
 					if obj != nil && obj.Parent() == types.Universe {
-						inf.objKind |= c.builtinArgKind(obj, node)
-
 						// Defer call to builtinArgType so we can provide it the
 						// inferred type from its parent node.
 						defer func() {
-							inf.objType, inf.typeName.wantTypeName, inf.variadic = c.builtinArgType(obj, node, inf.objType)
+							inf = c.builtinArgType(obj, node, inf)
+							inf.objKind = c.builtinArgKind(obj, node)
 						}()
 
 						// The expected type of builtin arguments like append() is
@@ -1559,7 +1559,6 @@ Nodes:
 				inf.modifiers = append(inf.modifiers, typeModifier{mod: address})
 			case token.ARROW:
 				inf.modifiers = append(inf.modifiers, typeModifier{mod: chanRead})
-				inf.objKind |= kindChan
 			}
 		default:
 			if breaksExpectedTypeInference(node) {
@@ -1624,7 +1623,7 @@ func (ci candidateInference) applyTypeNameModifiers(typ types.Type) types.Type {
 // matchesVariadic returns true if we are completing a variadic
 // parameter and candType is a compatible slice type.
 func (ci candidateInference) matchesVariadic(candType types.Type) bool {
-	return ci.variadic && types.AssignableTo(ci.objType, candType)
+	return ci.variadicType != nil && types.AssignableTo(ci.objType, candType)
 
 }
 
@@ -1778,10 +1777,81 @@ func (c *completer) fakeObj(T types.Type) *types.Var {
 	return types.NewVar(token.NoPos, c.pkg.GetTypes(), "", T)
 }
 
-// matchingCandidate reports whether a candidate matches our type
-// inferences. seen is used to detect recursive types in certain cases
-// and should be set to nil when calling matchingCandidate.
-func (c *completer) matchingCandidate(cand *candidate, seen map[types.Type]struct{}) bool {
+// anyCandType reports whether f returns true for any candidate type
+// derivable from c. For example, from "foo" we might derive "&foo",
+// and "foo()".
+func (c *candidate) anyCandType(f func(t types.Type, addressable bool) bool) bool {
+	if c.obj == nil || c.obj.Type() == nil {
+		return false
+	}
+
+	objType := c.obj.Type()
+
+	if f(objType, c.addressable) {
+		return true
+	}
+
+	// If c is a func type with a single result, offer the result type.
+	if sig, ok := objType.Underlying().(*types.Signature); ok {
+		if sig.Results().Len() == 1 && f(sig.Results().At(0).Type(), false) {
+			// Mark the candidate so we know to append "()" when formatting.
+			c.expandFuncCall = true
+			return true
+		}
+	}
+
+	var (
+		seenPtrTypes map[types.Type]bool
+		ptrType      = objType
+		ptrDepth     int
+	)
+
+	// Check if dereferencing c would match our type inference. We loop
+	// since c could have arbitrary levels of pointerness.
+	for {
+		ptr, ok := ptrType.Underlying().(*types.Pointer)
+		if !ok {
+			break
+		}
+
+		ptrDepth++
+
+		// Avoid pointer type cycles.
+		if seenPtrTypes[ptrType] {
+			break
+		}
+
+		if _, named := ptrType.(*types.Named); named {
+			// Lazily allocate "seen" since it isn't used normally.
+			if seenPtrTypes == nil {
+				seenPtrTypes = make(map[types.Type]bool)
+			}
+
+			// Track named pointer types we have seen to detect cycles.
+			seenPtrTypes[ptrType] = true
+		}
+
+		if f(ptr.Elem(), false) {
+			// Mark the candidate so we know to prepend "*" when formatting.
+			c.dereference = ptrDepth
+			return true
+		}
+
+		ptrType = ptr.Elem()
+	}
+
+	// Check if c is addressable and a pointer to c matches our type inference.
+	if c.addressable && f(types.NewPointer(objType), false) {
+		// Mark the candidate so we know to prepend "&" when formatting.
+		c.takeAddress = true
+		return true
+	}
+
+	return false
+}
+
+// matchingCandidate reports whether cand matches our type inferences.
+func (c *completer) matchingCandidate(cand *candidate) bool {
 	if isTypeName(cand.obj) {
 		return c.matchingTypeName(cand)
 	} else if c.wantTypeName() {
@@ -1789,100 +1859,98 @@ func (c *completer) matchingCandidate(cand *candidate, seen map[types.Type]struc
 		return false
 	}
 
+	if c.inference.candTypeMatches(cand) {
+		return true
+	}
+
 	candType := cand.obj.Type()
 	if candType == nil {
 		return false
 	}
 
+	if sig, ok := candType.Underlying().(*types.Signature); ok {
+		if c.inference.assigneesMatch(cand, sig) {
+			// Invoke the candidate if its results are multi-assignable.
+			cand.expandFuncCall = true
+			return true
+		}
+	}
+
 	// Default to invoking *types.Func candidates. This is so function
 	// completions in an empty statement (or other cases with no expected type)
 	// are invoked by default.
 	cand.expandFuncCall = isFunc(cand.obj)
 
-	if c.inference.typeMatches(cand, c.inference.objType, candType) {
-		// If obj's type matches, we don't want to expand to an invocation of obj.
-		cand.expandFuncCall = false
-		return true
-	}
-
-	// Try using a function's return type as its type.
-	if sig, ok := candType.Underlying().(*types.Signature); ok {
-		if c.inference.signatureMatches(cand, sig) {
-			// If obj's signature's return value matches the expected type,
-			// we need to invoke obj in the completion.
-			cand.expandFuncCall = true
-			return true
-		}
-	}
-
-	// When completing the variadic parameter, if the expected type is
-	// []T then check candType against T.
-	if c.inference.variadic {
-		if slice, ok := c.inference.objType.(*types.Slice); ok {
-			if c.inference.typeMatches(cand, slice.Elem(), candType) {
-				return true
-			}
-		}
-	}
-
-	if c.inference.convertibleTo != nil && types.ConvertibleTo(candType, c.inference.convertibleTo) {
-		return true
-	}
-
-	// Check if dereferencing cand would match our type inference.
-	if ptr, ok := cand.obj.Type().Underlying().(*types.Pointer); ok {
-		// Notice if we have already encountered this pointer type before.
-		_, saw := seen[cand.obj.Type()]
-
-		if _, named := cand.obj.Type().(*types.Named); named {
-			// Lazily allocate "seen" since it isn't used normally.
-			if seen == nil {
-				seen = make(map[types.Type]struct{})
-			}
-
-			// Track named pointer types we have seen to detect cycles.
-			seen[cand.obj.Type()] = struct{}{}
-		}
-
-		fakeCandidate := candidate{obj: c.fakeObj(ptr.Elem())}
-		if !saw && c.matchingCandidate(&fakeCandidate, seen) {
-			// Mark the candidate so we know to prepend "*" when formatting.
-			cand.dereference = 1 + fakeCandidate.dereference
-			return true
-		}
-	}
-
-	// Check if cand is addressable and a pointer to cand matches our type inference.
-	if cand.addressable && c.matchingCandidate(&candidate{obj: c.fakeObj(types.NewPointer(candType))}, seen) {
-		// Mark the candidate so we know to prepend "&" when formatting.
-		cand.takeAddress = true
-		return true
-	}
-
 	return false
 }
 
-// typeMatches reports whether an object of candType makes a good
-// completion candidate given the expected type expType. The
-// candidate's score may be mutated to downrank the candidate in
-// certain situations.
-func (ci *candidateInference) typeMatches(cand *candidate, expType, candType types.Type) bool {
-	if expType == nil {
-		// If we don't expect a specific type, check if we expect a particular
-		// kind of object (map, slice, etc).
-		if ci.objKind > 0 {
-			return ci.objKind&candKind(candType) > 0
+// candTypeMatches reports whether cand makes a good completion
+// candidate given the candidate inference. cand's score may be
+// mutated to downrank the candidate in certain situations.
+func (ci *candidateInference) candTypeMatches(cand *candidate) bool {
+	expTypes := make([]types.Type, 0, 2)
+	if ci.objType != nil {
+		expTypes = append(expTypes, ci.objType)
+	}
+	if ci.variadicType != nil {
+		expTypes = append(expTypes, ci.variadicType)
+	}
+
+	return cand.anyCandType(func(candType types.Type, addressable bool) bool {
+		// Take into account any type modifiers on the expected type.
+		candType = ci.applyTypeModifiers(candType, addressable)
+		if candType == nil {
+			return false
+		}
+
+		if ci.convertibleTo != nil && types.ConvertibleTo(candType, ci.convertibleTo) {
+			return true
+		}
+
+		if len(expTypes) == 0 {
+			// If we have no expected type but were able to apply type
+			// modifiers to our candidate type, count that as a match. This
+			// handles cases like:
+			//
+			//   var foo chan int
+			//   <-fo<>
+			//
+			// There is no exected type at "<>", but we were able to apply
+			// the "<-" type modifier to "foo", so it matches.
+			if len(ci.modifiers) > 0 {
+				return true
+			}
+
+			// If we have no expected type, fall back to checking the
+			// expected "kind" of object, if available.
+			return ci.kindMatches(candType)
+		}
+
+		for _, expType := range expTypes {
+			matches, untyped := ci.typeMatches(expType, candType)
+			if !matches {
+				continue
+			}
+
+			// Lower candidate score for untyped conversions. This avoids
+			// ranking untyped constants above candidates with an exact type
+			// match. Don't lower score of builtin constants, e.g. "true".
+			if untyped && !types.Identical(candType, expType) && cand.obj.Parent() != types.Universe {
+				cand.score /= 2
+			}
+
+			return true
 		}
 
 		return false
-	}
-
-	// Take into account any type modifiers on the expected type.
-	candType = ci.applyTypeModifiers(candType, cand.addressable)
-	if candType == nil {
-		return false
-	}
+	})
+}
 
+// typeMatches reports whether an object of candType makes a good
+// completion candidate given the expected type expType. It also
+// returns a second bool which is true if both types are basic types
+// of the same kind, and at least one is untyped.
+func (ci *candidateInference) typeMatches(expType, candType types.Type) (bool, bool) {
 	// Handle untyped values specially since AssignableTo gives false negatives
 	// for them (see https://golang.org/issue/32146).
 	if candBasic, ok := candType.Underlying().(*types.Basic); ok {
@@ -1893,13 +1961,7 @@ func (ci *candidateInference) typeMatches(cand *candidate, expType, candType typ
 				// This doesn't take into account the constant value, so there will be some
 				// false positives due to integer sign and overflow.
 				if candBasic.Info()&types.IsConstType == wantBasic.Info()&types.IsConstType {
-					// Lower candidate score if the types are not identical. This avoids
-					// ranking untyped constants above candidates with an exact type
-					// match. Don't lower score of builtin constants (e.g. "true").
-					if !types.Identical(candType, expType) && cand.obj.Parent() != types.Universe {
-						cand.score /= 2
-					}
-					return true
+					return true, true
 				}
 			}
 		}
@@ -1907,20 +1969,25 @@ func (ci *candidateInference) typeMatches(cand *candidate, expType, candType typ
 
 	// AssignableTo covers the case where the types are equal, but also handles
 	// cases like assigning a concrete type to an interface type.
-	return types.AssignableTo(candType, expType)
+	return types.AssignableTo(candType, expType), false
 }
 
-// signatureMatches reports whether an invocation of sig makes a good
-// completion candidate. The candidate's score may be mutated to
-// downrank the candidate in certain situations.
-func (ci *candidateInference) signatureMatches(cand *candidate, sig *types.Signature) bool {
-	// If sig returns a single value and it matches our expected type,
-	// invocation of sig is a good candidate.
-	if sig.Results().Len() == 1 {
-		return ci.typeMatches(cand, ci.objType, sig.Results().At(0).Type())
+// kindMatches reports whether candType's kind matches our expected
+// kind (e.g. slice, map, etc.).
+func (ci *candidateInference) kindMatches(candType types.Type) bool {
+	return ci.objKind&candKind(candType) > 0
+}
+
+// assigneesMatch reports whether an invocation of sig matches the
+// number and type of any assignees.
+func (ci *candidateInference) assigneesMatch(cand *candidate, sig *types.Signature) bool {
+	if len(ci.assignees) == 0 {
+		return false
 	}
 
-	if len(ci.assignees) == 0 {
+	// Uniresult functions are always usable and are handled by the
+	// normal, non-assignees type matching logic.
+	if sig.Results().Len() == 1 {
 		return false
 	}
 
@@ -1952,14 +2019,14 @@ func (ci *candidateInference) signatureMatches(cand *candidate, sig *types.Signa
 		// expected variadic type.
 		if ci.variadicAssignees && i >= len(ci.assignees)-1 {
 			assignee = ci.assignees[len(ci.assignees)-1]
-			if slice, ok := assignee.Underlying().(*types.Slice); ok {
-				assignee = slice.Elem()
+			if elem := deslice(assignee); elem != nil {
+				assignee = elem
 			}
 		} else {
 			assignee = ci.assignees[i]
 		}
 
-		allMatch = ci.typeMatches(cand, assignee, sig.Results().At(i).Type())
+		allMatch, _ = ci.typeMatches(assignee, sig.Results().At(i).Type())
 		if !allMatch {
 			break
 		}

internal/lsp/source/completion_builtin.go

@@ -49,23 +49,25 @@ func (c *completer) builtinArgKind(obj types.Object, call *ast.CallExpr) objKind
 }
 
 // builtinArgType infers the type of an argument to a builtin
-// function. "parentType" is the inferred type for the builtin call's
-// parent node.
-func (c *completer) builtinArgType(obj types.Object, call *ast.CallExpr, parentType types.Type) (infType types.Type, wantType, variadic bool) {
-	exprIdx := exprAtPos(c.pos, call.Args)
+// function. parentInf is the inferred type info for the builtin
+// call's parent node.
+func (c *completer) builtinArgType(obj types.Object, call *ast.CallExpr, parentInf candidateInference) candidateInference {
+	var (
+		exprIdx = exprAtPos(c.pos, call.Args)
+		inf     = candidateInference{}
+	)
 
 	switch obj.Name() {
 	case "append":
-		// Check if we are completing the variadic append() param.
-		variadic = exprIdx == 1 && len(call.Args) <= 2
-		infType = parentType
+		inf.objType = parentInf.objType
 
-		// If we are completing an individual element of the variadic
-		// param, "deslice" the expected type.
-		if !variadic && exprIdx > 0 {
-			if slice, ok := parentType.(*types.Slice); ok {
-				infType = slice.Elem()
-			}
+		// Check if we are completing the variadic append() param.
+		if exprIdx == 1 && len(call.Args) <= 2 {
+			inf.variadicType = deslice(inf.objType)
+		} else if exprIdx > 0 {
+			// If we are completing an individual element of the variadic
+			// param, "deslice" the expected type.
+			inf.objType = deslice(inf.objType)
 		}
 	case "delete":
 		if exprIdx > 0 && len(call.Args) > 0 {
@@ -73,7 +75,7 @@ func (c *completer) builtinArgType(obj types.Object, call *ast.CallExpr, parentT
 			firstArgType := c.pkg.GetTypesInfo().TypeOf(call.Args[0])
 			if firstArgType != nil {
 				if mt, ok := firstArgType.Underlying().(*types.Map); ok {
-					infType = mt.Key()
+					inf.objType = mt.Key()
 				}
 			}
 		}
@@ -88,26 +90,26 @@ func (c *completer) builtinArgType(obj types.Object, call *ast.CallExpr, parentT
 
 		// Fill in expected type of either arg if the other is already present.
 		if exprIdx == 1 && t1 != nil {
-			infType = t1
+			inf.objType = t1
 		} else if exprIdx == 0 && t2 != nil {
-			infType = t2
+			inf.objType = t2
 		}
 	case "new":
-		wantType = true
-		if parentType != nil {
+		inf.typeName.wantTypeName = true
+		if parentInf.objType != nil {
 			// Expected type for "new" is the de-pointered parent type.
-			if ptr, ok := parentType.Underlying().(*types.Pointer); ok {
-				infType = ptr.Elem()
+			if ptr, ok := parentInf.objType.Underlying().(*types.Pointer); ok {
+				inf.objType = ptr.Elem()
 			}
 		}
 	case "make":
 		if exprIdx == 0 {
-			wantType = true
-			infType = parentType
+			inf.typeName.wantTypeName = true
+			inf.objType = parentInf.objType
 		} else {
-			infType = types.Typ[types.Int]
+			inf.objType = types.Typ[types.Int]
 		}
 	}
 
-	return infType, wantType, variadic
+	return inf
 }

internal/lsp/source/completion_literal.go

@@ -26,7 +26,7 @@ func (c *completer) literal(literalType types.Type, imp *importInfo) {
 
 	expType := c.inference.objType
 
-	if c.inference.variadic {
+	if c.inference.variadicType != nil {
 		// Don't offer literal slice candidates for variadic arguments.
 		// For example, don't offer "[]interface{}{}" in "fmt.Print(<>)".
 		if c.inference.matchesVariadic(literalType) {
@@ -35,9 +35,7 @@ func (c *completer) literal(literalType types.Type, imp *importInfo) {
 
 		// Otherwise, consider our expected type to be the variadic
 		// element type, not the slice type.
-		if slice, ok := expType.(*types.Slice); ok {
-			expType = slice.Elem()
-		}
+		expType = c.inference.variadicType
 	}
 
 	// Avoid literal candidates if the expected type is an empty
@@ -75,7 +73,7 @@ func (c *completer) literal(literalType types.Type, imp *importInfo) {
 		cand.addressable = true
 	}
 
-	if !c.matchingCandidate(&cand, nil) {
+	if !c.matchingCandidate(&cand) {
 		return
 	}
 

internal/lsp/source/util.go

@@ -425,6 +425,13 @@ func isASTFile(n ast.Node) bool {
 	return ok
 }
 
+func deslice(T types.Type) types.Type {
+	if slice, ok := T.Underlying().(*types.Slice); ok {
+		return slice.Elem()
+	}
+	return nil
+}
+
 // isSelector returns the enclosing *ast.SelectorExpr when pos is in the
 // selector.
 func enclosingSelector(path []ast.Node, pos token.Pos) *ast.SelectorExpr {

internal/lsp/testdata/lsp/primarymod/builtins/builtin_args.go

@@ -31,6 +31,11 @@ func _() {
 	delete()         //@rank(")", builtinMap, builtinChan)
 	delete(aMap, aS) //@rank(")", builtinString, builtinSlice)
 
+	aMapFunc := func() map[int]int { //@item(builtinMapFunc, "aMapFunc", "func() map[int]int", "var")
+		return nil
+	}
+	delete() //@rank(")", builtinMapFunc, builtinSlice)
+
 	len() //@rank(")", builtinSlice, builtinInt),rank(")", builtinMap, builtinInt),rank(")", builtinString, builtinInt),rank(")", builtinArray, builtinInt),rank(")", builtinArrayPtr, builtinPtr),rank(")", builtinChan, builtinInt)
 
 	cap() //@rank(")", builtinSlice, builtinMap),rank(")", builtinArray, builtinString),rank(")", builtinArrayPtr, builtinPtr),rank(")", builtinChan, builtinInt)

internal/lsp/testdata/lsp/primarymod/rank/convert_rank.go.in

@@ -39,4 +39,7 @@ func _() {
 	type myUint uint32
 	var mu myUint
 	mu = conv //@rank(" //", convertD, convertE)
+
+	// don't downrank constants when assigning to interface{}
+	var _ interface{} = c //@rank(" //", convertD, complex)
 }

internal/lsp/testdata/lsp/primarymod/variadic/variadic.go.in

@@ -21,3 +21,10 @@ func _() {
   // snippet will add the "..." for you
 	foo(123, ) //@snippet(")", vStrSlice, "ss...", "ss..."),snippet(")", vFunc, "bar()...", "bar()..."),snippet(")", vStr, "s", "s")
 }
+
+func qux(...func()) {}
+func f()            {} //@item(vVarArg, "f", "func()", "func")
+
+func _() {
+	qux(f) //@snippet(")", vVarArg, "f", "f")
+}

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

@@ -1,11 +1,11 @@
 -- summary --
 CodeLensCount = 0
 CompletionsCount = 226
-CompletionSnippetCount = 67
+CompletionSnippetCount = 68
 UnimportedCompletionsCount = 11
 DeepCompletionsCount = 5
 FuzzyCompletionsCount = 8
-RankedCompletionsCount = 109
+RankedCompletionsCount = 111
 CaseSensitiveCompletionsCount = 4
 DiagnosticsCount = 38
 FoldingRangesCount = 2