Skip to content
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

Remove lens.forceCustomCommands config #17643

Closed
Veykril opened this issue Jul 20, 2024 · 2 comments · Fixed by #17675
Closed

Remove lens.forceCustomCommands config #17643

Veykril opened this issue Jul 20, 2024 · 2 comments · Fixed by #17675
Assignees
Labels
A-config configuration A-lsp LSP conformance issues and missing features C-bug Category: bug E-easy

Comments

@Veykril
Copy link
Member

Veykril commented Jul 20, 2024

          > Is there any impact of this other than an unnecessary round trip? I'm trying to understand what the error / problem is here, but I'm not seeing it.

That depends on the client, we are violating the LSP (or in this case the lsp extension I suppose), we shouldn't be doing that.

The problem is this config being enabled by default for some reason https://github.com/veykril/rust-analyzer/blob/09ef75c717e9ffb4cbc71ce1cfec7694244742c2/crates/rust-analyzer/src/config.rs#L564 which then force enables all of the commands here https://github.com/veykril/rust-analyzer/blob/09ef75c717e9ffb4cbc71ce1cfec7694244742c2/crates/rust-analyzer/src/config.rs#L1896-L1911
We should just get rid of lens_forceCustomCommands, I see no value in it for debugging.

Originally posted by @Veykril in #17587 (comment)

@Veykril Veykril added E-easy C-bug Category: bug A-lsp LSP conformance issues and missing features A-config configuration labels Jul 20, 2024
@joshka
Copy link
Contributor

joshka commented Jul 20, 2024

For context, the original reason for this setting being set to true is documented in Veykril@be84f85#diff-779cb31c0defa0631d570725eb4ca124c0860d6ec07bb5b9deddc2dd34966315 in July 2021:

feat: gate custom clint-side commands behind capabilities

Some features of rust-analyzer requires support for custom commands on
the client side. Specifically, hover & code lens need this.

Stock LSP doesn't have a way for the server to know which client-side
commands are available. For that reason, we historically were just
sending the commands, not worrying whether the client supports then or
not.

That's not really great though, so in this PR we add infrastructure for
the client to explicitly opt-into custom commands, via extensions
field of the ClientCapabilities.

To preserve backwards compatability, if the client doesn't set the
field, we assume that it does support all custom commands. In the
future, we'll start treating that case as if the client doesn't support
commands.

So, if you maintain a rust-analyzer client and implement
rust-analyzer/runSingle and such, please also advertise this via a
capability.

@PaulDotSH
Copy link
Contributor

@rustbot claim

bors added a commit that referenced this issue Jul 23, 2024
…nfig, r=lnicola

Remove lens.forceCustomCommands config

Closes #17643
A very simple PR that removes the lens.forceCustomCommands config feature without side effects.
@bors bors closed this as completed in b2e3cec Jul 23, 2024
lnicola pushed a commit to lnicola/rust that referenced this issue Jul 28, 2024
…mmands-config, r=lnicola

Remove lens.forceCustomCommands config

Closes rust-lang/rust-analyzer#17643
A very simple PR that removes the lens.forceCustomCommands config feature without side effects.
RalfJung pushed a commit to RalfJung/rust that referenced this issue Aug 1, 2024
…mmands-config, r=lnicola

Remove lens.forceCustomCommands config

Closes rust-lang/rust-analyzer#17643
A very simple PR that removes the lens.forceCustomCommands config feature without side effects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-config configuration A-lsp LSP conformance issues and missing features C-bug Category: bug E-easy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants