-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Consider type_check_only when ranking completions
#20910
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
[ty] Consider type_check_only when ranking completions
#20910
Conversation
For now we only support classes
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
type_check_only when ranking completionstype_check_only when ranking completions
Fine to defer functions for now, IMO! I think the vast majority of uses are with classes. Let's get an MVP in first that we can iterate on in followups |
| /// 1) A `_[^_]` prefix sorts last and | ||
| /// 2) A `__` prefix sorts last except before (1) | ||
| /// 1) Names with no underscore prefix | ||
| /// 2) Names starting with `_` but not dunders |
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.
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.
Yeah, agree. I think that sounds like a great change for another PR
…nes/ruff into df-type-check-only
type_check_only when ranking completionstype_check_only when ranking completions
| } | ||
|
|
||
| /// Return `true` if `self` is defined in `module` at runtime. | ||
| /// Return `true` if `self` is defined in `module` |
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.
Was it an important detail that it's really defined in the module at runtime? I am not sure, but type_check_only should be the only exception, probably
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! This looks great to me. I've one question about whether we can compute the is_type_check_only flag lazily rather than storing it on every ClassLiteral.
It would also be great if we could add an integration test for this. See
ruff/crates/ty_completion_eval/README.md
Line 11 in 651f796
| ``` |
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, thank you!
| module_name: Some(symbol.module.name(db)), | ||
| import: import_action.import().cloned(), | ||
| builtin: false, | ||
| is_type_check_only: false, |
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.
This seems to mean that completions that would require an auto-import are always considered "not type-check-only". Here's a screenshot from a local playground build using your branch (to build the playground locally, use cd playground && npm start --workspace ty-playground from the Ruff repo root):
module.py has these contents, so I would expect Foo to be ranked below Foooo:
from typing import type_check_only
@type_check_only
class Foo: ...
class Foooo: ...since that's what I get in other situations on your branch (which is great!):
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.
if getting this right is difficult for auto-import completions, it's fine to defer it for now, but we should add a TODO comment here if so
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.
I think let's defer it for now 👍
I added some TODOs in the integration test.
Right now deprecation and type-check-only-ness are stored on types, not symbols, which makes this difficult. (we'd have to eagerly infer types to produce autoimport suggestions if I understand it correctly)
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.
Hmm, so we'd have to infer the types of every symbol that could possibly be imported in order to provide autocomplete suggestions for a single x = Fo<CURSOR> situation... that would indeed undermine the "only lazily infer exactly what we need" architecture of ty 😆
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.
situation... that would indeed undermine the "only lazily infer exactly what we need" architecture of ty 😆
Yeah, let's wait for @BurntSushi on this, but we can definitely infer the type after we've done some filtering
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.
My loose plan at this point was to basically never infer the type of unimported symbols, by virtue of there being so many. It's true that we could try doing that after we've done filtering, but I fear this could still have a negative overall effect where we end needing to type check a substantial portion of third party code just by virtue of using completions. And I of course worry about the latency impact this will have on auto-import completions.
This is why the way auto-import works currently is to basically bypass any type checking and just pick up symbols straight from the AST. The way I would try to tackle type_check_only support here is to see if we can add it to that AST extraction bit and not try to bring in type checking machinery for it.
I think it is nicely symmetrical with storing This is something |
|
@decorator-factory I'm not sure what the state of this PR is. Would you mind requesting re-review when it's ready for another round of feedback or you can put your PR in draft mode and change it to "ready for review" once it's ready for another round. Thank you |
|
I think it's ready to merge in the current state. As Alex said, the main use case for |
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.
Thanks, and sorry for the delay in getting back to this! Looks good now, just one comment about the tests
| # TODO: bound methods don't preserve type-check-only-ness, this is a bug | ||
| # Class().meth_[CURSOR:meth_azurous] | ||
|
|
||
| # TODO: auto-imports don't take type-check-only-ness into account, this is a bug | ||
| # UniquePrefixA[CURSOR:module.UniquePrefixAzurous] |
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.
I believe the idea with the tests in this crate is that for autocomplete suggestions that don't achieve a great result currently, that's simply reflected in a lower score in crates/ty_completion_eval/completion-evaluation-tasks.csv. So that implies that we should uncomment these lines, and then when we improve our ranking in the future that will simply result in us having to update the score in that CSV file to reflect the fact that the ranking has improved.
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.
I see, that's how it seems to work in the numpy-array and scope-existing-over-new-import cases indeed. Fixed in the latest commit
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.
@AlexWaygood Yup that's exactly right! More broadly, it may be the case that some rankings can't improve or can't improve without regressing others.

Summary
This pull request enhances
Types with information on whether they are "for type checking only", and changes autocompletion ranking so thatThis is the first step in solving astral-sh/ty#816
@type_check_onlyfor classes, right now I'm figuring out how to do it for functions. It's a bit puzzling why this decorator supports functions in the first place. The only place I found it used successfully isscipy-stubs, where "imaginary methods" are used to make certain classes match certain protocols.Test Plan
Added a
import_type_check_only_lowers_rankingtest case, which used to fail.