internal/golang: add a fast path for FormatVarType with gopls at 1.23
Now that gopls is only built with Go 1.23, we can rely on
types.TypeString to preserve alias information, and only need the
bespoke logic of FormatVarType to handle the cases where types contain
an invalid type.
This should improve performance for completion, particularly since it
affects the single-threaded construction of candidates.
For example, here is the impact on a relevant benchmark:
Completion/kubernetes_selector/edit=false/unimported=false/budget=100ms
Results:
│ before.txt │ after.txt │
│ sec/op │ sec/op vs base │
81.29m ± 1% 65.83m ± 1% -19.02% (p=0.000 n=10)
│ before.txt │ after.txt │
│ cpu_seconds/op │ cpu_seconds/op vs base │
151.8m ± 15% 101.1m ± 33% -33.36% (p=0.000 n=10)
Change-Id: I60d890ca102a97cf6b198621ba82afe7eeab7fb9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/611836
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/golang/types_format.go b/gopls/internal/golang/types_format.go
index 51584bc..4182824 100644
--- a/gopls/internal/golang/types_format.go
+++ b/gopls/internal/golang/types_format.go
@@ -214,6 +214,9 @@
if err != nil {
return nil, err
}
+ if sig.Variadic() && i == sig.Params().Len()-1 {
+ typ = strings.Replace(typ, "[]", "...", 1)
+ }
p := typ
if el.Name() != "" {
p = el.Name() + " " + typ
@@ -261,6 +264,10 @@
}, nil
}
+// We look for 'invalidTypeString' to determine if we can use the fast path for
+// FormatVarType.
+var invalidTypeString = types.Typ[types.Invalid].String()
+
// 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 falls back to types.TypeString.
@@ -268,6 +275,21 @@
// TODO(rfindley): this function could return the actual name used in syntax,
// for better parameter names.
func FormatVarType(ctx context.Context, snapshot *cache.Snapshot, srcpkg *cache.Package, obj *types.Var, qf types.Qualifier, mq MetadataQualifier) (string, error) {
+ typeString := types.TypeString(obj.Type(), qf)
+ // Fast path: if the type string does not contain 'invalid type', we no
+ // longer need to do any special handling, thanks to materialized aliases in
+ // Go 1.23+.
+ //
+ // Unfortunately, due to the handling of invalid types, we can't quite delete
+ // the rather complicated preexisting logic of FormatVarType--it isn't an
+ // acceptable regression to start printing "invalid type" in completion or
+ // signature help. strings.Contains is conservative: the type string of a
+ // valid type may actually contain "invalid type" (due to struct tags or
+ // field formatting), but such cases should be exceedingly rare.
+ if !strings.Contains(typeString, invalidTypeString) {
+ return typeString, nil
+ }
+
// TODO(rfindley): This looks wrong. The previous comment said:
// "If the given expr refers to a type parameter, then use the
// object's Type instead of the type parameter declaration. This helps
@@ -280,13 +302,13 @@
//
// Left this during refactoring in order to preserve pre-existing logic.
if typeparams.IsTypeParam(obj.Type()) {
- return types.TypeString(obj.Type(), qf), nil
+ return typeString, nil
}
if isBuiltin(obj) {
// This is defensive, though it is extremely unlikely we'll ever have a
// builtin var.
- return types.TypeString(obj.Type(), qf), nil
+ return typeString, nil
}
// TODO(rfindley): parsing to produce candidates can be costly; consider
@@ -309,7 +331,7 @@
// for parameterized decls.
if decl, _ := decl.(*ast.FuncDecl); decl != nil {
if decl.Type.TypeParams.NumFields() > 0 {
- return types.TypeString(obj.Type(), qf), nil // in generic function
+ return typeString, nil // in generic function
}
if decl.Recv != nil && len(decl.Recv.List) > 0 {
rtype := decl.Recv.List[0].Type
@@ -317,18 +339,18 @@
rtype = e.X
}
if x, _, _, _ := typeparams.UnpackIndexExpr(rtype); x != nil {
- return types.TypeString(obj.Type(), qf), nil // in method of generic type
+ return typeString, nil // in method of generic type
}
}
}
if spec, _ := spec.(*ast.TypeSpec); spec != nil && spec.TypeParams.NumFields() > 0 {
- return types.TypeString(obj.Type(), qf), nil // in generic type decl
+ return typeString, nil // in generic type decl
}
if field == nil {
// TODO(rfindley): we should never reach here from an ordinary var, so
// should probably return an error here.
- return types.TypeString(obj.Type(), qf), nil
+ return typeString, nil
}
expr := field.Type
diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go
index d3a7685..f5f4ea0 100644
--- a/gopls/internal/test/marker/marker_test.go
+++ b/gopls/internal/test/marker/marker_test.go
@@ -1412,7 +1412,7 @@
return
}
if got != want {
- mark.errorf("snippets do not match: got %q, want %q", got, want)
+ mark.errorf("snippets do not match: got:\n%q\nwant:\n%q", got, want)
}
}
diff --git a/gopls/internal/test/marker/testdata/completion/foobarbaz.txt b/gopls/internal/test/marker/testdata/completion/foobarbaz.txt
index 24ac717..1da0a40 100644
--- a/gopls/internal/test/marker/testdata/completion/foobarbaz.txt
+++ b/gopls/internal/test/marker/testdata/completion/foobarbaz.txt
@@ -476,7 +476,7 @@
const two = 2
var builtinTypes func([]int, [two]bool, map[string]string, struct{ i int }, interface{ foo() }, <-chan int)
- builtinTypes = f //@snippet(" //", litFunc, "func(i1 []int, b [two]bool, m map[string]string, s struct{ i int \\}, i2 interface{ foo() \\}, c <-chan int) {$0\\}")
+ builtinTypes = f //@snippet(" //", litFunc, "func(i1 []int, b [2]bool, m map[string]string, s struct{i int\\}, i2 interface{foo()\\}, c <-chan int) {$0\\}")
var _ func(ast.Node) = f //@snippet(" //", litFunc, "func(n ast.Node) {$0\\}")
var _ func(error) = f //@snippet(" //", litFunc, "func(err error) {$0\\}")
