internal/lsp/source: improve completion of printf operands

We now rank printf operand candidates according to the corresponding
formatting verb. We follow what fmt allows for the most part, but I
omitted some things because they are uncommon and would result in many
false positives, or didn't seem worth it to support:

- We don't prefer fmt.Stringer or error types for "%x" or "%X".
- We don't prefer pointers for any verbs except "%p".
- We don't prefer recursive application of verbs (e.g. we won't prefer
  []string for "%s").

I decided against sharing code with the printf analyzer. It was
tangled somewhat with go/analysis, and I needed only a very small
subset of the format parsing.

I tweaked candidate type evaluation to accommodate the printf hints.
We now skip expected type of "interface{}" when matching candidate
types because it matches everything and would always supersede the
coarser object kind checks.

Fixes golang/go#40485.

Change-Id: I6440702e33d5ec85d701f8be65453044b5dab746
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246699
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

@@ -1545,12 +1545,21 @@ const (
 type objKind int
 
 const (
+	kindAny   objKind = 0
 	kindArray objKind = 1 << iota
 	kindSlice
 	kindChan
 	kindMap
 	kindStruct
 	kindString
+	kindInt
+	kindBool
+	kindBytes
+	kindPtr
+	kindFloat
+	kindComplex
+	kindError
+	kindStringer
 	kindFunc
 )
 
@@ -1749,6 +1758,9 @@ Nodes:
 							inf.objType = deslice(inf.objType)
 							// Record whether we are completing the initial variadic param.
 							inf.variadic = exprIdx == numParams-1 && len(node.Args) <= numParams
+
+							// Check if we can infer object kind from printf verb.
+							inf.objKind |= printfArgKind(c.pkg.GetTypesInfo(), node, exprIdx)
 						}
 					}
 				}
@@ -2288,6 +2300,7 @@ func (ci *candidateInference) candTypeMatches(cand *candidate) bool {
 	)
 	if ci.objType != nil {
 		expTypes = append(expTypes, ci.objType)
+
 		if ci.variadic {
 			variadicType = types.NewSlice(ci.objType)
 			expTypes = append(expTypes, variadicType)
@@ -2305,31 +2318,11 @@ func (ci *candidateInference) candTypeMatches(cand *candidate) bool {
 			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.
-			if ci.kindMatches(candType) {
-				if ci.objKind == kindFunc {
-					cand.expandFuncCall = true
-				}
-				return true
-			}
-		}
-
 		for _, expType := range expTypes {
+			if isEmptyInterface(expType) {
+				continue
+			}
+
 			matches, untyped := ci.typeMatches(expType, candType)
 			if !matches {
 				continue
@@ -2349,6 +2342,31 @@ func (ci *candidateInference) candTypeMatches(cand *candidate) bool {
 			return true
 		}
 
+		// If we don't have a specific expected type, fall back to coarser
+		// object kind checks.
+		if ci.objType == nil || isEmptyInterface(ci.objType) {
+			// If we were able to apply type modifiers to our candidate type,
+			// count that as a match. For example:
+			//
+			//   var foo chan int
+			//   <-fo<>
+			//
+			// We were able to apply the "<-" type modifier to "foo", so "foo"
+			// matches.
+			if len(ci.modifiers) > 0 {
+				return true
+			}
+
+			// If we didn't have an exact type match, check if our object kind
+			// matches.
+			if ci.kindMatches(candType) {
+				if ci.objKind == kindFunc {
+					cand.expandFuncCall = true
+				}
+				return true
+			}
+		}
+
 		return false
 	})
 }
@@ -2382,7 +2400,7 @@ func (ci *candidateInference) typeMatches(expType, candType types.Type) (bool, b
 // 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
+	return ci.objKind > 0 && ci.objKind&candKind(candType) > 0
 }
 
 // assigneesMatch reports whether an invocation of sig matches the
@@ -2508,30 +2526,74 @@ func (c *completer) matchingTypeName(cand *candidate) bool {
 	return false
 }
 
+var (
+	// "interface { Error() string }" (i.e. error)
+	errorIntf = types.Universe.Lookup("error").Type().Underlying().(*types.Interface)
+
+	// "interface { String() string }" (i.e. fmt.Stringer)
+	stringerIntf = types.NewInterfaceType([]*types.Func{
+		types.NewFunc(token.NoPos, nil, "String", types.NewSignature(
+			nil,
+			nil,
+			types.NewTuple(types.NewParam(token.NoPos, nil, "", types.Typ[types.String])),
+			false,
+		)),
+	}, nil).Complete()
+
+	byteType = types.Universe.Lookup("byte").Type()
+)
+
 // candKind returns the objKind of candType, if any.
 func candKind(candType types.Type) objKind {
+	var kind objKind
+
 	switch t := candType.Underlying().(type) {
 	case *types.Array:
-		return kindArray
+		kind |= kindArray
+		if t.Elem() == byteType {
+			kind |= kindBytes
+		}
 	case *types.Slice:
-		return kindSlice
+		kind |= kindSlice
+		if t.Elem() == byteType {
+			kind |= kindBytes
+		}
 	case *types.Chan:
-		return kindChan
+		kind |= kindChan
 	case *types.Map:
-		return kindMap
+		kind |= kindMap
 	case *types.Pointer:
+		kind |= kindPtr
+
 		// Some builtins handle array pointers as arrays, so just report a pointer
 		// to an array as an array.
 		if _, isArray := t.Elem().Underlying().(*types.Array); isArray {
-			return kindArray
+			kind |= kindArray
 		}
 	case *types.Basic:
-		if t.Info()&types.IsString > 0 {
-			return kindString
+		switch info := t.Info(); {
+		case info&types.IsString > 0:
+			kind |= kindString
+		case info&types.IsInteger > 0:
+			kind |= kindInt
+		case info&types.IsFloat > 0:
+			kind |= kindFloat
+		case info&types.IsComplex > 0:
+			kind |= kindComplex
+		case info&types.IsBoolean > 0:
+			kind |= kindBool
 		}
 	case *types.Signature:
 		return kindFunc
 	}
 
-	return 0
+	if types.Implements(candType, errorIntf) {
+		kind |= kindError
+	}
+
+	if types.Implements(candType, stringerIntf) {
+		kind |= kindStringer
+	}
+
+	return kind
 }

internal/lsp/source/completion_printf.go

@@ -0,0 +1,172 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+	"go/ast"
+	"go/constant"
+	"go/types"
+	"strconv"
+	"strings"
+	"unicode/utf8"
+)
+
+// printfArgKind returns the expected objKind when completing a
+// printf-like operand. call is the printf-like function call, and
+// argIdx is the index of call.Args being completed.
+func printfArgKind(info *types.Info, call *ast.CallExpr, argIdx int) objKind {
+	// Printf-like function name must end in "f".
+	fn := exprObj(info, call.Fun)
+	if fn == nil || !strings.HasSuffix(fn.Name(), "f") {
+		return kindAny
+	}
+
+	sig, _ := fn.Type().(*types.Signature)
+	if sig == nil {
+		return kindAny
+	}
+
+	// Must be variadic and take at least two params.
+	numParams := sig.Params().Len()
+	if !sig.Variadic() || numParams < 2 || argIdx < numParams-1 {
+		return kindAny
+	}
+
+	// Param preceding variadic args must be a (format) string.
+	if !types.Identical(sig.Params().At(numParams-2).Type(), types.Typ[types.String]) {
+		return kindAny
+	}
+
+	// Format string must be a constant.
+	strArg := info.Types[call.Args[numParams-2]].Value
+	if strArg == nil || strArg.Kind() != constant.String {
+		return kindAny
+	}
+
+	return formatOperandKind(constant.StringVal(strArg), argIdx-(numParams-1)+1)
+}
+
+// formatOperandKind returns the objKind corresponding to format's
+// operandIdx'th operand.
+func formatOperandKind(format string, operandIdx int) objKind {
+	var (
+		prevOperandIdx int
+		kind           = kindAny
+	)
+	for {
+		i := strings.Index(format, "%")
+		if i == -1 {
+			break
+		}
+
+		var operands []formatOperand
+		format, operands = parsePrintfVerb(format[i+1:], prevOperandIdx)
+
+		// Check if any this verb's operands correspond to our target
+		// operandIdx.
+		for _, v := range operands {
+			if v.idx == operandIdx {
+				if kind == kindAny {
+					kind = v.kind
+				} else if v.kind != kindAny {
+					// If mulitple verbs refer to the same operand, take the
+					// intersection of their kinds.
+					kind &= v.kind
+				}
+			}
+
+			prevOperandIdx = v.idx
+		}
+	}
+	return kind
+}
+
+type formatOperand struct {
+	// idx is the one-based printf operand index.
+	idx int
+	// kind is a mask of expected kinds of objects for this operand.
+	kind objKind
+}
+
+// parsePrintfVerb parses the leading printf verb in f. The opening
+// "%" must already be trimmed from f. prevIdx is the previous
+// operand's index, or zero if this is the first verb. The format
+// string is returned with the leading verb removed. Multiple operands
+// can be returned in the case of dynamic widths such as "%*.*f".
+func parsePrintfVerb(f string, prevIdx int) (string, []formatOperand) {
+	var verbs []formatOperand
+
+	addVerb := func(k objKind) {
+		verbs = append(verbs, formatOperand{
+			idx:  prevIdx + 1,
+			kind: k,
+		})
+		prevIdx++
+	}
+
+	for len(f) > 0 {
+		// Trim first rune off of f so we are guaranteed to make progress.
+		r, l := utf8.DecodeRuneInString(f)
+		f = f[l:]
+
+		// We care about three things:
+		// 1. The verb, which maps directly to object kind.
+		// 2. Explicit operand indices like "%[2]s".
+		// 3. Dynamic widths using "*".
+		switch r {
+		case '%':
+			return f, nil
+		case '*':
+			addVerb(kindInt)
+			continue
+		case '[':
+			// Parse operand index as in "%[2]s".
+			i := strings.Index(f, "]")
+			if i == -1 {
+				return f, nil
+			}
+
+			idx, err := strconv.Atoi(f[:i])
+			f = f[i+1:]
+			if err != nil {
+				return f, nil
+			}
+
+			prevIdx = idx - 1
+			continue
+		case 'v', 'T':
+			addVerb(kindAny)
+		case 't':
+			addVerb(kindBool)
+		case 'c', 'd', 'o', 'O', 'U':
+			addVerb(kindInt)
+		case 'e', 'E', 'f', 'F', 'g', 'G':
+			addVerb(kindFloat | kindComplex)
+		case 'b':
+			addVerb(kindInt | kindFloat | kindComplex | kindBytes)
+		case 'q', 's':
+			addVerb(kindString | kindBytes | kindStringer | kindError)
+		case 'x', 'X':
+			// Omit kindStringer and kindError though technically allowed.
+			addVerb(kindString | kindBytes | kindInt | kindFloat | kindComplex)
+		case 'p':
+			addVerb(kindPtr | kindSlice)
+		case 'w':
+			addVerb(kindError)
+		case '+', '-', '#', ' ', '.', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9':
+			// Flag or numeric width/precicision value.
+			continue
+		default:
+			// Assume unrecognized rune is a custom fmt.Formatter verb.
+			addVerb(kindAny)
+		}
+
+		if len(verbs) > 0 {
+			break
+		}
+	}
+
+	return f, verbs
+}

internal/lsp/source/completion_printf_test.go

@@ -0,0 +1,72 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestFormatOperandKind(t *testing.T) {
+	cases := []struct {
+		f    string
+		idx  int
+		kind objKind
+	}{
+		{"", 1, kindAny},
+		{"%", 1, kindAny},
+		{"%%%", 1, kindAny},
+		{"%[1", 1, kindAny},
+		{"%[?%s", 2, kindAny},
+		{"%[abc]v", 1, kindAny},
+
+		{"%v", 1, kindAny},
+		{"%T", 1, kindAny},
+		{"%t", 1, kindBool},
+		{"%d", 1, kindInt},
+		{"%c", 1, kindInt},
+		{"%o", 1, kindInt},
+		{"%O", 1, kindInt},
+		{"%U", 1, kindInt},
+		{"%e", 1, kindFloat | kindComplex},
+		{"%E", 1, kindFloat | kindComplex},
+		{"%f", 1, kindFloat | kindComplex},
+		{"%F", 1, kindFloat | kindComplex},
+		{"%g", 1, kindFloat | kindComplex},
+		{"%G", 1, kindFloat | kindComplex},
+		{"%b", 1, kindInt | kindFloat | kindComplex | kindBytes},
+		{"%q", 1, kindString | kindBytes | kindStringer | kindError},
+		{"%s", 1, kindString | kindBytes | kindStringer | kindError},
+		{"%x", 1, kindString | kindBytes | kindInt | kindFloat | kindComplex},
+		{"%X", 1, kindString | kindBytes | kindInt | kindFloat | kindComplex},
+		{"%p", 1, kindPtr | kindSlice},
+		{"%w", 1, kindError},
+
+		{"%1.2f", 1, kindFloat | kindComplex},
+		{"%*f", 1, kindInt},
+		{"%*f", 2, kindFloat | kindComplex},
+		{"%*.*f", 1, kindInt},
+		{"%*.*f", 2, kindInt},
+		{"%*.*f", 3, kindFloat | kindComplex},
+		{"%[3]*.[2]*[1]f", 1, kindFloat | kindComplex},
+		{"%[3]*.[2]*[1]f", 2, kindInt},
+		{"%[3]*.[2]*[1]f", 3, kindInt},
+
+		{"foo %% %d", 1, kindInt},
+		{"%#-12.34f", 1, kindFloat | kindComplex},
+		{"% d", 1, kindInt},
+
+		{"%s %[1]X %d", 1, kindString | kindBytes},
+		{"%s %[1]X %d", 2, kindInt},
+	}
+
+	for _, c := range cases {
+		t.Run(fmt.Sprintf("%q#%d", c.f, c.idx), func(t *testing.T) {
+			if got := formatOperandKind(c.f, c.idx); got != c.kind {
+				t.Errorf("expected %d (%[1]b), got %d (%[2]b)", c.kind, got)
+			}
+		})
+	}
+}

internal/lsp/source/util.go

@@ -457,11 +457,11 @@ func enclosingValueSpec(path []ast.Node) *ast.ValueSpec {
 	return nil
 }
 
-// typeConversion returns the type being converted to if call is a type
-// conversion expression.
-func typeConversion(call *ast.CallExpr, info *types.Info) types.Type {
+// exprObj returns the types.Object associated with the *ast.Ident or
+// *ast.SelectorExpr e.
+func exprObj(info *types.Info, e ast.Expr) types.Object {
 	var ident *ast.Ident
-	switch expr := call.Fun.(type) {
+	switch expr := e.(type) {
 	case *ast.Ident:
 		ident = expr
 	case *ast.SelectorExpr:
@@ -470,8 +470,14 @@ func typeConversion(call *ast.CallExpr, info *types.Info) types.Type {
 		return nil
 	}
 
+	return info.ObjectOf(ident)
+}
+
+// typeConversion returns the type being converted to if call is a type
+// conversion expression.
+func typeConversion(call *ast.CallExpr, info *types.Info) types.Type {
 	// Type conversion (e.g. "float64(foo)").
-	if fun, _ := info.ObjectOf(ident).(*types.TypeName); fun != nil {
+	if fun, _ := exprObj(info, call.Fun).(*types.TypeName); fun != nil {
 		return fun.Type()
 	}
 

internal/lsp/testdata/lsp/primarymod/printf/printf.go

@@ -0,0 +1,33 @@
+package printf
+
+import "fmt"
+
+func myPrintf(string, ...interface{}) {}
+
+func _() {
+	var (
+		aInt      int          //@item(printfInt, "aInt", "int", "var")
+		aFloat    float64      //@item(printfFloat, "aFloat", "float64", "var")
+		aString   string       //@item(printfString, "aString", "string", "var")
+		aBytes    []byte       //@item(printfBytes, "aBytes", "[]byte", "var")
+		aStringer fmt.Stringer //@item(printfStringer, "aStringer", "fmt.Stringer", "var")
+		aError    error        //@item(printfError, "aError", "error", "var")
+		aBool     bool         //@item(printfBool, "aBool", "bool", "var")
+	)
+
+	myPrintf("%d", a)       //@rank(")", printfInt, printfFloat)
+	myPrintf("%s", a)       //@rank(")", printfString, printfInt),rank(")", printfBytes, printfInt),rank(")", printfStringer, printfInt),rank(")", printfError, printfInt)
+	myPrintf("%w", a)       //@rank(")", printfError, printfInt)
+	myPrintf("%x %[1]b", a) //@rank(")", printfInt, printfString)
+
+	fmt.Printf("%t", a) //@rank(")", printfBool, printfInt)
+
+	fmt.Fprintf(nil, "%f", a) //@rank(")", printfFloat, printfInt)
+
+	fmt.Sprintf("%[2]q %[1]*.[3]*[4]f",
+		a, //@rank(",", printfInt, printfFloat)
+		a, //@rank(",", printfString, printfFloat)
+		a, //@rank(",", printfInt, printfFloat)
+		a, //@rank(",", printfFloat, printfInt)
+	)
+}

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

@@ -6,7 +6,7 @@ CompletionSnippetCount = 85
 UnimportedCompletionsCount = 6
 DeepCompletionsCount = 5
 FuzzyCompletionsCount = 8
-RankedCompletionsCount = 139
+RankedCompletionsCount = 152
 CaseSensitiveCompletionsCount = 4
 DiagnosticsCount = 44
 FoldingRangesCount = 2