Skip to content

Commit d2e4621

Browse files
adonovangopherbot
adonovan
authored and
gopherbot
committed
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>
1 parent 4e80b32 commit d2e4621

File tree

9 files changed

+139
-70
lines changed

9 files changed

+139
-70
lines changed

‎gopls/internal/cmd/capabilities_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,9 @@ func TestCapabilities(t *testing.T) {
104104
TextDocument: protocol.TextDocumentIdentifier{
105105
URI: uri,
106106
},
107+
Context: protocol.CodeActionContext{
108+
Only: []protocol.CodeActionKind{protocol.SourceOrganizeImports},
109+
},
107110
})
108111
if err != nil {
109112
t.Fatal(err)

‎gopls/internal/cmd/cmd.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,13 @@ func (c *connection) initialize(ctx context.Context, options func(*settings.Opti
365365
params.Capabilities.TextDocument.SemanticTokens.Requests.Full = &protocol.Or_ClientSemanticTokensRequestOptions_full{Value: true}
366366
params.Capabilities.TextDocument.SemanticTokens.TokenTypes = protocol.SemanticTypes()
367367
params.Capabilities.TextDocument.SemanticTokens.TokenModifiers = protocol.SemanticModifiers()
368+
params.Capabilities.TextDocument.CodeAction = protocol.CodeActionClientCapabilities{
369+
CodeActionLiteralSupport: protocol.ClientCodeActionLiteralOptions{
370+
CodeActionKind: protocol.ClientCodeActionKindOptions{
371+
ValueSet: []protocol.CodeActionKind{protocol.Empty}, // => all
372+
},
373+
},
374+
}
368375
params.Capabilities.Window.WorkDoneProgress = true
369376

370377
params.InitializationOptions = map[string]interface{}{

‎gopls/internal/cmd/codeaction.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,8 @@ func (cmd *codeaction) Run(ctx context.Context, args ...string) error {
144144
for _, kind := range strings.Split(cmd.Kind, ",") {
145145
kinds = append(kinds, protocol.CodeActionKind(kind))
146146
}
147+
} else {
148+
kinds = append(kinds, protocol.Empty) // => all
147149
}
148150
actions, err := conn.CodeAction(ctx, &protocol.CodeActionParams{
149151
TextDocument: protocol.TextDocumentIdentifier{URI: uri},

‎gopls/internal/cmd/imports.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,15 @@ func (t *imports) Run(ctx context.Context, args ...string) error {
5959
TextDocument: protocol.TextDocumentIdentifier{
6060
URI: uri,
6161
},
62+
Context: protocol.CodeActionContext{
63+
Only: []protocol.CodeActionKind{protocol.SourceOrganizeImports},
64+
},
6265
})
6366
if err != nil {
6467
return fmt.Errorf("%v: %v", from, err)
6568
}
6669
var edits []protocol.TextEdit
6770
for _, a := range actions {
68-
if a.Title != "Organize Imports" {
69-
continue
70-
}
7171
for _, c := range a.Edit.DocumentChanges {
7272
// This code action should affect only the specified file;
7373
// it is safe to ignore others.

‎gopls/internal/cmd/integration_test.go

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ import (
4848
"golang.org/x/tools/txtar"
4949
)
5050

51-
// TestVersion tests the 'version' subcommand (../info.go).
51+
// TestVersion tests the 'version' subcommand (info.go).
5252
func TestVersion(t *testing.T) {
5353
t.Parallel()
5454

@@ -84,7 +84,7 @@ func TestVersion(t *testing.T) {
8484
}
8585
}
8686

87-
// TestCheck tests the 'check' subcommand (../check.go).
87+
// TestCheck tests the 'check' subcommand (check.go).
8888
func TestCheck(t *testing.T) {
8989
t.Parallel()
9090

@@ -143,7 +143,7 @@ var C int
143143
}
144144
}
145145

146-
// TestCallHierarchy tests the 'call_hierarchy' subcommand (../call_hierarchy.go).
146+
// TestCallHierarchy tests the 'call_hierarchy' subcommand (call_hierarchy.go).
147147
func TestCallHierarchy(t *testing.T) {
148148
t.Parallel()
149149

@@ -186,7 +186,7 @@ func h() {
186186
}
187187
}
188188

189-
// TestCodeLens tests the 'codelens' subcommand (../codelens.go).
189+
// TestCodeLens tests the 'codelens' subcommand (codelens.go).
190190
func TestCodeLens(t *testing.T) {
191191
t.Parallel()
192192

@@ -238,7 +238,7 @@ func TestFail(t *testing.T) { t.Fatal("fail") }
238238
}
239239
}
240240

241-
// TestDefinition tests the 'definition' subcommand (../definition.go).
241+
// TestDefinition tests the 'definition' subcommand (definition.go).
242242
func TestDefinition(t *testing.T) {
243243
t.Parallel()
244244

@@ -289,7 +289,7 @@ func g() {
289289
}
290290
}
291291

292-
// TestExecute tests the 'execute' subcommand (../execute.go).
292+
// TestExecute tests the 'execute' subcommand (execute.go).
293293
func TestExecute(t *testing.T) {
294294
t.Parallel()
295295

@@ -363,7 +363,7 @@ func TestHello(t *testing.T) {
363363
}
364364
}
365365

366-
// TestFoldingRanges tests the 'folding_ranges' subcommand (../folding_range.go).
366+
// TestFoldingRanges tests the 'folding_ranges' subcommand (folding_range.go).
367367
func TestFoldingRanges(t *testing.T) {
368368
t.Parallel()
369369

@@ -393,7 +393,7 @@ func f(x int) {
393393
}
394394
}
395395

396-
// TestFormat tests the 'format' subcommand (../format.go).
396+
// TestFormat tests the 'format' subcommand (format.go).
397397
func TestFormat(t *testing.T) {
398398
t.Parallel()
399399

@@ -453,7 +453,7 @@ func f() {}
453453
}
454454
}
455455

456-
// TestHighlight tests the 'highlight' subcommand (../highlight.go).
456+
// TestHighlight tests the 'highlight' subcommand (highlight.go).
457457
func TestHighlight(t *testing.T) {
458458
t.Parallel()
459459

@@ -482,7 +482,7 @@ func f() {
482482
}
483483
}
484484

485-
// TestImplementations tests the 'implementation' subcommand (../implementation.go).
485+
// TestImplementations tests the 'implementation' subcommand (implementation.go).
486486
func TestImplementations(t *testing.T) {
487487
t.Parallel()
488488

@@ -511,7 +511,7 @@ func (T) String() string { return "" }
511511
}
512512
}
513513

514-
// TestImports tests the 'imports' subcommand (../imports.go).
514+
// TestImports tests the 'imports' subcommand (imports.go).
515515
func TestImports(t *testing.T) {
516516
t.Parallel()
517517

@@ -560,7 +560,7 @@ func _() {
560560
}
561561
}
562562

563-
// TestLinks tests the 'links' subcommand (../links.go).
563+
// TestLinks tests the 'links' subcommand (links.go).
564564
func TestLinks(t *testing.T) {
565565
t.Parallel()
566566

@@ -605,7 +605,7 @@ func f() {}
605605
}
606606
}
607607

608-
// TestReferences tests the 'references' subcommand (../references.go).
608+
// TestReferences tests the 'references' subcommand (references.go).
609609
func TestReferences(t *testing.T) {
610610
t.Parallel()
611611

@@ -643,7 +643,7 @@ func g() {
643643
}
644644
}
645645

646-
// TestSignature tests the 'signature' subcommand (../signature.go).
646+
// TestSignature tests the 'signature' subcommand (signature.go).
647647
func TestSignature(t *testing.T) {
648648
t.Parallel()
649649

@@ -674,7 +674,7 @@ func f() {
674674
}
675675
}
676676

677-
// TestPrepareRename tests the 'prepare_rename' subcommand (../prepare_rename.go).
677+
// TestPrepareRename tests the 'prepare_rename' subcommand (prepare_rename.go).
678678
func TestPrepareRename(t *testing.T) {
679679
t.Parallel()
680680

@@ -713,7 +713,7 @@ func oldname() {}
713713
}
714714
}
715715

716-
// TestRename tests the 'rename' subcommand (../rename.go).
716+
// TestRename tests the 'rename' subcommand (rename.go).
717717
func TestRename(t *testing.T) {
718718
t.Parallel()
719719

@@ -759,7 +759,7 @@ func oldname() {}
759759
}
760760
}
761761

762-
// TestSymbols tests the 'symbols' subcommand (../symbols.go).
762+
// TestSymbols tests the 'symbols' subcommand (symbols.go).
763763
func TestSymbols(t *testing.T) {
764764
t.Parallel()
765765

@@ -790,7 +790,7 @@ const c = 0
790790
}
791791
}
792792

793-
// TestSemtok tests the 'semtok' subcommand (../semantictokens.go).
793+
// TestSemtok tests the 'semtok' subcommand (semantictokens.go).
794794
func TestSemtok(t *testing.T) {
795795
t.Parallel()
796796

@@ -941,7 +941,7 @@ package foo
941941
}
942942
}
943943

944-
// TestCodeAction tests the 'codeaction' subcommand (../codeaction.go).
944+
// TestCodeAction tests the 'codeaction' subcommand (codeaction.go).
945945
func TestCodeAction(t *testing.T) {
946946
t.Parallel()
947947

@@ -1040,7 +1040,7 @@ func (c C) Read(p []byte) (n int, err error) {
10401040
}
10411041
}
10421042

1043-
// TestWorkspaceSymbol tests the 'workspace_symbol' subcommand (../workspace_symbol.go).
1043+
// TestWorkspaceSymbol tests the 'workspace_symbol' subcommand (workspace_symbol.go).
10441044
func TestWorkspaceSymbol(t *testing.T) {
10451045
t.Parallel()
10461046

‎gopls/internal/server/code_action.go

Lines changed: 59 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -36,34 +36,65 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
3636
// Determine the supported code action kinds for this file.
3737
//
3838
// We interpret CodeActionKinds hierarchically, so refactor.rewrite
39-
// subsumes refactor.rewrite.change_quote, for example.
39+
// subsumes refactor.rewrite.change_quote, for example,
40+
// and "" (protocol.Empty) subsumes all kinds.
4041
// See ../protocol/codeactionkind.go for some code action theory.
41-
codeActionKinds, ok := snapshot.Options().SupportedCodeActions[kind]
42-
if !ok {
43-
return nil, fmt.Errorf("no supported code actions for %v file kind", kind)
44-
}
45-
46-
// The Only field of the context specifies which code actions
47-
// the client wants. If Only is empty, assume the client wants
48-
// all supported code actions.
42+
//
43+
// The Context.Only field specifies which code actions
44+
// the client wants. According to LSP 3.18 textDocument_codeAction,
45+
// an Only=[] should be interpreted as Only=["quickfix"]:
46+
//
47+
// "In version 1.0 of the protocol, there weren’t any
48+
// source or refactoring code actions. Code actions
49+
// were solely used to (quick) fix code, not to
50+
// write/rewrite code. So if a client asks for code
51+
// actions without any kind, the standard quick fix
52+
// code actions should be returned."
53+
//
54+
// However, this would deny clients (e.g. Vim+coc.nvim,
55+
// Emacs+eglot, and possibly others) the easiest and most
56+
// natural way of querying the server for the entire set of
57+
// available code actions. But reporting all available code
58+
// actions would be a nuisance for VS Code, since mere cursor
59+
// motion into a region with a code action (~anywhere) would
60+
// trigger a lightbulb usually associated with quickfixes.
61+
//
62+
// As a compromise, we use the trigger kind as a heuristic: if
63+
// the query was triggered by cursor motion (Automatic), we
64+
// respond with only quick fixes; if the query was invoked
65+
// explicitly (Invoked), we respond with all available
66+
// actions.
67+
codeActionKinds := make(map[protocol.CodeActionKind]bool)
4968
if len(params.Context.Only) > 0 {
50-
codeActionKinds = make(map[protocol.CodeActionKind]bool)
51-
for _, kind := range params.Context.Only {
69+
for _, kind := range params.Context.Only { // kind may be "" (=> all)
5270
codeActionKinds[kind] = true
5371
}
72+
} else {
73+
// No explicit kind specified.
74+
// Heuristic: decide based on trigger.
75+
if triggerKind(params) == protocol.CodeActionAutomatic {
76+
// e.g. cursor motion: show only quick fixes
77+
codeActionKinds[protocol.QuickFix] = true
78+
} else {
79+
// e.g. a menu selection (or unknown trigger kind,
80+
// as in our tests): show all available code actions.
81+
codeActionKinds[protocol.Empty] = true
82+
}
5483
}
5584

5685
// enabled reports whether the specified kind of code action is required.
5786
enabled := func(kind protocol.CodeActionKind) bool {
5887
// Given "refactor.rewrite.foo", check for it,
59-
// then "refactor.rewrite", "refactor".
88+
// then "refactor.rewrite", "refactor", then "".
6089
// A false map entry prunes the search for ancestors.
90+
//
91+
// If codeActionKinds contains protocol.Empty (""),
92+
// all kinds are enabled.
6193
for {
6294
if v, ok := codeActionKinds[kind]; ok {
6395
return v
6496
}
65-
dot := strings.LastIndexByte(string(kind), '.')
66-
if dot < 0 {
97+
if kind == "" {
6798
return false
6899
}
69100

@@ -88,7 +119,12 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
88119
return false // don't search ancestors
89120
}
90121

91-
kind = kind[:dot]
122+
// Try the parent.
123+
if dot := strings.LastIndexByte(string(kind), '.'); dot >= 0 {
124+
kind = kind[:dot] // "refactor.foo" -> "refactor"
125+
} else {
126+
kind = "" // "refactor" -> ""
127+
}
92128
}
93129
}
94130

@@ -139,11 +175,7 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
139175
}
140176

141177
// computed code actions (may include quickfixes from diagnostics)
142-
trigger := protocol.CodeActionUnknownTrigger
143-
if k := params.Context.TriggerKind; k != nil { // (some clients omit it)
144-
trigger = *k
145-
}
146-
moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, enabled, trigger)
178+
moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, enabled, triggerKind(params))
147179
if err != nil {
148180
return nil, err
149181
}
@@ -175,6 +207,13 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
175207
}
176208
}
177209

210+
func triggerKind(params *protocol.CodeActionParams) protocol.CodeActionTriggerKind {
211+
if kind := params.Context.TriggerKind; kind != nil { // (some clients omit it)
212+
return *kind
213+
}
214+
return protocol.CodeActionUnknownTrigger
215+
}
216+
178217
// ResolveCodeAction resolves missing Edit information (that is, computes the
179218
// details of the necessary patch) in the given code action using the provided
180219
// Data field of the CodeAction, which should contain the raw json of a protocol.Command.

0 commit comments

Comments
 (0)