- 
                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
Changes from all commits
3318e96
              12865e4
              70ff7ad
              974b874
              45c09d6
              7b85ec6
              4928fbe
              a3cfa2d
              463a815
              0a0a3ab
              ca26e14
              49da6cd
              3646b1b
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| [settings] | ||
| auto-import = true | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| from module import UniquePrefixA<CURSOR:UniquePrefixAzurous> | ||
| from module import unique_prefix_<CURSOR:unique_prefix_azurous> | ||
| 
     | 
||
| from module import Class | ||
| 
     | 
||
| Class.meth_<CURSOR:meth_azurous> | ||
| 
     | 
||
| # 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> | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| from typing import type_check_only | ||
| 
     | 
||
| 
     | 
||
| @type_check_only | ||
| class UniquePrefixApple: pass | ||
| 
     | 
||
| class UniquePrefixAzurous: pass | ||
| 
     | 
||
| 
     | 
||
| @type_check_only | ||
| def unique_prefix_apple() -> None: pass | ||
| 
     | 
||
| def unique_prefix_azurous() -> None: pass | ||
| 
     | 
||
| 
     | 
||
| class Class: | ||
| @type_check_only | ||
| def meth_apple(self) -> None: pass | ||
| 
     | 
||
| def meth_azurous(self) -> None: pass | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| [project] | ||
| name = "test" | ||
| version = "0.1.0" | ||
| requires-python = ">=3.13" | ||
| dependencies = [] | 
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -65,6 +65,9 @@ pub struct Completion<'db> { | |
| /// use it mainly in tests so that we can write less | ||
| /// noisy tests. | ||
| pub builtin: bool, | ||
| /// Whether this item only exists for type checking purposes and | ||
| /// will be missing at runtime | ||
| pub is_type_check_only: bool, | ||
| /// The documentation associated with this item, if | ||
| /// available. | ||
| pub documentation: Option<Docstring>, | ||
| 
        
          
        
         | 
    @@ -79,6 +82,7 @@ impl<'db> Completion<'db> { | |
| .ty | ||
| .and_then(|ty| DefinitionsOrTargets::from_ty(db, ty)); | ||
| let documentation = definition.and_then(|def| def.docstring(db)); | ||
| let is_type_check_only = semantic.is_type_check_only(db); | ||
| Completion { | ||
| name: semantic.name, | ||
| insert: None, | ||
| 
        
          
        
         | 
    @@ -87,6 +91,7 @@ impl<'db> Completion<'db> { | |
| module_name: None, | ||
| import: None, | ||
| builtin: semantic.builtin, | ||
| is_type_check_only, | ||
| documentation, | ||
| } | ||
| } | ||
| 
          
            
          
           | 
    @@ -294,6 +299,7 @@ fn add_keyword_value_completions<'db>( | |
| kind: None, | ||
| module_name: None, | ||
| import: None, | ||
| is_type_check_only: false, | ||
| builtin: true, | ||
| documentation: None, | ||
| }); | ||
| 
          
            
          
           | 
    @@ -339,6 +345,8 @@ fn add_unimported_completions<'db>( | |
| module_name: Some(symbol.module.name(db)), | ||
| import: import_action.import().cloned(), | ||
| builtin: false, | ||
| // TODO: `is_type_check_only` requires inferring the type of the symbol | ||
| is_type_check_only: false, | ||
| documentation: None, | ||
| }); | ||
| } | ||
| 
          
            
          
           | 
    @@ -837,16 +845,21 @@ fn is_in_string(parsed: &ParsedModuleRef, offset: TextSize) -> bool { | |
| }) | ||
| } | ||
| 
     | 
||
| /// Order completions lexicographically, with these exceptions: | ||
| /// Order completions according to the following rules: | ||
| /// | ||
| /// 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 commentThe 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 commentThe 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  | 
||
| /// 3) `__dunder__` names | ||
| /// | ||
| /// Among each category, type-check-only items are sorted last, | ||
| /// and otherwise completions are sorted lexicographically. | ||
| /// | ||
| /// This has the effect of putting all dunder attributes after "normal" | ||
| /// attributes, and all single-underscore attributes after dunder attributes. | ||
| fn compare_suggestions(c1: &Completion, c2: &Completion) -> Ordering { | ||
| let (kind1, kind2) = (NameKind::classify(&c1.name), NameKind::classify(&c2.name)); | ||
| kind1.cmp(&kind2).then_with(|| c1.name.cmp(&c2.name)) | ||
| 
     | 
||
| (kind1, c1.is_type_check_only, &c1.name).cmp(&(kind2, c2.is_type_check_only, &c2.name)) | ||
| } | ||
| 
     | 
||
| #[cfg(test)] | ||
| 
          
            
          
           | 
    @@ -3378,6 +3391,65 @@ from os.<CURSOR> | |
| ); | ||
| } | ||
| 
     | 
||
| #[test] | ||
| fn import_type_check_only_lowers_ranking() { | ||
| let test = CursorTest::builder() | ||
| .source( | ||
| "main.py", | ||
| r#" | ||
| import foo | ||
| foo.A<CURSOR> | ||
| "#, | ||
| ) | ||
| .source( | ||
| "foo/__init__.py", | ||
| r#" | ||
| from typing import type_check_only | ||
| 
     | 
||
| @type_check_only | ||
| class Apple: pass | ||
| 
     | 
||
| class Banana: pass | ||
| class Cat: pass | ||
| class Azorubine: pass | ||
| "#, | ||
| ) | ||
| .build(); | ||
| 
     | 
||
| let settings = CompletionSettings::default(); | ||
| let completions = completion(&test.db, &settings, test.cursor.file, test.cursor.offset); | ||
| 
     | 
||
| let [apple_pos, banana_pos, cat_pos, azo_pos, ann_pos] = | ||
| ["Apple", "Banana", "Cat", "Azorubine", "__annotations__"].map(|name| { | ||
| completions | ||
| .iter() | ||
| .position(|comp| comp.name == name) | ||
| .unwrap() | ||
| }); | ||
| 
     | 
||
| assert!(completions[apple_pos].is_type_check_only); | ||
| assert!(apple_pos > banana_pos.max(cat_pos).max(azo_pos)); | ||
| assert!(ann_pos > apple_pos); | ||
| } | ||
| 
     | 
||
| #[test] | ||
| fn type_check_only_is_type_check_only() { | ||
| // `@typing.type_check_only` is a function that's unavailable at runtime | ||
| // and so should be the last "non-underscore" completion in `typing` | ||
| let test = cursor_test("from typing import t<CURSOR>"); | ||
| 
     | 
||
| let settings = CompletionSettings::default(); | ||
| let completions = completion(&test.db, &settings, test.cursor.file, test.cursor.offset); | ||
| let last_nonunderscore = completions | ||
| .into_iter() | ||
| .filter(|c| !c.name.starts_with('_')) | ||
| .next_back() | ||
| .unwrap(); | ||
| 
     | 
||
| assert_eq!(&last_nonunderscore.name, "type_check_only"); | ||
| assert!(last_nonunderscore.is_type_check_only); | ||
| } | ||
| 
     | 
||
| #[test] | ||
| fn regression_test_issue_642() { | ||
| // Regression test for https://github.com/astral-sh/ty/issues/642 | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -121,6 +121,8 @@ bitflags! { | |
| const STATICMETHOD = 1 << 5; | ||
| /// `@typing.override` | ||
| const OVERRIDE = 1 << 6; | ||
| /// `@typing.type_check_only` | ||
| const TYPE_CHECK_ONLY = 1 << 7; | ||
| } | ||
| } | ||
| 
     | 
||
| 
        
          
        
         | 
    @@ -135,6 +137,7 @@ impl FunctionDecorators { | |
| Some(KnownFunction::AbstractMethod) => FunctionDecorators::ABSTRACT_METHOD, | ||
| Some(KnownFunction::Final) => FunctionDecorators::FINAL, | ||
| Some(KnownFunction::Override) => FunctionDecorators::OVERRIDE, | ||
| Some(KnownFunction::TypeCheckOnly) => FunctionDecorators::TYPE_CHECK_ONLY, | ||
| _ => FunctionDecorators::empty(), | ||
| }, | ||
| Type::ClassLiteral(class) => match class.known(db) { | ||
| 
          
            
          
           | 
    @@ -1278,6 +1281,8 @@ pub enum KnownFunction { | |
| DisjointBase, | ||
| /// [`typing(_extensions).no_type_check`](https://typing.python.org/en/latest/spec/directives.html#no-type-check) | ||
| NoTypeCheck, | ||
| /// `typing(_extensions).type_check_only` | ||
| TypeCheckOnly, | ||
| 
     | 
||
| /// `typing(_extensions).assert_type` | ||
| AssertType, | ||
| 
          
            
          
           | 
    @@ -1362,7 +1367,7 @@ impl KnownFunction { | |
| .then_some(candidate) | ||
| } | ||
| 
     | 
||
| /// 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 commentThe 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   | 
||
| const fn check_module(self, module: KnownModule) -> bool { | ||
| match self { | ||
| Self::IsInstance | ||
| 
          
            
          
           | 
    @@ -1416,6 +1421,8 @@ impl KnownFunction { | |
| | Self::NegatedRangeConstraint | ||
| | Self::AllMembers => module.is_ty_extensions(), | ||
| Self::ImportModule => module.is_importlib(), | ||
| 
     | 
||
| Self::TypeCheckOnly => matches!(module, KnownModule::Typing), | ||
| } | ||
| } | ||
| 
     | 
||
| 
          
            
          
           | 
    @@ -1821,6 +1828,8 @@ pub(crate) mod tests { | |
| | KnownFunction::DisjointBase | ||
| | KnownFunction::NoTypeCheck => KnownModule::TypingExtensions, | ||
| 
     | 
||
| KnownFunction::TypeCheckOnly => KnownModule::Typing, | ||
| 
     | 
||
| KnownFunction::IsSingleton | ||
| | KnownFunction::IsSubtypeOf | ||
| | KnownFunction::GenericContext | ||
| 
          
            
          
           | 
    ||

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-playgroundfrom the Ruff repo root):module.pyhas these contents, so I would expectFooto be ranked belowFoooo: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.
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_onlysupport 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.