gopls/internal/golang: make GCDetails a code action, not a code lens

Before, GCDetails was a code lens source that annotated the package
declaration of each file with a command. The command, gopls.gc_details,
toggles the per-package flag that controls whether Go compiler
optimization details are reported as diagnostics.

Code lenses are poorly supported across editors, and this code lens
was disabled by default (presumably because VS Code has an alternative
custom client-side command to toggle the flag).

This change changes the command from being a code lens to code action,
offered for any selection in the file; the mechanism of toggling the
flag (the gopls.gc_details command) is unchanged, though the redundant
ToggleGCDetails command has been eliminated.

This change should make it easier for users in a range of
editors to both discover and use this useful feature.

The new nomenclature avoids "GC" (the old name for the Go compiler,
easily confused with "garbage collector"), and instead uses
"Toggle compiler optimization details" consistently.
(The one exception is the string value of the command, whose name must
not be changed since it would break the VS Code client side logic.)

+ Test, relnote, docs

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

Author: adonovan

gopls/doc/codelenses.md

@@ -23,30 +23,6 @@ Client support:
 
 <!-- This portion is generated by doc/generate from the ../internal/settings package. -->
 <!-- BEGIN Lenses: DO NOT MANUALLY EDIT THIS SECTION -->
-## `gc_details`: Toggle display of Go compiler optimization decisions
-
-
-This codelens source causes the `package` declaration of
-each file to be annotated with a command to toggle the
-state of the per-session variable that controls whether
-optimization decisions from the Go compiler (formerly known
-as "gc") should be displayed as diagnostics.
-
-Optimization decisions include:
-- whether a variable escapes, and how escape is inferred;
-- whether a nil-pointer check is implied or eliminated;
-- whether a function can be inlined.
-
-TODO(adonovan): this source is off by default because the
-annotation is annoying and because VS Code has a separate
-"Toggle gc details" command. Replace it with a Code Action
-("Source action...").
-
-
-Default: off
-
-File type: Go
-
 ## `generate`: Run `go generate`
 
 

gopls/doc/features/diagnostics.md

@@ -49,6 +49,26 @@ build`. Gopls doesn't actually run the compiler; that would be too
   The example above shows a `printf` formatting mistake. The diagnostic contains
   a link to the documentation for the `printf` analyzer.
 
+There is an optional third source of diagnostics:
+
+<a id='toggleCompilerOptDetails'/>
+
+- **Compiler optimization details** are diagnostics that report
+  details relevant to optimization decisions made by the Go
+  compiler, such as whether a variable escapes or a slice index
+  requires a bounds check.
+
+  Optimization decisions include:
+  whether a variable escapes, and how escape is inferred;
+  whether a nil-pointer check is implied or eliminated; and
+  whether a function can be inlined.
+
+  This source is disabled by default but can be enabled on a
+  package-by-package basis by invoking the
+  `source.toggleCompilerOptDetails` ("Toggle compiler optimization
+  details") code action.
+
+
 ## Recomputation of diagnostics
 
 By default, diagnostics are automatically recomputed each time the source files

gopls/doc/features/transformation.md

@@ -70,6 +70,7 @@ Gopls supports the following code actions:
 - [`source.freesymbols`](web.md#freesymbols)
 - `source.test` (undocumented) <!-- TODO: fix that -->
 - [`source.addTest`](#source.addTest)
+- [`source.toggleCompilerOptDetails`](diagnostics.md#toggleCompilerOptDetails)
 - [`gopls.doc.features`](README.md), which opens gopls' index of features in a browser
 - [`refactor.extract.constant`](#extract)
 - [`refactor.extract.function`](#extract)

gopls/doc/release/v0.18.0.md

@@ -2,8 +2,24 @@
 
 - The experimental `hoverKind=Structured` setting is no longer supported.
 
+- The `gc_details` code lens has been deleted. (It was previously
+  disabled by default.) This functionality is now available through
+  the `settings.toggleCompilerOptDetails` code action (documented
+  below), as code actions are better supported than code lenses across
+  a range of clients.
+
+  VS Code's special "Go: Toggle GC details" command continues to work.
+
 # New features
 
+## "Toggle 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
+details to be reported as diagnostics. For example, it indicates which
+variables escape to the heap, and which array accesses require bounds
+checks.
+
 ## New `modernize` analyzer
 
 Gopls will now report when code could be simplified or clarified by

gopls/doc/settings.md

@@ -184,13 +184,12 @@ Example Usage:
 ...
   "codelenses": {
     "generate": false,  // Don't show the `go generate` lens.
-    "gc_details": true  // Show a code lens toggling the display of gc's choices.
   }
 ...
 }
 ```
 
-Default: `{"gc_details":false,"generate":true,"regenerate_cgo":true,"run_govulncheck":false,"tidy":true,"upgrade_dependency":true,"vendor":true}`.
+Default: `{"generate":true,"regenerate_cgo":true,"run_govulncheck":false,"tidy":true,"upgrade_dependency":true,"vendor":true}`.
 
 <a id='semanticTokens'></a>
 ### `semanticTokens bool`
@@ -321,8 +320,10 @@ Default: `false`.
 
 **This setting is experimental and may be deleted.**
 
-annotations specifies the various kinds of optimization diagnostics
-that should be reported by the gc_details command.
+annotations specifies the various kinds of compiler
+optimization details that should be reported as diagnostics
+when enabled for a package by the "Toggle compiler
+optimization details" (`gopls.gc_details`) command.
 
 Each enum must be one of:
 

gopls/internal/cache/diagnostics.go

@@ -95,21 +95,24 @@ func (d *Diagnostic) Hash() file.Hash {
 	return hash
 }
 
+// A DiagnosticSource identifies the source of a diagnostic.
+//
+// Its value may be one of the distinguished string values below, or
+// the Name of an [analysis.Analyzer].
 type DiagnosticSource string
 
 const (
-	UnknownError             DiagnosticSource = "<Unknown source>"
-	ListError                DiagnosticSource = "go list"
-	ParseError               DiagnosticSource = "syntax"
-	TypeError                DiagnosticSource = "compiler"
-	ModTidyError             DiagnosticSource = "go mod tidy"
-	OptimizationDetailsError DiagnosticSource = "optimizer details"
-	UpgradeNotification      DiagnosticSource = "upgrade available"
-	Vulncheck                DiagnosticSource = "vulncheck imports"
-	Govulncheck              DiagnosticSource = "govulncheck"
-	TemplateError            DiagnosticSource = "template"
-	WorkFileError            DiagnosticSource = "go.work file"
-	ConsistencyInfo          DiagnosticSource = "consistency"
+	UnknownError           DiagnosticSource = "<Unknown source>"
+	ListError              DiagnosticSource = "go list"
+	ParseError             DiagnosticSource = "syntax"
+	TypeError              DiagnosticSource = "compiler"
+	ModTidyError           DiagnosticSource = "go mod tidy"
+	CompilerOptDetailsInfo DiagnosticSource = "optimizer details" // cmd/compile -json=0,dir
+	UpgradeNotification    DiagnosticSource = "upgrade available"
+	Vulncheck              DiagnosticSource = "vulncheck imports"
+	Govulncheck            DiagnosticSource = "govulncheck"
+	TemplateError          DiagnosticSource = "template"
+	WorkFileError          DiagnosticSource = "go.work file"
 )
 
 // A SuggestedFix represents a suggested fix (for a diagnostic)

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]
 
-	// gcOptimizationDetails describes the packages for which we want
-	// optimization details to be included in the diagnostics.
-	gcOptimizationDetails map[metadata.PackageID]unit
+	// compilerOptDetails describes the packages for which we want
+	// compiler optimization details to be included in the diagnostics.
+	compilerOptDetails map[metadata.PackageID]unit
 
 	// Concurrent type checking:
 	// typeCheckMu guards the ongoing type checking batch, and reference count of
@@ -1492,7 +1492,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f
 
 	// TODO(rfindley): reorganize this function to make the derivation of
 	// needsDiagnosis clearer.
-	needsDiagnosis := len(changed.GCDetails) > 0 || len(changed.ModuleUpgrades) > 0 || len(changed.Vulns) > 0
+	needsDiagnosis := len(changed.CompilerOptDetails) > 0 || len(changed.ModuleUpgrades) > 0 || len(changed.Vulns) > 0
 
 	bgCtx, cancel := context.WithCancel(bgCtx)
 	result := &Snapshot{
@@ -1522,22 +1522,22 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f
 		vulns:             cloneWith(s.vulns, changed.Vulns),
 	}
 
-	// Compute the new set of packages for which we want gc details, after
-	// applying changed.GCDetails.
-	if len(s.gcOptimizationDetails) > 0 || len(changed.GCDetails) > 0 {
-		newGCDetails := make(map[metadata.PackageID]unit)
-		for id := range s.gcOptimizationDetails {
-			if _, ok := changed.GCDetails[id]; !ok {
-				newGCDetails[id] = unit{} // no change
+	// 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
 			}
 		}
-		for id, want := range changed.GCDetails {
+		for id, want := range changed.CompilerOptDetails {
 			if want {
-				newGCDetails[id] = unit{}
+				newCompilerOptDetails[id] = unit{}
 			}
 		}
-		if len(newGCDetails) > 0 {
-			result.gcOptimizationDetails = newGCDetails
+		if len(newCompilerOptDetails) > 0 {
+			result.compilerOptDetails = newCompilerOptDetails
 		}
 	}
 
@@ -2161,10 +2161,10 @@ func (s *Snapshot) setBuiltin(path string) {
 	s.builtin = protocol.URIFromPath(path)
 }
 
-// WantGCDetails reports whether to compute GC optimization details for the
-// specified package.
-func (s *Snapshot) WantGCDetails(id metadata.PackageID) bool {
-	_, ok := s.gcOptimizationDetails[id]
+// WantCompilerOptDetails reports whether to compute compiler
+// optimization details for the specified package.
+func (s *Snapshot) WantCompilerOptDetails(id metadata.PackageID) bool {
+	_, ok := s.compilerOptDetails[id]
 	return ok
 }
 

gopls/internal/cache/view.go

@@ -736,11 +736,11 @@ func (s *Snapshot) initialize(ctx context.Context, firstAttempt bool) {
 // By far the most common of these is a change to file state, but a query of
 // module upgrade information or vulnerabilities also affects gopls' behavior.
 type StateChange struct {
-	Modifications  []file.Modification // if set, the raw modifications originating this change
-	Files          map[protocol.DocumentURI]file.Handle
-	ModuleUpgrades map[protocol.DocumentURI]map[string]string
-	Vulns          map[protocol.DocumentURI]*vulncheck.Result
-	GCDetails      map[metadata.PackageID]bool // package -> whether or not we want details
+	Modifications      []file.Modification // if set, the raw modifications originating this change
+	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
 }
 
 // InvalidateView processes the provided state change, invalidating any derived

gopls/internal/cmd/integration_test.go

@@ -994,6 +994,8 @@ type C struct{}
 		res.checkExit(true)
 		got := res.stdout
 		want := `command	"Browse documentation for package a" [source.doc]` +
+			"\n" +
+			`command	"Toggle compiler optimization details" [source.toggleCompilerOptDetails]` +
 			"\n"
 		if got != want {
 			t.Errorf("codeaction: got <<%s>>, want <<%s>>\nstderr:\n%s", got, want, res.stderr)

gopls/internal/doc/api.json

@@ -638,7 +638,7 @@
 			{
 				"Name": "annotations",
 				"Type": "map[enum]bool",
-				"Doc": "annotations specifies the various kinds of optimization diagnostics\nthat should be reported by the gc_details command.\n",
+				"Doc": "annotations specifies the various kinds of compiler\noptimization details that should be reported as diagnostics\nwhen enabled for a package by the \"Toggle compiler\noptimization details\" (`gopls.gc_details`) command.\n",
 				"EnumKeys": {
 					"ValueType": "bool",
 					"Keys": [
@@ -791,15 +791,10 @@
 			{
 				"Name": "codelenses",
 				"Type": "map[enum]bool",
-				"Doc": "codelenses overrides the enabled/disabled state of each of gopls'\nsources of [Code Lenses](codelenses.md).\n\nExample Usage:\n\n```json5\n\"gopls\": {\n...\n  \"codelenses\": {\n    \"generate\": false,  // Don't show the `go generate` lens.\n    \"gc_details\": true  // Show a code lens toggling the display of gc's choices.\n  }\n...\n}\n```\n",
+				"Doc": "codelenses overrides the enabled/disabled state of each of gopls'\nsources of [Code Lenses](codelenses.md).\n\nExample Usage:\n\n```json5\n\"gopls\": {\n...\n  \"codelenses\": {\n    \"generate\": false,  // Don't show the `go generate` lens.\n  }\n...\n}\n```\n",
 				"EnumKeys": {
 					"ValueType": "bool",
 					"Keys": [
-						{
-							"Name": "\"gc_details\"",
-							"Doc": "`\"gc_details\"`: Toggle display of Go compiler optimization decisions\n\nThis codelens source causes the `package` declaration of\neach file to be annotated with a command to toggle the\nstate of the per-session variable that controls whether\noptimization decisions from the Go compiler (formerly known\nas \"gc\") should be displayed as diagnostics.\n\nOptimization decisions include:\n- whether a variable escapes, and how escape is inferred;\n- whether a nil-pointer check is implied or eliminated;\n- whether a function can be inlined.\n\nTODO(adonovan): this source is off by default because the\nannotation is annoying and because VS Code has a separate\n\"Toggle gc details\" command. Replace it with a Code Action\n(\"Source action...\").\n",
-							"Default": "false"
-						},
 						{
 							"Name": "\"generate\"",
 							"Doc": "`\"generate\"`: Run `go generate`\n\nThis codelens source annotates any `//go:generate` comments\nwith commands to run `go generate` in this directory, on\nall directories recursively beneath this one.\n\nSee [Generating code](https://go.dev/blog/generate) for\nmore details.\n",
@@ -843,7 +838,7 @@
 					]
 				},
 				"EnumValues": null,
-				"Default": "{\"gc_details\":false,\"generate\":true,\"regenerate_cgo\":true,\"run_govulncheck\":false,\"tidy\":true,\"upgrade_dependency\":true,\"vendor\":true}",
+				"Default": "{\"generate\":true,\"regenerate_cgo\":true,\"run_govulncheck\":false,\"tidy\":true,\"upgrade_dependency\":true,\"vendor\":true}",
 				"Status": "",
 				"Hierarchy": "ui"
 			},
@@ -928,13 +923,6 @@
 		]
 	},
 	"Lenses": [
-		{
-			"FileType": "Go",
-			"Lens": "gc_details",
-			"Title": "Toggle display of Go compiler optimization decisions",
-			"Doc": "\nThis codelens source causes the `package` declaration of\neach file to be annotated with a command to toggle the\nstate of the per-session variable that controls whether\noptimization decisions from the Go compiler (formerly known\nas \"gc\") should be displayed as diagnostics.\n\nOptimization decisions include:\n- whether a variable escapes, and how escape is inferred;\n- whether a nil-pointer check is implied or eliminated;\n- whether a function can be inlined.\n\nTODO(adonovan): this source is off by default because the\nannotation is annoying and because VS Code has a separate\n\"Toggle gc details\" command. Replace it with a Code Action\n(\"Source action...\").\n",
-			"Default": false
-		},
 		{
 			"FileType": "Go",
 			"Lens": "generate",

gopls/internal/golang/code_lens.go

@@ -23,10 +23,9 @@ import (
 // CodeLensSources returns the supported sources of code lenses for Go files.
 func CodeLensSources() map[settings.CodeLensSource]cache.CodeLensSourceFunc {
 	return map[settings.CodeLensSource]cache.CodeLensSourceFunc{
-		settings.CodeLensGenerate:      goGenerateCodeLens,    // commands: Generate
-		settings.CodeLensTest:          runTestCodeLens,       // commands: Test
-		settings.CodeLensRegenerateCgo: regenerateCgoLens,     // commands: RegenerateCgo
-		settings.CodeLensGCDetails:     toggleDetailsCodeLens, // commands: GCDetails
+		settings.CodeLensGenerate:      goGenerateCodeLens, // commands: Generate
+		settings.CodeLensTest:          runTestCodeLens,    // commands: Test
+		settings.CodeLensRegenerateCgo: regenerateCgoLens,  // commands: RegenerateCgo
 	}
 }
 
@@ -196,21 +195,3 @@ func regenerateCgoLens(ctx context.Context, snapshot *cache.Snapshot, fh file.Ha
 	cmd := command.NewRegenerateCgoCommand("regenerate cgo definitions", command.URIArg{URI: puri})
 	return []protocol.CodeLens{{Range: rng, Command: cmd}}, nil
 }
-
-func toggleDetailsCodeLens(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle) ([]protocol.CodeLens, error) {
-	pgf, err := snapshot.ParseGo(ctx, fh, parsego.Full)
-	if err != nil {
-		return nil, err
-	}
-	if !pgf.File.Package.IsValid() {
-		// Without a package name we have nowhere to put the codelens, so give up.
-		return nil, nil
-	}
-	rng, err := pgf.PosRange(pgf.File.Package, pgf.File.Package)
-	if err != nil {
-		return nil, err
-	}
-	puri := fh.URI()
-	cmd := command.NewGCDetailsCommand("Toggle gc annotation details", puri)
-	return []protocol.CodeLens{{Range: rng, Command: cmd}}, nil
-}

gopls/internal/golang/codeaction.go

@@ -231,6 +231,7 @@ var codeActionProducers = [...]codeActionProducer{
 	{kind: settings.GoDoc, fn: goDoc, needPkg: true},
 	{kind: settings.GoFreeSymbols, fn: goFreeSymbols},
 	{kind: settings.GoTest, fn: goTest},
+	{kind: settings.GoToggleCompilerOptDetails, fn: toggleCompilerOptDetails},
 	{kind: settings.GoplsDocFeatures, fn: goplsDocFeatures},
 	{kind: settings.RefactorExtractFunction, fn: refactorExtractFunction},
 	{kind: settings.RefactorExtractMethod, fn: refactorExtractMethod},
@@ -871,3 +872,11 @@ 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.
+func toggleCompilerOptDetails(ctx context.Context, req *codeActionsRequest) error {
+	cmd := command.NewGCDetailsCommand("Toggle compiler optimization details", req.fh.URI())
+	req.addCommandAction(cmd, false)
+	return nil
+}