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 <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

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

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
 	}

gopls/internal/lsp/source/completion/deep_completion.go

@@ -113,15 +113,17 @@ 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.
 		c.deepState.thisQueue = c.deepState.thisQueue[:0]
 		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

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,
 		},
 		{

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 := `