[red-knot] add auto-completion MVP to LSP#17744
Conversation
BurntSushi
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
|
Demo: red-knot-auto-completion-mvp.mp4 |
BurntSushi
requested review from
dhruvmanila
and removed request for
AlexWaygood,
carljm,
dcreager and
sharkdp
BurntSushi
changed the title
|
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Looks reasonable to me, but I'd rather have @dhruvmanila or @MichaReiser look at the LSP side.
My main concern here is that I would probably prefer to release the alpha with no autocomplete than with autocomplete that doesn't make use of type information. But we can always ship a quick diff to turn it off right before the alpha release, if we don't get far enough.
|
@carljm Would you be okay to have it disabled by default, but allow an env var or some other undocumented toggle enable it? Just for practical purposes of keeping it in tree and being able to iterate. |
Oh for sure, that's what I was imagining, definitely not that we'd rip out the code entirely. I'm fine with doing that now, or waiting and seeing where we're at with it end of next week. |
|
SGTM! |
|
We could do something similar to what r-a does with their "experimental" capabilities which requires explicit opt-in from client side. Refer to the first paragraph in https://github.com/rust-lang/rust-analyzer/blob/master/docs/book/src/contributing/lsp-extensions.md#lsp-extensions:
Specifically, here they send the capabilities that the VS Code extension supports. So, for us it could look like below which would be sent when during the server initialization process: "capabilities": {
"experimental": {
"completions": {
"enabled": true
}
}
} |
MichaReiser
changed the title
| .collect() | ||
| } | ||
|
|
||
| fn identifiers(node: AnyNodeRef) -> Vec<String> { |
There was a problem hiding this comment.
This doesn't have to be as part of this PR because I think it would require fleshing out some API for it but we could do this by resolving the semantic_index, iterating over all scopes, and collecting the names from every scope.
This would be a nice stepping stone to the next step where you'd identify in which scope you are and only suggest the names from that or its ancestor scopes.
There was a problem hiding this comment.
we could do this by resolving the
semantic_index, iterating over all scopes, and collecting the names from every scope.
we already have a query that basically does that in https://github.com/astral-sh/ruff/blob/main/crates/red_knot_python_semantic/src/semantic_index/re_exports.rs (but it only visits the global scope, currently at least)
There was a problem hiding this comment.
This sounds great, thank you for the idea. I didn't know about this! This looks like a next good step.
| pub struct Completion { | ||
| pub label: String, | ||
| } |
There was a problem hiding this comment.
This could, probably return a &'db str but I'm fine with a String.
There was a problem hiding this comment.
Yeah it eventually has to get turned into a String anyway. And I figured this would all be refactored soon anyway.
| completion_provider: Some(lsp_types::CompletionOptions { | ||
| ..Default::default() | ||
| }), |
There was a problem hiding this comment.
Nit: We probably want to tinker with the options here in the future
There was a problem hiding this comment.
Yup I looked, but they all seemed pretty in the weeds. I also noticed that even if I didn't set this, the client still tried to send completion requests. But this seemed the "most correct" given what exists now.
|
Thank you @dhruvmanila for the tip about an experimental opt-in! I created an issue tracking that for the alpha milestone: astral-sh/ty#74 |
This PR does the wiring necessary to respond to completion requests from LSP clients. As far as the actual completion results go, they are nearly about the dumbest and simplest thing we can do: we simply return a de-duplicated list of all identifiers from the current module.
BurntSushi
force-pushed
the
ag/completion-end-to-end-mvp
branch
from
02a0f38 to
179e546
Compare
5 participants
This PR does the wiring necessary to respond to completion requests from
LSP clients.
As far as the actual completion results go, they are nearly about the
dumbest and simplest thing we can do: we simply return a de-duplicated
list of all identifiers from the current module.
Ref astral-sh/ty#86