-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LSP always indicates possible actions, even if they are no-op #91
Comments
I think that makes sense. I'd have to look into what we would do in VS Code (where the actions should probably still show up if you search in the action menu). |
I definitely lack context when it comes to |
I don't really understand which convention you mean. I've used a lot of other LSPs on neovim and I've never seen any show unavailable actions. Plugins like nvim-lightbulb that show an indicator if the position under the cursor has available code actions. I understand these are inspired by similar plugins in VSCode. Wouldn't this convention break these on VSCode too? If you have an example of another LSP that does the same, I can try out if it behaves different to ruff-lsp in any way. Honestly, the flow right now feels like this:
Informing me that there are code actions when the only possible flow is no-op can lead nowhere. |
|
Sure thing, Ruff is mostly modeled after the ESLint and isort VS Code extensions. And what I was trying to convey earlier is that those extensions register the relevant code actions regardless of the state of the code. So, e.g., isort registers as an Another important distinction here though is that none of these are tied to diagnostics. They're not associated with any specific part of the code, so I'm surprised they show up inline for you, but I'm guessing it's something that's been configured. |
If you're looking for consistency, maybe it's best to follow what other LSPs do, instead of follow what other VSCodeExtensions do.
I've understood this part, and I would consider this a bug, not a feature worth imitating.
This sounds like someone just took the lazy route TBH and didn't bother to register/deregister based on availability.
You don't consider this a bug? Ruff has a lot of other code actions, registering them when unavailable would be a bug, right? It just adds noise. It's like having a "format all files" button enabled when there are no files open. Sure, technically you can format all files (formatting an empty set is no-op), but it's just not good UX.
I don't have any special configuration. If you have any example of another LSP that has a similar behavior I can give it a try and see how it behaves on this same setup. |
I'll have to take a look at what other similar tools do before responding further. To be clear, we're talking about two actions here, right? "Ruff: Fix all auto-fixable violations" and "Organize Imports"? |
Do you mind including a screenshot of what you see in your editor? |
Sure, here's a couple of screenshots. The light bulb on the right indicates "code action available for this context": As a different example, here's Note that if I move the cursor onto the line where an action is availble, the same lightbulb indicator appears: This other line also has code actions available (in this case, "merge imports"): This last case is the scenario where the lightbulb is useful: there's no diagnostic or anything alike, so the lightbulb lets me know that there's actions available (otherwise, they'd be impossible to discover). This is all done by this plugin.
Correct. These are the only two that show up permanently. |
Please ignore |
I'll add my two cents from a perspective of someone who maintains a (Sublime Text) LSP client and at least one LSP server. The two code actions that are being discussed here are code actions of a Those should be presented when explicitly invoking "Source action..." from a menu item or a command palette entry. So I would say that the behavior of neovim (or whichever editor we are talking about here) is not ideal as it should just filter those out. But at the same time, the server shouldn't really need to return those for those "basic" requests. What's the difference between client doing the "basic" request and doing a request on invoking "Source action..."? In the latter case the |
Right, in neovim's case, these would be the ones listed when some mapping triggers
I don't see any references of
I think this behaviour should yield the results that @charliermarsh expects on Code/Codium. |
@rchl When a client specifies |
It depends. If the server can quickly and cheaply answer the question "are there any fixes available?" then I would expect that it only returns the action if there are fixes available. Potentially even with a text edit already to avoid another round trip to the server. But for many servers this can be rather expensive operation (especially for "organize imports") so then it's fine to always return a code action (that calls a 'command' rather than returning an "edit") that potentially does nothing on invoking. |
Actually, ruff doesn't always show "fix import" as a code action. If the cursor is on top of an import warning, there are no code actions available. edit: missing word |
I haven't read the entire discussion, but from the mentioned problem description I think we have the capability to differentiate between code actions and commands (context: #119 (comment)). Code actions generate commands to be executed but the commands can exists independently without the need to generate the code actions.
I'm not exactly sure about PyCharm, but I believe that should be using the |
LSP clients request code actions for the current cursor position automatically (implicitly) and those code actions should be specific to the cursor position and should not return source-level code actions. Most clients also allow the user to request code actions manually and those typically return source level code actions (like "organize imports" or "fix all issues"). Additionally, clients often expose "source" code actions ( Only supporting those through |
Having a similar issue here. The two code actions |
The mentioned code actions are source level code actions (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeActionKind), so you can filter them out when sending the code action request. I'm not sure how to do it with LspSaga but the LSP API provides a way to ask only for specific kinds of code actions which can be used to do what you want. Maybe this might help: here's what I have in my dotfiles which is a minimal implementation of the lightbulb and here's how to register them. |
@WhyNotHugo Can you inform us which editor are you referring to? I don't think the problem here is in the server itself but rather the implementation of the lightbulb functionality. The implementation of lightbulb in VS Code only requests the quickfix code actions (https://github.com/microsoft/vscode/blob/246d700c4604eb5ebdbb561a1a86562bf9217a62/src/vs/workbench/contrib/markers/browser/markersTreeViewer.ts#L639) and this is how it should be. Now, the reason it shows up in the code action requests is because the client didn't apply any filter and the server lazily computes the source level code actions through the resolve request for code action. The flow would be something like:
|
I am using neovim. I just ended up filtering the two always-on code
actions.
…On March 27, 2024 at 14:36:58, Dhruv Manilawala ***@***.***) wrote:
@WhyNotHugo <https://github.com/WhyNotHugo> Can you inform us which
editor are you referring to?
I don't think the problem here is in the server itself but rather the
implementation of the lightbulb functionality. The implementation of
lightbulb in VS Code only requests the quickfix code actions (
https://github.com/microsoft/vscode/blob/246d700c4604eb5ebdbb561a1a86562bf9217a62/src/vs/workbench/contrib/markers/browser/markersTreeViewer.ts#L639)
and this is how it should be.
Now, the reason it shows up in the code action requests is because the
client didn't apply any filter and the server lazily computes the source
level code actions through the resolve request for code action. The flow
would be something like:
1. Client asks for *all* code actions
2. Server returns all of them, including source code actions. But, the
source code actions doesn't contain the edit
3. User wants to apply a source code action
4. Client sees that there's no edit available, so it asks the server
to compute the edit via codeAction/resolve request
5. Server computes the edit and the client applies it
—
Reply to this email directly, view it on GitHub
<#91 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARYQB24N6AEEHS4H5CIM4DY2JLHVAVCNFSM6AAAAAAWNVVJP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRRHE4DGNBWGU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I'm using neovim.
What do you mean here by "the edit"? Nothing is being edited in this issue. I didn't include proper reproduction steps previous, so here goes:
Both of these actions are no-ops. E.g.: they do nothing. They're are relevant as showing a "trim flowers" action; there are no flowers to trim. I could write some logic on the client to hide these actions. However, if I hide them they also won't show up then they are actually valid. |
@WhyNotHugo Apologies if my message wasn't clear. Now that I read it again, it does seem that it assumes that the reader has some familiarity with the LSP terminology 😅. I'll try to clear them up but what I was trying to say is that this is probably not an issue in the server implementation. Before that, I want to get a clear idea about the issue. Is it that the:
If it's (1), then I believe the issue is with the lightbulb implementation (refer nvimdev/lspsaga.nvim#1413). I've provided a reference implementation for VS Code lightbulb in the linked issue. If it's (2), then I'll have to think more about it and possibly look at the LSP specification and implementation from other servers.
By "the edit", I'm referring to the
Can you clarify what do you mean by "fix import" here? I think that's a quickfix code action kind and not a source code action kind (
Currently, @snowsignal is working towards implementing Ruff's language server natively in Rust. I believe this could potentially avoid this by querying the cached diagnostics. |
It's both really. The lightbulb indicates that some any actions are available. The suggestion in nvimdev/lspsaga.nvim#1413 would be problematic for any other LSP; the "actions available" indicator would not show for some types of actions, which would make them impossible to discover (one would have to "guess" that they are available). A key detail here is that no other language server (rust_analyzer, volar, tsserver, lua-ls, gopls, etc) has this issue. They only show actions that are "really" available.
The code actions show as follows when calling
I'm not sure what kind of action they are internally. I only know that they are listed as available, even in, for example, a file with no imports.
Perhaps it's best to merely keep this issue in scope with the rewrite, and not bother fixing it in the current implementation (it sounds like fixing it is non-trivial?). Mainly, I think that:
|
Problem description
My editor setup shows an indicator when there are code actions available for the current line.
This works well on most languages, but ruff always shows available actions:
Organize Imports
andFix All
. Even if they are both no-op.This kind of negates the value of code actions entirely, because instead of being able to tell at a glace if any are possible, the answer is always yes (though generally, they're no-op actions).
Possible solution
Only show code-actions available if any are really available. E.g.: if any imports can be sorted or if there's actually anything to fix.
The text was updated successfully, but these errors were encountered: