[gopls-release-branch.0.15] gopls/internal/cache: fix spurious diagnostics in multi-root workspaces

In golang/go#66145, users reported spurious import errors in multi-root
workspaces. The problem was related to scenarios where module A had a
local replace of module B, and the user opened a file F in module B that
wasn't in the forward dependencies of module A. In this case, if the
View of module A tried to load F, it would get real packages (not
command-line-arguments), but due to module graph pruning the View of
module A would not have access to the full set of dependencies for
module B, resulting in the potential for import errors. Even this would
not be a problem, as long as the package that module A loaded for F was
not considered a 'workspace' package.

Unfortunately a couple of incorrect heuristics in gopls added along with
the zero-config work of [email protected] allowed us to diagnose these
broken packages:

1. In resolveImportGraph, we called MetadataForFile for each overlay. As
   a result, the import graph optimization caused gopls to attempt
   loading packages for each open file, for each View. It was wrong for
   an optimization to have this side effect.
2. In golang/go#65801, we observed that it was inconsistent to diagnose
   changed packages independent of whether they're workspace packages.
   To fix that, I made all open packages workspace packages. It was
   probably wrong for the set of workspace packages to depend on open
   files. To summarize a rather long philosophical digression: open
   files should determine Views, not packages.

Fixing either one of these incorrect heuristics would have prevented
golang/go#66145. In this CL, we fix (2) by building the import graph
based on existing metadata, without triggering an additional load.

For (1), we check IsWorkspacePackage in diagnoseChangedFiles to enforce
consistency in the set of diagnosed packages. It would be nice to also
remove the heuristic that "all open packages are workspace packages",
but we can't do that yet as it would mean no diagnostics for files
outside the workspace, after e.g. jumping to definition. A TODO is left
to address this another day, when we can be less conservative.

Fixes golang/go#66145

Change-Id: Ic4cf2bbbb515b6ea0df24b8e6e46c725b82b4779
Reviewed-on: https://go-review.googlesource.com/c/tools/+/569836
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
(cherry picked from commit 93c0ca5)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/569876
Auto-Submit: Robert Findley <[email protected]>

Author: findleyr

gopls/internal/cache/check.go

@@ -204,9 +204,17 @@ func (s *Snapshot) resolveImportGraph() (*importGraph, error) {
 
 	openPackages := make(map[PackageID]bool)
 	for _, fh := range s.Overlays() {
-		mps, err := s.MetadataForFile(ctx, fh.URI())
-		if err != nil {
-			return nil, err
+		// golang/go#66145: don't call MetadataForFile here. This function, which
+		// builds a shared import graph, is an optimization. We don't want it to
+		// have the side effect of triggering a load.
+		//
+		// In the past, a call to MetadataForFile here caused a bunch of
+		// unnecessary loads in multi-root workspaces (and as a result, spurious
+		// diagnostics).
+		g := s.MetadataGraph()
+		var mps []*metadata.Package
+		for _, id := range g.IDs[fh.URI()] {
+			mps = append(mps, g.Packages[id])
 		}
 		metadata.RemoveIntermediateTestVariants(&mps)
 		for _, mp := range mps {

gopls/internal/cache/load.go

@@ -564,6 +564,22 @@ func computeLoadDiagnostics(ctx context.Context, snapshot *Snapshot, mp *metadat
 	return diags
 }
 
+// IsWorkspacePackage reports whether id points to a workspace package in s.
+//
+// Currently, the result depends on the current set of loaded packages, and so
+// is not guaranteed to be stable.
+func (s *Snapshot) IsWorkspacePackage(ctx context.Context, id PackageID) bool {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	mg := s.meta
+	m := mg.Packages[id]
+	if m == nil {
+		return false
+	}
+	return isWorkspacePackageLocked(ctx, s, mg, m)
+}
+
 // isWorkspacePackageLocked reports whether p is a workspace package for the
 // snapshot s.
 //
@@ -575,6 +591,12 @@ func computeLoadDiagnostics(ctx context.Context, snapshot *Snapshot, mp *metadat
 // heuristics.
 //
 // s.mu must be held while calling this function.
+//
+// TODO(rfindley): remove 'meta' from this function signature. Whether or not a
+// package is a workspace package should depend only on the package, view
+// definition, and snapshot file source. While useful, the heuristic
+// "allFilesHaveRealPackages" does not add that much value and is path
+// dependent as it depends on the timing of loads.
 func isWorkspacePackageLocked(ctx context.Context, s *Snapshot, meta *metadata.Graph, pkg *metadata.Package) bool {
 	if metadata.IsCommandLineArguments(pkg.ID) {
 		// Ad-hoc command-line-arguments packages aren't workspace packages.
@@ -599,12 +621,13 @@ func isWorkspacePackageLocked(ctx context.Context, s *Snapshot, meta *metadata.G
 		return containsOpenFileLocked(s, pkg)
 	}
 
-	// golang/go#65801: any (non command-line-arguments) open package is a
-	// workspace package.
+	// If a real package is open, consider it to be part of the workspace.
 	//
-	// Otherwise, we'd display diagnostics for changes in an open package (due to
-	// the logic of diagnoseChangedFiles), but then erase those diagnostics when
-	// we do the final diagnostics pass. Diagnostics should be stable.
+	// TODO(rfindley): reconsider this. In golang/go#66145, we saw that even if a
+	// View sees a real package for a file, it doesn't mean that View is able to
+	// cleanly diagnose the package. Yet, we do want to show diagnostics for open
+	// packages outside the workspace. Is there a better way to ensure that only
+	// the 'best' View gets a workspace package for the open file?
 	if containsOpenFileLocked(s, pkg) {
 		return true
 	}

gopls/internal/server/diagnostics.go

@@ -276,7 +276,12 @@ func (s *server) diagnoseChangedFiles(ctx context.Context, snapshot *cache.Snaps
 			// noisy to log (and we'll handle things later in the slow pass).
 			continue
 		}
-		toDiagnose[meta.ID] = meta
+		// golang/go#65801: only diagnose changes to workspace packages. Otherwise,
+		// diagnostics will be unstable, as the slow-path diagnostics will erase
+		// them.
+		if snapshot.IsWorkspacePackage(ctx, meta.ID) {
+			toDiagnose[meta.ID] = meta
+		}
 	}
 	diags, err := snapshot.PackageDiagnostics(ctx, maps.Keys(toDiagnose)...)
 	if err != nil {

gopls/internal/test/integration/workspace/multi_folder_test.go

@@ -51,3 +51,78 @@ func _() {
 		)
 	})
 }
+
+func TestMultiView_LocalReplace(t *testing.T) {
+	// This is a regression test for #66145, where gopls attempted to load a
+	// package in a locally replaced module as a workspace package, resulting in
+	// spurious import diagnostics because the module graph had been pruned.
+
+	const proxy = `
+-- example.com/[email protected]/go.mod --
+module example.com/c
+
+go 1.20
+
+-- example.com/[email protected]/c.go --
+package c
+
+const C = 3
+
+`
+	// In the past, gopls would only diagnose one View at a time
+	// (the last to have changed).
+	//
+	// This test verifies that gopls can maintain diagnostics for multiple Views.
+	const files = `
+-- a/go.mod --
+module golang.org/lsptests/a
+
+go 1.20
+
+require golang.org/lsptests/b v1.2.3
+
+replace golang.org/lsptests/b => ../b
+
+-- a/a.go --
+package a
+
+import "golang.org/lsptests/b"
+
+const A = b.B - 1
+
+-- b/go.mod --
+module golang.org/lsptests/b
+
+go 1.20
+
+require example.com/c v1.2.3
+
+-- b/go.sum --
+example.com/c v1.2.3 h1:hsOPhoHQLZPEn7l3kNya3fR3SfqW0/rafZMP8ave6fg=
+example.com/c v1.2.3/go.mod h1:4uG6Y5qX88LrEd4KfRoiguHZIbdLKUEHD1wXqPyrHcA=
+-- b/b.go --
+package b
+
+const B = 2
+
+-- b/unrelated/u.go --
+package unrelated
+
+import "example.com/c"
+
+const U = c.C
+`
+
+	WithOptions(
+		WorkspaceFolders("a", "b"),
+		ProxyFiles(proxy),
+	).Run(t, files, func(t *testing.T, env *Env) {
+		// Opening unrelated first ensures that when we compute workspace packages
+		// for the "a" workspace, it includes the unrelated package, which will be
+		// unloadable from a as there is no a/go.sum.
+		env.OpenFile("b/unrelated/u.go")
+		env.AfterChange()
+		env.OpenFile("a/a.go")
+		env.AfterChange(NoDiagnostics())
+	})
+}