-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Add some completion ranking improvements #20807
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
9415542 to
a788c47
Compare
|
a788c47 to
94d68c4
Compare
AlexWaygood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
I planned to take a look at this, but it looks like we have two reviews already. |
This means that when new characters are typed, the LSP client should send new completion requests to the LSP server. Previously, only one request would be emitted and then the client would filter them. The primary reason to do this is so the server can take what has been typed into account when returning completions. ty is already doing this with auto-import, because it isn't really feasible to do otherwise: always returning *every* symbol throughout an environment isn't a good way to do business. But, we likely also want to take what is typed into account for all completion requests so that we can do our own ranking. Moreover, if we take what is typed into account when doing a completion request, then consider a request that was initiated with `fubar` already typed. If the user hits backspace and gets `fub`, then since the LSP client isn't sending another request, the user won't see all matching completions. This is why we need to mark the response as "incomplete." This will instruct the LSP client to re-send completion requests. The trade-off is that the client will make more completion requests. And if completions are slow, this will magnify that effect for users.
Instead of only extracting this for scoped completions using the AST, we now extract the typed text for all requests in a uniform way directly from the token stream. This doesn't otherwise change any behavior.
This doesn't do any ranking or sorting, but it does filter our completions by the text the user has typed so far. We were already doing this for auto-import. For non-auto-import completions, the number of symbols tends to be much smaller, so I don't really expect this to result in any meaningful performance improvements. This does mean our eval score gets a lot better, and indeed, the eval reflects the real world more accurate since LSP clients do their own filtering.
I had the logic flipped here, which resulted in `show-one` not matching expected tasks in some cases. Basically, when the `--file-name` and `--index` flags aren't given, then the CLI should behave as if all file names and indices are matched.
This is mostly a basic sanity test that we can return completions inside of the interpolated segments of f-strings and t-strings.
This had been annoying me for a while: completions shouldn't be offered inside of comments. We detect this by looking for a `Comment` token before the offset (with only `NonLogicalNewline` being allowed as an intermediate token).
... except when we're in the interpolated segments of an f-string or a t-string. Previously, we would show completions for interpolated segments of an f-string or a t-string. Which is good. But we did this by virtue of never trying to suppress completions when in the non-interpolated part of an f-string/t-string, or any other kind of string. Whcih is bad. Here, we do our best to detect whether the cursor is within a string or not. This seems somewhat straight-forward in the case of a string that is properly opened/closed. But for a string that isn't closed yet---which is likely common while typing---this is less straight-forward. For the most part, I looked at some examples and tried to reverse engineer the kinds of patterns we see in the token stream when there is an unclosed string. It's likely not fully correct, but we can improve it as we go.
Since `None` is a keyword, it isn't currently included in completions. But this is a common papercut that is really annoying, and actually makes typing `None` into the playground nearly impossible. So let's try to fix this.
This doesn't implement fully context aware keyword completions, but it does let us include the 3 keywords that are also values in completions. This helps improve the completion experience when trying to write one of these values in the ty playground. Ranking could still be improved here. We haven't yet devised a mechanism for ranking "builtin" symbols above others. It's even plausible that these _particular_ symbols should get special treatment over and above others.
94d68c4 to
7bd9ee2
Compare
|
Hmm, I think the evaluation results differ between mac and linux? |
|
Sorry @BurntSushi, I'll squash-merge the commits because @AlexWaygood and I don't have as good a commit hygiene as you. |
…rable * origin/main: [ty] Add (unused) `inferable` parameter to type property methods (#20865) Run macos tests on macos (#20889) Remove `release` CI job (#20887) [ty] CI: Faster ecosystem analysis (#20886) Remove `strip` from release profile (#20885) [ty] Sync vendored typeshed stubs (#20876) [ty] Add some completion ranking improvements (#20807) Improved error recovery for unclosed strings (including f- and t-strings) (#20848) Enable lto=fat (#20863) [`pyupgrade`] Extend `UP019` to detect `typing_extensions.Text` (`UP019`) (#20825) [`flake8-bugbear`] Omit annotation in preview fix for `B006` (#20877) fix(docs): Fix typo in `RUF015` description (#20873) [ty] Improve and extend tests for instance attributes redeclared in subclasses (#20866) [ty] Ignore slow seeds as a temporary measure (#20870) Remove parentheses around multiple exception types on Python 3.14+ (#20768) Update Black tests (#20794)
…nt-sets * dcreager/non-non-inferable: (174 commits) [ty] Add (unused) `inferable` parameter to type property methods (#20865) Run macos tests on macos (#20889) Remove `release` CI job (#20887) [ty] CI: Faster ecosystem analysis (#20886) Remove `strip` from release profile (#20885) [ty] Sync vendored typeshed stubs (#20876) [ty] Add some completion ranking improvements (#20807) Improved error recovery for unclosed strings (including f- and t-strings) (#20848) Enable lto=fat (#20863) [`pyupgrade`] Extend `UP019` to detect `typing_extensions.Text` (`UP019`) (#20825) [`flake8-bugbear`] Omit annotation in preview fix for `B006` (#20877) fix(docs): Fix typo in `RUF015` description (#20873) [ty] Improve and extend tests for instance attributes redeclared in subclasses (#20866) [ty] Ignore slow seeds as a temporary measure (#20870) use existing method Remove parentheses around multiple exception types on Python 3.14+ (#20768) Update Black tests (#20794) just the api parts [ty] Fix further issues in `super()` inference logic (#20843) [ty] Document when a rule was added (#20859) ...
This PR contains a few improvements for completions:
generating completions in all cases. Previously, we only did this for
auto-import completions.
None,TrueandFalseare now included inscoped completions.
f-strings and t-strings.
We add some new evaluation tasks to capture some of these cases. (And
lots of tests for the string detection logic.)
Note: I did make some changes to the
TokenFlagsrepresentation.I'm open to other ideas. But I wanted to flag this since
TokenFlagsis used within the representation of a
Token, which is a fundamentaldata type inside of Ruff. Thankfully, even though
TokenFlagsgetsbigger,
Tokenitself does not (because of padding/alignment).