Skip to content

Commit

Permalink
gopls/internal/lsp/source/completion: ensuring completion completeness
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
findleyr authored and gopherbot committed Jun 14, 2023
1 parent ac29460 commit 41e4e56
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 29 deletions.
2 changes: 1 addition & 1 deletion gopls/internal/lsp/cache/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 24 additions & 22 deletions gopls/internal/lsp/source/completion/completion.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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
}
Expand Down
8 changes: 5 additions & 3 deletions gopls/internal/lsp/source/completion/deep_completion.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion gopls/internal/lsp/source/typerefs/refs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ type P struct{}
func (a) x(P)
`},
want: map[string][]string{},
want: map[string][]string{},
allowErrs: true,
},
{
Expand Down
65 changes: 63 additions & 2 deletions gopls/internal/regtest/completion/completion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 := `
Expand Down

0 comments on commit 41e4e56

Please sign in to comment.