gopls/internal/lsp/cache: better import path hygiene

An import path is not the same as a package path.
For example, the declaration:
   import "example.com/foo"
may load a package whose PkgPath() is "vendor/example.com/foo".
There was a comment hinting at this in load.go.

This change introduces the concept of ImportPath as a
distinct type from PackagePath, and documents clearly
throughout the relevant APIs which one is expected.

Notes:
- Metadata.PkgPath is consistently set from the PackagePath,
  not sometimes (when a root) from PackagePath and sometimes (when
  indirectly loaded) from an arbitrary ImportPath that resolves to it.
  I expect this means some packages will now (correctly)
  appear to be called "vendor/example.com/foo"
  in the user interface where previously the vendor prefix was omitted.
- Metadata.Deps is gone.
- Metadata.Imports is a new map from ImportPath to ID.
- Metadata.MissingDeps is now a set of ImportPath.
- the package loading key is now based on a hash of
  unique imports. (Duplicates would cancel out.)
- The ImporterFunc no longer needs the guesswork of
  resolveImportPath, since 'go list' already told us
  the correct answer.
- Package.GetImport is renamed DirectDep(packagePath)
  to avoid the suggestion that it accepts an ImportPath.
- Package.ResolveImportPath is the analogous method
  for import paths. Most clients seem to want this.

Change-Id: I4999709120fff4663ba8669358fe149f1626bb8e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/443636
Run-TryBot: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: adonovan

gopls/internal/lsp/cache/analysis.go

@@ -132,7 +132,7 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A
 	}
 
 	// Add a dependency on each required analyzer.
-	var deps []*actionHandle
+	var deps []*actionHandle // unordered
 	for _, req := range a.Requires {
 		// TODO(adonovan): opt: there's no need to repeat the package-handle
 		// portion of the recursion here, since we have the pkg already.
@@ -150,7 +150,7 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A
 		// An analysis that consumes/produces facts
 		// must run on the package's dependencies too.
 		if len(a.FactTypes) > 0 {
-			for _, importID := range ph.m.Deps {
+			for _, importID := range ph.m.Imports {
 				depActionHandle, err := s.actionHandle(ctx, importID, a)
 				if err != nil {
 					return nil, err

gopls/internal/lsp/cache/check.go

@@ -11,7 +11,6 @@ import (
 	"fmt"
 	"go/ast"
 	"go/types"
-	"path"
 	"path/filepath"
 	"regexp"
 	"strings"
@@ -99,9 +98,13 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
 	// TODO(adonovan): use a promise cache to ensure that the key
 	// for each package is computed by at most one thread, then do
 	// the recursive key building of dependencies in parallel.
-	deps := make(map[PackagePath]*packageHandle)
-	depKeys := make([]packageHandleKey, len(m.Deps))
-	for i, depID := range m.Deps {
+	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
+		}
 		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
@@ -125,8 +128,9 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
 			continue
 		}
 
-		deps[depHandle.m.PkgPath] = depHandle
-		depKeys[i] = depHandle.key
+		depKey.XORWith(source.Hash(depHandle.key))
+
+		deps[depID] = depHandle
 	}
 
 	// Read both lists of files of this package, in parallel.
@@ -144,7 +148,7 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
 	// All the file reading has now been done.
 	// Create a handle for the result of type checking.
 	experimentalKey := s.View().Options().ExperimentalPackageCacheKey
-	phKey := computePackageKey(m.ID, compiledGoFiles, m, depKeys, mode, experimentalKey)
+	phKey := computePackageKey(m.ID, compiledGoFiles, m, depKey, mode, experimentalKey)
 	promise, release := s.store.Promise(phKey, func(ctx context.Context, arg interface{}) interface{} {
 
 		pkg, err := typeCheckImpl(ctx, arg.(*snapshot), goFiles, compiledGoFiles, m.Metadata, mode, deps)
@@ -225,8 +229,8 @@ func (s *snapshot) workspaceParseMode(id PackageID) source.ParseMode {
 
 // computePackageKey returns a key representing the act of type checking
 // a package named id containing the specified files, metadata, and
-// dependency hashes.
-func computePackageKey(id PackageID, files []source.FileHandle, m *KnownMetadata, deps []packageHandleKey, mode source.ParseMode, experimentalKey bool) packageHandleKey {
+// combined dependency hash.
+func computePackageKey(id PackageID, files []source.FileHandle, m *KnownMetadata, depsKey source.Hash, mode source.ParseMode, experimentalKey bool) packageHandleKey {
 	// TODO(adonovan): opt: no need to materalize the bytes; hash them directly.
 	// Also, use field separators to avoid spurious collisions.
 	b := bytes.NewBuffer(nil)
@@ -243,9 +247,7 @@ func computePackageKey(id PackageID, files []source.FileHandle, m *KnownMetadata
 		b.Write(hc[:])
 	}
 	b.WriteByte(byte(mode))
-	for _, dep := range deps {
-		b.Write(dep[:])
-	}
+	b.Write(depsKey[:])
 	for _, file := range files {
 		b.WriteString(file.FileIdentity().String())
 	}
@@ -311,7 +313,7 @@ func (ph *packageHandle) cached() (*pkg, error) {
 // typeCheckImpl type checks the parsed source files in compiledGoFiles.
 // (The resulting pkg also holds the parsed but not type-checked goFiles.)
 // deps holds the future results of type-checking the direct dependencies.
-func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *Metadata, mode source.ParseMode, deps map[PackagePath]*packageHandle) (*pkg, error) {
+func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *Metadata, mode source.ParseMode, deps map[PackageID]*packageHandle) (*pkg, error) {
 	// Start type checking of direct dependencies,
 	// in parallel and asynchronously.
 	// As the type checker imports each of these
@@ -446,15 +448,15 @@ func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoF
 
 var goVersionRx = regexp.MustCompile(`^go([1-9][0-9]*)\.(0|[1-9][0-9]*)$`)
 
-func doTypeCheck(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *Metadata, mode source.ParseMode, deps map[PackagePath]*packageHandle, astFilter *unexportedFilter) (*pkg, error) {
+func doTypeCheck(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *Metadata, mode source.ParseMode, deps map[PackageID]*packageHandle, astFilter *unexportedFilter) (*pkg, error) {
 	ctx, done := event.Start(ctx, "cache.typeCheck", tag.Package.Of(string(m.ID)))
 	defer done()
 
 	pkg := &pkg{
-		m:       m,
-		mode:    mode,
-		imports: make(map[PackagePath]*pkg),
-		types:   types.NewPackage(string(m.PkgPath), string(m.Name)),
+		m:             m,
+		mode:          mode,
+		depsByPkgPath: make(map[PackagePath]*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),
@@ -522,23 +524,30 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFil
 		Error: func(e error) {
 			pkg.typeErrors = append(pkg.typeErrors, e.(types.Error))
 		},
-		Importer: importerFunc(func(pkgPath string) (*types.Package, error) {
-			// If the context was cancelled, we should abort.
-			if ctx.Err() != nil {
-				return nil, ctx.Err()
+		Importer: importerFunc(func(path string) (*types.Package, error) {
+			// While all of the import errors could be reported
+			// 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)]
+			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 := resolveImportPath(pkgPath, pkg, deps)
-			if dep == nil {
-				return nil, snapshot.missingPkgError(ctx, pkgPath)
+			dep, ok := deps[id]
+			if !ok {
+				return nil, snapshot.missingPkgError(ctx, path)
 			}
 			if !source.IsValidImport(string(m.PkgPath), string(dep.m.PkgPath)) {
-				return nil, fmt.Errorf("invalid use of internal package %s", pkgPath)
+				return nil, fmt.Errorf("invalid use of internal package %s", path)
 			}
 			depPkg, err := dep.await(ctx, snapshot)
 			if err != nil {
 				return nil, err
 			}
-			pkg.imports[depPkg.m.PkgPath] = depPkg
+			pkg.depsByPkgPath[depPkg.m.PkgPath] = depPkg
 			return depPkg.types, nil
 		}),
 	}
@@ -840,41 +849,6 @@ func expandErrors(errs []types.Error, supportsRelatedInformation bool) []extende
 	return result
 }
 
-// resolveImportPath resolves an import path in pkg to a package from deps.
-// It should produce the same results as resolveImportPath:
-// https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/load/pkg.go;drc=641918ee09cb44d282a30ee8b66f99a0b63eaef9;l=990.
-func resolveImportPath(importPath string, pkg *pkg, deps map[PackagePath]*packageHandle) *packageHandle {
-	if dep := deps[PackagePath(importPath)]; dep != nil {
-		return dep
-	}
-	// We may be in GOPATH mode, in which case we need to check vendor dirs.
-	searchDir := path.Dir(pkg.PkgPath())
-	for {
-		vdir := PackagePath(path.Join(searchDir, "vendor", importPath))
-		if vdep := deps[vdir]; vdep != nil {
-			return vdep
-		}
-
-		// Search until Dir doesn't take us anywhere new, e.g. "." or "/".
-		next := path.Dir(searchDir)
-		if searchDir == next {
-			break
-		}
-		searchDir = next
-	}
-
-	// Vendor didn't work. Let's try minimal module compatibility mode.
-	// In MMC, the packagePath is the canonical (.../vN/...) path, which
-	// is hard to calculate. But the go command has already resolved the ID
-	// to the non-versioned path, and we can take advantage of that.
-	for _, dep := range deps {
-		if dep.ID() == importPath {
-			return dep
-		}
-	}
-	return nil
-}
-
 // An importFunc is an implementation of the single-method
 // types.Importer interface based on a function value.
 type importerFunc func(path string) (*types.Package, error)

gopls/internal/lsp/cache/graph.go

@@ -53,7 +53,7 @@ func (g *metadataGraph) build() {
 	// Build the import graph.
 	g.importedBy = make(map[PackageID][]PackageID)
 	for id, m := range g.metadata {
-		for _, importID := range m.Deps {
+		for _, importID := range uniqueDeps(m.Imports) {
 			g.importedBy[importID] = append(g.importedBy[importID], id)
 		}
 	}
@@ -129,8 +129,27 @@ func (g *metadataGraph) build() {
 	}
 }
 
+// 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
-// reach an id in ids via their Deps. The result also includes given ids.
+// import an id in ids. The result also includes given ids.
 //
 // If includeInvalid is false, the algorithm ignores packages with invalid
 // metadata (including those in the given list of ids).

gopls/internal/lsp/cache/load.go

@@ -207,7 +207,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 		if s.view.allFilesExcluded(pkg, filterer) {
 			continue
 		}
-		if err := buildMetadata(ctx, PackagePath(pkg.PkgPath), pkg, cfg, query, newMetadata, nil); err != nil {
+		if err := buildMetadata(ctx, pkg, cfg, query, newMetadata, nil); err != nil {
 			return err
 		}
 	}
@@ -476,7 +476,9 @@ func makeWorkspaceDir(ctx context.Context, workspace *workspace, fs source.FileS
 // buildMetadata populates the updates map with metadata updates to
 // apply, based on the given pkg. It recurs through pkg.Imports to ensure that
 // metadata exists for all dependencies.
-func buildMetadata(ctx context.Context, pkgPath PackagePath, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*KnownMetadata, path []PackageID) error {
+func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*KnownMetadata, path []PackageID) error {
+	// Allow for multiple ad-hoc packages in the workspace (see #47584).
+	pkgPath := PackagePath(pkg.PkgPath)
 	id := PackageID(pkg.ID)
 	if source.IsCommandLineArguments(pkg.ID) {
 		suffix := ":" + strings.Join(query, ",")
@@ -540,33 +542,66 @@ func buildMetadata(ctx context.Context, pkgPath PackagePath, pkg *packages.Packa
 		m.GoFiles = append(m.GoFiles, uri)
 	}
 
-	for importPath, importPkg := range pkg.Imports {
-		// TODO(rfindley): in rare cases it is possible that the import package
-		// path is not the same as the package path of the import. That is to say
-		// (quoting adonovan):
-		// "The importPath string is the path by which one package is imported from
-		// another, but that needn't be the same as its internal name (sometimes
-		// called the "package path") used to prefix its linker symbols"
-		//
-		// We should not set this package path on the metadata of the dep.
-		importPkgPath := PackagePath(importPath)
-		importID := PackageID(importPkg.ID)
+	imports := make(map[ImportPath]PackageID)
+	for importPath, imported := range pkg.Imports {
+		importPath := ImportPath(importPath)
+		imports[importPath] = PackageID(imported.ID)
 
-		m.Deps = append(m.Deps, importID)
+		// It is not an invariant that importPath == imported.PkgPath.
+		// For example, package "net" imports "golang.org/x/net/dns/dnsmessage"
+		// which refers to the package whose ID and PkgPath are both
+		// "vendor/golang.org/x/net/dns/dnsmessage". Notice the ImportMap,
+		// which maps ImportPaths to PackagePaths:
+		//
+		// $ go list -json net vendor/golang.org/x/net/dns/dnsmessage
+		// {
+		// 	"ImportPath": "net",
+		// 	"Name": "net",
+		// 	"Imports": [
+		// 		"C",
+		// 		"vendor/golang.org/x/net/dns/dnsmessage",
+		// 		"vendor/golang.org/x/net/route",
+		// 		...
+		// 	],
+		// 	"ImportMap": {
+		// 		"golang.org/x/net/dns/dnsmessage": "vendor/golang.org/x/net/dns/dnsmessage",
+		// 		"golang.org/x/net/route": "vendor/golang.org/x/net/route"
+		// 	},
+		//      ...
+		// }
+		// {
+		// 	"ImportPath": "vendor/golang.org/x/net/dns/dnsmessage",
+		// 	"Name": "dnsmessage",
+		//      ...
+		// }
+		//
+		// (Beware that, for historical reasons, go list uses
+		// the JSON field "ImportPath" for the package's
+		// path--effectively the linker symbol prefix.)
 
 		// Don't remember any imports with significant errors.
-		if importPkgPath != "unsafe" && len(importPkg.CompiledGoFiles) == 0 {
+		//
+		// The len=0 condition is a heuristic check for imports of
+		// non-existent packages (for which go/packages will create
+		// an edge to a synthesized node). The heuristic is unsound
+		// because some valid packages have zero files, for example,
+		// a directory containing only the file p_test.go defines an
+		// empty package p.
+		// 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[PackagePath]struct{})
+				m.MissingDeps = make(map[ImportPath]struct{})
 			}
-			m.MissingDeps[importPkgPath] = struct{}{}
+			m.MissingDeps[importPath] = struct{}{}
 			continue
 		}
-		if err := buildMetadata(ctx, importPkgPath, importPkg, cfg, query, updates, append(path, id)); err != nil {
+
+		if err := buildMetadata(ctx, imported, cfg, query, updates, append(path, id)); err != nil {
 			event.Error(ctx, "error in dependency", err)
 		}
 	}
-	sort.Slice(m.Deps, func(i, j int) bool { return m.Deps[i] < m.Deps[j] }) // for determinism
+	m.Imports = imports
 
 	return nil
 }

gopls/internal/lsp/cache/metadata.go

@@ -17,9 +17,10 @@ import (
 // it would result in confusing errors because package IDs often look like
 // package paths.
 type (
-	PackageID   string
-	PackagePath string
-	PackageName string
+	PackageID   string // go list's unique identifier for a package (e.g. "vendor/example.com/foo [vendor/example.com/bar.test]")
+	PackagePath string // name used to prefix linker symbols (e.g. "vendor/example.com/foo")
+	PackageName string // identifier in 'package' declaration (e.g. "foo")
+	ImportPath  string // path that appears in an import declaration (e.g. "example.com/foo")
 )
 
 // Metadata holds package Metadata extracted from a call to packages.Load.
@@ -32,8 +33,8 @@ type Metadata struct {
 	ForTest         PackagePath // package path under test, or ""
 	TypesSizes      types.Sizes
 	Errors          []packages.Error
-	Deps            []PackageID // direct dependencies, in string order
-	MissingDeps     map[PackagePath]struct{}
+	Imports         map[ImportPath]PackageID // may contain duplicate IDs
+	MissingDeps     map[ImportPath]struct{}
 	Module          *packages.Module
 	depsErrors      []*packagesinternal.PackageError
 

gopls/internal/lsp/cache/mod_tidy.go

@@ -466,9 +466,8 @@ func spanFromPositions(m *protocol.ColumnMapper, s, e modfile.Position) (span.Sp
 // the set of strings that appear in import declarations within
 // GoFiles. Errors are ignored.
 //
-// (We can't simply use ph.m.Metadata.Deps because it contains
-// PackageIDs--not import paths--and is based on CompiledGoFiles,
-// after cgo processing.)
+// (We can't simply use Metadata.Imports because it is based on
+// CompiledGoFiles, after cgo processing.)
 func parseImports(ctx context.Context, s *snapshot, files []source.FileHandle) map[string]bool {
 	s.mu.Lock() // peekOrParse requires a locked snapshot (!)
 	defer s.mu.Unlock()