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 <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

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()

gopls/internal/lsp/cache/pkg.go

@@ -23,7 +23,7 @@ type pkg struct {
 	goFiles         []*source.ParsedGoFile
 	compiledGoFiles []*source.ParsedGoFile
 	diagnostics     []*source.Diagnostic
-	imports         map[PackagePath]*pkg
+	depsByPkgPath   map[PackagePath]*pkg
 	version         *module.Version
 	parseErrors     []scanner.ErrorList
 	typeErrors      []types.Error
@@ -111,27 +111,57 @@ func (p *pkg) ForTest() string {
 	return string(p.m.ForTest)
 }
 
-func (p *pkg) GetImport(pkgPath string) (source.Package, error) {
-	if imp := p.imports[PackagePath(pkgPath)]; imp != nil {
+// DirectDep returns the directly imported dependency of this package,
+// given its PackagePath.  (If you have an ImportPath, e.g. a string
+// 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
 	}
 	// Don't return a nil pointer because that still satisfies the interface.
 	return nil, fmt.Errorf("no imported package for %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
+			}
+		}
+	}
+	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 i := range p.m.MissingDeps {
-			md = append(md, string(i))
+		for importPath := range p.m.MissingDeps {
+			md = append(md, string(importPath))
 		}
 		return md
 	}
+
+	// This looks wrong.
+	//
+	// rfindley says: it looks like this is intending to implement
+	// a heuristic "if go list couldn't resolve import paths to
+	// packages, then probably you're not in GOPATH or a module".
+	// This is used to determine if we need to show a warning diagnostic.
+	// It looks like this logic is implementing the heuristic that
+	// "even if the metadata has a MissingDep, if the types.Package
+	// 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
 	for _, pkg := range p.types.Imports() {
-		if _, ok := p.m.MissingDeps[PackagePath(pkg.Path())]; ok {
+		if _, ok := p.m.MissingDeps[ImportPath(pkg.Path())]; ok {
 			md = append(md, pkg.Path())
 		}
 	}
@@ -140,7 +170,7 @@ func (p *pkg) MissingDependencies() []string {
 
 func (p *pkg) Imports() []source.Package {
 	var result []source.Package
-	for _, imp := range p.imports {
+	for _, imp := range p.depsByPkgPath {
 		result = append(result, imp)
 	}
 	return result

gopls/internal/lsp/cache/snapshot.go

@@ -892,7 +892,7 @@ func (s *snapshot) isActiveLocked(id PackageID) (active bool) {
 	}
 	// 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.Deps {
+	for _, dep := range m.Imports {
 		if s.isActiveLocked(dep) {
 			return true
 		}
@@ -1209,14 +1209,14 @@ func (s *snapshot) CachedImportPaths(ctx context.Context) (map[string]source.Pac
 		if err != nil {
 			return
 		}
-		for importPath, newPkg := range cachedPkg.imports {
-			if oldPkg, ok := results[string(importPath)]; ok {
+		for pkgPath, newPkg := range cachedPkg.depsByPkgPath {
+			if oldPkg, ok := results[string(pkgPath)]; ok {
 				// Using the same trick as NarrowestPackage, prefer non-variants.
 				if len(newPkg.compiledGoFiles) < len(oldPkg.(*pkg).compiledGoFiles) {
-					results[string(importPath)] = newPkg
+					results[string(pkgPath)] = newPkg
 				}
 			} else {
-				results[string(importPath)] = newPkg
+				results[string(pkgPath)] = newPkg
 			}
 		}
 	})

gopls/internal/lsp/link.go

@@ -174,8 +174,8 @@ func goLinks(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle
 	return links, nil
 }
 
-func moduleAtVersion(target string, pkg source.Package) (string, string, bool) {
-	impPkg, err := pkg.GetImport(target)
+func moduleAtVersion(targetImportPath string, pkg source.Package) (string, string, bool) {
+	impPkg, err := pkg.ResolveImportPath(targetImportPath)
 	if err != nil {
 		return "", "", false
 	}

gopls/internal/lsp/semantic.go

@@ -15,14 +15,15 @@ import (
 	"log"
 	"path/filepath"
 	"sort"
+	"strconv"
 	"strings"
 	"time"
 
-	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/lsp/source"
 	"golang.org/x/tools/gopls/internal/lsp/template"
+	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/typeparams"
 )
 
@@ -875,31 +876,25 @@ func (e *encoded) importSpec(d *ast.ImportSpec) {
 		}
 		return // don't mark anything for . or _
 	}
-	val := d.Path.Value
-	if len(val) < 2 || val[0] != '"' || val[len(val)-1] != '"' {
-		// avoid panics on imports without a properly quoted string
+	importPath, err := strconv.Unquote(d.Path.Value)
+	if err != nil {
 		return
 	}
-	nm := val[1 : len(val)-1] // remove surrounding "s
 	// Import strings are implementation defined. Try to match with parse information.
-	x, err := e.pkg.GetImport(nm)
+	imported, err := e.pkg.ResolveImportPath(importPath)
 	if err != nil {
 		// unexpected, but impact is that maybe some import is not colored
 		return
 	}
-	// expect that nm is x.PkgPath and that x.Name() is a component of it
-	if x.PkgPath() != nm {
-		// don't know how or what to color (if this can happen at all)
-		return
-	}
-	// this is not a precise test: imagine "github.com/nasty/v/v2"
-	j := strings.LastIndex(nm, x.Name())
+	// Check whether the original literal contains the package's declared name.
+	j := strings.LastIndex(d.Path.Value, imported.Name())
 	if j == -1 {
 		// name doesn't show up, for whatever reason, so nothing to report
 		return
 	}
-	start := d.Path.Pos() + 1 + token.Pos(j) // skip the initial quote
-	e.token(start, len(x.Name()), tokNamespace, nil)
+	// Report virtual declaration at the position of the substring.
+	start := d.Path.Pos() + token.Pos(j)
+	e.token(start, len(imported.Name()), tokNamespace, nil)
 }
 
 // log unexpected state

gopls/internal/lsp/source/completion/completion.go

@@ -264,7 +264,6 @@ type compLitInfo struct {
 type importInfo struct {
 	importPath string
 	name       string
-	pkg        source.Package
 }
 
 type methodSetKey struct {
@@ -1165,7 +1164,6 @@ func (c *completer) unimportedMembers(ctx context.Context, id *ast.Ident) error
 		}
 		imp := &importInfo{
 			importPath: path,
-			pkg:        pkg,
 		}
 		if imports.ImportPathToAssumedName(path) != pkg.GetTypes().Name() {
 			imp.name = pkg.GetTypes().Name()
@@ -1517,7 +1515,6 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru
 		}
 		imp := &importInfo{
 			importPath: path,
-			pkg:        pkg,
 		}
 		if imports.ImportPathToAssumedName(path) != pkg.GetTypes().Name() {
 			imp.name = pkg.GetTypes().Name()

gopls/internal/lsp/source/completion/statements.go

@@ -336,7 +336,7 @@ func getTestVar(enclosingFunc *funcInfo, pkg source.Package) string {
 		if param.Name() == "_" {
 			continue
 		}
-		testingPkg, err := pkg.GetImport("testing")
+		testingPkg, err := pkg.DirectDep("testing")
 		if err != nil {
 			continue
 		}

gopls/internal/lsp/source/hover.go

@@ -342,7 +342,7 @@ func HoverIdentifier(ctx context.Context, i *IdentifierInfo) (*HoverJSON, error)
 
 	// See golang/go#36998: don't link to modules matching GOPRIVATE.
 	//
-	// The path returned by linkData is an import path.
+	// The path returned by linkData is a package path.
 	if i.Snapshot.View().IsGoPrivatePath(h.LinkPath) {
 		h.LinkPath = ""
 	} else if mod, version, ok := moduleAtVersion(h.LinkPath, i); ok {
@@ -352,11 +352,11 @@ func HoverIdentifier(ctx context.Context, i *IdentifierInfo) (*HoverJSON, error)
 	return h, nil
 }
 
-// linkData returns the name, import path, and anchor to use in building links
+// linkData returns the name, package path, and anchor to use in building links
 // to obj.
 //
 // If obj is not visible in documentation, the returned name will be empty.
-func linkData(obj types.Object, enclosing *types.TypeName) (name, importPath, anchor string) {
+func linkData(obj types.Object, enclosing *types.TypeName) (name, packagePath, anchor string) {
 	// Package names simply link to the package.
 	if obj, ok := obj.(*types.PkgName); ok {
 		return obj.Name(), obj.Imported().Path(), ""
@@ -430,7 +430,7 @@ func linkData(obj types.Object, enclosing *types.TypeName) (name, importPath, an
 		return "", "", ""
 	}
 
-	importPath = obj.Pkg().Path()
+	packagePath = obj.Pkg().Path()
 	if recv != nil {
 		anchor = fmt.Sprintf("%s.%s", recv.Name(), obj.Name())
 		name = fmt.Sprintf("(%s.%s).%s", obj.Pkg().Name(), recv.Name(), obj.Name())
@@ -439,7 +439,7 @@ func linkData(obj types.Object, enclosing *types.TypeName) (name, importPath, an
 		anchor = obj.Name()
 		name = fmt.Sprintf("%s.%s", obj.Pkg().Name(), obj.Name())
 	}
-	return name, importPath, anchor
+	return name, packagePath, anchor
 }
 
 func moduleAtVersion(path string, i *IdentifierInfo) (string, string, bool) {
@@ -448,7 +448,7 @@ func moduleAtVersion(path string, i *IdentifierInfo) (string, string, bool) {
 	if strings.ToLower(i.Snapshot.View().Options().LinkTarget) != "pkg.go.dev" {
 		return "", "", false
 	}
-	impPkg, err := i.pkg.GetImport(path)
+	impPkg, err := i.pkg.DirectDep(path)
 	if err != nil {
 		return "", "", false
 	}
@@ -535,11 +535,11 @@ func FindHoverContext(ctx context.Context, s Snapshot, pkg Package, obj types.Ob
 		}
 	case *ast.ImportSpec:
 		// Try to find the package documentation for an imported package.
-		pkgPath, err := strconv.Unquote(node.Path.Value)
+		importPath, err := strconv.Unquote(node.Path.Value)
 		if err != nil {
 			return nil, err
 		}
-		imp, err := pkg.GetImport(pkgPath)
+		imp, err := pkg.ResolveImportPath(importPath)
 		if err != nil {
 			return nil, err
 		}

gopls/internal/lsp/source/identifier.go

@@ -456,21 +456,21 @@ func importSpec(snapshot Snapshot, pkg Package, file *ast.File, pos token.Pos) (
 	if err != nil {
 		return nil, fmt.Errorf("import path not quoted: %s (%v)", imp.Path.Value, err)
 	}
+	imported, err := pkg.ResolveImportPath(importPath)
+	if err != nil {
+		return nil, err
+	}
 	result := &IdentifierInfo{
 		Snapshot: snapshot,
-		Name:     importPath,
+		Name:     importPath, // should this perhaps be imported.PkgPath()?
 		pkg:      pkg,
 	}
 	if result.MappedRange, err = posToMappedRange(snapshot.FileSet(), pkg, imp.Path.Pos(), imp.Path.End()); err != nil {
 		return nil, err
 	}
 	// Consider the "declaration" of an import spec to be the imported package.
-	importedPkg, err := pkg.GetImport(importPath)
-	if err != nil {
-		return nil, err
-	}
 	// Return all of the files in the package as the definition of the import spec.
-	for _, dst := range importedPkg.GetSyntax() {
+	for _, dst := range imported.GetSyntax() {
 		rng, err := posToMappedRange(snapshot.FileSet(), pkg, dst.Pos(), dst.End())
 		if err != nil {
 			return nil, err

gopls/internal/lsp/source/known_packages.go

@@ -28,6 +28,8 @@ func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandl
 	for _, imp := range pgf.File.Imports {
 		alreadyImported[imp.Path.Value] = 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

gopls/internal/lsp/source/rename_check.go

@@ -838,11 +838,11 @@ func pathEnclosingInterval(fset *token.FileSet, pkg Package, start, end token.Po
 			if err != nil {
 				continue
 			}
-			importPkg, err := pkg.GetImport(importPath)
+			imported, err := pkg.ResolveImportPath(importPath)
 			if err != nil {
 				return nil, nil, nil, false
 			}
-			pkgs = append(pkgs, importPkg)
+			pkgs = append(pkgs, imported)
 		}
 	}
 	for _, p := range pkgs {

gopls/internal/lsp/source/stub.go

@@ -255,8 +255,10 @@ func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.Method
 		eiface := iface.Embedded(i).Obj()
 		depPkg := ifacePkg
 		if eiface.Pkg().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.GetImport(eiface.Pkg().Path())
+			depPkg, err = ifacePkg.DirectDep(eiface.Pkg().Path())
 			if err != nil {
 				return nil, err
 			}

gopls/internal/lsp/source/view.go

@@ -152,8 +152,8 @@ type Snapshot interface {
 	GetReverseDependencies(ctx context.Context, id string) ([]Package, error)
 
 	// CachedImportPaths returns all the imported packages loaded in this
-	// snapshot, indexed by their import path and checked in TypecheckWorkspace
-	// mode.
+	// snapshot, indexed by their package path (not import path, despite the name)
+	// and checked in TypecheckWorkspace mode.
 	CachedImportPaths(ctx context.Context) (map[string]Package, error)
 
 	// KnownPackages returns all the packages loaded in this snapshot, checked
@@ -510,6 +510,14 @@ func (h Hash) Less(other Hash) bool {
 	return bytes.Compare(h[:], other[:]) < 0
 }
 
+// XORWith updates *h to *h XOR h2.
+func (h *Hash) XORWith(h2 Hash) {
+	// Small enough that we don't need crypto/subtle.XORBytes.
+	for i := range h {
+		h[i] ^= h2[i]
+	}
+}
+
 // FileIdentity uniquely identifies a file at a version from a FileSystem.
 type FileIdentity struct {
 	URI  span.URI
@@ -581,18 +589,19 @@ func (a Analyzer) IsEnabled(view View) bool {
 // Package represents a Go package that has been type-checked. It maintains
 // only the relevant fields of a *go/packages.Package.
 type Package interface {
-	ID() string
-	Name() string
-	PkgPath() string
+	ID() string      // logically a cache.PackageID
+	Name() string    // logically a cache.PackageName
+	PkgPath() string // logically a cache.PackagePath
 	CompiledGoFiles() []*ParsedGoFile
 	File(uri span.URI) (*ParsedGoFile, error)
 	GetSyntax() []*ast.File
 	GetTypes() *types.Package
 	GetTypesInfo() *types.Info
 	GetTypesSizes() types.Sizes
 	ForTest() string
-	GetImport(pkgPath string) (Package, error)
-	MissingDependencies() []string
+	DirectDep(packagePath string) (Package, error)        // logically a cache.PackagePath
+	ResolveImportPath(importPath string) (Package, error) // logically a cache.ImportPath
+	MissingDependencies() []string                        // unordered; logically cache.ImportPaths
 	Imports() []Package
 	Version() *module.Version
 	HasListOrParseErrors() bool