gopls/internal/server: avoid VS Code lightbulb

VS Code has a complex and undocumented logic for presenting
Code Actions of various kinds in the user interface.
This CL documents the empirically observed behavior at
CodeActionKind.

Previously, users found that "nearly always available"
code actions such as "Inline call to f" were a distracting
source of lightbulb icons in the UI. This change suppresses
non-diagnostic-associated Code Actions (such as "Inline call")
when the CodeAction request does not have TriggerKind=Invoked.
(Invoked means the CodeAction request was caused by opening
a menu, as opposed to mere cursor motion.)

Also, rename BundleQuickFixes et al using "lazy" instead
of "quick" as QuickFix has a different special meaning
and lazy fixes do not necesarily have kind "quickfix"
(though all currently do).

Fixes golang/go#65167
Update golang/go#40438

Change-Id: I83563e1bb476e56a8404443d7e48b7c240bfa2e0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/587555
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/internal/cache/check.go

@@ -1770,7 +1770,7 @@ func depsErrors(ctx context.Context, snapshot *Snapshot, mp *metadata.Package) (
 					Message:        fmt.Sprintf("error while importing %v: %v", item, depErr.Err),
 					SuggestedFixes: goGetQuickFixes(mp.Module != nil, imp.cgf.URI, item),
 				}
-				if !bundleQuickFixes(diag) {
+				if !bundleLazyFixes(diag) {
 					bug.Reportf("failed to bundle fixes for diagnostic %q", diag.Message)
 				}
 				errors = append(errors, diag)
@@ -1813,7 +1813,7 @@ func depsErrors(ctx context.Context, snapshot *Snapshot, mp *metadata.Package) (
 				Message:        fmt.Sprintf("error while importing %v: %v", item, depErr.Err),
 				SuggestedFixes: goGetQuickFixes(true, pm.URI, item),
 			}
-			if !bundleQuickFixes(diag) {
+			if !bundleLazyFixes(diag) {
 				bug.Reportf("failed to bundle fixes for diagnostic %q", diag.Message)
 			}
 			errors = append(errors, diag)

gopls/internal/cache/diagnostics.go

@@ -49,13 +49,13 @@ type Diagnostic struct {
 	Tags    []protocol.DiagnosticTag
 	Related []protocol.DiagnosticRelatedInformation
 
-	// Fields below are used internally to generate quick fixes. They aren't
+	// Fields below are used internally to generate lazy fixes. They aren't
 	// part of the LSP spec and historically didn't leave the server.
 	//
 	// Update(2023-05): version 3.16 of the LSP spec included support for the
 	// Diagnostic.data field, which holds arbitrary data preserved in the
 	// diagnostic for codeAction requests. This field allows bundling additional
-	// information for quick-fixes, and gopls can (and should) use this
+	// information for lazy fixes, and gopls can (and should) use this
 	// information to avoid re-evaluating diagnostics in code-action handlers.
 	//
 	// In order to stage this transition incrementally, the 'BundledFixes' field
@@ -111,18 +111,20 @@ func SuggestedFixFromCommand(cmd protocol.Command, kind protocol.CodeActionKind)
 	}
 }
 
-// quickFixesJSON is a JSON-serializable list of quick fixes
-// to be saved in the protocol.Diagnostic.Data field.
-type quickFixesJSON struct {
+// lazyFixesJSON is a JSON-serializable list of code actions (arising
+// from "lazy" SuggestedFixes with no Edits) to be saved in the
+// protocol.Diagnostic.Data field. Computation of the edits is thus
+// deferred until the action's command is invoked.
+type lazyFixesJSON struct {
 	// TODO(rfindley): pack some sort of identifier here for later
 	// lookup/validation?
-	Fixes []protocol.CodeAction
+	Actions []protocol.CodeAction
 }
 
-// bundleQuickFixes attempts to bundle sd.SuggestedFixes into the
+// bundleLazyFixes attempts to bundle sd.SuggestedFixes into the
 // sd.BundledFixes field, so that it can be round-tripped through the client.
-// It returns false if the quick-fixes cannot be bundled.
-func bundleQuickFixes(sd *Diagnostic) bool {
+// It returns false if the fixes cannot be bundled.
+func bundleLazyFixes(sd *Diagnostic) bool {
 	if len(sd.SuggestedFixes) == 0 {
 		return true
 	}
@@ -148,34 +150,34 @@ func bundleQuickFixes(sd *Diagnostic) bool {
 		}
 		actions = append(actions, action)
 	}
-	fixes := quickFixesJSON{
-		Fixes: actions,
+	fixes := lazyFixesJSON{
+		Actions: actions,
 	}
 	data, err := json.Marshal(fixes)
 	if err != nil {
-		bug.Reportf("marshalling quick fixes: %v", err)
+		bug.Reportf("marshalling lazy fixes: %v", err)
 		return false
 	}
 	msg := json.RawMessage(data)
 	sd.BundledFixes = &msg
 	return true
 }
 
-// BundledQuickFixes extracts any bundled codeActions from the
+// BundledLazyFixes extracts any bundled codeActions from the
 // diag.Data field.
-func BundledQuickFixes(diag protocol.Diagnostic) []protocol.CodeAction {
-	var fix quickFixesJSON
+func BundledLazyFixes(diag protocol.Diagnostic) []protocol.CodeAction {
+	var fix lazyFixesJSON
 	if diag.Data != nil {
 		err := protocol.UnmarshalJSON(*diag.Data, &fix)
 		if err != nil {
-			bug.Reportf("unmarshalling quick fix: %v", err)
+			bug.Reportf("unmarshalling lazy fix: %v", err)
 			return nil
 		}
 	}
 
 	var actions []protocol.CodeAction
-	for _, action := range fix.Fixes {
-		// See BundleQuickFixes: for now we only support bundling commands.
+	for _, action := range fix.Actions {
+		// See bundleLazyFixes: for now we only support bundling commands.
 		if action.Edit != nil {
 			bug.Reportf("bundled fix %q includes workspace edits", action.Title)
 			continue

gopls/internal/cache/snapshot.go

@@ -1547,7 +1547,7 @@ https://github.com/golang/tools/blob/master/gopls/doc/settings.md#buildflags.`
 				Message:        msg,
 				SuggestedFixes: suggestedFixes,
 			}
-			if ok := bundleQuickFixes(d); !ok {
+			if ok := bundleLazyFixes(d); !ok {
 				bug.Reportf("failed to bundle quick fixes for %v", d)
 			}
 			// Only report diagnostics if we detect an actual exclusion.

gopls/internal/protocol/codeactionkind.go

@@ -20,6 +20,59 @@ package protocol
 //	"source.organizeImports"
 //	"source.fixAll"
 //	"notebook"
+//
+// The effects of CodeActionKind on the behavior of VS Code are
+// baffling and undocumented. Here's what we have observed.
+//
+// Clicking on the "Refactor..." menu item shows a submenu of actions
+// with kind="refactor.*", and clicking on "Source action..." shows
+// actions with kind="source.*". A lightbulb appears in both cases.
+// A third menu, "Quick fix...", not found on the usual context
+// menu but accessible through the command palette or "⌘.",
+// displays code actions of kind "quickfix.*" and "refactor.*".
+// All of these CodeAction requests have triggerkind=Invoked.
+//
+// Cursor motion also performs a CodeAction request, but with
+// triggerkind=Automatic. Even if this returns a mix of action kinds,
+// only the "refactor" and "quickfix" actions seem to matter.
+// A lightbulb appears if that subset of actions is non-empty, and the
+// menu displays them. (This was noisy--see #65167--so gopls now only
+// reports diagnostic-associated code actions if kind is Invoked or
+// missing.)
+//
+// None of these CodeAction requests specifies a "kind" restriction;
+// the filtering is done on the response, by the client.
+//
+// In all these menus, VS Code organizes the actions' menu items
+// into groups based on their kind, with hardwired captions such as
+// "Extract", "Inline", "More actions", and "Quick fix".
+//
+// The special category "source.fixAll" is intended for actions that
+// are unambiguously safe to apply so that clients may automatically
+// apply all actions matching this category on save. (That said, this
+// is not VS Code's default behavior; see editor.codeActionsOnSave.)
+//
+// TODO(adonovan): the intent of CodeActionKind is a hierarchy. We
+// should changes gopls so that we don't create instances of the
+// predefined kinds directly, but treat them as interfaces.
+//
+// For example,
+//
+//	instead of:		we should create:
+//	refactor.extract	refactor.extract.const
+//				refactor.extract.var
+//				refactor.extract.func
+//	refactor.rewrite	refactor.rewrite.fillstruct
+//				refactor.rewrite.unusedparam
+//	quickfix		quickfix.govulncheck.reset
+//				quickfix.govulncheck.upgrade
+//
+// etc, so that client editors and scripts can be more specific in
+// their requests.
+//
+// This entails that we use a segmented-path matching operator
+// instead of == for CodeActionKinds throughout gopls.
+// See golang/go#40438 for related discussion.
 const (
 	GoTest        CodeActionKind = "goTest"
 	GoDoc         CodeActionKind = "source.doc"

gopls/internal/server/code_action.go

@@ -49,7 +49,15 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
 
 		// Explicit Code Actions are opt-in and shouldn't be
 		// returned to the client unless requested using Only.
-		// TODO: Add other CodeLenses such as GoGenerate, RegenerateCgo, etc..
+		//
+		// This mechanim exists to avoid a distracting
+		// lightbulb (code action) on each Test function.
+		// These actions are unwanted in VS Code because it
+		// has Test Explorer, and in other editors because
+		// the UX of executeCommand is unsatisfactory for tests:
+		// it doesn't show the complete streaming output.
+		// See https://github.com/joaotavora/eglot/discussions/1402
+		// for a better solution.
 		explicit := map[protocol.CodeActionKind]bool{
 			protocol.GoTest: true,
 		}
@@ -101,16 +109,29 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
 		return actions, nil
 
 	case file.Go:
+		// diagnostic-associated code actions (problematic code)
+		//
+		// The diagnostics already have a UI presence (e.g. squiggly underline);
+		// the associated action may additionally show (in VS Code) as a lightbulb.
 		actions, err := s.codeActionsMatchingDiagnostics(ctx, uri, snapshot, params.Context.Diagnostics, want)
 		if err != nil {
 			return nil, err
 		}
 
-		moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, want)
-		if err != nil {
-			return nil, err
+		// non-diagnostic code actions (non-problematic)
+		//
+		// Don't report these for mere cursor motion (trigger=Automatic), only
+		// when the menu is opened, to avoid a distracting lightbulb in VS Code.
+		// (See protocol/codeactionkind.go for background.)
+		//
+		// Some clients (e.g. eglot) do not set TriggerKind at all.
+		if k := params.Context.TriggerKind; k == nil || *k != protocol.CodeActionAutomatic {
+			moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, want)
+			if err != nil {
+				return nil, err
+			}
+			actions = append(actions, moreActions...)
 		}
-		actions = append(actions, moreActions...)
 
 		// Don't suggest fixes for generated files, since they are generally
 		// not useful and some editors may apply them automatically on save.
@@ -177,16 +198,16 @@ func (s *server) ResolveCodeAction(ctx context.Context, ca *protocol.CodeAction)
 	return ca, nil
 }
 
-// codeActionsMatchingDiagnostics fetches code actions for the provided
-// diagnostics, by first attempting to unmarshal code actions directly from the
-// bundled protocol.Diagnostic.Data field, and failing that by falling back on
-// fetching a matching Diagnostic from the set of stored diagnostics for
-// this file.
+// codeActionsMatchingDiagnostics creates code actions for the
+// provided diagnostics, by unmarshalling actions bundled in the
+// protocol.Diagnostic.Data field or, if there were none, by creating
+// actions from edits associated with a matching Diagnostic from the
+// set of stored diagnostics for this file.
 func (s *server) codeActionsMatchingDiagnostics(ctx context.Context, uri protocol.DocumentURI, snapshot *cache.Snapshot, pds []protocol.Diagnostic, want map[protocol.CodeActionKind]bool) ([]protocol.CodeAction, error) {
 	var actions []protocol.CodeAction
 	var unbundled []protocol.Diagnostic // diagnostics without bundled code actions in their Data field
 	for _, pd := range pds {
-		bundled := cache.BundledQuickFixes(pd)
+		bundled := cache.BundledLazyFixes(pd)
 		if len(bundled) > 0 {
 			for _, fix := range bundled {
 				if want[fix.Kind] {

gopls/internal/server/server.go

@@ -105,7 +105,7 @@ type server struct {
 	watchedGlobPatterns    map[protocol.RelativePattern]unit
 	watchRegistrationCount int
 
-	diagnosticsMu sync.Mutex
+	diagnosticsMu sync.Mutex // guards map and its values
 	diagnostics   map[protocol.DocumentURI]*fileDiagnostics
 
 	// diagnosticsSema limits the concurrency of diagnostics runs, which can be