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

Auto-imports not working in Kate #18363

Closed
rrconkle opened this issue Oct 21, 2024 · 11 comments
Closed

Auto-imports not working in Kate #18363

rrconkle opened this issue Oct 21, 2024 · 11 comments
Labels
A-completion autocompletion C-bug Category: bug

Comments

@rrconkle
Copy link

rust-analyzer version: rust-analyzer 1 (9323b53 2024-10-20)

rustc version: rustc 1.84.0-nightly (662180b34 2024-10-20)

editor or extension: Kate 24.08.2 on Arch linux

Auto-imports not working in Kate

Downgrading to rust-analyzer 2024-09-30 fixes it. In versions after that nothing happens.

@rrconkle rrconkle added the C-bug Category: bug label Oct 21, 2024
@monoid
Copy link

monoid commented Oct 25, 2024

It seems I have the same problem using Emacs + rustic + lsp (latest Doom Emacs, to be exact).

@lnicola lnicola added the A-completion autocompletion label Oct 25, 2024
@justindthomas
Copy link

It seems I have the same problem using Emacs + rustic + lsp (latest Doom Emacs, to be exact).

Same here, using vanilla Emacs from brew. It stopped working a few weeks ago but I hadn't taken the time to research why until now.

@traviscross
Copy link

traviscross commented Nov 11, 2024

Automatically adding imports is broken in Emacs + lsp-mode on master. I've verified this with a fresh installation and minimal configuration in a container image.

I bisected the problem to commit 950bb83 ("Resolve completion items"). Reverting that commit on top of master (currently at aabab29) fixes the problem.

This was part of PR:

Testing procedure: Open a file in Emacs, start LSP, and type:

fn main() {
    let x: Pin// Wait, then select `Pin<..> (use std::pin::Pin) (Struct)`.
}

At and after commit 950bb83, this does not cause Pin to be imported.

cc @Veykril @SomeoneToIgnore

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Nov 11, 2024

So, Kate (and others) seem to declare something in their client completion resolve capabilities, that they are not able to resolve, presumably textEdit?

To quote the spec: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion

Since 3.16.0 the client can signal that it can resolve more properties lazily. This is done using the completionItem#resolveSupport client capability which lists all properties that can be filled in during a ‘completionItem/resolve’ request. All other properties (usually sortText, filterText, insertText and textEdit) must be provided in the textDocument/completion response and must not be changed during resolve.

So, if your editor declares a completion resolve support for the textEdit, rust-analyzer as a server can omit it in the initial response and now it does so.
I do not think it's a rust-analyzer's issue that the client cannot handle the resolution (or its capabilities list) properly.

A proper fix seems to come over each client's completion resolve capabilities and remove the textEdit and other things it is not capable of resolving.

@traviscross
Copy link

Filed for visibility over on the lsp-mode side here:

@SomeoneToIgnore
Copy link
Contributor

Ok, so it is indeed a bug in r-a as it seems.
A quick fix that made lsp-mode in Emacs to work looks like

diff --git a/crates/rust-analyzer/src/handlers/request.rs b/crates/rust-analyzer/src/handlers/request.rs
index d546dfd424..b9bed7ae50 100644
--- a/crates/rust-analyzer/src/handlers/request.rs
+++ b/crates/rust-analyzer/src/handlers/request.rs
@@ -1143,7 +1143,7 @@ pub(crate) fn handle_completion_resolve(
     let Some(mut resolved_completion) = resolved_completions.into_iter().find(|completion| {
         completion.label == original_completion.label
             && completion.kind == original_completion.kind
-            && completion.deprecated == original_completion.deprecated
+            // && completion.deprecated == original_completion.deprecated
             && completion.preselect == original_completion.preselect
             && completion.sort_text == original_completion.sort_text
     }) else {

Certain editors want to resolve tags and this was written incorrectly.

I'm not sure that it's all though, as the whole "find the item with the same label and search score" is bad and there has to be a better id-like thing we could check.
I will look for that thing meanwhile, but reverting this all for now seems more compelling for me.

@SomeoneToIgnore
Copy link
Contributor

A more proper solution #18503 still feels somewhat fragile but much better the current one.
Seems to work for me in Emacs with lsp-mode, but I still cannot invoke any completions in Kate, even with the old, working version so could not check that.

@dbalcomb
Copy link

Is this related to recent completion issues in Zed? I meant to file an issue over at zed-industries/zed last week but traced it back to rust-analyzer sending the wrong completion in completionItem/resolve. Import completions work but, as an example, if I try to import the Config struct from my own crate it comes back with another Config from a dependency instead.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Nov 11, 2024

Yes, that morbid resolved_completions.into_iter().find thing above causes this and the PR seems to fix.
Related Zed issue: zed-industries/zed#20148

@monoid
Copy link

monoid commented Nov 20, 2024

Seems to work with HomeBrew's rust-analyzer 2024-11-17.

@SomeoneToIgnore
Copy link
Contributor

So, seems that both Emacs and Kate issues were fixed and this can be closed.

@lnicola lnicola closed this as completed Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-completion autocompletion C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants