From 9ac8e33b364013c87ce70b51928d01d52c302476 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Mon, 3 Aug 2020 16:50:08 -0700 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Robert Findley --- internal/lsp/source/completion.go | 128 +++++++++---- internal/lsp/source/completion_printf.go | 172 ++++++++++++++++++ internal/lsp/source/completion_printf_test.go | 72 ++++++++ internal/lsp/source/util.go | 16 +- .../testdata/lsp/primarymod/printf/printf.go | 33 ++++ internal/lsp/testdata/lsp/summary.txt.golden | 2 +- 6 files changed, 384 insertions(+), 39 deletions(-) create mode 100644 internal/lsp/source/completion_printf.go create mode 100644 internal/lsp/source/completion_printf_test.go create mode 100644 internal/lsp/testdata/lsp/primarymod/printf/printf.go diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 2955af6f47..639e6a6857 100644 --- a/internal/lsp/source/completion.go +++ b/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 } diff --git a/internal/lsp/source/completion_printf.go b/internal/lsp/source/completion_printf.go new file mode 100644 index 0000000000..e13a938ec9 --- /dev/null +++ b/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 +} diff --git a/internal/lsp/source/completion_printf_test.go b/internal/lsp/source/completion_printf_test.go new file mode 100644 index 0000000000..ab20f27d09 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index d52f90b0b0..a043a1746c 100644 --- a/internal/lsp/source/util.go +++ b/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() } diff --git a/internal/lsp/testdata/lsp/primarymod/printf/printf.go b/internal/lsp/testdata/lsp/primarymod/printf/printf.go new file mode 100644 index 0000000000..6e56549c14 --- /dev/null +++ b/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) + ) +} diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden index 654d04c449..8ff02aff16 100644 --- a/internal/lsp/testdata/lsp/summary.txt.golden +++ b/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