gopls/internal/cache/metadata: use pointers for Graph indexes

As noted in a recent CL, using pointers in the metadata graph is both
simpler and more efficient, and avoids some unnecessarily defensive code
that was looking up IDs.

Change-Id: I8a45e78c4f9be88ae3460ec77189ca43dda163dd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/686839
Auto-Submit: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

gopls/internal/cache/load.go

@@ -792,8 +792,8 @@ func allFilesHaveRealPackages(g *metadata.Graph, mp *metadata.Package) bool {
 	n := len(mp.CompiledGoFiles)
 checkURIs:
 	for _, uri := range slices.Concat(mp.CompiledGoFiles[0:n:n], mp.GoFiles) {
-		for _, id := range g.IDs[uri] {
-			if !metadata.IsCommandLineArguments(id) {
+		for _, pkg := range g.ForFile[uri] {
+			if !metadata.IsCommandLineArguments(pkg.ID) {
 				continue checkURIs
 			}
 		}

gopls/internal/cache/metadata/graph.go

@@ -22,22 +22,20 @@ type Graph struct {
 	// Packages maps package IDs to their associated Packages.
 	Packages map[PackageID]*Package
 
+	// Each of the three maps below is an index of the pointer values held
+	// by the Packages map. However, Package pointers are not generally canonical.
+
 	// ImportedBy maps package IDs to the list of packages that import them.
-	ImportedBy map[PackageID][]PackageID
+	ImportedBy map[PackageID][]*Package
 
-	// ByPackagePath maps package by their package path to their package ID.
+	// ForPackagePath maps package by their package path to their package ID.
 	// Non-test packages appear before test packages, and within each of those
 	// categories, packages with fewer CompiledGoFiles appear first.
-	//
-	// TODO(rfindley): there's no reason for ImportedBy and IDs to not also hold
-	// pointers, rather than IDs, and pointers are more convenient.
-	ByPackagePath map[PackagePath][]*Package
+	ForPackagePath map[PackagePath][]*Package
 
-	// IDs maps file URIs to package IDs, sorted by (!valid, cli, packageID).
+	// ForFile maps file URIs to packages, sorted by (!valid, cli, packageID).
 	// A single file may belong to multiple packages due to tests packages.
-	//
-	// Invariant: all IDs present in the IDs map exist in the metadata map.
-	IDs map[protocol.DocumentURI][]PackageID
+	ForFile map[protocol.DocumentURI][]*Package
 }
 
 // Metadata implements the [Source] interface
@@ -93,18 +91,18 @@ func (g *Graph) Update(updates map[PackageID]*Package) *Graph {
 // deriving relations from the specified metadata.
 func newGraph(pkgs map[PackageID]*Package) *Graph {
 	// Build the import graph.
-	importedBy := make(map[PackageID][]PackageID)
+	importedBy := make(map[PackageID][]*Package)
 	byPackagePath := make(map[PackagePath][]*Package)
-	for id, mp := range pkgs {
+	for _, mp := range pkgs {
 		for _, depID := range mp.DepsByPkgPath {
-			importedBy[depID] = append(importedBy[depID], id)
+			importedBy[depID] = append(importedBy[depID], mp)
 		}
 		byPackagePath[mp.PkgPath] = append(byPackagePath[mp.PkgPath], mp)
 	}
 
 	// Collect file associations.
-	uriIDs := make(map[protocol.DocumentURI][]PackageID)
-	for id, mp := range pkgs {
+	uriPkgs := make(map[protocol.DocumentURI][]*Package)
+	for _, mp := range pkgs {
 		uris := map[protocol.DocumentURI]struct{}{}
 		for _, uri := range mp.CompiledGoFiles {
 			uris[uri] = struct{}{}
@@ -118,34 +116,34 @@ func newGraph(pkgs map[PackageID]*Package) *Graph {
 			}
 		}
 		for uri := range uris {
-			uriIDs[uri] = append(uriIDs[uri], id)
+			uriPkgs[uri] = append(uriPkgs[uri], mp)
 		}
 	}
 
 	// Sort and filter file associations.
-	for uri, ids := range uriIDs {
-		sort.Slice(ids, func(i, j int) bool {
-			cli := IsCommandLineArguments(ids[i])
-			clj := IsCommandLineArguments(ids[j])
+	for uri, pkgs := range uriPkgs {
+		sort.Slice(pkgs, func(i, j int) bool {
+			cli := IsCommandLineArguments(pkgs[i].ID)
+			clj := IsCommandLineArguments(pkgs[j].ID)
 			if cli != clj {
 				return clj
 			}
 
 			// 2. packages appear in name order.
-			return ids[i] < ids[j]
+			return pkgs[i].ID < pkgs[j].ID
 		})
 
-		// Choose the best IDs for each URI, according to the following rules:
+		// Choose the best packages for each URI, according to the following rules:
 		//  - If there are any valid real packages, choose them.
 		//  - Else, choose the first valid command-line-argument package, if it exists.
 		//
-		// TODO(rfindley): it might be better to track all IDs here, and exclude
+		// TODO(rfindley): it might be better to track all packages here, and exclude
 		// them later when type checking, but this is the existing behavior.
-		for i, id := range ids {
+		for i, pkg := range pkgs {
 			// If we've seen *anything* prior to command-line arguments package, take
-			// it. Note that ids[0] may itself be command-line-arguments.
-			if i > 0 && IsCommandLineArguments(id) {
-				uriIDs[uri] = ids[:i]
+			// it. Note that pkgs[0] may itself be command-line-arguments.
+			if i > 0 && IsCommandLineArguments(pkg.ID) {
+				uriPkgs[uri] = pkgs[:i]
 				break
 			}
 		}
@@ -167,10 +165,10 @@ func newGraph(pkgs map[PackageID]*Package) *Graph {
 	}
 
 	return &Graph{
-		Packages:      pkgs,
-		ImportedBy:    importedBy,
-		ByPackagePath: byPackagePath,
-		IDs:           uriIDs,
+		Packages:       pkgs,
+		ImportedBy:     importedBy,
+		ForPackagePath: byPackagePath,
+		ForFile:        uriPkgs,
 	}
 }
 
@@ -179,18 +177,22 @@ func newGraph(pkgs map[PackageID]*Package) *Graph {
 // transitively imports one of them, keyed by ID, including all the initial packages.
 func (g *Graph) ReverseReflexiveTransitiveClosure(ids ...PackageID) map[PackageID]*Package {
 	seen := make(map[PackageID]*Package)
-	var visitAll func([]PackageID)
-	visitAll = func(ids []PackageID) {
-		for _, id := range ids {
-			if seen[id] == nil {
-				if mp := g.Packages[id]; mp != nil {
-					seen[id] = mp
-					visitAll(g.ImportedBy[id])
-				}
+	var visitAll func([]*Package)
+	visitAll = func(pkgs []*Package) {
+		for _, pkg := range pkgs {
+			if seen[pkg.ID] == nil {
+				seen[pkg.ID] = pkg
+				visitAll(g.ImportedBy[pkg.ID])
 			}
 		}
 	}
-	visitAll(ids)
+	var initial []*Package
+	for _, id := range ids {
+		if pkg := g.Packages[id]; pkg != nil {
+			initial = append(initial, pkg)
+		}
+	}
+	visitAll(initial)
 	return seen
 }
 

gopls/internal/cache/session.go

@@ -436,8 +436,8 @@ func (s *Session) SnapshotOf(ctx context.Context, uri protocol.DocumentURI) (*Sn
 		//
 		// Any view can load a command-line-arguments package; aggregate those into
 		// views[0] below.
-		for _, id := range g.IDs[uri] {
-			if !metadata.IsCommandLineArguments(id) || g.Packages[id].Standalone {
+		for _, pkg := range g.ForFile[uri] {
+			if !metadata.IsCommandLineArguments(pkg.ID) || pkg.Standalone {
 				return snapshot, release, nil
 			}
 		}

gopls/internal/cache/snapshot.go

@@ -690,11 +690,11 @@ func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI
 	s.mu.Lock()
 
 	// Start with the set of package associations derived from the last load.
-	ids := s.meta.IDs[uri]
+	pkgs := s.meta.ForFile[uri]
 
 	shouldLoad := false // whether any packages containing uri are marked 'shouldLoad'
-	for _, id := range ids {
-		if pkgs, _ := s.shouldLoad.Get(id); len(pkgs) > 0 {
+	for _, pkg := range pkgs {
+		if p, _ := s.shouldLoad.Get(pkg.ID); len(p) > 0 {
 			shouldLoad = true
 		}
 	}
@@ -708,7 +708,7 @@ func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI
 	//  - uri is not contained in any valid packages
 	//  - ...or one of the packages containing uri is marked 'shouldLoad'
 	//  - ...but uri is not unloadable
-	if (shouldLoad || len(ids) == 0) && !unloadable {
+	if (shouldLoad || len(pkgs) == 0) && !unloadable {
 		scope := fileLoadScope(uri)
 		err := s.load(ctx, NoNetwork, scope)
 
@@ -741,18 +741,14 @@ func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI
 	// Retrieve the metadata.
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	ids = s.meta.IDs[uri]
-	metas := make([]*metadata.Package, len(ids))
-	for i, id := range ids {
-		metas[i] = s.meta.Packages[id]
-		if metas[i] == nil {
-			panic("nil metadata")
-		}
-	}
+	// TODO(rfindley): is there any reason not to make the sorting below the
+	// canonical sorting, so that we don't need to mutate this slice?
+	metas := slices.Clone(s.meta.ForFile[uri])
+
 	// Metadata is only ever added by loading,
 	// so if we get here and still have
-	// no IDs, uri is unloadable.
-	if !unloadable && len(ids) == 0 {
+	// no packages, uri is unloadable.
+	if !unloadable && len(metas) == 0 {
 		s.unloadableFiles.Add(uri)
 	}
 
@@ -796,10 +792,8 @@ func (s *Snapshot) ReverseDependencies(ctx context.Context, id PackageID, transi
 	} else {
 		// direct reverse dependencies
 		rdeps = make(map[PackageID]*metadata.Package)
-		for _, rdepID := range meta.ImportedBy[id] {
-			if rdep := meta.Packages[rdepID]; rdep != nil {
-				rdeps[rdepID] = rdep
-			}
+		for _, rdep := range meta.ImportedBy[id] {
+			rdeps[rdep.ID] = rdep
 		}
 	}
 
@@ -1061,9 +1055,8 @@ func (s *Snapshot) clearShouldLoad(scopes ...loadScope) {
 			}
 		case fileLoadScope:
 			uri := protocol.DocumentURI(scope)
-			ids := s.meta.IDs[uri]
-			for _, id := range ids {
-				s.shouldLoad.Delete(id)
+			for _, pkg := range s.meta.ForFile[uri] {
+				s.shouldLoad.Delete(pkg.ID)
 			}
 		}
 	}
@@ -1706,7 +1699,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f
 		anyPkgFileChanged = anyPkgFileChanged || pkgFileChanged
 
 		// Mark all of the package IDs containing the given file.
-		filePackageIDs := invalidatedPackageIDs(uri, s.meta.IDs, pkgFileChanged)
+		filePackageIDs := invalidatedPackageIDs(uri, s.meta.ForFile, pkgFileChanged)
 		for id := range filePackageIDs {
 			directIDs[id] = directIDs[id] || invalidateMetadata // may insert 'false'
 		}
@@ -1807,8 +1800,8 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f
 			return
 		}
 		idsToInvalidate[id] = newInvalidateMetadata
-		for _, rid := range s.meta.ImportedBy[id] {
-			addRevDeps(rid, invalidateMetadata)
+		for _, rdep := range s.meta.ImportedBy[id] {
+			addRevDeps(rdep.ID, invalidateMetadata)
 		}
 	}
 	for id, invalidateMetadata := range directIDs {
@@ -1942,12 +1935,12 @@ func deleteMostRelevantModFile(m *persistent.Map[protocol.DocumentURI, *memoize.
 // If packageFileChanged is set, the file is either a new file, or has a new
 // package name. In this case, all known packages in the directory will be
 // invalidated.
-func invalidatedPackageIDs(uri protocol.DocumentURI, known map[protocol.DocumentURI][]PackageID, packageFileChanged bool) map[PackageID]struct{} {
+func invalidatedPackageIDs(uri protocol.DocumentURI, known map[protocol.DocumentURI][]*metadata.Package, packageFileChanged bool) map[PackageID]struct{} {
 	invalidated := make(map[PackageID]struct{})
 
 	// At a minimum, we invalidate packages known to contain uri.
-	for _, id := range known[uri] {
-		invalidated[id] = struct{}{}
+	for _, pkg := range known[uri] {
+		invalidated[pkg.ID] = struct{}{}
 	}
 
 	// If the file didn't move to a new package, we should only invalidate the
@@ -1980,15 +1973,15 @@ func invalidatedPackageIDs(uri protocol.DocumentURI, known map[protocol.Document
 	fi, err := getInfo(dir)
 	if err == nil {
 		// Aggregate all possibly relevant package IDs.
-		for knownURI, ids := range known {
+		for knownURI, pkgs := range known {
 			knownDir := knownURI.DirPath()
 			knownFI, err := getInfo(knownDir)
 			if err != nil {
 				continue
 			}
 			if os.SameFile(fi, knownFI) {
-				for _, id := range ids {
-					invalidated[id] = struct{}{}
+				for _, pkg := range pkgs {
+					invalidated[pkg.ID] = struct{}{}
 				}
 			}
 		}

gopls/internal/debug/serve.go

@@ -921,7 +921,7 @@ var MetadataTmpl = template.Must(template.Must(BaseTemplate.Clone()).Parse(`
 
 <h3 id='hdr-Files'>Files</h3>
 <ul>
-{{range $uri, $ids := .IDs}}<li>{{$uri}} →{{range $ids}} <a href='#{{.}}'>{{.}}</a>{{end}}</li>{{end}}
+{{range $uri, $pkgs := .ForFile}}<li>{{$uri}} →{{range $pkgs}} <a href='#{{.ID}}'>{{.ID}}</a>{{end}}</li>{{end}}
 </ul>
 
 {{end}}

gopls/internal/golang/completion/unimported.go

@@ -79,7 +79,10 @@ func (c *completer) findPackageIDs(pkgname metadata.PackageName) (wsIDs, thisPkg
 			continue
 		}
 		imports := g.ImportedBy[pid]
-		if slices.Contains(imports, c.pkg.Metadata().ID) {
+		// Metadata is not canonical: it may be held onto by a package. Therefore,
+		// we must compare by ID.
+		thisPkg := func(mp *metadata.Package) bool { return mp.ID == c.pkg.Metadata().ID }
+		if slices.ContainsFunc(imports, thisPkg) {
 			thisPkgIDs = append(thisPkgIDs, pid)
 		} else {
 			wsIDs = append(wsIDs, pid)

gopls/internal/mcp/outline.go

@@ -41,7 +41,7 @@ func (h *handler) outlineHandler(ctx context.Context, _ *mcp.ServerSession, para
 		return nil, err
 	}
 
-	var toSummarize []metadata.PackageID
+	var toSummarize []*metadata.Package
 	for _, imp := range params.Arguments.PackagePaths {
 		pkgPath := metadata.PackagePath(imp)
 		if len(imp) > 0 && imp[0] == '"' {
@@ -51,18 +51,17 @@ func (h *handler) outlineHandler(ctx context.Context, _ *mcp.ServerSession, para
 			}
 			pkgPath = metadata.PackagePath(unquoted)
 		}
-		if mps := md.ByPackagePath[pkgPath]; len(mps) > 0 {
-			toSummarize = append(toSummarize, mps[0].ID) // first is best
+		if mps := md.ForPackagePath[pkgPath]; len(mps) > 0 {
+			toSummarize = append(toSummarize, mps[0]) // first is best
 		}
 	}
 
 	var content []*mcp.Content
-	for _, id := range toSummarize {
-		md := snapshot.Metadata(id)
+	for _, mp := range toSummarize {
 		if md == nil {
 			continue // ignore error
 		}
-		if summary := summarizePackage(ctx, snapshot, md); summary != "" {
+		if summary := summarizePackage(ctx, snapshot, mp); summary != "" {
 			content = append(content, mcp.NewTextContent(summary))
 		}
 	}

gopls/internal/server/server.go

@@ -356,7 +356,7 @@ func (s *server) initWeb() (*web, error) {
 
 		// Find package by path.
 		pkgPath := metadata.PackagePath(req.URL.Path)
-		mps := snapshot.MetadataGraph().ByPackagePath[pkgPath]
+		mps := snapshot.MetadataGraph().ForPackagePath[pkgPath]
 		if len(mps) == 0 {
 			// TODO(adonovan): what should we do for external test packages?
 			http.Error(w, "package not found", http.StatusNotFound)