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

LSP fail to auto-complete the import #10787

Closed
Luv-Ray opened this issue May 18, 2024 · 5 comments · Fixed by #10873
Closed

LSP fail to auto-complete the import #10787

Luv-Ray opened this issue May 18, 2024 · 5 comments · Fixed by #10873
Labels
A-language-server Area: Language server client C-bug Category: This is a bug
Milestone

Comments

@Luv-Ray
Copy link
Contributor

Luv-Ray commented May 18, 2024

Summary

I update my helix from 2209eff to d8701bf, then this bug happens.

When using TAB to select an item to auto-complete, it may also auto-complete the import (like use std::sync::Arc)
image
But when I press enter, nothing happens and it didn't auto-complete the import.
image

Also, when I trying to import the module (like std::sync), it works fine.
image

I know this looks very much like an LSP error, but it works fine at 2209eff without any LSP change. So I guess it's a helix bug.

Reproduction Steps

I tried this:

  1. hx
  2. input following and press enter behind Arc:
fn main() {
    Arc
}

I expected this to happen:
import use std::sync::Arc at beginning

Instead, this happened:
nothing happens

Helix log

~/.cache/helix/helix.log
empty

Platform

Arch Linux

Terminal Emulator

warp-terminal v0.2024.05.14.08.01.stable_04

Installation Method

source

Helix Version

helix 24.3 (d8701bf)

@Luv-Ray Luv-Ray added the C-bug Category: This is a bug label May 18, 2024
@kirawi kirawi added the A-language-server Area: Language server client label May 18, 2024
@RoloEdits
Copy link
Contributor

Seems it started here #10539

@RoloEdits
Copy link
Contributor

As a quick scout I found that commenting out this early return check brought back the auto-import:

let needs_resolve = item.item.documentation.is_none()
|| item.item.detail.is_none()
|| item.item.additional_text_edits.is_none();
if !needs_resolve {
item.resolved = true;
return;
}

@Luv-Ray
Copy link
Contributor Author

Luv-Ray commented May 19, 2024

I find that when importing std::sync the item.detail.is_none() is true, and when importing std::sync::Arc it's false and then the function early returned. This explain why importing modules works fine.

Maybe we should remove needs_resolve and set item.resolved to true only after the ResolveRequest has been sent.

@pascalkuthe
Copy link
Member

pascalkuthe commented May 19, 2024

The imports come from additionTextEdits and we check wehter that is none too to determine wether we need to resolve. VSCode has similar logic. It's wasteful to resolve requests that don't need to be resolved.

@kladd
Copy link

kladd commented May 19, 2024

Not familiar with LSP, but it looks like additionalTextEdits is set in the response to completionItem/resolve, but not in the response to textDocument/completion. This early return prevents the completionItem/resolve call from happening.

Edit: Maybe I misread this condition. additional_texts_edits is not None because it's Some([]).


textDocument/completion:

rust-analyzer <- {
  "jsonrpc": "2.0",
  "id": 6,
  "result": {
    "isIncomplete": true,
    "items": [
      {
        "label": "Subcommand (use clap::Subcommand)", "kind": 3, "detail": "proc_macro Subcommand", ...
        "additionalTextEdits": [],
        "data": {
            "position": { ... },
            "imports": [{"full_import_path": "clap::Subcommand", "imported_name": "Subcommand"}]
        }
      }, ...

ensure_item_resolved() returns, but without this the completion resolve response contains additionalTextEdits:

rust-analyzer -> {
  "jsonrpc": "2.0",
  "method": "completionItem/resolve",
  "params": {
    "additionalTextEdits": [],
    "data": {
      "imports": [{"full_import_path": "clap::Subcommand", "imported_name": "Subcommand"}], ...
    }, ...
}

and response, with additionalTextEdits

rust-analyzer <- {
  "jsonrpc": "2.0",
  "id": 7,
  "result": {
    "label": "Subcommand (use clap::Subcommand)",
    "kind": 3,
    "detail": "proc_macro Subcommand",

    "additionalTextEdits": [ { "range": { ... }. "newText": "use clap::Subcommand;\n\n" } ]
    ...
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants