[ty] Fix edge cases for autocomplete suppressions in variable bindings #21576
Conversation
RasmusNygren
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
AlexWaygood
added
server
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
crates/ty_ide/src/completion.rs
Outdated
| let tokens = tokens_start_before(parsed.tokens(), offset); | ||
| // `typed` contains the entire token even if the cursor sits in the | ||
| // middle of it (pa<CURSOR>ram). To correctly suggest completions | ||
| // based on the cursor's current position, we must only consider | ||
| // the characters in the token that are left of the cursor. | ||
| // E.g. we should only count "pa" if the state is "pa<CUSROR>ram". | ||
| let typed_len = typed | ||
| .text_len() | ||
| .min(tokens.last().map_or(typed.text_len(), |token| { | ||
| offset.saturating_sub(token.start()) | ||
| })); | ||
| let start = offset.saturating_sub(typed_len); |
There was a problem hiding this comment.
This is technically not relevant to this PR but I realised from #21570 that this calculation was logically incorrect. typed.textlen() is the length of the entire token, even if the cursor is currently in the middle of the token which is not quite what we want.
I have not managed to write a test based on suggestions that highlights this bug, but by calling is_in_variable_binding directly with different inputs it became obvious that it was previously incorrect.
RasmusNygren
force-pushed
the
improve-variable-binding-suggestions
branch
from
ee861d2 to
0308c41
Compare
crates/ty_ide/src/completion.rs
Outdated
| // middle of it (pa<CURSOR>ram). To correctly suggest completions | ||
| // based on the cursor's current position, we must only consider | ||
| // the characters in the token that are left of the cursor. | ||
| // E.g. we should only count "pa" if the state is "pa<CUSROR>ram". |
There was a problem hiding this comment.
This actually wasn't my intent, but maybe I was wrong. In particular, rust-analyzer and pylance work how you describe: it only considers the characters before the cursor when the cursor splits a token. ty doesn't. It uses the entire token as the query.
I initially went this route because when you select a completion, clients seem to usually replace the entire token with the selected suggestion. So in my mind, it made sense for the query to also correspond to the entire token. With that said, while rust-analyzer replaces the entire token, pylance does not.
I do kind of feel like only taking the text before the cursor is perhaps more intuitive and even more useful. (I'm undecided on the token replacement strategy though.)
So to that end, perhaps the code that extracts the typed text should cut itself off based on the offset? Then I think that would fix this without needing to do this extra arithmetic here.
There was a problem hiding this comment.
Right, I agree with you that only considering text to the left of the cursor feels more intuitive, independently from whether the client replaces the entire token or not. Given that, it would be quite nice to only extract the relevant bit from the start, so we can be consistent without any special handling.
Would you like me to open a separate PR (maybe even an issue) for that (as I jumbled two issues into one PR here) or are you fine if I execute on that directly here?
There was a problem hiding this comment.
I think just doing that directly here is fine. :-) Thank you!
| .iter() | ||
| .map(|param| param.range()) | ||
| .all(|r| !r.contains_range(range)) | ||
| } |
There was a problem hiding this comment.
This makes sense to me. Nice find!
RasmusNygren
force-pushed
the
improve-variable-binding-suggestions
branch
from
0308c41 to
ea88222
Compare
Previously we extracted the entire token as the query independently of the cursor position. By not doing that you avoid having to do special range handling to figure out the start position of the current token. It's likely also more intuitive from a user perspective to only consider characters left of the cursor when suggesting autocompletions.
Autocomplete suggestions were not suppressed correctly during some variable bindings if the parameter name was currently matching a keyword. E.g. `def f(foo<CURSOR>` was handled correctly but not `def f(in<CURSOR>`.
BurntSushi
force-pushed
the
improve-variable-binding-suggestions
branch
from
ea88222 to
2b54227
Compare
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
Beautiful. I also added a test that foo<CURSOR>bar only takes foo into account when offering suggestions.
Before:
2025-11-25T08.22.41-05.00.mp4
After:
2025-11-25T08.23.41-05.00.mp4
3 participants
Summary
This handles a few edge cases related to astral-sh/ty#1563 that was not fully handled in #21549
Only using the individual
Parameternodes in the AST to determine parameter bindings is not enough when you're in a state where the syntax is invalid. For example when the name of the parameter clashes with a keyword. In those scenarios we now go up a level and inspect theParametersnode to determine if we're still binding variables.Test Plan
New tests and ty-playground sanity-check.