From 41e4e565498859435a2ad44a71cf6701a6afb585 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 13 Jun 2023 15:43:44 -0400 Subject: [PATCH] gopls/internal/lsp/source/completion: ensuring completion completeness Ensure that completion processes at least depth=0 elements, by switching to a different model for truncating search. Don't encode the search deadline in the context, as the handling for context cancellation should differ from the handling of being out of budget. For example, we should not fail to format a completion item if we are out of budget. While at it, don't include type checking time in the completion budget, as it is highly variable and depends on the ordering of requests from the client: for example, if the client has already requested code lens, then the type-checked package will already exist and completion will not include type-checking in the budget. No documentation needs to be updated as the current documentation already says "this normally takes milliseconds", which can only be true if it doesn't include type checking. Also add a regression test that asserts we find all struct fields in completion results. Fixes golang/go#53992 Change-Id: I1aeb749cf64052b6a444166638a78b9945964e84 Reviewed-on: https://go-review.googlesource.com/c/tools/+/503016 Auto-Submit: Robert Findley Run-TryBot: Robert Findley Reviewed-by: Alan Donovan TryBot-Result: Gopher Robot --- gopls/internal/lsp/cache/check.go | 2 +- .../lsp/source/completion/completion.go | 46 ++++++------- .../lsp/source/completion/deep_completion.go | 8 ++- .../internal/lsp/source/typerefs/refs_test.go | 2 +- .../regtest/completion/completion_test.go | 65 ++++++++++++++++++- 5 files changed, 94 insertions(+), 29 deletions(-) diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go index 6cfabe3dbcb..59b19e03bab 100644 --- a/gopls/internal/lsp/cache/check.go +++ b/gopls/internal/lsp/cache/check.go @@ -748,7 +748,7 @@ func (ph *packageHandle) clone(validated bool) *packageHandle { } // getPackageHandles gets package handles for all given ids and their -// dependencies. +// dependencies, recursively. func (s *snapshot) getPackageHandles(ctx context.Context, ids []PackageID) (map[PackageID]*packageHandle, error) { s.mu.Lock() meta := s.meta diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go index de54a3f11f5..45c92d66f20 100644 --- a/gopls/internal/lsp/source/completion/completion.go +++ b/gopls/internal/lsp/source/completion/completion.go @@ -232,10 +232,6 @@ type completer struct { // mapper converts the positions in the file from which the completion originated. mapper *protocol.Mapper - // startTime is when we started processing this completion request. It does - // not include any time the request spent in the queue. - startTime time.Time - // scopes contains all scopes defined by nodes in our path, // including nil values for nodes that don't defined a scope. It // also includes our package scope and the universal scope at the @@ -445,8 +441,6 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan ctx, done := event.Start(ctx, "completion.Completion") defer done() - startTime := time.Now() - pkg, pgf, err := source.NarrowestPackageForFile(ctx, snapshot, fh.URI()) if err != nil || pgf.File.Package == token.NoPos { // If we can't parse this file or find position for the package @@ -555,22 +549,30 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan matcher: prefixMatcher(""), methodSetCache: make(map[methodSetKey]*types.MethodSet), mapper: pgf.Mapper, - startTime: startTime, scopes: scopes, } - var cancel context.CancelFunc - if c.opts.budget == 0 { - ctx, cancel = context.WithCancel(ctx) - } else { - // timeoutDuration is the completion budget remaining. If less than - // 10ms, set to 10ms - timeoutDuration := time.Until(c.startTime.Add(c.opts.budget)) - if timeoutDuration < 10*time.Millisecond { - timeoutDuration = 10 * time.Millisecond - } - ctx, cancel = context.WithTimeout(ctx, timeoutDuration) + ctx, cancel := context.WithCancel(ctx) + + // Compute the deadline for this operation. Deadline is relative to the + // search operation, not the entire completion RPC, as the work up until this + // point depends significantly on how long it took to type-check, which in + // turn depends on the timing of the request relative to other operations on + // the snapshot. Including that work in the budget leads to inconsistent + // results (and realistically, if type-checking took 200ms already, the user + // is unlikely to be significantly more bothered by e.g. another 100ms of + // search). + // + // Don't overload the context with this deadline, as we don't want to + // conflate user cancellation (=fail the operation) with our time limit + // (=stop searching and succeed with partial results). + start := time.Now() + var deadline *time.Time + if c.opts.budget > 0 { + d := start.Add(c.opts.budget) + deadline = &d } + defer cancel() if surrounding := c.containingIdent(pgf.Src); surrounding != nil { @@ -585,7 +587,7 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan } // Deep search collected candidates and their members for more candidates. - c.deepSearch(ctx) + c.deepSearch(ctx, start, deadline) for _, callback := range c.completionCallbacks { if err := c.snapshot.RunProcessEnvFunc(ctx, callback); err != nil { @@ -595,7 +597,7 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan // Search candidates populated by expensive operations like // unimportedMembers etc. for more completion items. - c.deepSearch(ctx) + c.deepSearch(ctx, start, deadline) // Statement candidates offer an entire statement in certain contexts, as // opposed to a single object. Add statement candidates last because they @@ -614,7 +616,7 @@ func (c *completer) collectCompletions(ctx context.Context) error { if !(importSpec.Path.Pos() <= c.pos && c.pos <= importSpec.Path.End()) { continue } - return c.populateImportCompletions(ctx, importSpec) + return c.populateImportCompletions(importSpec) } // Inside comments, offer completions for the name of the relevant symbol. @@ -767,7 +769,7 @@ func (c *completer) emptySwitchStmt() bool { // Completions for "golang.org/" yield its subdirectories // (i.e. "golang.org/x/"). The user is meant to accept completion suggestions // until they reach a complete import path. -func (c *completer) populateImportCompletions(ctx context.Context, searchImport *ast.ImportSpec) error { +func (c *completer) populateImportCompletions(searchImport *ast.ImportSpec) error { if !strings.HasPrefix(searchImport.Path.Value, `"`) { return nil } diff --git a/gopls/internal/lsp/source/completion/deep_completion.go b/gopls/internal/lsp/source/completion/deep_completion.go index a72d5619105..66309530e73 100644 --- a/gopls/internal/lsp/source/completion/deep_completion.go +++ b/gopls/internal/lsp/source/completion/deep_completion.go @@ -113,7 +113,7 @@ func (s *deepCompletionState) newPath(cand candidate, obj types.Object) []types. // deepSearch searches a candidate and its subordinate objects for completion // items if deep completion is enabled and adds the valid candidates to // completion items. -func (c *completer) deepSearch(ctx context.Context) { +func (c *completer) deepSearch(ctx context.Context, start time.Time, deadline *time.Time) { defer func() { // We can return early before completing the search, so be sure to // clear out our queues to not impact any further invocations. @@ -121,7 +121,9 @@ func (c *completer) deepSearch(ctx context.Context) { c.deepState.nextQueue = c.deepState.nextQueue[:0] }() - for len(c.deepState.nextQueue) > 0 { + first := true // always fully process the first set of candidates + for len(c.deepState.nextQueue) > 0 && (first || deadline == nil || time.Now().Before(*deadline)) { + first = false c.deepState.thisQueue, c.deepState.nextQueue = c.deepState.nextQueue, c.deepState.thisQueue[:0] outer: @@ -170,7 +172,7 @@ func (c *completer) deepSearch(ctx context.Context) { c.deepState.candidateCount++ if c.opts.budget > 0 && c.deepState.candidateCount%100 == 0 { - spent := float64(time.Since(c.startTime)) / float64(c.opts.budget) + spent := float64(time.Since(start)) / float64(c.opts.budget) select { case <-ctx.Done(): return diff --git a/gopls/internal/lsp/source/typerefs/refs_test.go b/gopls/internal/lsp/source/typerefs/refs_test.go index eb1b1e1bb2f..adf79bd8f7e 100644 --- a/gopls/internal/lsp/source/typerefs/refs_test.go +++ b/gopls/internal/lsp/source/typerefs/refs_test.go @@ -485,7 +485,7 @@ type P struct{} func (a) x(P) `}, - want: map[string][]string{}, + want: map[string][]string{}, allowErrs: true, }, { diff --git a/gopls/internal/regtest/completion/completion_test.go b/gopls/internal/regtest/completion/completion_test.go index 7f865942206..0a898c48d16 100644 --- a/gopls/internal/regtest/completion/completion_test.go +++ b/gopls/internal/regtest/completion/completion_test.go @@ -8,14 +8,15 @@ import ( "fmt" "strings" "testing" + "time" "github.com/google/go-cmp/cmp" "golang.org/x/tools/gopls/internal/bug" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/gopls/internal/lsp/fake" + "golang.org/x/tools/gopls/internal/lsp/protocol" . "golang.org/x/tools/gopls/internal/lsp/regtest" "golang.org/x/tools/internal/testenv" - - "golang.org/x/tools/gopls/internal/lsp/protocol" ) func TestMain(m *testing.M) { @@ -622,6 +623,66 @@ func main() { }) } +func TestCompleteAllFields(t *testing.T) { + // This test verifies that completion results always include all struct fields. + // See golang/go#53992. + + const src = ` +-- go.mod -- +module mod.com + +go 1.18 + +-- p/p.go -- +package p + +import ( + "fmt" + + . "net/http" + . "runtime" + . "go/types" + . "go/parser" + . "go/ast" +) + +type S struct { + a, b, c, d, e, f, g, h, i, j, k, l, m int + n, o, p, q, r, s, t, u, v, w, x, y, z int +} + +func _() { + var s S + fmt.Println(s.) +} +` + + WithOptions(Settings{ + "completionBudget": "1ns", // must be non-zero as 0 => infinity + }).Run(t, src, func(t *testing.T, env *Env) { + wantFields := make(map[string]bool) + for c := 'a'; c <= 'z'; c++ { + wantFields[string(c)] = true + } + + env.OpenFile("p/p.go") + // Make an arbitrary edit to ensure we're not hitting the cache. + env.EditBuffer("p/p.go", fake.NewEdit(0, 0, 0, 0, fmt.Sprintf("// current time: %v\n", time.Now()))) + loc := env.RegexpSearch("p/p.go", `s\.()`) + completions := env.Completion(loc) + gotFields := make(map[string]bool) + for _, item := range completions.Items { + if item.Kind == protocol.FieldCompletion { + gotFields[item.Label] = true + } + } + + if diff := cmp.Diff(wantFields, gotFields); diff != "" { + t.Errorf("Completion(...) returned mismatching fields (-want +got):\n%s", diff) + } + }) +} + func TestDefinition(t *testing.T) { testenv.NeedsGo1Point(t, 17) // in go1.16, The FieldList in func x is not empty files := `