gopls/internal/cache: fix two bugs related to workspace packages

Fix two bugs related to workspace packages that contributed to
golang/go#65801.

The first is that we didn't consider packages with open files to be
workspace packages. This was pre-existing behavior, but in the past we
simply relied on diagnoseChangedFiles to provide diagnostics for open
packages, even though we didn't consider them to be part of the
workspace. That led to races and inconsistent behavior: a change in one
file would wipe out diagnostics in another, and so on. It's much simpler
to say that if the package is open, it is part of the workspace. This
leads to consistent behavior, no matter where diagnostics originate.

The second bug is related to loading std and cmd. The new workspace
package heuristics relied on go/packages.Package.Module to determine if
a package is included in the workspace. For std and cmd, this field is
nil (golang/go#65816), apparently due to limitations of `go list`. As a
result, we were finding no workspace packages for std or cmd. Fix this
by falling back to searching for the relevant go.mod file in the
filesystem. Unfortunately this required reinstating the lockedSnapshot
file source.

These bugs revealed a rather large gap in our test coverage for std. Add
a test that verifies that we compute std workspace packages.

Fixes golang/go#65801

Change-Id: Ic454d4a43e34af10e1224755a09d6c94c728c97d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565475
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

gopls/internal/cache/load.go

@@ -27,6 +27,7 @@ import (
 	"golang.org/x/tools/internal/event/tag"
 	"golang.org/x/tools/internal/gocommand"
 	"golang.org/x/tools/internal/packagesinternal"
+	"golang.org/x/tools/internal/xcontext"
 )
 
 var loadID uint64 // atomic identifier for loads
@@ -283,7 +284,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 	event.Log(ctx, fmt.Sprintf("%s: updating metadata for %d packages", eventName, len(updates)))
 
 	meta := s.meta.Update(updates)
-	workspacePackages := computeWorkspacePackagesLocked(s, meta)
+	workspacePackages := computeWorkspacePackagesLocked(ctx, s, meta)
 	s.meta = meta
 	s.workspacePackages = workspacePackages
 	s.resetActivePackagesLocked()
@@ -563,7 +564,7 @@ func computeLoadDiagnostics(ctx context.Context, snapshot *Snapshot, mp *metadat
 // heuristics.
 //
 // s.mu must be held while calling this function.
-func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.Package) bool {
+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.
 		// With zero-config gopls (golang/go#57979) they should be very rare, as
@@ -587,6 +588,16 @@ func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.P
 		return containsOpenFileLocked(s, pkg)
 	}
 
+	// golang/go#65801: any (non command-line-arguments) open package is a
+	// workspace package.
+	//
+	// 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.
+	if containsOpenFileLocked(s, pkg) {
+		return true
+	}
+
 	// Apply filtering logic.
 	//
 	// Workspace packages must contain at least one non-filtered file.
@@ -624,10 +635,24 @@ func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.P
 	// In module mode, a workspace package must be contained in a workspace
 	// module.
 	if s.view.moduleMode() {
-		if pkg.Module == nil {
-			return false
+		var modURI protocol.DocumentURI
+		if pkg.Module != nil {
+			modURI = protocol.URIFromPath(pkg.Module.GoMod)
+		} else {
+			// golang/go#65816: for std and cmd, Module is nil.
+			// Fall back to an inferior heuristic.
+			if len(pkg.CompiledGoFiles) == 0 {
+				return false // need at least one file to guess the go.mod file
+			}
+			dir := pkg.CompiledGoFiles[0].Dir()
+			var err error
+			modURI, err = findRootPattern(ctx, dir, "go.mod", lockedSnapshot{s})
+			if err != nil || modURI == "" {
+				// err != nil implies context cancellation, in which case the result of
+				// this query does not matter.
+				return false
+			}
 		}
-		modURI := protocol.URIFromPath(pkg.Module.GoMod)
 		_, ok := s.view.workspaceModFiles[modURI]
 		return ok
 	}
@@ -662,10 +687,14 @@ func containsOpenFileLocked(s *Snapshot, mp *metadata.Package) bool {
 // contain intermediate test variants.
 //
 // s.mu must be held while calling this function.
-func computeWorkspacePackagesLocked(s *Snapshot, meta *metadata.Graph) immutable.Map[PackageID, PackagePath] {
+func computeWorkspacePackagesLocked(ctx context.Context, s *Snapshot, meta *metadata.Graph) immutable.Map[PackageID, PackagePath] {
+	// The provided context is used for reading snapshot files, which can only
+	// fail due to context cancellation. Don't let this happen as it could lead
+	// to inconsistent results.
+	ctx = xcontext.Detach(ctx)
 	workspacePackages := make(map[PackageID]PackagePath)
 	for _, mp := range meta.Packages {
-		if !isWorkspacePackageLocked(s, meta, mp) {
+		if !isWorkspacePackageLocked(ctx, s, meta, mp) {
 			continue
 		}
 

gopls/internal/cache/snapshot.go

@@ -1274,14 +1274,26 @@ func (s *Snapshot) ReadFile(ctx context.Context, uri protocol.DocumentURI) (file
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	fh, ok := s.files.get(uri)
+	return lockedSnapshot{s}.ReadFile(ctx, uri)
+}
+
+// lockedSnapshot implements the file.Source interface, while holding s.mu.
+//
+// TODO(rfindley): This unfortunate type had been eliminated, but it had to be
+// restored to fix golang/go#65801. We should endeavor to remove it again.
+type lockedSnapshot struct {
+	s *Snapshot
+}
+
+func (s lockedSnapshot) ReadFile(ctx context.Context, uri protocol.DocumentURI) (file.Handle, error) {
+	fh, ok := s.s.files.get(uri)
 	if !ok {
 		var err error
-		fh, err = s.view.fs.ReadFile(ctx, uri)
+		fh, err = s.s.view.fs.ReadFile(ctx, uri)
 		if err != nil {
 			return nil, err
 		}
-		s.files.set(uri, fh)
+		s.s.files.set(uri, fh)
 	}
 	return fh, nil
 }
@@ -2017,7 +2029,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f
 	// Update workspace and active packages, if necessary.
 	if result.meta != s.meta || anyFileOpenedOrClosed {
 		needsDiagnosis = true
-		result.workspacePackages = computeWorkspacePackagesLocked(result, result.meta)
+		result.workspacePackages = computeWorkspacePackagesLocked(ctx, result, result.meta)
 		result.resetActivePackagesLocked()
 	} else {
 		result.workspacePackages = s.workspacePackages

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

@@ -0,0 +1,73 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package workspace
+
+import (
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+
+	. "golang.org/x/tools/gopls/internal/test/integration"
+)
+
+func TestStdWorkspace(t *testing.T) {
+	// This test checks that we actually load workspace packages when opening
+	// GOROOT.
+	//
+	// In golang/go#65801, we failed to do this because go/packages returns nil
+	// Module for std and cmd.
+	//
+	// Because this test loads std as a workspace, it may be slow on smaller
+	// builders.
+	if testing.Short() {
+		t.Skip("skipping with -short: loads GOROOT")
+	}
+
+	// The test also fails on Windows because an absolute path does not match
+	// (likely a misspelling due to slashes).
+	// TODO(rfindley): investigate and fix this on windows.
+	if runtime.GOOS == "windows" {
+		t.Skip("skipping on windows: fails to to misspelled paths")
+	}
+
+	// Query GOROOT. This is slightly more precise than e.g. runtime.GOROOT, as
+	// it queries the go command in the environment.
+	goroot, err := exec.Command("go", "env", "GOROOT").Output()
+	if err != nil {
+		t.Fatal(err)
+	}
+	stdDir := filepath.Join(strings.TrimSpace(string(goroot)), "src")
+	WithOptions(
+		Modes(Default), // This test may be slow. No reason to run it multiple times.
+		WorkspaceFolders(stdDir),
+	).Run(t, "", func(t *testing.T, env *Env) {
+		// Find parser.ParseFile. Query with `'` to get an exact match.
+		syms := env.Symbol("'go/parser.ParseFile")
+		if len(syms) != 1 {
+			t.Fatalf("got %d symbols, want exactly 1. Symbols:\n%v", len(syms), syms)
+		}
+		parserPath := syms[0].Location.URI.Path()
+		env.OpenFile(parserPath)
+
+		// Find the reference to ast.File from the signature of ParseFile. This
+		// helps guard against matching a comment.
+		astFile := env.RegexpSearch(parserPath, `func ParseFile\(.*ast\.(File)`)
+		refs := env.References(astFile)
+
+		// If we've successfully loaded workspace packages for std, we should find
+		// a reference in go/types.
+		foundGoTypesReference := false
+		for _, ref := range refs {
+			if strings.Contains(string(ref.URI), "go/types") {
+				foundGoTypesReference = true
+			}
+		}
+		if !foundGoTypesReference {
+			t.Errorf("references(ast.File) did not return a go/types reference. Refs:\n%v", refs)
+		}
+	})
+}

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

@@ -7,9 +7,11 @@ package workspace
 import (
 	"context"
 	"fmt"
+	"sort"
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"golang.org/x/tools/gopls/internal/hooks"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/test/integration/fake"
@@ -1123,16 +1125,139 @@ import (
 		env.AfterChange(
 			Diagnostics(env.AtRegexp("a/main.go", "V"), WithMessage("not used")),
 		)
+		// Here, diagnostics are added because of zero-config gopls.
+		// In the past, they were added simply due to diagnosing changed files.
+		// (see TestClearNonWorkspaceDiagnostics_NoView below for a
+		// reimplementation of that test).
+		if got, want := len(env.Views()), 2; got != want {
+			t.Errorf("after opening a/main.go, got %d views, want %d", got, want)
+		}
 		env.CloseBuffer("a/main.go")
+		env.AfterChange(
+			NoDiagnostics(ForFile("a/main.go")),
+		)
+		if got, want := len(env.Views()), 1; got != want {
+			t.Errorf("after closing a/main.go, got %d views, want %d", got, want)
+		}
+	})
+}
+
+// This test is like TestClearNonWorkspaceDiagnostics, but bypasses the
+// zero-config algorithm by opening a nested workspace folder.
+//
+// We should still compute diagnostics correctly for open packages.
+func TestClearNonWorkspaceDiagnostics_NoView(t *testing.T) {
+	const ws = `
+-- a/go.mod --
+module example.com/a
+
+go 1.18
+
+require example.com/b v1.2.3
+
+replace example.com/b => ../b
+
+-- a/a.go --
+package a
+
+import "example.com/b"
+
+func _() {
+	V := b.B // unused
+}
+
+-- b/go.mod --
+module b
+
+go 1.18
+
+-- b/b.go --
+package b
+
+const B = 2
+
+func _() {
+	var V int // unused
+}
+
+-- b/b2.go --
+package b
+
+const B2 = B
+
+-- c/c.go --
+package main
+
+func main() {
+	var V int // unused
+}
+`
+	WithOptions(
+		WorkspaceFolders("a"),
+	).Run(t, ws, func(t *testing.T, env *Env) {
+		env.OpenFile("a/a.go")
+		env.AfterChange(
+			Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")),
+			NoDiagnostics(ForFile("b/b.go")),
+			NoDiagnostics(ForFile("c/c.go")),
+		)
+		env.OpenFile("b/b.go")
+		env.AfterChange(
+			Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")),
+			Diagnostics(env.AtRegexp("b/b.go", "V"), WithMessage("not used")),
+			NoDiagnostics(ForFile("c/c.go")),
+		)
 
-		// Make an arbitrary edit because gopls explicitly diagnoses a/main.go
-		// whenever it is "changed".
+		// Opening b/b.go should not result in a new view, because b is not
+		// contained in a workspace folder.
 		//
-		// TODO(rfindley): it should not be necessary to make another edit here.
-		// Gopls should be smart enough to avoid diagnosing a.
-		env.RegexpReplace("b/main.go", "package b", "package b // a package")
+		// Yet we should get diagnostics for b, because it is open.
+		if got, want := len(env.Views()), 1; got != want {
+			t.Errorf("after opening b/b.go, got %d views, want %d", got, want)
+		}
+		env.CloseBuffer("b/b.go")
 		env.AfterChange(
-			NoDiagnostics(ForFile("a/main.go")),
+			Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")),
+			NoDiagnostics(ForFile("b/b.go")),
+			NoDiagnostics(ForFile("c/c.go")),
+		)
+
+		// We should get references in the b package.
+		bUse := env.RegexpSearch("a/a.go", `b\.(B)`)
+		refs := env.References(bUse)
+		wantRefs := []string{"a/a.go", "b/b.go", "b/b2.go"}
+		var gotRefs []string
+		for _, ref := range refs {
+			gotRefs = append(gotRefs, env.Sandbox.Workdir.URIToPath(ref.URI))
+		}
+		sort.Strings(gotRefs)
+		if diff := cmp.Diff(wantRefs, gotRefs); diff != "" {
+			t.Errorf("references(b.B) mismatch (-want +got)\n%s", diff)
+		}
+
+		// Opening c/c.go should also not result in a new view, yet we should get
+		// orphaned file diagnostics.
+		env.OpenFile("c/c.go")
+		env.AfterChange(
+			Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")),
+			NoDiagnostics(ForFile("b/b.go")),
+			Diagnostics(env.AtRegexp("c/c.go", "V"), WithMessage("not used")),
+		)
+		if got, want := len(env.Views()), 1; got != want {
+			t.Errorf("after opening b/b.go, got %d views, want %d", got, want)
+		}
+
+		env.CloseBuffer("c/c.go")
+		env.AfterChange(
+			Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")),
+			NoDiagnostics(ForFile("b/b.go")),
+			NoDiagnostics(ForFile("c/c.go")),
+		)
+		env.CloseBuffer("a/a.go")
+		env.AfterChange(
+			Diagnostics(env.AtRegexp("a/a.go", "V"), WithMessage("not used")),
+			NoDiagnostics(ForFile("b/b.go")),
+			NoDiagnostics(ForFile("c/c.go")),
 		)
 	})
 }