astral-sh / ruff-lsp

A Language Server Protocol implementation for Ruff.
Other
1.34k stars 46 forks source link

LSP always indicates possible actions, even if they are no-op #91

Open WhyNotHugo opened 1 year ago

WhyNotHugo commented 1 year ago

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 and Fix 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.

charliermarsh commented 1 year ago

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).

WhyNotHugo commented 1 year ago

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 code, but why would you want it to show up when unavailable/no-op?

charliermarsh commented 1 year ago

It's mostly about convention and expectation. If I open up a command palette in an editor, I expect those kinds of code actions to be available even if they're no-ops / idempotent. The same is true in PyCharm: Organize Imports always shows up, even if the imports are already organized.

Screen Shot 2023-04-03 at 3 44 46 PM

WhyNotHugo commented 1 year ago

It's mostly about convention and expectation.

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.

WhyNotHugo commented 1 year ago

rust_analyzer does import-formatting-and-sorting as part of the format code request, not as a code action. But I'm not sure if I'd consider imports in python as just formatting; the order of imports does have side effects.

charliermarsh commented 1 year ago

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 organizeImports handler, and you can always hit "Organize Imports" in the command palette to trigger it. Similarly, ESLint always shows the "Fix all auto-fixable problems" command, regardless of whether there are any errors in the file. They register the code actions unconditionally.

Screen Shot 2023-04-03 at 6 34 05 PM

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.

WhyNotHugo commented 1 year ago

Sure thing, Ruff is mostly modeled after the ESLint and isort VS Code extensions.

If you're looking for consistency, maybe it's best to follow what other LSPs do, instead of follow what other VSCodeExtensions do.

So, e.g., isort registers as an organizeImports handler, and you can always hit "Organize Imports" in the command palette to trigger it.

I've understood this part, and I would consider this a bug, not a feature worth imitating.

Similarly, ESLint always shows the "Fix all auto-fixable problems" command, regardless of whether there are any errors in the file.

This sounds like someone just took the lazy route TBH and didn't bother to register/deregister based on availability.

They register the code actions unconditionally.

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.

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.

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.

charliermarsh commented 1 year ago

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"?

charliermarsh commented 1 year ago

Do you mind including a screenshot of what you see in your editor?

WhyNotHugo commented 1 year ago

Sure, here's a couple of screenshots. The light bulb on the right indicates "code action available for this context":

image

As a different example, here's rust_analyzer, where actions are available elsewhere, but not at the cursor position:

image

Note that if I move the cursor onto the line where an action is availble, the same lightbulb indicator appears:

image

This other line also has code actions available (in this case, "merge imports"):

image

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.

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"?

Correct. These are the only two that show up permanently.

WhyNotHugo commented 1 year ago

Please ignore [Fzf-lua] No code actions found in the status line in the screenshots; that showed up when I tried to manually activate an action on a line that had none, and persists until something else writes into the status line.

rchl commented 1 year ago

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 source.* kind. Unlike the quickfix.* code actions, those are not really meant to be presented in the editor when doing a "default" code action request performed on cursor changes. ST client just filters those out as does VSCode too (which is the base for ST behavior here).

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 "only": ["source"] property is set on the context object. And speaking of that, it seems like "ruff-lsp" has a bug here as it doesn't return any actions in that case, even though it does return both when only is not set.

WhyNotHugo commented 1 year ago

Those should be presented when explicitly invoking "Source action..." from a menu item or a command palette entry.

Right, in neovim's case, these would be the ones listed when some mapping triggers vim.lsp.buf.code_action().

In the latter case the "only": ["source"] property is set on the context object. And speaking of that, it seems like "ruff-lsp" has a bug here as it doesn't return any actions in that case, even though it does return both when only is not set.

I don't see any references of nvim-lightbulb setting this. I think it should, so I'll try and see what I can do on that end.

And speaking of that, it seems like "ruff-lsp" has a bug here as it doesn't return any actions in that case, even though it does return both when only is not set.

I think this behaviour should yield the results that @charliermarsh expects on Code/Codium.

WhyNotHugo commented 1 year ago

@rchl When a client specifies 'source.fixAll', do you expect the action to be returned in the server's response as available if the action is currently a no op (e.g.: all = empty set).

rchl commented 1 year ago

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.

WhyNotHugo commented 1 year ago

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

dhruvmanila commented 1 year ago

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: https://github.com/astral-sh/ruff-lsp/issues/119#issuecomment-1595628355). Code actions generate commands to be executed but the commands can exists independently without the need to generate the code actions.

... The same is true in PyCharm: Organize Imports always shows up, even if the imports are already organized.

I'm not exactly sure about PyCharm, but I believe that should be using the executeCommand request to perform the action instead of using code actions. Code actions are mostly meant to be for a specific range of the document and they shouldn't contain anything if there aren't any actions available.

rchl commented 1 year ago

but I believe that should be using the executeCommand request to perform the action instead of using code actions. Code actions are mostly meant to be for a specific range of the document and they shouldn't contain anything if there aren't any actions available.

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 (source.*) in a separate context menu for the user to see and execute those at will.

Only supporting those through executeCommand would mean that LSP client would have to implement a server-specific glue code for executing those actions as there is no way for the server to specify the schema of such actions.

yasserfarouk commented 9 months ago

Having a similar issue here. The two code actions Organize Imports and Fix All are ALWAYS available at every line which means I get the lightbulb (From LspSaga) everywhere. Is there a way to filter out these two code actions? I am OK with never having them even when they are going to do something because import organization already happens whenever the code is formatted and Fix All can always be invoked explicitly.

dhruvmanila commented 9 months ago

I am OK with never having them even when they are going to do something because import organization already happens whenever the code is formatted and Fix All can always be invoked explicitly.

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.

dhruvmanila commented 8 months ago

@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
yasserfarouk commented 8 months ago

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 https://github.com/astral-sh/ruff-lsp/issues/91#issuecomment-2021983465, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARYQB24N6AEEHS4H5CIM4DY2JLHVAVCNFSM6AAAAAAWNVVJP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRRHE4DGNBWGU . You are receiving this because you commented.Message ID: @.***>

WhyNotHugo commented 8 months ago

Can you inform us which editor are you referring to?

I'm using neovim.

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:

  • Client asks for all code actions
  • Server returns all of them, including source code actions. But, the source code actions doesn't contain the edit
  • User wants to apply a source code action
  • Client sees that there's no edit available, so it asks the server to compute the edit via codeAction/resolve request
  • Server computes the edit and the client applies it

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.

dhruvmanila commented 8 months ago

@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:

  1. Lightbulb is shown on every line
  2. Two source-level code actions (source.fixAll and source.organizeImports) are always shown in the dropdown when vim.lsp.buf.code_action is invoked

If it's (1), then I believe the issue is with the lightbulb implementation (refer https://github.com/nvimdev/lspsaga.nvim/issues/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.

What do you mean here by "the edit"? Nothing is being edited in this issue.

By "the edit", I'm referring to the WorkspaceEdit in the code action response.

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.

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 (CodeActionKind reference).

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.

ruff-lsp computes these source code actions lazily as described by @rchl above. This is what I was trying to say with the steps mentioned in my comment. It uses the codeAction/resolve capability to resolve these source code actions only when the user requests for it. This is when you select the "Ruff: Organize imports" action from the code action dropdown.

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.

WhyNotHugo commented 8 months ago

Before that, I want to get a clear idea about the issue. Is it that the:

  1. Lightbulb is shown on every line
  2. Two source-level code actions (source.fixAll and source.organizeImports) are always shown in the dropdown when vim.lsp.buf.code_action is invoked

It's both really. The lightbulb indicates that some any actions are available. vim.lsp.buf.code_action() indicates which actions are available. In other words, (1) is merely a visual representation of the state of (2).

The suggestion in https://github.com/nvimdev/lspsaga.nvim/issues/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.

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 (CodeActionKind reference).

The code actions show as follows when calling vim.lsp.buf.code_action():

  1. Ruff: Organize Imports
  2. Ruff: Fix All

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.

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.

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: