diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index cd8e498e7c1..f81c54adb74 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -1113,14 +1113,6 @@ func (s *snapshot) AllMetadata(ctx context.Context) ([]*source.Metadata, error) return meta, nil } -func (s *snapshot) CachedPackages(ctx context.Context) []source.Package { - // Cached packages do not make sense with incremental gopls. - // - // TODO(golang/go#58663): re-implement unimported completions to not depend - // on cached import paths. - return nil -} - // TODO(rfindley): clarify that this is only active modules. Or update to just // use findRootPattern. func (s *snapshot) GoModForFile(uri span.URI) span.URI { diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go index 7ecd7a78679..f8c7654f68d 100644 --- a/gopls/internal/lsp/source/completion/completion.go +++ b/gopls/internal/lsp/source/completion/completion.go @@ -7,10 +7,12 @@ package completion import ( + "bytes" "context" "fmt" "go/ast" "go/constant" + "go/parser" "go/scanner" "go/token" "go/types" @@ -19,20 +21,28 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "time" "unicode" + "golang.org/x/sync/errgroup" "golang.org/x/tools/go/ast/astutil" "golang.org/x/tools/gopls/internal/lsp/protocol" + "golang.org/x/tools/gopls/internal/lsp/safetoken" "golang.org/x/tools/gopls/internal/lsp/snippet" "golang.org/x/tools/gopls/internal/lsp/source" + "golang.org/x/tools/gopls/internal/span" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/fuzzy" "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/typeparams" ) +// A CompletionItem represents a possible completion suggested by the algorithm. type CompletionItem struct { + + // Invariant: CompletionItem does not refer to syntax or types. + // Label is the primary text the user sees for this completion item. Label string @@ -85,9 +95,10 @@ type CompletionItem struct { // Documentation is the documentation for the completion item. Documentation string - // obj is the object from which this candidate was derived, if any. - // obj is for internal use only. - obj types.Object + // isSlice reports whether the underlying type of the object + // from which this candidate was derived is a slice. + // (Used to complete append() calls.) + isSlice bool } // completionOptions holds completion specific configuration. @@ -1095,92 +1106,91 @@ const ( func (c *completer) selector(ctx context.Context, sel *ast.SelectorExpr) error { c.inference.objChain = objChain(c.pkg.GetTypesInfo(), sel.X) - // Is sel a qualified identifier? - if id, ok := sel.X.(*ast.Ident); ok { - if pkgName, ok := c.pkg.GetTypesInfo().Uses[id].(*types.PkgName); ok { - // If this package path is not a known dep, it means that it resolves to - // an import path that couldn't be resolved by go/packages. - // - // Try to complete from the package cache. - pkgPath := source.PackagePath(pkgName.Imported().Path()) - if _, ok := c.pkg.Metadata().DepsByPkgPath[pkgPath]; !ok && c.opts.unimported { - if err := c.unimportedMembers(ctx, id); err != nil { - return err - } - } - c.packageMembers(pkgName.Imported(), stdScore, nil, func(cand candidate) { - c.deepState.enqueue(cand) - }) - return nil - } - } - - // Invariant: sel is a true selector. - tv, ok := c.pkg.GetTypesInfo().Types[sel.X] - if ok { - c.methodsAndFields(tv.Type, tv.Addressable(), nil, func(cand candidate) { - c.deepState.enqueue(cand) - }) - + // True selector? + if tv, ok := c.pkg.GetTypesInfo().Types[sel.X]; ok { + c.methodsAndFields(tv.Type, tv.Addressable(), nil, c.deepState.enqueue) c.addPostfixSnippetCandidates(ctx, sel) - return nil } - // Try unimported packages. - if id, ok := sel.X.(*ast.Ident); ok && c.opts.unimported { - if err := c.unimportedMembers(ctx, id); err != nil { - return err - } + id, ok := sel.X.(*ast.Ident) + if !ok { + return nil } - return nil -} -func (c *completer) unimportedMembers(ctx context.Context, id *ast.Ident) error { - // Try loaded packages first. They're relevant, fast, and fully typed. - // - // Find all cached packages that are imported a nonzero amount of time. - // - // TODO(rfindley): this is pre-existing behavior, and a test fails if we - // don't do the importCount filter, but why do we care if a package is - // imported a nonzero amount of times? - // - // TODO(adonovan): avoid the need for type-checked packges entirely here. - pkgs := make(map[source.PackagePath]source.Package) - imported := make(map[source.PackagePath]bool) - for _, pkg := range c.snapshot.CachedPackages(ctx) { - m := pkg.Metadata() - for dep := range m.DepsByPkgPath { - imported[dep] = true + // Treat sel as a qualified identifier. + var filter func(*source.Metadata) bool + needImport := false + if pkgName, ok := c.pkg.GetTypesInfo().Uses[id].(*types.PkgName); ok { + // Qualified identifier with import declaration. + imp := pkgName.Imported() + + // Known direct dependency? Expand using type information. + if _, ok := c.pkg.Metadata().DepsByPkgPath[source.PackagePath(imp.Path())]; ok { + c.packageMembers(imp, stdScore, nil, c.deepState.enqueue) + return nil } - if m.Name == "main" { - continue + + // Imported declaration with missing type information. + // Fall through to shallow completion of unimported package members. + // Match candidate packages by path. + // TODO(adonovan): simplify by merging with else case and matching on name only? + filter = func(m *source.Metadata) bool { + return strings.TrimPrefix(string(m.PkgPath), "vendor/") == imp.Path() } - if old, ok := pkgs[m.PkgPath]; ok { - if len(m.CompiledGoFiles) < len(old.Metadata().CompiledGoFiles) { - pkgs[m.PkgPath] = pkg - } - } else { - pkgs[m.PkgPath] = pkg + } else { + // Qualified identifier without import declaration. + // Match candidate packages by name. + filter = func(m *source.Metadata) bool { + return string(m.Name) == id.Name } + needImport = true } - known := make(map[source.PackagePath]*types.Package) - for pkgPath, pkg := range pkgs { - if imported[pkgPath] { - known[pkgPath] = pkg.GetTypes() - } + + // Search unimported packages. + if !c.opts.unimported { + return nil // feature disabled } + // The deep completion algorithm is exceedingly complex and + // deeply coupled to the now obsolete notions that all + // token.Pos values can be interpreted by as a single FileSet + // belonging to the Snapshot and that all types.Object values + // are canonicalized by a single types.Importer mapping. + // These invariants are no longer true now that gopls uses + // an incremental approach, parsing and type-checking each + // package separately. + // + // Consequently, completion of symbols defined in packages that + // are not currently imported by the query file cannot use the + // deep completion machinery which is based on type information. + // Instead it must use only syntax information from a quick + // parse of top-level declarations (but not function bodies). + // + // TODO(adonovan): rewrite the deep completion machinery to + // not assume global Pos/Object realms and then use export + // data instead of the quick parse approach taken here. + + // First, we search among packages in the workspace. + // We'll use a fast parse to extract package members + // from those that match the name/path criterion. + all, err := c.snapshot.AllMetadata(ctx) + if err != nil { + return err + } var paths []string - for path, pkg := range known { - if pkg.Name() != id.Name { + known := make(map[source.PackagePath][]*source.Metadata) // may include test variant + for _, m := range all { + if m.IsIntermediateTestVariant() || m.Name == "main" || !filter(m) { continue } - paths = append(paths, string(path)) + known[m.PkgPath] = append(known[m.PkgPath], m) + paths = append(paths, string(m.PkgPath)) } + // Rank import paths as goimports would. var relevances map[string]float64 - if len(paths) != 0 { + if len(paths) > 0 { if err := c.snapshot.RunProcessEnvFunc(ctx, func(opts *imports.Options) error { var err error relevances, err = imports.ScoreImportPaths(ctx, opts.Env, paths) @@ -1188,32 +1198,119 @@ func (c *completer) unimportedMembers(ctx context.Context, id *ast.Ident) error }); err != nil { return err } + sort.Slice(paths, func(i, j int) bool { + return relevances[paths[i]] > relevances[paths[j]] + }) } - sort.Slice(paths, func(i, j int) bool { - return relevances[paths[i]] > relevances[paths[j]] - }) - for _, path := range paths { - pkg := known[source.PackagePath(path)] - if pkg.Name() != id.Name { - continue + // quickParse does a quick parse of a single file of package m, + // extracts exported package members and adds candidates to c.items. + var itemsMu sync.Mutex // guards c.items + var enough int32 // atomic bool + quickParse := func(uri span.URI, m *source.Metadata) error { + if atomic.LoadInt32(&enough) != 0 { + return nil } - imp := &importInfo{ - importPath: path, + + fh, err := c.snapshot.GetFile(ctx, uri) + if err != nil { + return err } - if imports.ImportPathToAssumedName(path) != pkg.Name() { - imp.name = pkg.Name() + content, err := fh.Read() + if err != nil { + return err } - c.packageMembers(pkg, unimportedScore(relevances[path]), imp, func(cand candidate) { - c.deepState.enqueue(cand) + path := string(m.PkgPath) + forEachPackageMember(content, func(tok token.Token, id *ast.Ident, fn *ast.FuncDecl) { + if atomic.LoadInt32(&enough) != 0 { + return + } + + if !id.IsExported() || + sel.Sel.Name != "_" && !strings.HasPrefix(id.Name, sel.Sel.Name) { + return // not a match + } + + // The only detail is the kind and package: `var (from "example.com/foo")` + // TODO(adonovan): pretty-print FuncDecl.FuncType or TypeSpec.Type? + item := CompletionItem{ + Label: id.Name, + Detail: fmt.Sprintf("%s (from %q)", strings.ToLower(tok.String()), m.PkgPath), + InsertText: id.Name, + Score: unimportedScore(relevances[path]), + } + switch tok { + case token.FUNC: + item.Kind = protocol.FunctionCompletion + case token.VAR: + item.Kind = protocol.VariableCompletion + case token.CONST: + item.Kind = protocol.ConstantCompletion + case token.TYPE: + // Without types, we can't distinguish Class from Interface. + item.Kind = protocol.ClassCompletion + } + + if needImport { + imp := &importInfo{importPath: path} + if imports.ImportPathToAssumedName(path) != string(m.Name) { + imp.name = string(m.Name) + } + item.AdditionalTextEdits, _ = c.importEdits(imp) + } + + // For functions, add a parameter snippet. + if fn != nil { + var sn snippet.Builder + sn.WriteText(id.Name) + sn.WriteText("(") + var nparams int + for _, field := range fn.Type.Params.List { + if field.Names != nil { + nparams += len(field.Names) + } else { + nparams++ + } + } + for i := 0; i < nparams; i++ { + if i > 0 { + sn.WriteText(", ") + } + sn.WritePlaceholder(nil) + } + sn.WriteText(")") + item.snippet = &sn + } + + itemsMu.Lock() + c.items = append(c.items, item) + if len(c.items) >= unimportedMemberTarget { + atomic.StoreInt32(&enough, 1) + } + itemsMu.Unlock() }) - if len(c.items) >= unimportedMemberTarget { - return nil + return nil + } + + // Extract the package-level candidates using a quick parse. + var g errgroup.Group + for _, path := range paths { + for _, m := range known[source.PackagePath(path)] { + m := m + for _, uri := range m.CompiledGoFiles { + uri := uri + g.Go(func() error { + return quickParse(uri, m) + }) + } } } + if err := g.Wait(); err != nil { + return err + } + // In addition, we search in the module cache using goimports. ctx, cancel := context.WithCancel(ctx) - var mu sync.Mutex add := func(pkgExport imports.PackageExport) { mu.Lock() @@ -3060,3 +3157,96 @@ func (c *completer) innermostScope() *types.Scope { } return nil } + +// isSlice reports whether the object's underlying type is a slice. +func isSlice(obj types.Object) bool { + if obj != nil && obj.Type() != nil { + if _, ok := obj.Type().Underlying().(*types.Slice); ok { + return true + } + } + return false +} + +// forEachPackageMember calls f(tok, id, fn) for each package-level +// TYPE/VAR/CONST/FUNC declaration in the Go source file, based on a +// quick partial parse. fn is non-nil only for function declarations. +// The AST position information is garbage. +func forEachPackageMember(content []byte, f func(tok token.Token, id *ast.Ident, fn *ast.FuncDecl)) { + purged := purgeFuncBodies(content) + file, _ := parser.ParseFile(token.NewFileSet(), "", purged, 0) + for _, decl := range file.Decls { + switch decl := decl.(type) { + case *ast.GenDecl: + for _, spec := range decl.Specs { + switch spec := spec.(type) { + case *ast.ValueSpec: // var/const + for _, id := range spec.Names { + f(decl.Tok, id, nil) + } + case *ast.TypeSpec: + f(decl.Tok, spec.Name, nil) + } + } + case *ast.FuncDecl: + if decl.Recv == nil { + f(token.FUNC, decl.Name, decl) + } + } + } +} + +// purgeFuncBodies returns a copy of src in which the contents of each +// outermost {...} region except struct and interface types have been +// deleted. It does not preserve newlines. This reduces the amount of +// work required to parse the top-level declarations. +func purgeFuncBodies(src []byte) []byte { + // Destroy the content of any {...}-bracketed regions that are + // not immediately preceded by a "struct" or "interface" + // token. That includes function bodies, composite literals, + // switch/select bodies, and all blocks of statements. + // This will lead to non-void functions that don't have return + // statements, which of course is a type error, but that's ok. + + var out bytes.Buffer + file := token.NewFileSet().AddFile("", -1, len(src)) + var sc scanner.Scanner + sc.Init(file, src, nil, 0) + var prev token.Token + var cursor int // last consumed src offset + var braces []token.Pos // stack of unclosed braces or -1 for struct/interface type + for { + pos, tok, _ := sc.Scan() + if tok == token.EOF { + break + } + switch tok { + case token.COMMENT: + // TODO(adonovan): opt: skip, to save an estimated 20% of time. + + case token.LBRACE: + if prev == token.STRUCT || prev == token.INTERFACE { + pos = -1 + } + braces = append(braces, pos) + + case token.RBRACE: + if last := len(braces) - 1; last >= 0 { + top := braces[last] + braces = braces[:last] + if top < 0 { + // struct/interface type: leave alone + } else if len(braces) == 0 { // toplevel only + // Delete {...} body. + start, _ := safetoken.Offset(file, top) + end, _ := safetoken.Offset(file, pos) + out.Write(src[cursor : start+len("{")]) + cursor = end + } + } + } + prev = tok + } + out.Write(src[cursor:]) + return out.Bytes() +} diff --git a/gopls/internal/lsp/source/completion/definition.go b/gopls/internal/lsp/source/completion/definition.go index a0160a1a1e3..d7f51f0029c 100644 --- a/gopls/internal/lsp/source/completion/definition.go +++ b/gopls/internal/lsp/source/completion/definition.go @@ -144,7 +144,7 @@ func defSnippet(prefix, suffix string, obj types.Object) CompletionItem { Score: 10, snippet: &sn, Documentation: prefix + " test function", - obj: obj, + isSlice: isSlice(obj), } } func defItem(val string, obj types.Object) CompletionItem { @@ -155,6 +155,6 @@ func defItem(val string, obj types.Object) CompletionItem { Depth: 0, Score: 9, // prefer the snippets when available Documentation: "complete the function name", - obj: obj, + isSlice: isSlice(obj), } } diff --git a/gopls/internal/lsp/source/completion/format.go b/gopls/internal/lsp/source/completion/format.go index 634cdc8bd00..c2d2c0bc035 100644 --- a/gopls/internal/lsp/source/completion/format.go +++ b/gopls/internal/lsp/source/completion/format.go @@ -228,7 +228,7 @@ Suffixes: Score: cand.score, Depth: len(cand.path), snippet: &snip, - obj: obj, + isSlice: isSlice(obj), } // If the user doesn't want documentation for completion items. if !c.opts.documentation { diff --git a/gopls/internal/lsp/source/completion/fuzz.go b/gopls/internal/lsp/source/completion/fuzz.go index d7912ceabc6..08e7654c7ed 100644 --- a/gopls/internal/lsp/source/completion/fuzz.go +++ b/gopls/internal/lsp/source/completion/fuzz.go @@ -121,7 +121,7 @@ Loop: Depth: 0, Score: 10, // pretty confident the user should see this Documentation: "argument types from f.Add", - obj: nil, + isSlice: false, } c.items = append(c.items, xx) for i := 0; i < mset.Len(); i++ { diff --git a/gopls/internal/lsp/source/completion/statements.go b/gopls/internal/lsp/source/completion/statements.go index 809cb808a60..707375fa19d 100644 --- a/gopls/internal/lsp/source/completion/statements.go +++ b/gopls/internal/lsp/source/completion/statements.go @@ -106,11 +106,7 @@ func (c *completer) addAssignAppend() { // Offer the long form assign + append candidate if our best // candidate is a slice. bestItem := c.topCandidate() - if bestItem == nil || bestItem.obj == nil || bestItem.obj.Type() == nil { - return - } - - if _, isSlice := bestItem.obj.Type().Underlying().(*types.Slice); !isSlice { + if bestItem == nil || !bestItem.isSlice { return } diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index ebb9128c92a..41bcbac4bbe 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -156,14 +156,6 @@ type Snapshot interface { // excluding id itself. ReverseDependencies(ctx context.Context, id PackageID, transitive bool) (map[PackageID]*Metadata, error) - // CachedPackages returns a new, unordered array of all - // packages currently cached in this snapshot, which is a - // poorly defined set that depends on the history of - // operations up to this point. Do not use it. - // - // TODO(adonovan): get rid of the last call from completions. - CachedPackages(ctx context.Context) []Package - // ActiveMetadata returns a new, unordered slice containing // metadata for all packages considered 'active' in the workspace. // diff --git a/gopls/internal/lsp/testdata/unimported/export_test.go b/gopls/internal/lsp/testdata/unimported/export_test.go index 964d27d3b94..707768e1da2 100644 --- a/gopls/internal/lsp/testdata/unimported/export_test.go +++ b/gopls/internal/lsp/testdata/unimported/export_test.go @@ -1,3 +1,3 @@ package unimported -var TestExport int //@item(testexport, "TestExport", "(from \"golang.org/lsptests/unimported\")", "var") +var TestExport int //@item(testexport, "TestExport", "var (from \"golang.org/lsptests/unimported\")", "var") diff --git a/gopls/internal/lsp/testdata/unimported/unimported.go.in b/gopls/internal/lsp/testdata/unimported/unimported.go.in index 4d1438d1bd8..74d51ffe82a 100644 --- a/gopls/internal/lsp/testdata/unimported/unimported.go.in +++ b/gopls/internal/lsp/testdata/unimported/unimported.go.in @@ -6,7 +6,7 @@ func _() { ring.Ring //@unimported("Ring", ringring) signature.Foo //@unimported("Foo", signaturefoo) - context.Bac //@unimported(" //", contextBackground, contextBackgroundErr) + context.Bac //@unimported(" //", contextBackground) } // Create markers for unimported std lib packages. Only for use by this test. @@ -14,7 +14,10 @@ func _() { /* ring.Ring */ //@item(ringring, "Ring", "(from \"container/ring\")", "var") -/* signature.Foo */ //@item(signaturefoo, "Foo", "func(a string, b int) (c bool) (from \"golang.org/lsptests/signature\")", "func") +/* signature.Foo */ //@item(signaturefoo, "Foo", "func (from \"golang.org/lsptests/signature\")", "func") -/* context.Background */ //@item(contextBackground, "Background", "func() context.Context (from \"context\")", "func") -/* context.Background().Err */ //@item(contextBackgroundErr, "Background().Err", "func() error (from \"context\")", "method") +/* context.Background */ //@item(contextBackground, "Background", "func (from \"context\")", "func") + +// Now that we no longer type-check imported completions, +// we don't expect the context.Background().Err method (see golang/go#58663). +/* context.Background().Err */ //@item(contextBackgroundErr, "Background().Err", "func (from \"context\")", "method") diff --git a/gopls/internal/lsp/tests/tests.go b/gopls/internal/lsp/tests/tests.go index 00521f54c2e..5bf8a92d006 100644 --- a/gopls/internal/lsp/tests/tests.go +++ b/gopls/internal/lsp/tests/tests.go @@ -611,7 +611,6 @@ func Run(t *testing.T, tests Tests, data *Data) { t.Run("UnimportedCompletion", func(t *testing.T) { t.Helper() - t.Skip("golang/go#58663: currently broken with incremental gopls") eachCompletion(t, data.UnimportedCompletions, tests.UnimportedCompletion) }) diff --git a/gopls/internal/regtest/completion/completion_test.go b/gopls/internal/regtest/completion/completion_test.go index a52fcc8fcea..81addba1116 100644 --- a/gopls/internal/regtest/completion/completion_test.go +++ b/gopls/internal/regtest/completion/completion_test.go @@ -297,7 +297,7 @@ func _() { if len(completions.Items) == 0 { t.Fatalf("no completion items") } - env.AcceptCompletion(loc, completions.Items[0]) + env.AcceptCompletion(loc, completions.Items[0]) // adds blah import to main.go env.Await(env.DoneWithChange()) // Trigger completions once again for the blah.<> selector. @@ -497,8 +497,6 @@ func doit() { } func TestUnimportedCompletion_VSCodeIssue1489(t *testing.T) { - t.Skip("golang/go#58663: currently broken with incremental gopls") - const src = ` -- go.mod -- module mod.com @@ -518,7 +516,7 @@ func main() { WithOptions( WindowsLineEndings(), ).Run(t, src, func(t *testing.T, env *Env) { - // Trigger unimported completions for the example.com/blah package. + // Trigger unimported completions for the mod.com package. env.OpenFile("main.go") env.Await(env.DoneWithOpen()) loc := env.RegexpSearch("main.go", "Sqr()") @@ -536,6 +534,47 @@ func main() { }) } +func TestPackageMemberCompletionAfterSyntaxError(t *testing.T) { + // This test documents the current broken behavior due to golang/go#58833. + const src = ` +-- go.mod -- +module mod.com + +go 1.14 + +-- main.go -- +package main + +import "math" + +func main() { + math.Sqrt(,0) + math.Ldex +} +` + Run(t, src, func(t *testing.T, env *Env) { + env.OpenFile("main.go") + env.Await(env.DoneWithOpen()) + loc := env.RegexpSearch("main.go", "Ldex()") + completions := env.Completion(loc) + if len(completions.Items) == 0 { + t.Fatalf("no completion items") + } + env.AcceptCompletion(loc, completions.Items[0]) + env.Await(env.DoneWithChange()) + got := env.BufferText("main.go") + // The completion of math.Ldex after the syntax error on the + // previous line is not "math.Ldexp" but "math.Ldexmath.Abs". + // (In VSCode, "Abs" wrongly appears in the completion menu.) + // This is a consequence of poor error recovery in the parser + // causing "math.Ldex" to become a BadExpr. + want := "package main\n\nimport \"math\"\n\nfunc main() {\n\tmath.Sqrt(,0)\n\tmath.Ldexmath.Abs(${1:})\n}\n" + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("unimported completion (-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 := `