gopls/internal/golang: improve "toggle compiler opt details"

This CL improves the usability of the "Toggle compiler optimization
details" code action by:

- making it work for _test.go files too (by running "go test -c"
  instead of "go build");
- changing the flag to be per directory, not per metadata.Package,
  as this led to confusing behaviour w.r.t. in-package test files.
- presenting specific "Show"/"Hide" command titles depending on
  the current state, instead of the vague "Toggle".

It includes a test for all the new functionality.

Also, document the "transitively error free" requirement.

Fixes golang/go#71106
Fixes golang/go#42812 (amusingly)

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

Author: adonovan

gopls/doc/features/diagnostics.md

@@ -65,9 +65,13 @@ There is an optional third source of diagnostics:
 
   This source is disabled by default but can be enabled on a
   package-by-package basis by invoking the
-  `source.toggleCompilerOptDetails` ("Toggle compiler optimization
+  `source.toggleCompilerOptDetails` ("{Show,Hide} compiler optimization
   details") code action.
 
+  Remember that the compiler's optimizer runs only on packages that
+  are transitively free from errors, so optimization diagnostics
+  will not be shown on packages that do not build.
+
 
 ## Recomputation of diagnostics
 

gopls/doc/release/v0.18.0.md

@@ -12,10 +12,10 @@
 
 # New features
 
-## "Toggle compiler optimization details" code action
+## "{Show,Hide} compiler optimization details" code action
 
 This code action, accessible through the "Source Action" menu in VS
-Code, toggles a per-package flag that causes Go compiler optimization
+Code, toggles a per-directory flag that causes Go compiler optimization
 details to be reported as diagnostics. For example, it indicates which
 variables escape to the heap, and which array accesses require bounds
 checks.

gopls/internal/cache/snapshot.go

@@ -183,9 +183,9 @@ type Snapshot struct {
 	// vulns maps each go.mod file's URI to its known vulnerabilities.
 	vulns *persistent.Map[protocol.DocumentURI, *vulncheck.Result]
 
-	// compilerOptDetails describes the packages for which we want
-	// compiler optimization details to be included in the diagnostics.
-	compilerOptDetails map[metadata.PackageID]unit
+	// compilerOptDetails is the set of directories whose packages
+	// and tests need compiler optimization details in the diagnostics.
+	compilerOptDetails map[protocol.DocumentURI]unit
 
 	// Concurrent type checking:
 	// typeCheckMu guards the ongoing type checking batch, and reference count of
@@ -1523,15 +1523,15 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f
 	// Compute the new set of packages for which we want compiler
 	// optimization details, after applying changed.CompilerOptDetails.
 	if len(s.compilerOptDetails) > 0 || len(changed.CompilerOptDetails) > 0 {
-		newCompilerOptDetails := make(map[metadata.PackageID]unit)
-		for id := range s.compilerOptDetails {
-			if _, ok := changed.CompilerOptDetails[id]; !ok {
-				newCompilerOptDetails[id] = unit{} // no change
+		newCompilerOptDetails := make(map[protocol.DocumentURI]unit)
+		for dir := range s.compilerOptDetails {
+			if _, ok := changed.CompilerOptDetails[dir]; !ok {
+				newCompilerOptDetails[dir] = unit{} // no change
 			}
 		}
-		for id, want := range changed.CompilerOptDetails {
+		for dir, want := range changed.CompilerOptDetails {
 			if want {
-				newCompilerOptDetails[id] = unit{}
+				newCompilerOptDetails[dir] = unit{}
 			}
 		}
 		if len(newCompilerOptDetails) > 0 {
@@ -2160,9 +2160,9 @@ func (s *Snapshot) setBuiltin(path string) {
 }
 
 // WantCompilerOptDetails reports whether to compute compiler
-// optimization details for the specified package.
-func (s *Snapshot) WantCompilerOptDetails(id metadata.PackageID) bool {
-	_, ok := s.compilerOptDetails[id]
+// optimization details for packages and tests in the given directory.
+func (s *Snapshot) WantCompilerOptDetails(dir protocol.DocumentURI) bool {
+	_, ok := s.compilerOptDetails[dir]
 	return ok
 }
 

gopls/internal/cache/view.go

@@ -27,7 +27,6 @@ import (
 	"sync"
 	"time"
 
-	"golang.org/x/tools/gopls/internal/cache/metadata"
 	"golang.org/x/tools/gopls/internal/cache/typerefs"
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/protocol"
@@ -745,7 +744,7 @@ type StateChange struct {
 	Files              map[protocol.DocumentURI]file.Handle
 	ModuleUpgrades     map[protocol.DocumentURI]map[string]string
 	Vulns              map[protocol.DocumentURI]*vulncheck.Result
-	CompilerOptDetails map[metadata.PackageID]bool // package -> whether or not we want details
+	CompilerOptDetails map[protocol.DocumentURI]bool // package directory -> whether or not we want details
 }
 
 // InvalidateView processes the provided state change, invalidating any derived

gopls/internal/cmd/integration_test.go

@@ -950,12 +950,12 @@ func TestCodeAction(t *testing.T) {
 module example.com
 go 1.18
 
--- a.go --
+-- a/a.go --
 package a
 type T int
 func f() (int, string) { return }
 
--- b.go --
+-- a/b.go --
 package a
 import "io"
 var _ io.Reader = C{}
@@ -970,14 +970,14 @@ type C struct{}
 	}
 	// list code actions in file
 	{
-		res := gopls(t, tree, "codeaction", "a.go")
+		res := gopls(t, tree, "codeaction", "a/a.go")
 		res.checkExit(true)
 		res.checkStdout(`edit	"Fill in return values" \[quickfix\]`)
 		res.checkStdout(`command	"Browse documentation for package a" \[source.doc\]`)
 	}
 	// list code actions in file, filtering by title
 	{
-		res := gopls(t, tree, "codeaction", "-title=Browse.*doc", "a.go")
+		res := gopls(t, tree, "codeaction", "-title=Browse.*doc", "a/a.go")
 		res.checkExit(true)
 		got := res.stdout
 		want := `command	"Browse gopls feature documentation" [gopls.doc.features]` +
@@ -990,26 +990,26 @@ type C struct{}
 	}
 	// list code actions in file, filtering (hierarchically) by kind
 	{
-		res := gopls(t, tree, "codeaction", "-kind=source", "a.go")
+		res := gopls(t, tree, "codeaction", "-kind=source", "a/a.go")
 		res.checkExit(true)
 		got := res.stdout
 		want := `command	"Browse documentation for package a" [source.doc]` +
 			"\n" +
-			`command	"Toggle compiler optimization details" [source.toggleCompilerOptDetails]` +
+			`command	"Show compiler optimization details for \"a\"" [source.toggleCompilerOptDetails]` +
 			"\n"
 		if got != want {
 			t.Errorf("codeaction: got <<%s>>, want <<%s>>\nstderr:\n%s", got, want, res.stderr)
 		}
 	}
 	// list code actions at position (of io.Reader)
 	{
-		res := gopls(t, tree, "codeaction", "b.go:#31")
+		res := gopls(t, tree, "codeaction", "a/b.go:#31")
 		res.checkExit(true)
 		res.checkStdout(`command	"Browse documentation for type io.Reader" \[source.doc]`)
 	}
 	// list quick fixes at position (of type T)
 	{
-		res := gopls(t, tree, "codeaction", "-kind=quickfix", "a.go:#15")
+		res := gopls(t, tree, "codeaction", "-kind=quickfix", "a/a.go:#15")
 		res.checkExit(true)
 		got := res.stdout
 		want := `edit	"Fill in return values" [quickfix]` + "\n"
@@ -1019,7 +1019,7 @@ type C struct{}
 	}
 	// success, with explicit CodeAction kind and diagnostics span.
 	{
-		res := gopls(t, tree, "codeaction", "-kind=quickfix", "-exec", "b.go:#40")
+		res := gopls(t, tree, "codeaction", "-kind=quickfix", "-exec", "a/b.go:#40")
 		res.checkExit(true)
 		got := res.stdout
 		want := `

gopls/internal/golang/codeaction.go

@@ -11,6 +11,7 @@ import (
 	"go/ast"
 	"go/token"
 	"go/types"
+	"path/filepath"
 	"reflect"
 	"slices"
 	"sort"
@@ -105,6 +106,8 @@ func CodeActions(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle,
 			req.pkg = nil
 		}
 		if err := p.fn(ctx, req); err != nil {
+			// TODO(adonovan): most errors in code action providers should
+			// not block other providers; see https://go.dev/issue/71275.
 			return nil, err
 		}
 	}
@@ -879,10 +882,22 @@ func goAssembly(ctx context.Context, req *codeActionsRequest) error {
 	return nil
 }
 
-// toggleCompilerOptDetails produces "Toggle compiler optimization details" code action.
-// See [server.commandHandler.ToggleCompilerOptDetails] for command implementation.
+// toggleCompilerOptDetails produces "{Show,Hide} compiler optimization details" code action.
+// See [server.commandHandler.GCDetails] for command implementation.
 func toggleCompilerOptDetails(ctx context.Context, req *codeActionsRequest) error {
-	cmd := command.NewGCDetailsCommand("Toggle compiler optimization details", req.fh.URI())
-	req.addCommandAction(cmd, false)
+	// TODO(adonovan): errors from code action providers should probably be
+	// logged, even if they aren't visible to the client; see https://go.dev/issue/71275.
+	if meta, err := NarrowestMetadataForFile(ctx, req.snapshot, req.fh.URI()); err == nil {
+		if len(meta.CompiledGoFiles) == 0 {
+			return fmt.Errorf("package %q does not compile file %q", meta.ID, req.fh.URI())
+		}
+		dir := meta.CompiledGoFiles[0].Dir()
+
+		title := fmt.Sprintf("%s compiler optimization details for %q",
+			cond(req.snapshot.WantCompilerOptDetails(dir), "Hide", "Show"),
+			filepath.Base(dir.Path()))
+		cmd := command.NewGCDetailsCommand(title, req.fh.URI())
+		req.addCommandAction(cmd, false)
+	}
 	return nil
 }

gopls/internal/golang/compileropt.go

@@ -11,22 +11,19 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 
 	"golang.org/x/tools/gopls/internal/cache"
-	"golang.org/x/tools/gopls/internal/cache/metadata"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/internal/event"
 )
 
 // CompilerOptDetails invokes the Go compiler with the "-json=0,dir"
-// flag on the specified package, parses its log of optimization
-// decisions, and returns them as a set of diagnostics.
-func CompilerOptDetails(ctx context.Context, snapshot *cache.Snapshot, mp *metadata.Package) (map[protocol.DocumentURI][]*cache.Diagnostic, error) {
-	if len(mp.CompiledGoFiles) == 0 {
-		return nil, nil
-	}
-	pkgDir := mp.CompiledGoFiles[0].DirPath()
+// flag on the packages and tests in the specified directory, parses
+// its log of optimization decisions, and returns them as a set of
+// diagnostics.
+func CompilerOptDetails(ctx context.Context, snapshot *cache.Snapshot, pkgDir protocol.DocumentURI) (map[protocol.DocumentURI][]*cache.Diagnostic, error) {
 	outDir, err := os.MkdirTemp("", fmt.Sprintf("gopls-%d.details", os.Getpid()))
 	if err != nil {
 		return nil, err
@@ -37,22 +34,20 @@ func CompilerOptDetails(ctx context.Context, snapshot *cache.Snapshot, mp *metad
 		}
 	}()
 
-	tmpFile, err := os.CreateTemp(os.TempDir(), "gopls-x")
-	if err != nil {
-		return nil, err
-	}
-	tmpFile.Close() // ignore error
-	defer os.Remove(tmpFile.Name())
-
 	outDirURI := protocol.URIFromPath(outDir)
 	// details doesn't handle Windows URIs in the form of "file:///C:/...",
 	// so rewrite them to "file://C:/...". See golang/go#41614.
 	if !strings.HasPrefix(outDir, "/") {
 		outDirURI = protocol.DocumentURI(strings.Replace(string(outDirURI), "file:///", "file://", 1))
 	}
-	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(cache.NoNetwork, pkgDir, "build", []string{
+
+	// We use "go test -c" not "go build" as it covers all three packages
+	// (p, "p [p.test]", "p_test [p.test]") in the directory, if they exist.
+	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(cache.NoNetwork, pkgDir.Path(), "test", []string{
+		"-c",
+		"-vet=off", // weirdly -c doesn't disable vet
 		fmt.Sprintf("-gcflags=-json=0,%s", outDirURI), // JSON schema version 0
-		fmt.Sprintf("-o=%s", tmpFile.Name()),
+		fmt.Sprintf("-o=%s", cond(runtime.GOOS == "windows", "NUL", "/dev/null")),
 		".",
 	})
 	if err != nil {
@@ -79,7 +74,8 @@ func CompilerOptDetails(ctx context.Context, snapshot *cache.Snapshot, mp *metad
 		if fh == nil {
 			continue
 		}
-		if pkgDir != fh.URI().DirPath() {
+		if pkgDir != fh.URI().Dir() {
+			// Filter compiler diagnostics to the requested directory.
 			// https://github.com/golang/go/issues/42198
 			// sometimes the detail diagnostics generated for files
 			// outside the package can never be taken back.

gopls/internal/server/command.go

@@ -1017,19 +1017,22 @@ func (s *server) getUpgrades(ctx context.Context, snapshot *cache.Snapshot, uri
 
 func (c *commandHandler) GCDetails(ctx context.Context, uri protocol.DocumentURI) error {
 	return c.run(ctx, commandConfig{
-		progress: "Toggling display of compiler optimization details",
-		forURI:   uri,
+		forURI: uri,
 	}, func(ctx context.Context, deps commandDeps) error {
 		return c.modifyState(ctx, FromToggleCompilerOptDetails, func() (*cache.Snapshot, func(), error) {
+			// Don't blindly use "dir := deps.fh.URI().Dir()"; validate.
 			meta, err := golang.NarrowestMetadataForFile(ctx, deps.snapshot, deps.fh.URI())
 			if err != nil {
 				return nil, nil, err
 			}
-			want := !deps.snapshot.WantCompilerOptDetails(meta.ID) // toggle per-package flag
+			if len(meta.CompiledGoFiles) == 0 {
+				return nil, nil, fmt.Errorf("package %q does not compile file %q", meta.ID, deps.fh.URI())
+			}
+			dir := meta.CompiledGoFiles[0].Dir()
+
+			want := !deps.snapshot.WantCompilerOptDetails(dir) // toggle per-directory flag
 			return c.s.session.InvalidateView(ctx, deps.snapshot.View(), cache.StateChange{
-				CompilerOptDetails: map[metadata.PackageID]bool{
-					meta.ID: want,
-				},
+				CompilerOptDetails: map[protocol.DocumentURI]bool{dir: want},
 			})
 		})
 	})

gopls/internal/server/diagnostics.go

@@ -561,25 +561,26 @@ func (s *server) compilerOptDetailsDiagnostics(ctx context.Context, snapshot *ca
 	// TODO(rfindley): This should memoize its results if the package has not changed.
 	// Consider that these points, in combination with the note below about
 	// races, suggest that compiler optimization details should be tracked on the Snapshot.
-	var detailPkgs map[metadata.PackageID]*metadata.Package
-	for _, mp := range toDiagnose {
-		if snapshot.WantCompilerOptDetails(mp.ID) {
-			if detailPkgs == nil {
-				detailPkgs = make(map[metadata.PackageID]*metadata.Package)
-			}
-			detailPkgs[mp.ID] = mp
-		}
-	}
-
 	diagnostics := make(diagMap)
-	for _, mp := range detailPkgs {
-		perFileDiags, err := golang.CompilerOptDetails(ctx, snapshot, mp)
-		if err != nil {
-			event.Error(ctx, "warning: compiler optimization details", err, append(snapshot.Labels(), label.Package.Of(string(mp.ID)))...)
+	seenDirs := make(map[protocol.DocumentURI]bool)
+	for _, mp := range toDiagnose {
+		if len(mp.CompiledGoFiles) == 0 {
 			continue
 		}
-		for uri, diags := range perFileDiags {
-			diagnostics[uri] = append(diagnostics[uri], diags...)
+		dir := mp.CompiledGoFiles[0].Dir()
+		if snapshot.WantCompilerOptDetails(dir) {
+			if !seenDirs[dir] {
+				seenDirs[dir] = true
+
+				perFileDiags, err := golang.CompilerOptDetails(ctx, snapshot, dir)
+				if err != nil {
+					event.Error(ctx, "warning: compiler optimization details", err, append(snapshot.Labels(), label.URI.Of(dir))...)
+					continue
+				}
+				for uri, diags := range perFileDiags {
+					diagnostics[uri] = append(diagnostics[uri], diags...)
+				}
+			}
 		}
 	}
 	return diagnostics, nil