[tools] gopls/internal/lsp/cache: add PkgPath->PackageID index to Metadata
172 views
Skip to first unread message
Alan Donovan (Gerrit)
Oct 20, 2022, 5:55:01 PM10/20/22
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Robert Findley, Gopher Robot, kokoro, golang-co...@googlegroups.com
Alan Donovan submitted this change.
Approvals:
Robert Findley: Looks good to me, approved
kokoro: gopls CI succeeded
Alan Donovan: Run TryBots
Gopher Robot: TryBots succeeded
gopls/internal/lsp/cache: add PkgPath->PackageID index to Metadata
DepsBy{Pkg,Imp}Path are two different ways to look up the PackageID
of a direct dependency. (We need both.)
MissingDeps is now represented as a blank value in DepsByImpPath.
Also try to make sense of MissingDependencies.
(The deleted pkg.types==nil condition was provably false.)
Change-Id: I6a04c69d3d97b3d78b77d4934592735bb941d05f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/444538
Run-TryBot: Alan Donovan <adon...@google.com>
gopls-CI: kokoro <noreply...@google.com>
Reviewed-by: Robert Findley <rfin...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
---
M gopls/internal/lsp/cache/analysis.go
M gopls/internal/lsp/cache/check.go
M gopls/internal/lsp/cache/graph.go
M gopls/internal/lsp/cache/load.go
M gopls/internal/lsp/cache/metadata.go
M gopls/internal/lsp/cache/pkg.go
M gopls/internal/lsp/cache/snapshot.go
7 files changed, 85 insertions(+), 80 deletions(-)
diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go
index fa2f7ea..e15b43e 100644
--- a/gopls/internal/lsp/cache/analysis.go
+++ b/gopls/internal/lsp/cache/analysis.go
@@ -150,8 +150,8 @@
// An analysis that consumes/produces facts
// must run on the package's dependencies too.
if len(a.FactTypes) > 0 {
- for _, importID := range ph.m.Imports {
- depActionHandle, err := s.actionHandle(ctx, importID, a)
+ for _, depID := range ph.m.DepsByPkgPath {
+ depActionHandle, err := s.actionHandle(ctx, depID, a)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index 70eaed0..65f786a 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -100,11 +100,7 @@
// the recursive key building of dependencies in parallel.
deps := make(map[PackageID]*packageHandle)
var depKey source.Hash // XOR of all unique deps
- for _, depID := range m.Imports {
- depHandle, ok := deps[depID]
- if ok {
- continue // e.g. duplicate import
- }
+ for _, depID := range m.DepsByPkgPath {
depHandle, err := s.buildPackageHandle(ctx, depID, s.workspaceParseMode(depID))
// Don't use invalid metadata for dependencies if the top-level
// metadata is valid. We only load top-level packages, so if the
@@ -452,10 +448,10 @@
defer done()
pkg := &pkg{
- m: m,
- mode: mode,
- depsByPkgPath: make(map[PackagePath]*pkg),
- types: types.NewPackage(string(m.PkgPath), string(m.Name)),
+ m: m,
+ mode: mode,
+ deps: make(map[PackageID]*pkg),
+ types: types.NewPackage(string(m.PkgPath), string(m.Name)),
typesInfo: &types.Info{
Types: make(map[ast.Expr]types.TypeAndValue),
Defs: make(map[*ast.Ident]types.Object),
@@ -528,14 +524,14 @@
// based on the metadata before we start type checking,
// reporting them via types.Importer places the errors
// at the correct source location.
- id, ok := pkg.m.Imports[ImportPath(path)]
+ id, ok := pkg.m.DepsByImpPath[ImportPath(path)]
if !ok {
// If the import declaration is broken,
// go list may fail to report metadata about it.
// See TestFixImportDecl for an example.
return nil, fmt.Errorf("missing metadata for import of %q", path)
}
- dep, ok := deps[id]
+ dep, ok := deps[id] // id may be ""
if !ok {
return nil, snapshot.missingPkgError(ctx, path)
}
@@ -546,7 +542,7 @@
if err != nil {
return nil, err
}
- pkg.depsByPkgPath[depPkg.m.PkgPath] = depPkg
+ pkg.deps[depPkg.m.ID] = depPkg
return depPkg.types, nil
}),
}
diff --git a/gopls/internal/lsp/cache/graph.go b/gopls/internal/lsp/cache/graph.go
index 047a55c..1dac076 100644
--- a/gopls/internal/lsp/cache/graph.go
+++ b/gopls/internal/lsp/cache/graph.go
@@ -53,8 +53,8 @@
// Build the import graph.
g.importedBy = make(map[PackageID][]PackageID)
for id, m := range g.metadata {
- for _, importID := range uniqueDeps(m.Imports) {
- g.importedBy[importID] = append(g.importedBy[importID], id)
+ for _, depID := range m.DepsByPkgPath {
+ g.importedBy[depID] = append(g.importedBy[depID], id)
}
}
@@ -129,25 +129,6 @@
}
}
-// uniqueDeps returns a new sorted and duplicate-free slice containing the
-// IDs of the package's direct dependencies.
-func uniqueDeps(imports map[ImportPath]PackageID) []PackageID {
- // TODO(adonovan): use generic maps.SortedUniqueValues(m.Imports) when available.
- ids := make([]PackageID, 0, len(imports))
- for _, id := range imports {
- ids = append(ids, id)
- }
- sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] })
- // de-duplicate in place
- out := ids[:0]
- for _, id := range ids {
- if len(out) == 0 || id != out[len(out)-1] {
- out = append(out, id)
- }
- }
- return out
-}
-
// reverseTransitiveClosure calculates the set of packages that transitively
// import an id in ids. The result also includes given ids.
//
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index 83ea2ca..67b235a 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -542,10 +542,10 @@
m.GoFiles = append(m.GoFiles, uri)
}
- imports := make(map[ImportPath]PackageID)
+ depsByImpPath := make(map[ImportPath]PackageID)
+ depsByPkgPath := make(map[PackagePath]PackageID)
for importPath, imported := range pkg.Imports {
importPath := ImportPath(importPath)
- imports[importPath] = PackageID(imported.ID)
// It is not an invariant that importPath == imported.PkgPath.
// For example, package "net" imports "golang.org/x/net/dns/dnsmessage"
@@ -590,18 +590,18 @@
// TODO(adonovan): clarify this. Perhaps go/packages should
// report which nodes were synthesized.
if importPath != "unsafe" && len(imported.CompiledGoFiles) == 0 {
- if m.MissingDeps == nil {
- m.MissingDeps = make(map[ImportPath]struct{})
- }
- m.MissingDeps[importPath] = struct{}{}
+ depsByImpPath[importPath] = "" // missing
continue
}
+ depsByImpPath[importPath] = PackageID(imported.ID)
+ depsByPkgPath[PackagePath(imported.PkgPath)] = PackageID(imported.ID)
if err := buildMetadata(ctx, imported, cfg, query, updates, append(path, id)); err != nil {
event.Error(ctx, "error in dependency", err)
}
}
- m.Imports = imports
+ m.DepsByImpPath = depsByImpPath
+ m.DepsByPkgPath = depsByPkgPath
return nil
}
diff --git a/gopls/internal/lsp/cache/metadata.go b/gopls/internal/lsp/cache/metadata.go
index 5ac741a..c8b0a53 100644
--- a/gopls/internal/lsp/cache/metadata.go
+++ b/gopls/internal/lsp/cache/metadata.go
@@ -33,8 +33,8 @@
ForTest PackagePath // package path under test, or ""
TypesSizes types.Sizes
Errors []packages.Error
- Imports map[ImportPath]PackageID // may contain duplicate IDs
- MissingDeps map[ImportPath]struct{}
+ DepsByImpPath map[ImportPath]PackageID // may contain dups; empty ID => missing
+ DepsByPkgPath map[PackagePath]PackageID // values are unique and non-empty
Module *packages.Module
depsErrors []*packagesinternal.PackageError
diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go
index 2f7389d..0b767b4 100644
--- a/gopls/internal/lsp/cache/pkg.go
+++ b/gopls/internal/lsp/cache/pkg.go
@@ -9,6 +9,7 @@
"go/ast"
"go/scanner"
"go/types"
+ "sort"
"golang.org/x/mod/module"
"golang.org/x/tools/gopls/internal/lsp/source"
@@ -23,7 +24,7 @@
goFiles []*source.ParsedGoFile
compiledGoFiles []*source.ParsedGoFile
diagnostics []*source.Diagnostic
- depsByPkgPath map[PackagePath]*pkg
+ deps map[PackageID]*pkg // use m.DepsBy{Pkg,Imp}Path to look up ID
version *module.Version
parseErrors []scanner.ErrorList
typeErrors []types.Error
@@ -116,38 +117,28 @@
// from an import declaration, use ResolveImportPath instead.
// They may differ in case of vendoring.)
func (p *pkg) DirectDep(pkgPath string) (source.Package, error) {
- if imp := p.depsByPkgPath[PackagePath(pkgPath)]; imp != nil {
- return imp, nil
+ if id, ok := p.m.DepsByPkgPath[PackagePath(pkgPath)]; ok {
+ if imp := p.deps[id]; imp != nil {
+ return imp, nil
+ }
}
- // Don't return a nil pointer because that still satisfies the interface.
- return nil, fmt.Errorf("no imported package for %s", pkgPath)
+ return nil, fmt.Errorf("package does not import package with path %s", pkgPath)
}
// ResolveImportPath returns the directly imported dependency of this package,
// given its ImportPath. See also DirectDep.
func (p *pkg) ResolveImportPath(importPath string) (source.Package, error) {
- if id, ok := p.m.Imports[ImportPath(importPath)]; ok {
- for _, imported := range p.depsByPkgPath {
- if PackageID(imported.ID()) == id {
- return imported, nil
- }
+ if id, ok := p.m.DepsByImpPath[ImportPath(importPath)]; ok && id != "" {
+ if imp := p.deps[id]; imp != nil {
+ return imp, nil
}
}
return nil, fmt.Errorf("package does not import %s", importPath)
}
func (p *pkg) MissingDependencies() []string {
- // We don't invalidate metadata for import deletions, so check the package
- // imports via the *types.Package. Only use metadata if p.types is nil.
- if p.types == nil {
- var md []string
- for importPath := range p.m.MissingDeps {
- md = append(md, string(importPath))
- }
- return md
- }
-
- // This looks wrong.
+ // We don't invalidate metadata for import deletions,
+ // so check the package imports via the *types.Package.
//
// rfindley says: it looks like this is intending to implement
// a heuristic "if go list couldn't resolve import paths to
@@ -158,20 +149,31 @@
// doesn't need that dep anymore we shouldn't show the warning".
// But either we're outside of GOPATH/Module, or we're not...
//
- // TODO(adonovan): figure out what it is trying to do.
- var md []string
+ // adonovan says: I think this effectively reverses the
+ // heuristic used by the type checker when Importer.Import
+ // returns an error---go/types synthesizes a package whose
+ // Path is the import path (sans "vendor/")---hence the
+ // dubious ImportPath() conversion. A blank DepsByImpPath
+ // entry means a missing import.
+ //
+ // If we invalidate the metadata for import deletions (which
+ // should be fast) then we can simply return the blank entries
+ // in DepsByImpPath. (They are PackageIDs not PackagePaths,
+ // but the caller only cares whether the set is empty!)
+ var missing []string
for _, pkg := range p.types.Imports() {
- if _, ok := p.m.MissingDeps[ImportPath(pkg.Path())]; ok {
- md = append(md, pkg.Path())
+ if id, ok := p.m.DepsByImpPath[ImportPath(pkg.Path())]; ok && id == "" {
+ missing = append(missing, pkg.Path())
}
}
- return md
+ sort.Strings(missing)
+ return missing
}
func (p *pkg) Imports() []source.Package {
- var result []source.Package
- for _, imp := range p.depsByPkgPath {
- result = append(result, imp)
+ var result []source.Package // unordered
+ for _, dep := range p.deps {
+ result = append(result, dep)
}
return result
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index c7e5e8a..30eb952 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -892,7 +892,7 @@
}
// TODO(rfindley): it looks incorrect that we don't also check GoFiles here.
// If a CGo file is open, we want to consider the package active.
- for _, dep := range m.Imports {
+ for _, dep := range m.DepsByPkgPath {
if s.isActiveLocked(dep) {
return true
}
@@ -1231,14 +1231,15 @@
if err != nil {
return
}
- for pkgPath, newPkg := range cachedPkg.depsByPkgPath {
- if oldPkg, ok := results[string(pkgPath)]; ok {
+ for _, newPkg := range cachedPkg.deps {
+ pkgPath := newPkg.PkgPath()
+ if oldPkg, ok := results[pkgPath]; ok {
// Using the same trick as NarrowestPackage, prefer non-variants.
if len(newPkg.compiledGoFiles) < len(oldPkg.(*pkg).compiledGoFiles) {
- results[string(pkgPath)] = newPkg
+ results[pkgPath] = newPkg
}
} else {
- results[string(pkgPath)] = newPkg
+ results[pkgPath] = newPkg
}
}
})
@@ -1893,8 +1894,11 @@
// package with missing dependencies.
if anyFileAdded {
for id, metadata := range s.meta.metadata {
- if len(metadata.MissingDeps) > 0 {
- directIDs[id] = true
+ for _, impID := range metadata.DepsByImpPath {
+ if impID == "" { // missing import
+ directIDs[id] = true
+ break
+ }
}
}
}
To view, visit change 444538. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages