gopls/internal/server: CodeAction: interpret Only=[] as [QuickFix]

This CL changes the interpretation of an empty list of
CodeActionKind. Previously, we have always used it to mean
"all kinds"; however, the new guidance in the LSP 3.18 spec
is that servers should treat it equivalent to [QuickFix].

Following the spec exactly would reduce the frequency of
distracting lightbulbs displayed by VS Code's ⌘-. menu
for actions that are not fixes (e.g. Inline call to f).
But it would deny most clients (VS Code, Emacs, Vim, ...) the
natural way to ask the server what code actions are
currently available, making it impossible to discover
any code action (e.g. Browse gopls docs) that doesn't
fit into one of the existing categories with its own
command (e.g. Refactor, Source Action).

So, we compromise: if the CodeAction query was triggered
by cursor motion (Automatic), we treat [] as [QuickFix].
But if it was explicitly Invoked, we respond with all
available actions, equivalent to [""].

This does unfortunately double the test space; all but
one of our tests (TestVSCodeIssue65167)use TriggerKindUnknown.

Details:
- Adjust hierarchical matching to permit kind="" (protocol.Empty)
  to match all kinds.
- Change CLI and fake.Editor clients to populate
   Capabilities.TextDocument.CodeAction.CodeActionLiteralSupport.\
     CodeActionKind.ValueSet (!!), a 3.18 feature.
   (This isn't really needed now that the latest draft
   returns all available actions when trigger=automatic.)
- The @codeaction marker passes kind="".
- 'gopls codeaction' now passes Only=[""] when no -kind flag is specified.
- 'gopls imports' now passes Only=[SourceOrganizeImports]
   instead of obsolete title filtering.
- Editor.{serverCapabilities,semTokOpts} are no longer
  unnecessarily guarded by the mutex.
  (In an earlier draft I needed to expose Editor.ServerCapabilities
  but it proved unnecessary.)

Fixes golang/go#68783

Change-Id: Ia4246c47b54b59f6f03eada3e916428de50c42f4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/616837
Commit-Queue: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

gopls/internal/cmd/capabilities_test.go

@@ -104,6 +104,9 @@ func TestCapabilities(t *testing.T) {
 		TextDocument: protocol.TextDocumentIdentifier{
 			URI: uri,
 		},
+		Context: protocol.CodeActionContext{
+			Only: []protocol.CodeActionKind{protocol.SourceOrganizeImports},
+		},
 	})
 	if err != nil {
 		t.Fatal(err)

gopls/internal/cmd/cmd.go

@@ -365,6 +365,13 @@ func (c *connection) initialize(ctx context.Context, options func(*settings.Opti
 	params.Capabilities.TextDocument.SemanticTokens.Requests.Full = &protocol.Or_ClientSemanticTokensRequestOptions_full{Value: true}
 	params.Capabilities.TextDocument.SemanticTokens.TokenTypes = protocol.SemanticTypes()
 	params.Capabilities.TextDocument.SemanticTokens.TokenModifiers = protocol.SemanticModifiers()
+	params.Capabilities.TextDocument.CodeAction = protocol.CodeActionClientCapabilities{
+		CodeActionLiteralSupport: protocol.ClientCodeActionLiteralOptions{
+			CodeActionKind: protocol.ClientCodeActionKindOptions{
+				ValueSet: []protocol.CodeActionKind{protocol.Empty}, // => all
+			},
+		},
+	}
 	params.Capabilities.Window.WorkDoneProgress = true
 
 	params.InitializationOptions = map[string]interface{}{

gopls/internal/cmd/codeaction.go

@@ -144,6 +144,8 @@ func (cmd *codeaction) Run(ctx context.Context, args ...string) error {
 		for _, kind := range strings.Split(cmd.Kind, ",") {
 			kinds = append(kinds, protocol.CodeActionKind(kind))
 		}
+	} else {
+		kinds = append(kinds, protocol.Empty) // => all
 	}
 	actions, err := conn.CodeAction(ctx, &protocol.CodeActionParams{
 		TextDocument: protocol.TextDocumentIdentifier{URI: uri},

gopls/internal/cmd/imports.go

@@ -59,15 +59,15 @@ func (t *imports) Run(ctx context.Context, args ...string) error {
 		TextDocument: protocol.TextDocumentIdentifier{
 			URI: uri,
 		},
+		Context: protocol.CodeActionContext{
+			Only: []protocol.CodeActionKind{protocol.SourceOrganizeImports},
+		},
 	})
 	if err != nil {
 		return fmt.Errorf("%v: %v", from, err)
 	}
 	var edits []protocol.TextEdit
 	for _, a := range actions {
-		if a.Title != "Organize Imports" {
-			continue
-		}
 		for _, c := range a.Edit.DocumentChanges {
 			// This code action should affect only the specified file;
 			// it is safe to ignore others.

gopls/internal/cmd/integration_test.go

@@ -48,7 +48,7 @@ import (
 	"golang.org/x/tools/txtar"
 )
 
-// TestVersion tests the 'version' subcommand (../info.go).
+// TestVersion tests the 'version' subcommand (info.go).
 func TestVersion(t *testing.T) {
 	t.Parallel()
 
@@ -84,7 +84,7 @@ func TestVersion(t *testing.T) {
 	}
 }
 
-// TestCheck tests the 'check' subcommand (../check.go).
+// TestCheck tests the 'check' subcommand (check.go).
 func TestCheck(t *testing.T) {
 	t.Parallel()
 
@@ -143,7 +143,7 @@ var C int
 	}
 }
 
-// TestCallHierarchy tests the 'call_hierarchy' subcommand (../call_hierarchy.go).
+// TestCallHierarchy tests the 'call_hierarchy' subcommand (call_hierarchy.go).
 func TestCallHierarchy(t *testing.T) {
 	t.Parallel()
 
@@ -186,7 +186,7 @@ func h() {
 	}
 }
 
-// TestCodeLens tests the 'codelens' subcommand (../codelens.go).
+// TestCodeLens tests the 'codelens' subcommand (codelens.go).
 func TestCodeLens(t *testing.T) {
 	t.Parallel()
 
@@ -238,7 +238,7 @@ func TestFail(t *testing.T) { t.Fatal("fail") }
 	}
 }
 
-// TestDefinition tests the 'definition' subcommand (../definition.go).
+// TestDefinition tests the 'definition' subcommand (definition.go).
 func TestDefinition(t *testing.T) {
 	t.Parallel()
 
@@ -289,7 +289,7 @@ func g() {
 	}
 }
 
-// TestExecute tests the 'execute' subcommand (../execute.go).
+// TestExecute tests the 'execute' subcommand (execute.go).
 func TestExecute(t *testing.T) {
 	t.Parallel()
 
@@ -363,7 +363,7 @@ func TestHello(t *testing.T) {
 	}
 }
 
-// TestFoldingRanges tests the 'folding_ranges' subcommand (../folding_range.go).
+// TestFoldingRanges tests the 'folding_ranges' subcommand (folding_range.go).
 func TestFoldingRanges(t *testing.T) {
 	t.Parallel()
 
@@ -393,7 +393,7 @@ func f(x int) {
 	}
 }
 
-// TestFormat tests the 'format' subcommand (../format.go).
+// TestFormat tests the 'format' subcommand (format.go).
 func TestFormat(t *testing.T) {
 	t.Parallel()
 
@@ -453,7 +453,7 @@ func f() {}
 	}
 }
 
-// TestHighlight tests the 'highlight' subcommand (../highlight.go).
+// TestHighlight tests the 'highlight' subcommand (highlight.go).
 func TestHighlight(t *testing.T) {
 	t.Parallel()
 
@@ -482,7 +482,7 @@ func f() {
 	}
 }
 
-// TestImplementations tests the 'implementation' subcommand (../implementation.go).
+// TestImplementations tests the 'implementation' subcommand (implementation.go).
 func TestImplementations(t *testing.T) {
 	t.Parallel()
 
@@ -511,7 +511,7 @@ func (T) String() string { return "" }
 	}
 }
 
-// TestImports tests the 'imports' subcommand (../imports.go).
+// TestImports tests the 'imports' subcommand (imports.go).
 func TestImports(t *testing.T) {
 	t.Parallel()
 
@@ -560,7 +560,7 @@ func _() {
 	}
 }
 
-// TestLinks tests the 'links' subcommand (../links.go).
+// TestLinks tests the 'links' subcommand (links.go).
 func TestLinks(t *testing.T) {
 	t.Parallel()
 
@@ -605,7 +605,7 @@ func f() {}
 	}
 }
 
-// TestReferences tests the 'references' subcommand (../references.go).
+// TestReferences tests the 'references' subcommand (references.go).
 func TestReferences(t *testing.T) {
 	t.Parallel()
 
@@ -643,7 +643,7 @@ func g() {
 	}
 }
 
-// TestSignature tests the 'signature' subcommand (../signature.go).
+// TestSignature tests the 'signature' subcommand (signature.go).
 func TestSignature(t *testing.T) {
 	t.Parallel()
 
@@ -674,7 +674,7 @@ func f() {
 	}
 }
 
-// TestPrepareRename tests the 'prepare_rename' subcommand (../prepare_rename.go).
+// TestPrepareRename tests the 'prepare_rename' subcommand (prepare_rename.go).
 func TestPrepareRename(t *testing.T) {
 	t.Parallel()
 
@@ -713,7 +713,7 @@ func oldname() {}
 	}
 }
 
-// TestRename tests the 'rename' subcommand (../rename.go).
+// TestRename tests the 'rename' subcommand (rename.go).
 func TestRename(t *testing.T) {
 	t.Parallel()
 
@@ -759,7 +759,7 @@ func oldname() {}
 	}
 }
 
-// TestSymbols tests the 'symbols' subcommand (../symbols.go).
+// TestSymbols tests the 'symbols' subcommand (symbols.go).
 func TestSymbols(t *testing.T) {
 	t.Parallel()
 
@@ -790,7 +790,7 @@ const c = 0
 	}
 }
 
-// TestSemtok tests the 'semtok' subcommand (../semantictokens.go).
+// TestSemtok tests the 'semtok' subcommand (semantictokens.go).
 func TestSemtok(t *testing.T) {
 	t.Parallel()
 
@@ -941,7 +941,7 @@ package foo
 	}
 }
 
-// TestCodeAction tests the 'codeaction' subcommand (../codeaction.go).
+// TestCodeAction tests the 'codeaction' subcommand (codeaction.go).
 func TestCodeAction(t *testing.T) {
 	t.Parallel()
 
@@ -1040,7 +1040,7 @@ func (c C) Read(p []byte) (n int, err error) {
 	}
 }
 
-// TestWorkspaceSymbol tests the 'workspace_symbol' subcommand (../workspace_symbol.go).
+// TestWorkspaceSymbol tests the 'workspace_symbol' subcommand (workspace_symbol.go).
 func TestWorkspaceSymbol(t *testing.T) {
 	t.Parallel()
 

gopls/internal/server/code_action.go

@@ -36,34 +36,65 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
 	// Determine the supported code action kinds for this file.
 	//
 	// We interpret CodeActionKinds hierarchically, so refactor.rewrite
-	// subsumes refactor.rewrite.change_quote, for example.
+	// subsumes refactor.rewrite.change_quote, for example,
+	// and "" (protocol.Empty) subsumes all kinds.
 	// See ../protocol/codeactionkind.go for some code action theory.
-	codeActionKinds, ok := snapshot.Options().SupportedCodeActions[kind]
-	if !ok {
-		return nil, fmt.Errorf("no supported code actions for %v file kind", kind)
-	}
-
-	// The Only field of the context specifies which code actions
-	// the client wants. If Only is empty, assume the client wants
-	// all supported code actions.
+	//
+	// The Context.Only field specifies which code actions
+	// the client wants. According to LSP 3.18 textDocument_codeAction,
+	// an Only=[] should be interpreted as Only=["quickfix"]:
+	//
+	//   "In version 1.0 of the protocol, there weren’t any
+	//   source or refactoring code actions. Code actions
+	//   were solely used to (quick) fix code, not to
+	//   write/rewrite code. So if a client asks for code
+	//   actions without any kind, the standard quick fix
+	//   code actions should be returned."
+	//
+	// However, this would deny clients (e.g. Vim+coc.nvim,
+	// Emacs+eglot, and possibly others) the easiest and most
+	// natural way of querying the server for the entire set of
+	// available code actions. But reporting all available code
+	// actions would be a nuisance for VS Code, since mere cursor
+	// motion into a region with a code action (~anywhere) would
+	// trigger a lightbulb usually associated with quickfixes.
+	//
+	// As a compromise, we use the trigger kind as a heuristic: if
+	// the query was triggered by cursor motion (Automatic), we
+	// respond with only quick fixes; if the query was invoked
+	// explicitly (Invoked), we respond with all available
+	// actions.
+	codeActionKinds := make(map[protocol.CodeActionKind]bool)
 	if len(params.Context.Only) > 0 {
-		codeActionKinds = make(map[protocol.CodeActionKind]bool)
-		for _, kind := range params.Context.Only {
+		for _, kind := range params.Context.Only { // kind may be "" (=> all)
 			codeActionKinds[kind] = true
 		}
+	} else {
+		// No explicit kind specified.
+		// Heuristic: decide based on trigger.
+		if triggerKind(params) == protocol.CodeActionAutomatic {
+			// e.g. cursor motion: show only quick fixes
+			codeActionKinds[protocol.QuickFix] = true
+		} else {
+			// e.g. a menu selection (or unknown trigger kind,
+			// as in our tests): show all available code actions.
+			codeActionKinds[protocol.Empty] = true
+		}
 	}
 
 	// enabled reports whether the specified kind of code action is required.
 	enabled := func(kind protocol.CodeActionKind) bool {
 		// Given "refactor.rewrite.foo", check for it,
-		// then "refactor.rewrite", "refactor".
+		// then "refactor.rewrite", "refactor", then "".
 		// A false map entry prunes the search for ancestors.
+		//
+		// If codeActionKinds contains protocol.Empty (""),
+		// all kinds are enabled.
 		for {
 			if v, ok := codeActionKinds[kind]; ok {
 				return v
 			}
-			dot := strings.LastIndexByte(string(kind), '.')
-			if dot < 0 {
+			if kind == "" {
 				return false
 			}
 
@@ -88,7 +119,12 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
 				return false // don't search ancestors
 			}
 
-			kind = kind[:dot]
+			// Try the parent.
+			if dot := strings.LastIndexByte(string(kind), '.'); dot >= 0 {
+				kind = kind[:dot] // "refactor.foo" -> "refactor"
+			} else {
+				kind = "" // "refactor" -> ""
+			}
 		}
 	}
 
@@ -139,11 +175,7 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
 		}
 
 		// computed code actions (may include quickfixes from diagnostics)
-		trigger := protocol.CodeActionUnknownTrigger
-		if k := params.Context.TriggerKind; k != nil { // (some clients omit it)
-			trigger = *k
-		}
-		moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, enabled, trigger)
+		moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, enabled, triggerKind(params))
 		if err != nil {
 			return nil, err
 		}
@@ -175,6 +207,13 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
 	}
 }
 
+func triggerKind(params *protocol.CodeActionParams) protocol.CodeActionTriggerKind {
+	if kind := params.Context.TriggerKind; kind != nil { // (some clients omit it)
+		return *kind
+	}
+	return protocol.CodeActionUnknownTrigger
+}
+
 // ResolveCodeAction resolves missing Edit information (that is, computes the
 // details of the necessary patch) in the given code action using the provided
 // Data field of the CodeAction, which should contain the raw json of a protocol.Command.