[ty] Improve completion ranking based on origin and exact match#22460
Merged
BurntSushi merged 11 commits intomainfrom Jan 9, 2026
Merged
[ty] Improve completion ranking based on origin and exact match#22460BurntSushi merged 11 commits intomainfrom
BurntSushi merged 11 commits intomainfrom
Conversation
BurntSushi
requested review from
AlexWaygood,
Gankra,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
BurntSushi
added
server
Member
Author
|
cc @RasmusNygren in case you want to review here. In particular the refactoring in the earlier commits here. |
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
BurntSushi
force-pushed
the
ag/ranking-improvements
branch
2 times, most recently
from
464d31e to
6ff8447
Compare
MichaReiser
approved these changes
Jan 9, 2026
| pub documentation: Option<Docstring>, | ||
| /// Information used to sort this completion relative to others | ||
| /// in the same collection. | ||
| relevance: Relevance, |
Member
There was a problem hiding this comment.
Should we make the fields on Completion private and expose getter methods instead to ensure a Completion is never modified after computing its relevance?
Member
Author
There was a problem hiding this comment.
That's kinda my inclination personally, yeah. Even aside from this new Relevance thing being attached to Completion.
I'll try this out in a follow-up PR.
| /// sequence of completions. | ||
| fn into_completions(mut self) -> Vec<Completion<'db>> { | ||
| self.items.sort_by(|c1, c2| self.context.compare(c1, c2)); | ||
| self.items.sort_by(rank); |
Member
There was a problem hiding this comment.
Could Completion import PartialOrd and Ord now that Relevance is stored on the Completion?
Member
Author
There was a problem hiding this comment.
It could, but I at least for now chose not to do that. See the rank function docs:
/// Return an ordering relating the two completions.
///
/// A `Ordering::Less` is returned when `c1` should be ranked above
/// `c2`. A `Ordering::Greater` is returned when `c1` should be ranked
/// below `c2`. In other words, a standard ascending sort used with
/// this comparison routine will yields the "best ranked" completions
/// first.
///
/// Note that this could have been implemented via `Eq` and `Ord`
/// impls on `Completion`, but is instead a separate function to avoid
/// conflating relevance ranking with identity.
fn rank(c1: &Completion<'_>, c2: &Completion<'_>) -> Ordering {
(&c1.relevance, &c1.name).cmp(&(&c2.relevance, &c2.name))
}(I think it's possible we do ultimately do this.)
| Namespace, | ||
| /// Symbols defined somewhere in a dependency, direct or | ||
| /// indirect. | ||
| ThirdParty, |
Member
There was a problem hiding this comment.
Is it intentional that we prioritize third-party dependencies over stdlib?
Member
Author
There was a problem hiding this comment.
That was my thinking yeah. With the idea being that third party dependencies are "more specific" to the user's context.
But this may very well be overruled by the ubiquity and frequency of use for stdlib packages.
Another factor here is that it's all third party code, not just direct dependencies. So it's likelier to be noisier.
Member
There was a problem hiding this comment.
Yeah, my thinking here is: if something's available in the standard library, I probably want to use that over any third-party equivalent.
I'm also surprised that this doesn't regress the typing use case, or import os as they are now both less specific than a similarly named module in any third-party library.
Member
Author
There was a problem hiding this comment.
For typing, a lot of the conflicts come from other standard library modules.
The reason why I went this route was this evaluation task:
# This test was originally written to
# check that a third party dependency
# gets priority over stdlib. But it was
# not clearly the right choice[1].
#
# 2026-01-09: We stuck with it for now,
# but it seems likely that we'll want
# to regress this task in favor of
# another.
#
# [1]: https://github.com/astral-sh/ruff/pull/22460#discussion_r2676343225
fullma<CURSOR: regex.fullmatch>
Where pyproject.toml contains the regex dependency. In this context, you probably want fullmatch from regex over the stdlib re module. I assume this applies to other third party packages that provide a drop-in replacement for stdlib modules. But maybe this case isn't more important than the ones you point out.
I added the comment above in response here. I think you're ultimately going to be right here, but I'd like to iterate on this in response to user feedback.
I went through astral-sh/ty#1274 and tried to extract what I could into eval tasks. Some of the suggestions from that issue have already been done, but most haven't. This captures the status quo.
I think "relevance" captures the meaning of this type more precisely.
I want to be able to attach extra data to each `Completion`, but not burden callers with the need to construct it. This commit helps get us to that point by requiring callers to use a `CompletionBuilder` for construction instead of a `Completion` itself. I think this will also help in the future if it proves to be the case that we can improve performance by delaying work until we actually build a `Completion`, which might only happen if we know we won't throw it out. But we aren't quite there yet. This also lets us tighten things up a little bit and makes completion construction less noisy. The downside is that callers no longer need to consider "every" completion field. There should not be any behavior changes here.
This moves the information we want to use to rank completions into `Completion` itself. (This was the primary motivation for creating a `CompletionBuilder` in the first place.) The principal advantage here is that we now only need to compute the relevance information for each completion exactly once. Previously, we were computing it on every comparison, which might end up doing redundant work for a not insignifcant number of completions. The relevance information is also specifically constructed from the builder so that, in the future, we might choose to short-circuit construction if we know we'll never send it back to the client (e.g., its ranking is worse than the lowest ranked completion). But we don't implement that optimization yet.
This addresses a number of minor annoyances where users really want imports from `typing` most of the time. For example: astral-sh/ty#1274 (comment)
Previously, we would only sort by name and file path (unless the symbol refers to an entire module). This led to somewhat confusing ordering, since the file path is absolute and can be anything. Sorting by module name after symbol name gives a more predictable ordering in common practice. This is mostly just an improvement for debugging purposes, i.e., when looking at the output of `all_symbols` directly. This mostly shouldn't impact completion ordering since completions do their own ranking.
I apparently don't know how to use my own API. Previously, we would skip, e.g., `.venv`, but still descend into it. This was annoying in practice because I sometimes have an environment in one of the truth task directories. The eval command should ignore that entirely, but it ended up choking on it without properly ignoring hidden files and directories.
BurntSushi
force-pushed
the
ag/ranking-improvements
branch
from
6ff8447 to
d9d8596
Compare
|
Member
Author
|
I checked out the mypy_primer differences, but they look like flakes. When I re-run it, it still reports differences, but different differences from the previous run. Since there's no conspicuous change in this PR that should lead to such differences, I'm assuming they are flakes. |
This commit adds two new tests. One checks that a symbol in the current project gets priority over a symbol in the standard library. Another checks that a symbol in a third party dependency gets priority over a symbol in the standard library. We don't get either of these right today. Note that these comparisons are done ceteris paribus. A symbol from the standard library could still be ranked above a symbol elsewhere. (Although I believe currently this is somewhat rare.)
Part of this was already done, but it was half-assed. We now look at the search path that a symbol came from and centralize a symbol's origin classification. The preference ordering here is maybe not the right one, but we can iterate as users give us feedback. Note also that the preference ordering based on the origin is pretty low in the relevance sorting. This means that other more specific criteria will and can override this. This results in some nice improvements to our evaluation tasks.
This is now strictly redundant with the `typing-gets-priority` task.
Aside from ruling out "impossible" completions in the current context, we give completions with an exact match the highest possible ranking.
BurntSushi
force-pushed
the
ag/ranking-improvements
branch
from
d9d8596 to
15cc74e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR does some refactoring to completions and improves ranking based
on the origin of the symbol and whether there is an exact match.
This addresses a lot of the feedback around preferring, e.g., symbols
from
typingover symbols from other stdlib modules. This alsoestablishes a more general preference ordering for symbols based on
their origin: current module, project modules, third party modules and
then stdlib modules.
Finally, we give very high ranking to symbols that exactly match what
the end user has typed.
Reviewers are encouraged to read this PR commit-by-commit.
Fixes astral-sh/ty#1274