internal/lsp/cache: sort Metadata.Deps, for determinism

...so that each client doesn't have to. Change-Id: I039c493031c5c90c4479741cf6f7572dad480808 Reviewed-on: https://go-review.googlesource.com/c/tools/+/415502 Run-TryBot: Alan Donovan <[email protected]> Reviewed-by: Robert Findley <[email protected]> Auto-Submit: Alan Donovan <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
golang · Jul 1, 2022 · 698251a · 698251a
1 parent f79f3aa
commit 698251a
Show file tree

Hide file tree

Showing 4 changed files with 5 additions and 17 deletions.
diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go
@@ -10,7 +10,6 @@ import (
 	"go/ast"
 	"go/types"
 	"reflect"
-	"sort"
 	"sync"
 
 	"golang.org/x/sync/errgroup"
@@ -122,13 +121,8 @@ 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 {
-			importIDs := make([]string, 0, len(ph.m.Deps))
 			for _, importID := range ph.m.Deps {
-				importIDs = append(importIDs, string(importID))
-			}
-			sort.Strings(importIDs) // for determinism
-			for _, importID := range importIDs {
-				depActionHandle, err := s.actionHandle(ctx, PackageID(importID), a)
+				depActionHandle, err := s.actionHandle(ctx, importID, a)
 				if err != nil {
 					return nil, err
 				}

diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
@@ -102,13 +102,6 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
 		return nil, fmt.Errorf("no metadata for %s", id)
 	}
 
-	// For key stability, sort depList.
-	// TODO(adonovan): make m.Deps have a well defined order.
-	depList := append([]PackageID{}, m.Deps...)
-	sort.Slice(depList, func(i, j int) bool {
-		return depList[i] < depList[j]
-	})
-
 	// Begin computing the key by getting the depKeys for all dependencies.
 	// This requires reading the transitive closure of dependencies' source files.
 	//
@@ -122,8 +115,8 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
 	// 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(depList))
-	for i, depID := range depList {
+	depKeys := make([]packageHandleKey, len(m.Deps))
+	for i, depID := range m.Deps {
 		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

diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
@@ -539,6 +539,7 @@ func computeMetadataUpdates(ctx context.Context, g *metadataGraph, pkgPath Packa
 			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
 
 	return nil
 }

diff --git a/internal/lsp/cache/metadata.go b/internal/lsp/cache/metadata.go
@@ -32,7 +32,7 @@ type Metadata struct {
 	ForTest         PackagePath
 	TypesSizes      types.Sizes
 	Errors          []packages.Error
-	Deps            []PackageID
+	Deps            []PackageID // direct dependencies, in string order
 	MissingDeps     map[PackagePath]struct{}
 	Module          *packages.Module
 	depsErrors      []*packagesinternal.PackageError
-Original file line number
+Diff line change
@@ Expand Up @@
     			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
     	return nil
     }
@@ Expand Down @@