LSP
LSP copied to clipboard
sublimelsp
Split up Code Actions into "Refactors" and "Source Actions"
Is your feature request related to a problem? Please describe.
Currently we bin all code actions into "Code Actions", but servers like
- jdtls
- OmniSharp
have a clear distinction between "Refactorings" and "Source Actions".
Describe the solution you'd like
In case of "Refactorings", these kinds should not always be directly visible to the user. Source Actions should be visible, like they are right now with annotations.
I think Refactorings should go into the "Edit" top menu to be selected with your mouse or in the command palette.
Source Actions should be visible, like they are right now with annotations.
So you want to show them in the same way as normal code actions or in some other way? We already have a Source action... in the right click context menu. Showing those as annotations would not be correct IMO as those don't bind to specific line in the code.
Or even if they are specific to a selected line, they often would show up for every line in the document which wouldn't be good either.
I wonder what specific code actions are we talking here and how would you see them being handled?
Sorry for the late reply. I'm talking about refactorings.
You can see below I get mostly refactoring kinds.
:: --> OmniSharp textDocument/codeAction(5): {'textDocument': {'uri': 'file:///Users/raoulwols/dev/csharp-project/Program.cs'}, 'context': {'diagnostics': []}, 'range': {'end': {'line': 14, 'character': 22}, 'start': {'line': 14, 'character': 22}}}
:: <<< OmniSharp 5: [{'diagnostics': [], 'data': {'$$__handler_id__$$': '971ea130-dc4d-4c43-9b50-74e6076b05e5'}, 'edit': {}, 'kind': 'refactor', 'title': 'Wrap every parameter -> Align wrapped parameters', 'command': {'arguments': [{'Uri': 'file:///Users/raoulwols/dev/csharp-project/Program.cs', 'Identifier': 'Align wrapped parameters', 'Name': 'Wrap every parameter -> Align wrapped parameters', 'Range': {'End': {'Line': 14, 'Character': 22}, 'Start': {'Line': 14, 'Character': 22}}}], 'command': 'omnisharp/executeCodeAction', 'title': 'Wrap every parameter -> Align wrapped parameters'}}, {'diagnostics': [], 'data': {'$$__handler_id__$$': '971ea130-dc4d-4c43-9b50-74e6076b05e5'}, 'edit': {}, 'kind': 'refactor', 'title': 'Wrap every parameter -> Indent all parameters', 'command': {'arguments': [{'Uri': 'file:///Users/raoulwols/dev/csharp-project/Program.cs', 'Identifier': 'Indent all parameters', 'Name': 'Wrap every parameter -> Indent all parameters', 'Range': {'End': {'Line': 14, 'Character': 22}, 'Start': {'Line': 14, 'Character': 22}}}], 'command': 'omnisharp/executeCodeAction', 'title': 'Wrap every parameter -> Indent all parameters'}}, {'diagnostics': [], 'data': {'$$__handler_id__$$': '971ea130-dc4d-4c43-9b50-74e6076b05e5'}, 'edit': {}, 'kind': 'refactor', 'title': 'Wrap every parameter -> Indent wrapped parameters', 'command': {'arguments': [{'Uri': 'file:///Users/raoulwols/dev/csharp-project/Program.cs', 'Identifier': 'Indent wrapped parameters', 'Name': 'Wrap every parameter -> Indent wrapped parameters', 'Range': {'End': {'Line': 14, 'Character': 22}, 'Start': {'Line': 14, 'Character': 22}}}], 'command': 'omnisharp/executeCodeAction', 'title': 'Wrap every parameter -> Indent wrapped parameters'}}, {'diagnostics': [], 'data': {'$$__handler_id__$$': '971ea130-dc4d-4c43-9b50-74e6076b05e5'}, 'edit': {}, 'kind': 'refactor', 'title': 'Unwrap and indent all parameters', 'command': {'arguments': [{'Uri': 'file:///Users/raoulwols/dev/csharp-project/Program.cs', 'Identifier': 'Unwrap and indent all parameters', 'Name': 'Unwrap and indent all parameters', 'Range': {'End': {'Line': 14, 'Character': 22}, 'Start': {'Line': 14, 'Character': 22}}}], 'command': 'omnisharp/executeCodeAction', 'title': 'Unwrap and indent all parameters'}}, {'diagnostics': [], 'data': {'$$__handler_id__$$': '971ea130-dc4d-4c43-9b50-74e6076b05e5'}, 'edit': {}, 'kind': 'refactor', 'title': 'Use expression body for methods', 'command': {'arguments': [{'Uri': 'file:///Users/raoulwols/dev/csharp-project/Program.cs', 'Identifier': 'Use expression body for methods', 'Name': 'Use expression body for methods', 'Range': {'End': {'Line': 14, 'Character': 22}, 'Start': {'Line': 14, 'Character': 22}}}], 'command': 'omnisharp/executeCodeAction', 'title': 'Use expression body for methods'}}, {'diagnostics': [], 'data': {'$$__handler_id__$$': '971ea130-dc4d-4c43-9b50-74e6076b05e5'}, 'edit': {}, 'kind': 'refactor.extract', 'title': 'Extract base class...', 'command': {'arguments': [{'Uri': 'file:///Users/raoulwols/dev/csharp-project/Program.cs', 'Identifier': 'Extract base class...', 'Name': 'Extract base class...', 'Range': {'End': {'Line': 14, 'Character': 22}, 'Start': {'Line': 14, 'Character': 22}}}], 'command': 'omnisharp/executeCodeAction', 'title': 'Extract base class...'}}]
Unwrapped:
[
{
"diagnostics": [],
"data": {
"$$__handler_id__$$": "971ea130-dc4d-4c43-9b50-74e6076b05e5"
},
"edit": {},
"kind": "refactor",
"title": "Wrap every parameter -> Align wrapped parameters",
"command": {
"arguments": [
{
"Uri": "file:///Users/raoulwols/dev/csharp-project/Program.cs",
"Identifier": "Align wrapped parameters",
"Name": "Wrap every parameter -> Align wrapped parameters",
"Range": {
"End": {
"Line": 14,
"Character": 22
},
"Start": {
"Line": 14,
"Character": 22
}
}
}
],
"command": "omnisharp/executeCodeAction",
"title": "Wrap every parameter -> Align wrapped parameters"
}
},
{
"diagnostics": [],
"data": {
"$$__handler_id__$$": "971ea130-dc4d-4c43-9b50-74e6076b05e5"
},
"edit": {},
"kind": "refactor",
"title": "Wrap every parameter -> Indent all parameters",
"command": {
"arguments": [
{
"Uri": "file:///Users/raoulwols/dev/csharp-project/Program.cs",
"Identifier": "Indent all parameters",
"Name": "Wrap every parameter -> Indent all parameters",
"Range": {
"End": {
"Line": 14,
"Character": 22
},
"Start": {
"Line": 14,
"Character": 22
}
}
}
],
"command": "omnisharp/executeCodeAction",
"title": "Wrap every parameter -> Indent all parameters"
}
},
{
"diagnostics": [],
"data": {
"$$__handler_id__$$": "971ea130-dc4d-4c43-9b50-74e6076b05e5"
},
"edit": {},
"kind": "refactor",
"title": "Wrap every parameter -> Indent wrapped parameters",
"command": {
"arguments": [
{
"Uri": "file:///Users/raoulwols/dev/csharp-project/Program.cs",
"Identifier": "Indent wrapped parameters",
"Name": "Wrap every parameter -> Indent wrapped parameters",
"Range": {
"End": {
"Line": 14,
"Character": 22
},
"Start": {
"Line": 14,
"Character": 22
}
}
}
],
"command": "omnisharp/executeCodeAction",
"title": "Wrap every parameter -> Indent wrapped parameters"
}
},
{
"diagnostics": [],
"data": {
"$$__handler_id__$$": "971ea130-dc4d-4c43-9b50-74e6076b05e5"
},
"edit": {},
"kind": "refactor",
"title": "Unwrap and indent all parameters",
"command": {
"arguments": [
{
"Uri": "file:///Users/raoulwols/dev/csharp-project/Program.cs",
"Identifier": "Unwrap and indent all parameters",
"Name": "Unwrap and indent all parameters",
"Range": {
"End": {
"Line": 14,
"Character": 22
},
"Start": {
"Line": 14,
"Character": 22
}
}
}
],
"command": "omnisharp/executeCodeAction",
"title": "Unwrap and indent all parameters"
}
},
{
"diagnostics": [],
"data": {
"$$__handler_id__$$": "971ea130-dc4d-4c43-9b50-74e6076b05e5"
},
"edit": {},
"kind": "refactor",
"title": "Use expression body for methods",
"command": {
"arguments": [
{
"Uri": "file:///Users/raoulwols/dev/csharp-project/Program.cs",
"Identifier": "Use expression body for methods",
"Name": "Use expression body for methods",
"Range": {
"End": {
"Line": 14,
"Character": 22
},
"Start": {
"Line": 14,
"Character": 22
}
}
}
],
"command": "omnisharp/executeCodeAction",
"title": "Use expression body for methods"
}
},
{
"diagnostics": [],
"data": {
"$$__handler_id__$$": "971ea130-dc4d-4c43-9b50-74e6076b05e5"
},
"edit": {},
"kind": "refactor.extract",
"title": "Extract base class...",
"command": {
"arguments": [
{
"Uri": "file:///Users/raoulwols/dev/csharp-project/Program.cs",
"Identifier": "Extract base class...",
"Name": "Extract base class...",
"Range": {
"End": {
"Line": 14,
"Character": 22
},
"Start": {
"Line": 14,
"Character": 22
}
}
}
],
"command": "omnisharp/executeCodeAction",
"title": "Extract base class..."
}
}
]
I think code actions of type "refactoring" should not be presented in clickable annotations or light bulbs. Those annotations/bulbs are meant for "quick fixes". Whereas "refactorings" don't actually fix anything (by design).
Instead, we should have a separate menu entry at the top menu bar called "Refactorings" that allow things like "Extract Method", "Move Class", etc. Although I don't know how to discover the various types of refactorings dynamically like that but I'm thinking out loud.
See also https://www.jetbrains.com/help/idea/refactoring-source-code.html
I was thinking about this, but I wonder whether there are some misunderstandings in the preceding comments:
In case of "Refactorings", these kinds should not always be directly visible to the user. Source Actions should be visible, like they are right now with annotations.
Shouldn't it be the exact opposite of that? "Source Actions" (i.e. kind source) are not specific to a particular line, so they shouldn't be shown in the annotation (or lightbulb), but instead go into the top menu under "Edit". See also the description in the LSP specs:
/**
* Base kind for source actions: `source`.
*
* Source code actions apply to the entire file.
*/
export const Source: [CodeActionKind](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeActionKind) = 'source';
On the other hand "Refactorings" (kind refactor) only apply to one particular function, or class etc., so they depend on the current cursor position.
It could be argued whether they should be shown in the view or only as an entry under the "Edit" top menu. Maybe the latter would be better - then only code actions of the kind quickfix, (and actions which don't have any kind specified) would be shown in the view.
We already have a
Source action...in the right click context menu. Showing those as annotations would not be correct IMO as those don't bind to specific line in the code.
The "Source" actions are currently shown in the view (!) (Not sure whether this behavior maybe was changed since that comment above was made.) So with the explanation above, I think they should be removed from the view and also added to another entry under "Edit", with running the request for code actions of that kind for the entire content range, when selected that way.
Furthermore I was looking through the source code, and I think there is a bug in this part for the code actions on save: https://github.com/sublimelsp/LSP/blob/509e4d599a5895727525a89d8963bedae4f57e16/plugin/code_actions.py#L146-L148
codeActionKinds is an optional property for the codeActionProvider server capability, and if the server doesn't provide this list, that does not mean that the server doesn't support the CodeAction.kind property in the response. So a request should also be made for sessions which don't declare codeActionProvider.codeActionKinds, and the response should simply be filtered with the matching kinds afterwards.
This statement is based on the following response: https://github.com/microsoft/language-server-protocol/issues/890#issuecomment-725504061
which I would interpret in a way that codeActionProvider.codeActionKinds should only be used to filter out when requests are unnecessary to sent, but not as a requirement that this property needs to exist for sending the request for code actions on save.
The "Source" actions are currently shown in the view (!) (Not sure whether this behavior maybe was changed since that comment above was made.)
I haven't looked at the LSP code but checking with typescript-language-server I'm getting Organize Imports when clicking the Source action... menu but not in the view anywhere.
EDIT:
typescript server only responds with source-level refactor if explicitly asked for it through kind.
So a request should also be made for sessions which don't declare
codeActionProvider.codeActionKinds, and the response should simply be filtered with the matching kinds afterwards.
What would we filter against when the server wouldn't provide codeActionKinds? Would we filter against source.* when triggering the Source action... menu? I suppose that makes sense.
As for on-save-actions do you mean we should do something like:
matching_kinds = get_matching_kinds(on_save_actions, supported_kinds) if supported_kinds != None else on_save_action_kinds
?
What would we filter against when the server wouldn't provide
codeActionKinds? Would we filter againstsource.*when triggering theSource action...menu?
Yes, I would filter against source.* (meaning just source would be included, as well as source.foobar), when manually triggering "Source actions..." via main menu or right-click context menu. For the code actions on save, the returned actions should be filtered against only the (sub-)kinds of what the user has in their settings file. So source.fixAll.foobar would be run if the user has source.fixAll activated, but source.foobar would be ignored. And if a returned action has no kind set, it would also be ignored.
Also there seems to be some oddities going on when using the "Source actions..." context menu. With LSP-marksman installed, I get a "Create Table of Contents" action shown in the view (lightbulb or annotation), on any line / cursor position (which is a bit annyoing in my opinion). From the logs I can see that this has the source kind. When I click "Code Action..." from the context menu, a request is made and I can select this same action from the command palette (OK). But when I click "Source Action..." from the context menu, no request is being made, and I don't get any result. In this case I would expect LSP to make a request with "only": ["source"] for the context parameter, hopefully the server to respond with this code action, and show it in the command palette.
I haven't investigated how the caching for code actions works and what's exactly happening here.
As for on-save-actions do you mean we should do something like:
matching_kinds = get_matching_kinds(on_save_actions, supported_kinds) if supported_kinds != None else on_save_action_kinds
Yes
Yes, I would filter against
source.*(meaning justsourcewould be included, as well assource.foobar), when manually triggering "Source actions..." via main menu or right-click context menu.
Agreed. This needs to be fixed. (https://github.com/sublimelsp/LSP/pull/2064)
For the code actions on save, the returned actions should be filtered against only the (sub-)kinds of what the user has in their settings file. So
source.fixAll.foobarwould be run if the user hassource.fixAllactivated, butsource.foobarwould be ignored. And if a returned action has no kind set, it would also be ignored.
I think that in the "on save" case we should actually still be intersecting user's code_actions_on_save and codeActionKinds' reported by the server and only send the request if those are matching. At least that's how VSCode behaves.
Also there seems to be some oddities going on when using the "Source actions..." context menu. With LSP-marksman installed, I get a "Create Table of Contents" action shown in the view (lightbulb or annotation)
I guess that either marksman should not be returning source actions until explicitly asked for those or we should be requesting view code actions with an explicit list of kinds that doesn't include source.
But looking at VSCode again (de-facto standard), it doesn't request with only kinds specified when requesting code actions for current selection so I think marksman is behaving incorrectly here.
Also see marksman issue I've just created: https://github.com/artempyanykh/marksman/issues/90