From f5357f59bf231b3cc90abf1ed6b29287dbe86597 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 7 Mar 2024 17:56:59 +0000 Subject: [PATCH] [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 gopls@v0.15.0 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 Reviewed-by: Alan Donovan (cherry picked from commit 93c0ca5673a16f730c2c886546bf80352dfd8908) Reviewed-on: https://go-review.googlesource.com/c/tools/+/569876 Auto-Submit: Robert Findley --- gopls/internal/cache/check.go | 14 +++- gopls/internal/cache/load.go | 33 ++++++-- gopls/internal/server/diagnostics.go | 7 +- .../workspace/multi_folder_test.go | 75 +++++++++++++++++++ 4 files changed, 120 insertions(+), 9 deletions(-) diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 0af59655ab1..61eb98f9641 100644 --- a/gopls/internal/cache/check.go +++ b/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 { diff --git a/gopls/internal/cache/load.go b/gopls/internal/cache/load.go index 4661fd444c6..bcc551099d0 100644 --- a/gopls/internal/cache/load.go +++ b/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 } diff --git a/gopls/internal/server/diagnostics.go b/gopls/internal/server/diagnostics.go index 4d8e3aae4d1..fe5e5055503 100644 --- a/gopls/internal/server/diagnostics.go +++ b/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 { diff --git a/gopls/internal/test/integration/workspace/multi_folder_test.go b/gopls/internal/test/integration/workspace/multi_folder_test.go index 3dace862c24..6adc1f8d5ce 100644 --- a/gopls/internal/test/integration/workspace/multi_folder_test.go +++ b/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/c@v1.2.3/go.mod -- +module example.com/c + +go 1.20 + +-- example.com/c@v1.2.3/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()) + }) +}