gopls/internal/golang/completion: honor std symbol versions (unimported) This change causes the imports package to return not just lists of symbol names, but stdlib.Symbol structures, and causes gopls to use them to filter candidates based on the Go version of the requesting file. This filtering is not applied to imported completions, or to methods of unimported types (which apparently we don't complete at all, surprisingly). These are left for follow-up changes. Also, support negative assertions in completion @rank markers. Change-Id: I2ae62c2b83a366a37bdd8db88e28cca4c5f92ae5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/569435 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/golang/completion/completion.go b/gopls/internal/golang/completion/completion.go index 8ff7f0b..6f3b7c1 100644 --- a/gopls/internal/golang/completion/completion.go +++ b/gopls/internal/golang/completion/completion.go
@@ -17,6 +17,7 @@ "go/token" "go/types" "math" + "reflect" "sort" "strconv" "strings" @@ -41,8 +42,10 @@ "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/fuzzy" "golang.org/x/tools/internal/imports" + "golang.org/x/tools/internal/stdlib" "golang.org/x/tools/internal/typeparams" "golang.org/x/tools/internal/typesinternal" + "golang.org/x/tools/internal/versions" ) // A CompletionItem represents a possible completion suggested by the algorithm. @@ -912,7 +915,7 @@ obj := types.NewPkgName(0, nil, name, types.NewPackage(pkgToConsider, name)) c.deepState.enqueue(candidate{ obj: obj, - detail: fmt.Sprintf("%q", pkgToConsider), + detail: strconv.Quote(pkgToConsider), score: score, }) } @@ -1185,6 +1188,8 @@ return nil // feature disabled } + // -- completion of symbols in unimported packages -- + // 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 @@ -1262,7 +1267,7 @@ // Consider adding a concurrency-safe API for completer. var cMu sync.Mutex // guards c.items and c.matcher var enough int32 // atomic bool - quickParse := func(uri protocol.DocumentURI, mp *metadata.Package) error { + quickParse := func(uri protocol.DocumentURI, mp *metadata.Package, tooNew map[string]bool) error { if atomic.LoadInt32(&enough) != 0 { return nil } @@ -1285,6 +1290,10 @@ return } + if tooNew[id.Name] { + return // symbol too new for requesting file's Go's version + } + cMu.Lock() score := c.matcher.Score(id.Name) cMu.Unlock() @@ -1372,14 +1381,34 @@ return nil } + var goversion string + // TODO(adonovan): after go1.21, replace with: + // goversion = c.pkg.GetTypesInfo().FileVersions[c.file] + if v := reflect.ValueOf(c.pkg.GetTypesInfo()).Elem().FieldByName("FileVersions"); v.IsValid() { + goversion = v.Interface().(map[*ast.File]string)[c.file] // may be "" + } + // Extract the package-level candidates using a quick parse. var g errgroup.Group for _, path := range paths { mp := known[golang.PackagePath(path)] + + // For standard packages, build a filter of symbols that + // are too new for the requesting file's Go version. + var tooNew map[string]bool + if syms, ok := stdlib.PackageSymbols[path]; ok && goversion != "" { + tooNew = make(map[string]bool) + for _, sym := range syms { + if versions.Before(goversion, sym.Version.String()) { + tooNew[sym.Name] = true + } + } + } + for _, uri := range mp.CompiledGoFiles { uri := uri g.Go(func() error { - return quickParse(uri, mp) + return quickParse(uri, mp, tooNew) }) } } @@ -1404,10 +1433,13 @@ // Continue with untyped proposals. pkg := types.NewPackage(pkgExport.Fix.StmtInfo.ImportPath, pkgExport.Fix.IdentName) - for _, export := range pkgExport.Exports { + for _, symbol := range pkgExport.Exports { + if goversion != "" && versions.Before(goversion, symbol.Version.String()) { + continue // symbol too new for this file + } score := unimportedScore(pkgExport.Fix.Relevance) c.deepState.enqueue(candidate{ - obj: types.NewVar(0, pkg, export, nil), + obj: types.NewVar(0, pkg, symbol.Name, nil), score: score, imp: &importInfo{ importPath: pkgExport.Fix.StmtInfo.ImportPath,
diff --git a/gopls/internal/test/marker/doc.go b/gopls/internal/test/marker/doc.go index e294d4c..d384886 100644 --- a/gopls/internal/test/marker/doc.go +++ b/gopls/internal/test/marker/doc.go
@@ -221,6 +221,8 @@ unexpected completion items may occur in the results. TODO(rfindley): this exists for compatibility with the old marker tests. Replace this with rankl, and rename. + A "!" prefix on a label asserts that the symbol is not a + completion candidate. - rankl(location, ...label): like rank, but only cares about completion item labels.
diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index c32b81a..bdff8f2 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go
@@ -40,6 +40,7 @@ "golang.org/x/tools/gopls/internal/test/integration/fake" "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/gopls/internal/util/safetoken" + "golang.org/x/tools/gopls/internal/util/slices" "golang.org/x/tools/internal/diff" "golang.org/x/tools/internal/diff/myers" "golang.org/x/tools/internal/jsonrpc2" @@ -295,6 +296,7 @@ // // It formats the error message using mark.sprintf. func (mark marker) errorf(format string, args ...any) { + mark.T().Helper() msg := mark.sprintf(format, args...) // TODO(adonovan): consider using fmt.Fprintf(os.Stderr)+t.Fail instead of // t.Errorf to avoid reporting uninteresting positions in the Go source of @@ -1237,19 +1239,35 @@ } func rankMarker(mark marker, src protocol.Location, items ...completionItem) { + // Separate positive and negative items (expectations). + var pos, neg []completionItem + for _, item := range items { + if strings.HasPrefix(item.Label, "!") { + neg = append(neg, item) + } else { + pos = append(pos, item) + } + } + + // Collect results that are present in items, preserving their order. list := mark.run.env.Completion(src) var got []string - // Collect results that are present in items, preserving their order. for _, g := range list.Items { - for _, w := range items { + for _, w := range pos { if g.Label == w.Label { got = append(got, g.Label) break } } + for _, w := range neg { + if g.Label == w.Label[len("!"):] { + mark.errorf("got unwanted completion: %s", g.Label) + break + } + } } var want []string - for _, w := range items { + for _, w := range pos { want = append(want, w.Label) } if diff := cmp.Diff(want, got); diff != "" { @@ -1258,18 +1276,27 @@ } func ranklMarker(mark marker, src protocol.Location, labels ...string) { - list := mark.run.env.Completion(src) - var got []string - // Collect results that are present in items, preserving their order. - for _, g := range list.Items { - for _, label := range labels { - if g.Label == label { - got = append(got, g.Label) - break - } + // Separate positive and negative labels (expectations). + var pos, neg []string + for _, label := range labels { + if strings.HasPrefix(label, "!") { + neg = append(neg, label[len("!"):]) + } else { + pos = append(pos, label) } } - if diff := cmp.Diff(labels, got); diff != "" { + + // Collect results that are present in items, preserving their order. + list := mark.run.env.Completion(src) + var got []string + for _, g := range list.Items { + if slices.Contains(pos, g.Label) { + got = append(got, g.Label) + } else if slices.Contains(neg, g.Label) { + mark.errorf("got unwanted completion: %s", g.Label) + } + } + if diff := cmp.Diff(pos, got); diff != "" { mark.errorf("completion rankings do not match (-want +got):\n%s", diff) } }
diff --git a/gopls/internal/test/marker/testdata/completion/unimported-std.txt b/gopls/internal/test/marker/testdata/completion/unimported-std.txt new file mode 100644 index 0000000..b99e85a --- /dev/null +++ b/gopls/internal/test/marker/testdata/completion/unimported-std.txt
@@ -0,0 +1,47 @@ +Test of unimported completions respecting the effective Go version of the file. + +These symbols below were introduced in go1.20: + + types.Satisfied + ast.File.FileStart + (*token.FileSet).RemoveFile + +The underlying logic depends on versions.FileVersion, which only +behaves correctly in go1.22. (When go1.22 is assured, we can remove +the min_go flag but leave the test inputs unchanged.) + +-- flags -- +-ignore_extra_diags -min_go=go1.22 + +-- go.mod -- +module example.com + +go 1.19 + +-- a/a.go -- +package a + +// package-level func +var _ = types.Imple //@rankl("Imple", "Implements") +var _ = types.Satis //@rankl("Satis", "!Satisfies") + +// (Apparently we don't even offer completions of methods +// of types from unimported packages, so the fact that +// we don't implement std version filtering isn't evident.) + +// field +var _ = new(ast.File).Packa //@rankl("Packa", "!Package") +var _ = new(ast.File).FileS //@rankl("FileS", "!FileStart") + +// method +var _ = new(token.FileSet).Ad //@rankl("Ad", "!Add") +var _ = new(token.FileSet).Remove //@rankl("Remove", "!RemoveFile") + +-- b/b.go -- +//go:build go1.20 + +package a + +// package-level func +var _ = types.Imple //@rankl("Imple", "Implements") +var _ = types.Satis //@rankl("Satis", "Satisfies")
diff --git a/internal/imports/fix.go b/internal/imports/fix.go index c3bba83..69e2ad5 100644 --- a/internal/imports/fix.go +++ b/internal/imports/fix.go
@@ -651,11 +651,11 @@ } dupCheck[importPath] = struct{}{} if notSelf(p) && wrappedCallback.dirFound(p) && wrappedCallback.packageNameLoaded(p) { - exports := make([]string, 0, len(symbols)) + var exports []stdlib.Symbol for _, sym := range symbols { switch sym.Kind { case stdlib.Func, stdlib.Type, stdlib.Var, stdlib.Const: - exports = append(exports, sym.Name) + exports = append(exports, sym) } } wrappedCallback.exportsLoaded(p, exports) @@ -678,7 +678,7 @@ dupCheck[pkg.importPathShort] = struct{}{} return notSelf(pkg) && wrappedCallback.packageNameLoaded(pkg) }, - exportsLoaded: func(pkg *pkg, exports []string) { + exportsLoaded: func(pkg *pkg, exports []stdlib.Symbol) { // If we're an x_test, load the package under test's test variant. if strings.HasSuffix(filePkg, "_test") && pkg.dir == filepath.Dir(filename) { var err error @@ -803,7 +803,7 @@ // A PackageExport is a package and its exports. type PackageExport struct { Fix *ImportFix - Exports []string + Exports []stdlib.Symbol } // GetPackageExports returns all known packages with name pkg and their exports. @@ -818,8 +818,8 @@ packageNameLoaded: func(pkg *pkg) bool { return pkg.packageName == searchPkg }, - exportsLoaded: func(pkg *pkg, exports []string) { - sort.Strings(exports) + exportsLoaded: func(pkg *pkg, exports []stdlib.Symbol) { + sortSymbols(exports) wrapped(PackageExport{ Fix: &ImportFix{ StmtInfo: ImportInfo{ @@ -1093,7 +1093,7 @@ // loadExports returns the set of exported symbols in the package at dir. // loadExports may be called concurrently. - loadExports(ctx context.Context, pkg *pkg, includeTest bool) (string, []string, error) + loadExports(ctx context.Context, pkg *pkg, includeTest bool) (string, []stdlib.Symbol, error) // scoreImportPath returns the relevance for an import path. scoreImportPath(ctx context.Context, path string) float64 @@ -1122,7 +1122,7 @@ // If it returns true, the package's exports will be loaded. packageNameLoaded func(pkg *pkg) bool // exportsLoaded is called when a package's exports have been loaded. - exportsLoaded func(pkg *pkg, exports []string) + exportsLoaded func(pkg *pkg, exports []stdlib.Symbol) } func addExternalCandidates(ctx context.Context, pass *pass, refs references, filename string) error { @@ -1518,7 +1518,7 @@ return result } -func (r *gopathResolver) loadExports(ctx context.Context, pkg *pkg, includeTest bool) (string, []string, error) { +func (r *gopathResolver) loadExports(ctx context.Context, pkg *pkg, includeTest bool) (string, []stdlib.Symbol, error) { if info, ok := r.cache.Load(pkg.dir); ok && !includeTest { return r.cache.CacheExports(ctx, r.env, info) } @@ -1538,7 +1538,7 @@ return ipath } -func loadExportsFromFiles(ctx context.Context, env *ProcessEnv, dir string, includeTest bool) (string, []string, error) { +func loadExportsFromFiles(ctx context.Context, env *ProcessEnv, dir string, includeTest bool) (string, []stdlib.Symbol, error) { // Look for non-test, buildable .go files which could provide exports. all, err := os.ReadDir(dir) if err != nil { @@ -1562,7 +1562,7 @@ } var pkgName string - var exports []string + var exports []stdlib.Symbol fset := token.NewFileSet() for _, fi := range files { select { @@ -1589,21 +1589,41 @@ continue } pkgName = f.Name.Name - for name := range f.Scope.Objects { + for name, obj := range f.Scope.Objects { if ast.IsExported(name) { - exports = append(exports, name) + var kind stdlib.Kind + switch obj.Kind { + case ast.Con: + kind = stdlib.Const + case ast.Typ: + kind = stdlib.Type + case ast.Var: + kind = stdlib.Var + case ast.Fun: + kind = stdlib.Func + } + exports = append(exports, stdlib.Symbol{ + Name: name, + Kind: kind, + Version: 0, // unknown; be permissive + }) } } } + sortSymbols(exports) if env.Logf != nil { - sortedExports := append([]string(nil), exports...) - sort.Strings(sortedExports) - env.Logf("loaded exports in dir %v (package %v): %v", dir, pkgName, strings.Join(sortedExports, ", ")) + env.Logf("loaded exports in dir %v (package %v): %v", dir, pkgName, exports) } return pkgName, exports, nil } +func sortSymbols(syms []stdlib.Symbol) { + sort.Slice(syms, func(i, j int) bool { + return syms[i].Name < syms[j].Name + }) +} + // findImport searches for a package with the given symbols. // If no package is found, findImport returns ("", false, nil) func findImport(ctx context.Context, pass *pass, candidates []pkgDistance, pkgName string, symbols map[string]bool) (*pkg, error) { @@ -1670,7 +1690,7 @@ exportsMap := make(map[string]bool, len(exports)) for _, sym := range exports { - exportsMap[sym] = true + exportsMap[sym.Name] = true } // If it doesn't have the right
diff --git a/internal/imports/fix_test.go b/internal/imports/fix_test.go index 5557b2d..08d90a5 100644 --- a/internal/imports/fix_test.go +++ b/internal/imports/fix_test.go
@@ -2855,8 +2855,8 @@ defer mu.Unlock() for _, csym := range c.Exports { for _, w := range want { - if c.Fix.StmtInfo.ImportPath == w.path && csym == w.symbol { - got = append(got, res{c.Fix.Relevance, c.Fix.IdentName, c.Fix.StmtInfo.ImportPath, csym}) + if c.Fix.StmtInfo.ImportPath == w.path && csym.Name == w.symbol { + got = append(got, res{c.Fix.Relevance, c.Fix.IdentName, c.Fix.StmtInfo.ImportPath, csym.Name}) } } }
diff --git a/internal/imports/mod.go b/internal/imports/mod.go index c2a4e90..34dc71d 100644 --- a/internal/imports/mod.go +++ b/internal/imports/mod.go
@@ -413,7 +413,7 @@ return r.otherCache.CachePackageName(info) } -func (r *ModuleResolver) cacheExports(ctx context.Context, env *ProcessEnv, info directoryPackageInfo) (string, []string, error) { +func (r *ModuleResolver) cacheExports(ctx context.Context, env *ProcessEnv, info directoryPackageInfo) (string, []stdlib.Symbol, error) { if info.rootType == gopathwalk.RootModuleCache { return r.moduleCacheCache.CacheExports(ctx, env, info) } @@ -711,7 +711,7 @@ return res, nil } -func (r *ModuleResolver) loadExports(ctx context.Context, pkg *pkg, includeTest bool) (string, []string, error) { +func (r *ModuleResolver) loadExports(ctx context.Context, pkg *pkg, includeTest bool) (string, []stdlib.Symbol, error) { if info, ok := r.cacheLoad(pkg.dir); ok && !includeTest { return r.cacheExports(ctx, r.env, info) }
diff --git a/internal/imports/mod_cache.go b/internal/imports/mod_cache.go index cfc5465..b119269 100644 --- a/internal/imports/mod_cache.go +++ b/internal/imports/mod_cache.go
@@ -14,6 +14,7 @@ "golang.org/x/mod/module" "golang.org/x/tools/internal/gopathwalk" + "golang.org/x/tools/internal/stdlib" ) // To find packages to import, the resolver needs to know about all of @@ -73,7 +74,7 @@ // the default build context GOOS and GOARCH. // // We can make this explicit, and key exports by GOOS, GOARCH. - exports []string + exports []stdlib.Symbol } // reachedStatus returns true when info has a status at least target and any error associated with @@ -229,7 +230,7 @@ return info.packageName, info.err } -func (d *DirInfoCache) CacheExports(ctx context.Context, env *ProcessEnv, info directoryPackageInfo) (string, []string, error) { +func (d *DirInfoCache) CacheExports(ctx context.Context, env *ProcessEnv, info directoryPackageInfo) (string, []stdlib.Symbol, error) { if reached, _ := info.reachedStatus(exportsLoaded); reached { return info.packageName, info.exports, info.err }