diff --git a/gopls/internal/cmd/capabilities_test.go b/gopls/internal/cmd/capabilities_test.go index 97eb49652..e1cc11bf4 100644 --- a/gopls/internal/cmd/capabilities_test.go +++ b/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) diff --git a/gopls/internal/cmd/cmd.go b/gopls/internal/cmd/cmd.go index 9ec5b630d..4afac6a7a 100644 --- a/gopls/internal/cmd/cmd.go +++ b/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{}{ diff --git a/gopls/internal/cmd/codeaction.go b/gopls/internal/cmd/codeaction.go index 84d7d181b..c349c7ab6 100644 --- a/gopls/internal/cmd/codeaction.go +++ b/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}, diff --git a/gopls/internal/cmd/imports.go b/gopls/internal/cmd/imports.go index c64c871e3..b0f675907 100644 --- a/gopls/internal/cmd/imports.go +++ b/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. diff --git a/gopls/internal/cmd/integration_test.go b/gopls/internal/cmd/integration_test.go index dfb2a164a..39698f373 100644 --- a/gopls/internal/cmd/integration_test.go +++ b/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() diff --git a/gopls/internal/server/code_action.go b/gopls/internal/server/code_action.go index 5fc0fb6aa..e00e34385 100644 --- a/gopls/internal/server/code_action.go +++ b/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. diff --git a/gopls/internal/settings/codeactionkind.go b/gopls/internal/settings/codeactionkind.go index e8e29d8dd..fa06b90e7 100644 --- a/gopls/internal/settings/codeactionkind.go +++ b/gopls/internal/settings/codeactionkind.go @@ -24,14 +24,15 @@ import "golang.org/x/tools/gopls/internal/protocol" // notebook // // Kinds are hierarchical: "refactor" subsumes "refactor.inline", -// which subsumes "refactor.inline.call". The "Only" field in a -// CodeAction request may specify a category such as "refactor"; any -// matching code action will be returned. +// which subsumes "refactor.inline.call". This rule implies that the +// empty string, confusingly named protocol.Empty, subsumes all kinds. +// The "Only" field in a CodeAction request may specify a category +// such as "refactor"; any matching code action will be returned. // // All CodeActions returned by gopls use a specific leaf kind such as // "refactor.inline.call", except for quick fixes, which all use // "quickfix". TODO(adonovan): perhaps quick fixes should also be -// hierarchical (e.g. quickfix.govulncheck.{reset,upgrade}). +// hierarchical (e.g. quickfix.govulncheck.{reset,upgrade})? // // # VS Code // @@ -41,10 +42,16 @@ import "golang.org/x/tools/gopls/internal/protocol" // 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.*", -// and ad hoc ones ("More actions...") such as "gopls.*". +// does not set the Only field in its request, so the set of +// kinds is determined by how the server interprets the default. +// The LSP 3.18 guidance is that this should be treated +// equivalent to Only=["quickfix"], and that is what gopls +// now does. (If the server responds with more kinds, they will +// be displayed in menu subsections.) +// // All of these CodeAction requests have triggerkind=Invoked. // // Cursor motion also performs a CodeAction request, but with @@ -60,7 +67,7 @@ import "golang.org/x/tools/gopls/internal/protocol" // // 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". +// "Refactor...", "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 @@ -74,6 +81,10 @@ const ( GoTest protocol.CodeActionKind = "source.test" // gopls + // TODO(adonovan): we should not use this category as it will + // never be requested now that we no longer interpret "no kind + // restriction" as "quickfix" instead of "all kinds". + // We need another way to make docs discoverable. GoplsDocFeatures protocol.CodeActionKind = "gopls.doc.features" // refactor.rewrite diff --git a/gopls/internal/test/integration/fake/editor.go b/gopls/internal/test/integration/fake/editor.go index 09cdbaf8c..876d055da 100644 --- a/gopls/internal/test/integration/fake/editor.go +++ b/gopls/internal/test/integration/fake/editor.go @@ -42,13 +42,15 @@ type Editor struct { // TODO(rfindley): buffers should be keyed by protocol.DocumentURI. mu sync.Mutex - config EditorConfig // editor configuration - buffers map[string]buffer // open buffers (relative path -> buffer content) - serverCapabilities protocol.ServerCapabilities // capabilities / options - semTokOpts protocol.SemanticTokensOptions - watchPatterns []*glob.Glob // glob patterns to watch + config EditorConfig // editor configuration + buffers map[string]buffer // open buffers (relative path -> buffer content) + watchPatterns []*glob.Glob // glob patterns to watch suggestionUseReplaceMode bool + // These fields are populated by Connect. + serverCapabilities protocol.ServerCapabilities + semTokOpts protocol.SemanticTokensOptions + // Call metrics for the purpose of expectations. This is done in an ad-hoc // manner for now. Perhaps in the future we should do something more // systematic. Guarded with a separate mutex as calls may need to be accessed @@ -320,10 +322,8 @@ func (e *Editor) initialize(ctx context.Context) error { if err != nil { return fmt.Errorf("unmarshalling semantic tokens options: %v", err) } - e.mu.Lock() e.serverCapabilities = resp.Capabilities e.semTokOpts = semTokOpts - e.mu.Unlock() if err := e.Server.Initialized(ctx, &protocol.InitializedParams{}); err != nil { return fmt.Errorf("initialized: %w", err) @@ -357,6 +357,14 @@ func clientCapabilities(cfg EditorConfig) (protocol.ClientCapabilities, error) { // Additional modifiers supported by this client: "interface", "struct", "signature", "pointer", "array", "map", "slice", "chan", "string", "number", "bool", "invalid", } + // Request that the server provide its complete list of code action kinds. + capabilities.TextDocument.CodeAction = protocol.CodeActionClientCapabilities{ + CodeActionLiteralSupport: protocol.ClientCodeActionLiteralOptions{ + CodeActionKind: protocol.ClientCodeActionKindOptions{ + ValueSet: []protocol.CodeActionKind{protocol.Empty}, // => all + }, + }, + } // The LSP tests have historically enabled this flag, // but really we should test both ways for older editors. capabilities.TextDocument.DocumentSymbol.HierarchicalDocumentSymbolSupport = true @@ -1570,6 +1578,7 @@ func (e *Editor) CodeAction(ctx context.Context, loc protocol.Location, diagnost Context: protocol.CodeActionContext{ Diagnostics: diagnostics, TriggerKind: &trigger, + Only: []protocol.CodeActionKind{protocol.Empty}, // => all }, Range: loc.Range, // may be zero } @@ -1680,9 +1689,7 @@ type SemanticToken struct { // Note: previously this function elided comment, string, and number tokens. // Instead, filtering of token types should be done by the caller. func (e *Editor) interpretTokens(x []uint32, contents string) []SemanticToken { - e.mu.Lock() legend := e.semTokOpts.Legend - e.mu.Unlock() lines := strings.Split(contents, "\n") ans := []SemanticToken{} line, col := 1, 1 diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index e566f922a..a128bcb7f 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go @@ -1933,7 +1933,7 @@ func codeActionMarker(mark marker, start, end protocol.Location, actionKind stri loc.Range.End = end.Range.End // Apply the fix it suggests. - changed, err := codeAction(mark.run.env, loc.URI, loc.Range, actionKind, nil) + changed, err := codeAction(mark.run.env, loc.URI, loc.Range, protocol.CodeActionKind(actionKind), nil) if err != nil { mark.errorf("codeAction failed: %v", err) return @@ -1944,7 +1944,7 @@ func codeActionMarker(mark marker, start, end protocol.Location, actionKind stri } func codeActionEditMarker(mark marker, loc protocol.Location, actionKind string, g *Golden) { - changed, err := codeAction(mark.run.env, loc.URI, loc.Range, actionKind, nil) + changed, err := codeAction(mark.run.env, loc.URI, loc.Range, protocol.CodeActionKind(actionKind), nil) if err != nil { mark.errorf("codeAction failed: %v", err) return @@ -1956,7 +1956,7 @@ func codeActionEditMarker(mark marker, loc protocol.Location, actionKind string, func codeActionErrMarker(mark marker, start, end protocol.Location, actionKind string, wantErr stringMatcher) { loc := start loc.Range.End = end.Range.End - _, err := codeAction(mark.run.env, loc.URI, loc.Range, actionKind, nil) + _, err := codeAction(mark.run.env, loc.URI, loc.Range, protocol.CodeActionKind(actionKind), nil) wantErr.checkErr(mark, err) } @@ -2071,8 +2071,8 @@ func suggestedfixErrMarker(mark marker, loc protocol.Location, re *regexp.Regexp // The resulting map contains resulting file contents after the code action is // applied. Currently, this function does not support code actions that return // edits directly; it only supports code action commands. -func codeAction(env *integration.Env, uri protocol.DocumentURI, rng protocol.Range, actionKind string, diag *protocol.Diagnostic) (map[string][]byte, error) { - changes, err := codeActionChanges(env, uri, rng, actionKind, diag) +func codeAction(env *integration.Env, uri protocol.DocumentURI, rng protocol.Range, kind protocol.CodeActionKind, diag *protocol.Diagnostic) (map[string][]byte, error) { + changes, err := codeActionChanges(env, uri, rng, kind, diag) if err != nil { return nil, err } @@ -2082,15 +2082,15 @@ func codeAction(env *integration.Env, uri protocol.DocumentURI, rng protocol.Ran // codeActionChanges executes a textDocument/codeAction request for the // specified location and kind, and captures the resulting document changes. // If diag is non-nil, it is used as the code action context. -func codeActionChanges(env *integration.Env, uri protocol.DocumentURI, rng protocol.Range, actionKind string, diag *protocol.Diagnostic) ([]protocol.DocumentChange, error) { +func codeActionChanges(env *integration.Env, uri protocol.DocumentURI, rng protocol.Range, kind protocol.CodeActionKind, diag *protocol.Diagnostic) ([]protocol.DocumentChange, error) { // Request all code actions that apply to the diagnostic. - // (The protocol supports filtering using Context.Only={actionKind} - // but we can give a better error if we don't filter.) + // A production client would set Only=[kind], + // but we can give a better error if we don't filter. params := &protocol.CodeActionParams{ TextDocument: protocol.TextDocumentIdentifier{URI: uri}, Range: rng, Context: protocol.CodeActionContext{ - Only: nil, // => all kinds + Only: []protocol.CodeActionKind{protocol.Empty}, // => all }, } if diag != nil { @@ -2106,7 +2106,7 @@ func codeActionChanges(env *integration.Env, uri protocol.DocumentURI, rng proto // (e.g. refactor.inline.call). var candidates []protocol.CodeAction for _, act := range actions { - if act.Kind == protocol.CodeActionKind(actionKind) { + if act.Kind == kind { candidates = append(candidates, act) } } @@ -2114,7 +2114,7 @@ func codeActionChanges(env *integration.Env, uri protocol.DocumentURI, rng proto for _, act := range actions { env.T.Logf("found CodeAction Kind=%s Title=%q", act.Kind, act.Title) } - return nil, fmt.Errorf("found %d CodeActions of kind %s for this diagnostic, want 1", len(candidates), actionKind) + return nil, fmt.Errorf("found %d CodeActions of kind %s for this diagnostic, want 1", len(candidates), kind) } action := candidates[0]