From 2592a854ecc2cbb00419ca9691925d963afe25fa Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 17 Nov 2022 12:02:44 -0500 Subject: [PATCH] gopls/internal/lsp: simplify KnownPackages This change simplifies the two functions called KnownPackages, and renames one of them. The first, source.KnownPackages, is a standalone function called in exactly one place: ListKnownPackages. It has been renamed KnownPackagePaths to distinguish from the other and to make clear that it returns only metadata. Its implementation could be greatly simplified in a follow-up, as noted. In the meantime, one obviously wrong use of ImportSpec.Path.Value (a quoted string literal!) as an package (not import!) path was fixed, and the package name was obtained directly, not via CompiledGoFiles. The second, Snapshot.KnownPackagePaths, is called from two places. This CL eliminates one of them: stubMethods used it apparently only to populate a field of missingInterface (pkg) that was unused. The other call (from 'implementations') is something that should eventually go away as we incrementalize; in any case it doesn't rely on the undocumented ordering invariant established by the implementation. This change removes the ordering invariant, documents the lack of order, and removes the ordering logic. Change-Id: Ia515a62c2d78276b3344f2880c22746b2c06ff64 Reviewed-on: https://go-review.googlesource.com/c/tools/+/451675 TryBot-Result: Gopher Robot Run-TryBot: Alan Donovan gopls-CI: kokoro Reviewed-by: Robert Findley --- gopls/internal/lsp/cache/snapshot.go | 9 +---- gopls/internal/lsp/command.go | 2 +- .../lsp/source/completion/completion.go | 3 ++ gopls/internal/lsp/source/known_packages.go | 40 +++++++++---------- gopls/internal/lsp/source/stub.go | 36 ++--------------- gopls/internal/lsp/source/view.go | 7 +++- internal/imports/fix.go | 3 ++ 7 files changed, 35 insertions(+), 65 deletions(-) diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 634afdff71..281c5d9b08 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -1150,19 +1150,14 @@ func (s *snapshot) KnownPackages(ctx context.Context) ([]source.Package, error) return nil, err } - // The WorkspaceSymbols implementation relies on this function returning - // workspace packages first. - ids := s.workspacePackageIDs() + ids := make([]source.PackageID, 0, len(s.meta.metadata)) s.mu.Lock() for id := range s.meta.metadata { - if _, ok := s.workspacePackages[id]; ok { - continue - } ids = append(ids, id) } s.mu.Unlock() - var pkgs []source.Package + pkgs := make([]source.Package, 0, len(ids)) for _, id := range ids { pkg, err := s.checkedPackage(ctx, id, s.workspaceParseMode(id)) if err != nil { diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go index a3d93e265c..28cf3471a7 100644 --- a/gopls/internal/lsp/command.go +++ b/gopls/internal/lsp/command.go @@ -735,7 +735,7 @@ func (c *commandHandler) ListKnownPackages(ctx context.Context, args command.URI progress: "Listing packages", forURI: args.URI, }, func(ctx context.Context, deps commandDeps) error { - pkgs, err := source.KnownPackages(ctx, deps.snapshot, deps.fh) + pkgs, err := source.KnownPackagePaths(ctx, deps.snapshot, deps.fh) for _, pkg := range pkgs { result.Packages = append(result.Packages, string(pkg)) } diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go index 1260e0e7c3..144d6587da 100644 --- a/gopls/internal/lsp/source/completion/completion.go +++ b/gopls/internal/lsp/source/completion/completion.go @@ -1475,6 +1475,9 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru count := 0 + // TODO(adonovan): strength-reduce to a metadata query. + // All that's needed below is Package.{Name,Path}. + // Presumably that can be answered more thoroughly more quickly. known, err := c.snapshot.CachedImportPaths(ctx) if err != nil { return err diff --git a/gopls/internal/lsp/source/known_packages.go b/gopls/internal/lsp/source/known_packages.go index efaaee57ae..6f3bf8c902 100644 --- a/gopls/internal/lsp/source/known_packages.go +++ b/gopls/internal/lsp/source/known_packages.go @@ -16,21 +16,24 @@ import ( "golang.org/x/tools/internal/imports" ) -// KnownPackages returns a list of all known packages -// in the package graph that could potentially be imported -// by the given file. -func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandle) ([]PackagePath, error) { +// KnownPackagePaths returns a new list of package paths of all known +// packages in the package graph that could potentially be imported by +// the given file. The list is ordered lexicographically, except that +// all dot-free paths (standard packages) appear before dotful ones. +func KnownPackagePaths(ctx context.Context, snapshot Snapshot, fh VersionedFileHandle) ([]PackagePath, error) { + // TODO(adonovan): this whole algorithm could be more + // simply expressed in terms of Metadata, not Packages. + // All we need below is: + // - for fh: Metadata.{DepsByPkgPath,Path,Name} + // - for all cached packages: Metadata.{Path,Name,ForTest,DepsByPkgPath}. pkg, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage) if err != nil { return nil, fmt.Errorf("GetParsedFile: %w", err) } alreadyImported := map[PackagePath]struct{}{} - for _, imp := range pgf.File.Imports { - // TODO(adonovan): the correct PackagePath might need a "vendor/" prefix. - alreadyImported[PackagePath(imp.Path.Value)] = struct{}{} + for _, imp := range pkg.Imports() { + alreadyImported[imp.PkgPath()] = struct{}{} } - // TODO(adonovan): this whole algorithm could be more - // simply expressed in terms of Metadata, not Packages. pkgs, err := snapshot.CachedImportPaths(ctx) if err != nil { return nil, err @@ -40,13 +43,8 @@ func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandl paths []PackagePath ) for path, knownPkg := range pkgs { - gofiles := knownPkg.CompiledGoFiles() - if len(gofiles) == 0 || gofiles[0].File.Name == nil { - continue - } - pkgName := gofiles[0].File.Name.Name // package main cannot be imported - if pkgName == "main" { + if knownPkg.Name() == "main" { continue } // test packages cannot be imported @@ -57,7 +55,7 @@ func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandl if _, ok := alreadyImported[path]; ok { continue } - // snapshot.KnownPackages could have multiple versions of a pkg + // snapshot.CachedImportPaths could have multiple versions of a pkg if _, ok := seen[path]; ok { continue } @@ -86,7 +84,8 @@ func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandl return } paths = append(paths, path) - }, "", pgf.URI.Filename(), pkg.GetTypes().Name(), o.Env) + seen[path] = struct{}{} + }, "", pgf.URI.Filename(), string(pkg.Name()), o.Env) }) if err != nil { // if an error occurred, we still have a decent list we can @@ -97,11 +96,8 @@ func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandl importI, importJ := paths[i], paths[j] iHasDot := strings.Contains(string(importI), ".") jHasDot := strings.Contains(string(importJ), ".") - if iHasDot && !jHasDot { - return false - } - if jHasDot && !iHasDot { - return true + if iHasDot != jHasDot { + return jHasDot // dot-free paths (standard packages) compare less } return importI < importJ }) diff --git a/gopls/internal/lsp/source/stub.go b/gopls/internal/lsp/source/stub.go index 3dcdb0ec6b..9e4b704c22 100644 --- a/gopls/internal/lsp/source/stub.go +++ b/gopls/internal/lsp/source/stub.go @@ -98,13 +98,8 @@ func stubSuggestedFixFunc(ctx context.Context, snapshot Snapshot, fh VersionedFi // stubMethods returns the Go code of all methods // that implement the given interface func stubMethods(ctx context.Context, concreteFile *ast.File, si *stubmethods.StubInfo, snapshot Snapshot) ([]byte, []*stubImport, error) { - ifacePkg, err := deducePkgFromTypes(ctx, snapshot, si.Interface) - if err != nil { - return nil, nil, err - } - si.Concrete.Obj().Type() concMS := types.NewMethodSet(types.NewPointer(si.Concrete.Obj().Type())) - missing, err := missingMethods(ctx, snapshot, concMS, si.Concrete.Obj().Pkg(), si.Interface, ifacePkg, map[string]struct{}{}) + missing, err := missingMethods(ctx, snapshot, concMS, si.Concrete.Obj().Pkg(), si.Interface, map[string]struct{}{}) if err != nil { return nil, nil, fmt.Errorf("missingMethods: %w", err) } @@ -188,19 +183,6 @@ func printStubMethod(md methodData) []byte { return b.Bytes() } -func deducePkgFromTypes(ctx context.Context, snapshot Snapshot, ifaceObj types.Object) (Package, error) { - pkgs, err := snapshot.KnownPackages(ctx) - if err != nil { - return nil, err - } - for _, p := range pkgs { - if p.PkgPath() == PackagePath(ifaceObj.Pkg().Path()) { - return p, nil - } - } - return nil, fmt.Errorf("pkg %q not found", ifaceObj.Pkg().Path()) -} - func deduceIfaceName(concretePkg, ifacePkg *types.Package, ifaceObj types.Object) string { if concretePkg.Path() == ifacePkg.Path() { return ifaceObj.Name() @@ -245,7 +227,7 @@ returns }, } */ -func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.MethodSet, concPkg *types.Package, ifaceObj types.Object, ifacePkg Package, visited map[string]struct{}) ([]*missingInterface, error) { +func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.MethodSet, concPkg *types.Package, ifaceObj types.Object, visited map[string]struct{}) ([]*missingInterface, error) { iface, ok := ifaceObj.Type().Underlying().(*types.Interface) if !ok { return nil, fmt.Errorf("expected %v to be an interface but got %T", iface, ifaceObj.Type().Underlying()) @@ -253,17 +235,7 @@ func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.Method missing := []*missingInterface{} for i := 0; i < iface.NumEmbeddeds(); i++ { eiface := iface.Embedded(i).Obj() - depPkg := ifacePkg - if path := PackagePath(eiface.Pkg().Path()); path != ifacePkg.PkgPath() { - // TODO(adonovan): I'm not sure what this is trying to do, but it - // looks wrong the in case of type aliases. - var err error - depPkg, err = ifacePkg.DirectDep(path) - if err != nil { - return nil, err - } - } - em, err := missingMethods(ctx, snapshot, concMS, concPkg, eiface, depPkg, visited) + em, err := missingMethods(ctx, snapshot, concMS, concPkg, eiface, visited) if err != nil { return nil, err } @@ -274,7 +246,6 @@ func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.Method return nil, fmt.Errorf("error getting iface file: %w", err) } mi := &missingInterface{ - pkg: ifacePkg, iface: iface, file: parsedFile.File, } @@ -322,7 +293,6 @@ func getStubFile(ctx context.Context, obj types.Object, snapshot Snapshot) (*Par type missingInterface struct { iface *types.Interface file *ast.File - pkg Package missing []*types.Func } diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index 47d642d95f..4178197e48 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -182,8 +182,11 @@ type Snapshot interface { // and checked in TypecheckWorkspace mode. CachedImportPaths(ctx context.Context) (map[PackagePath]Package, error) - // KnownPackages returns all the packages loaded in this snapshot, checked - // in TypecheckWorkspace mode. + // KnownPackages returns a new unordered list of all packages + // loaded in this snapshot, checked in TypecheckWorkspace mode. + // + // TODO(adonovan): opt: rewrite 'implementations' to avoid the + // need ever to "load everything at once" using this function. KnownPackages(ctx context.Context) ([]Package, error) // ActivePackages returns the packages considered 'active' in the workspace. diff --git a/internal/imports/fix.go b/internal/imports/fix.go index 9b7b106fde..646455802b 100644 --- a/internal/imports/fix.go +++ b/internal/imports/fix.go @@ -697,6 +697,9 @@ func candidateImportName(pkg *pkg) string { // GetAllCandidates calls wrapped for each package whose name starts with // searchPrefix, and can be imported from filename with the package name filePkg. +// +// Beware that the wrapped function may be called multiple times concurrently. +// TODO(adonovan): encapsulate the concurrency. func GetAllCandidates(ctx context.Context, wrapped func(ImportFix), searchPrefix, filename, filePkg string, env *ProcessEnv) error { callback := &scanCallback{ rootFound: func(gopathwalk.Root) bool {