From f1883d71a4d47a56f8dd8c1874db40f12bc3b105 Mon Sep 17 00:00:00 2001 From: David Peter Date: Wed, 4 Jun 2025 16:11:05 +0200 Subject: [PATCH] [ty] IDE: only provide declarations and bindings as completions (#18456) ## Summary Previously, all symbols where provided as possible completions. In an example like the following, both `foo` and `f` were suggested as completions, because `f` itself is a symbol. ```py foo = 1 f ``` Similarly, in the following example, `hidden_symbol` was suggested, even though it is not statically visible: ```py if 1 + 2 != 3: hidden_symbol = 1 hidden_ ``` With the change suggested here, we only use statically visible declarations and bindings as a source for completions. ## Test Plan - Updated snapshot tests - New test for statically hidden definitions - Added test for star import --- crates/ty_ide/src/completion.rs | 151 ++++++++++-------- .../ty_python_semantic/src/semantic_model.rs | 8 +- crates/ty_python_semantic/src/types.rs | 2 +- .../src/types/ide_support.rs | 51 +++--- 4 files changed, 126 insertions(+), 86 deletions(-) diff --git a/crates/ty_ide/src/completion.rs b/crates/ty_ide/src/completion.rs index cfbd998d31..ebc0c9197e 100644 --- a/crates/ty_ide/src/completion.rs +++ b/crates/ty_ide/src/completion.rs @@ -127,10 +127,7 @@ f ", ); - assert_snapshot!(test.completions(), @r" - f - foo - "); + assert_snapshot!(test.completions(), @"foo"); } #[test] @@ -143,10 +140,7 @@ g ", ); - assert_snapshot!(test.completions(), @r" - foo - g - "); + assert_snapshot!(test.completions(), @"foo"); } #[test] @@ -175,10 +169,7 @@ f ", ); - assert_snapshot!(test.completions(), @r" - f - foo - "); + assert_snapshot!(test.completions(), @"foo"); } #[test] @@ -208,7 +199,6 @@ def foo(): ); assert_snapshot!(test.completions(), @r" - f foo foofoo "); @@ -259,7 +249,6 @@ def foo(): ); assert_snapshot!(test.completions(), @r" - f foo foofoo "); @@ -276,7 +265,6 @@ def foo(): ); assert_snapshot!(test.completions(), @r" - f foo foofoo "); @@ -295,7 +283,6 @@ def frob(): ... ); assert_snapshot!(test.completions(), @r" - f foo foofoo frob @@ -315,7 +302,6 @@ def frob(): ... ); assert_snapshot!(test.completions(), @r" - f foo frob "); @@ -334,7 +320,6 @@ def frob(): ... ); assert_snapshot!(test.completions(), @r" - f foo foofoo foofoofoo @@ -451,15 +436,10 @@ def frob(): ... ", ); - // It's not totally clear why `for` shows up in the - // symbol tables of the detected scopes here. My guess - // is that there's perhaps some sub-optimal behavior - // here because the list comprehension as written is not - // valid. - assert_snapshot!(test.completions(), @r" - bar - for - "); + // TODO: it would be good if `bar` was included here, but + // the list comprehension is not yet valid and so we do not + // detect this as a definition of `bar`. + assert_snapshot!(test.completions(), @""); } #[test] @@ -470,10 +450,7 @@ def frob(): ... ", ); - assert_snapshot!(test.completions(), @r" - f - foo - "); + assert_snapshot!(test.completions(), @"foo"); } #[test] @@ -484,10 +461,7 @@ def frob(): ... ", ); - assert_snapshot!(test.completions(), @r" - f - foo - "); + assert_snapshot!(test.completions(), @"foo"); } #[test] @@ -498,10 +472,7 @@ def frob(): ... ", ); - assert_snapshot!(test.completions(), @r" - f - foo - "); + assert_snapshot!(test.completions(), @"foo"); } #[test] @@ -512,10 +483,7 @@ def frob(): ... ", ); - assert_snapshot!(test.completions(), @r" - f - foo - "); + assert_snapshot!(test.completions(), @"foo"); } #[test] @@ -526,10 +494,7 @@ def frob(): ... ", ); - assert_snapshot!(test.completions(), @r" - f - foo - "); + assert_snapshot!(test.completions(), @"foo"); } #[test] @@ -602,7 +567,6 @@ class Foo: assert_snapshot!(test.completions(), @r" Foo - b bar frob quux @@ -621,7 +585,6 @@ class Foo: assert_snapshot!(test.completions(), @r" Foo - b bar quux "); @@ -750,7 +713,6 @@ bar(o assert_snapshot!(test.completions(), @r" bar foo - o "); } @@ -788,7 +750,6 @@ class C: assert_snapshot!(test.completions(), @r" C bar - f foo self "); @@ -825,7 +786,6 @@ class C: assert_snapshot!(test.completions(), @r" C bar - f foo self "); @@ -854,11 +814,55 @@ print(f\"{some ", ); - assert_snapshot!(test.completions(), @r" - print - some - some_symbol - "); + assert_snapshot!(test.completions(), @"some_symbol"); + } + + #[test] + fn statically_invisible_symbols() { + let test = cursor_test( + "\ +if 1 + 2 != 3: + hidden_symbol = 1 + +hidden_ +", + ); + + assert_snapshot!(test.completions(), @""); + } + + #[test] + fn completions_inside_unreachable_sections() { + let test = cursor_test( + "\ +import sys + +if sys.platform == \"not-my-current-platform\": + only_available_in_this_branch = 1 + + on +", + ); + + // TODO: ideally, `only_available_in_this_branch` should be available here, but we + // currently make no effort to provide a good IDE experience within sections that + // are unreachable + assert_snapshot!(test.completions(), @"sys"); + } + + #[test] + fn star_import() { + let test = cursor_test( + "\ +from typing import * + +Re +", + ); + + test.assert_completions_include("Reversible"); + // `ReadableBuffer` is a symbol in `typing`, but it is not re-exported + test.assert_completions_do_not_include("ReadableBuffer"); } // Ref: https://github.com/astral-sh/ty/issues/572 @@ -921,10 +925,7 @@ Fo = float ", ); - assert_snapshot!(test.completions(), @r" - Fo - float - "); + assert_snapshot!(test.completions(), @"Fo"); } // Ref: https://github.com/astral-sh/ty/issues/572 @@ -999,9 +1000,7 @@ except Type: ", ); - assert_snapshot!(test.completions(), @r" - Type - "); + assert_snapshot!(test.completions(), @""); } // Ref: https://github.com/astral-sh/ty/issues/572 @@ -1029,5 +1028,29 @@ def _(): .collect::>() .join("\n") } + + #[track_caller] + fn assert_completions_include(&self, expected: &str) { + let completions = completion(&self.db, self.file, self.cursor_offset); + + assert!( + completions + .iter() + .any(|completion| completion.label == expected), + "Expected completions to include `{expected}`" + ); + } + + #[track_caller] + fn assert_completions_do_not_include(&self, unexpected: &str) { + let completions = completion(&self.db, self.file, self.cursor_offset); + + assert!( + completions + .iter() + .all(|completion| completion.label != unexpected), + "Expected completions to not include `{unexpected}`", + ); + } } } diff --git a/crates/ty_python_semantic/src/semantic_model.rs b/crates/ty_python_semantic/src/semantic_model.rs index c112fc1ba3..bc4204386e 100644 --- a/crates/ty_python_semantic/src/semantic_model.rs +++ b/crates/ty_python_semantic/src/semantic_model.rs @@ -10,6 +10,7 @@ use crate::module_resolver::{Module, resolve_module}; use crate::semantic_index::ast_ids::HasScopedExpressionId; use crate::semantic_index::semantic_index; use crate::semantic_index::symbol::FileScopeId; +use crate::types::ide_support::all_declarations_and_bindings; use crate::types::{Type, binding_type, infer_scope_types}; pub struct SemanticModel<'db> { @@ -66,9 +67,10 @@ impl<'db> SemanticModel<'db> { }; let mut symbols = vec![]; for (file_scope, _) in index.ancestor_scopes(file_scope) { - for symbol in index.symbol_table(file_scope).symbols() { - symbols.push(symbol.name().clone()); - } + symbols.extend(all_declarations_and_bindings( + self.db, + file_scope.to_scope_id(self.db, self.file), + )); } symbols } diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index aec03a73a6..b2d1c6b91f 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -65,7 +65,7 @@ mod diagnostic; mod display; mod function; mod generics; -mod ide_support; +pub(crate) mod ide_support; mod infer; mod instance; mod mro; diff --git a/crates/ty_python_semantic/src/types/ide_support.rs b/crates/ty_python_semantic/src/types/ide_support.rs index 2ba4606d6a..c8f150aa43 100644 --- a/crates/ty_python_semantic/src/types/ide_support.rs +++ b/crates/ty_python_semantic/src/types/ide_support.rs @@ -8,6 +8,37 @@ use crate::types::{ClassBase, ClassLiteral, KnownClass, Type}; use ruff_python_ast::name::Name; use rustc_hash::FxHashSet; +pub(crate) fn all_declarations_and_bindings<'db>( + db: &'db dyn Db, + scope_id: ScopeId<'db>, +) -> impl Iterator + 'db { + let use_def_map = use_def_map(db, scope_id); + let symbol_table = symbol_table(db, scope_id); + + use_def_map + .all_public_declarations() + .filter_map(move |(symbol_id, declarations)| { + if symbol_from_declarations(db, declarations) + .is_ok_and(|result| !result.symbol.is_unbound()) + { + Some(symbol_table.symbol(symbol_id).name().clone()) + } else { + None + } + }) + .chain( + use_def_map + .all_public_bindings() + .filter_map(move |(symbol_id, bindings)| { + if symbol_from_bindings(db, bindings).is_unbound() { + None + } else { + Some(symbol_table.symbol(symbol_id).name().clone()) + } + }), + ) +} + struct AllMembers { members: FxHashSet, } @@ -118,24 +149,8 @@ impl AllMembers { } fn extend_with_declarations_and_bindings(&mut self, db: &dyn Db, scope_id: ScopeId) { - let use_def_map = use_def_map(db, scope_id); - let symbol_table = symbol_table(db, scope_id); - - for (symbol_id, declarations) in use_def_map.all_public_declarations() { - if symbol_from_declarations(db, declarations) - .is_ok_and(|result| !result.symbol.is_unbound()) - { - self.members - .insert(symbol_table.symbol(symbol_id).name().clone()); - } - } - - for (symbol_id, bindings) in use_def_map.all_public_bindings() { - if !symbol_from_bindings(db, bindings).is_unbound() { - self.members - .insert(symbol_table.symbol(symbol_id).name().clone()); - } - } + self.members + .extend(all_declarations_and_bindings(db, scope_id)); } fn extend_with_class_members<'db>(