[ty] Fix rename/references to find all overloaded function definitions

This fixes an issue where renaming an overloaded function only renamed
a single definition instead of all overloads and the implementation.

The fix addresses two issues:

1. `navigation_targets_match` in references.rs now checks if any current
   target matches any of the target definitions (not just the first).
   This is important for overloaded functions where each overload is a
   separate definition but they all refer to the same logical symbol.

2. `get_definition_targets` for FunctionDef in goto.rs now uses a new
   `definitions_for_name_in_scope` function that returns all definitions
   with the same name in the same scope, not just the single function's
   definition.

The new `definitions_for_name_in_scope` function in ide_support.rs takes
a file and scope directly, bypassing the scope lookup from a node, which
allows proper handling of function name identifiers.

This also fixes property getter/setter/deleter renaming and submodule
import reference finding, which now correctly identify all related
definitions.
This commit is contained in:
Claude 2025-12-08 17:20:44 +00:00
parent 0ab8521171
commit d4358cda65
No known key found for this signature in database
7 changed files with 191 additions and 82 deletions

View File

@ -2101,9 +2101,21 @@ func<CURSOR>_alias()
) )
.build(); .build();
// TODO: this should also highlight the RHS subpkg in the import // The references correctly identify:
// 1. The RHS of the import (`from .subpkg import subpkg`)
// 2. The usage (`x = subpkg`)
// 3. The original definition in the subpackage (`subpkg: int = 10`)
assert_snapshot!(test.references(), @r" assert_snapshot!(test.references(), @r"
info[references]: Reference 1 info[references]: Reference 1
--> mypackage/__init__.py:2:21
|
2 | from .subpkg import subpkg
| ^^^^^^
3 |
4 | x = subpkg
|
info[references]: Reference 2
--> mypackage/__init__.py:4:5 --> mypackage/__init__.py:4:5
| |
2 | from .subpkg import subpkg 2 | from .subpkg import subpkg
@ -2111,6 +2123,13 @@ func<CURSOR>_alias()
4 | x = subpkg 4 | x = subpkg
| ^^^^^^ | ^^^^^^
| |
info[references]: Reference 3
--> mypackage/subpkg/__init__.py:2:1
|
2 | subpkg: int = 10
| ^^^^^^
|
"); ");
} }

View File

@ -19,7 +19,7 @@ use ty_python_semantic::types::ide_support::{
}; };
use ty_python_semantic::{ use ty_python_semantic::{
HasDefinition, HasType, ImportAliasResolution, SemanticModel, definitions_for_imported_symbol, HasDefinition, HasType, ImportAliasResolution, SemanticModel, definitions_for_imported_symbol,
definitions_for_name, definitions_for_name, definitions_for_name_in_scope,
}; };
#[derive(Clone, Debug)] #[derive(Clone, Debug)]
@ -392,10 +392,24 @@ impl GotoTarget<'_> {
GotoTarget::Expression(expression) => { GotoTarget::Expression(expression) => {
definitions_for_expression(model, *expression, alias_resolution) definitions_for_expression(model, *expression, alias_resolution)
} }
// For already-defined symbols, they are their own definitions // For function definitions, get all definitions with the same name in the scope.
GotoTarget::FunctionDef(function) => Some(vec![ResolvedDefinition::Definition( // This is important for overloaded functions where multiple definitions share the same name.
function.definition(model), GotoTarget::FunctionDef(function) => {
)]), let definition = function.definition(model);
let db = model.db();
let file = definition.file(db);
let file_scope = definition.file_scope(db);
let Some(name) = definition.name(db) else {
return Some(Definitions(vec![ResolvedDefinition::Definition(definition)]));
};
Some(definitions_for_name_in_scope(
db,
file,
file_scope,
&name,
alias_resolution,
))
}
GotoTarget::ClassDef(class) => Some(vec![ResolvedDefinition::Definition( GotoTarget::ClassDef(class) => Some(vec![ResolvedDefinition::Definition(
class.definition(model), class.definition(model),

View File

@ -364,12 +364,13 @@ impl LocalReferencesFinder<'_> {
/// Check if `Vec<NavigationTarget>` match our target definitions /// Check if `Vec<NavigationTarget>` match our target definitions
fn navigation_targets_match(&self, current_targets: &NavigationTargets) -> bool { fn navigation_targets_match(&self, current_targets: &NavigationTargets) -> bool {
// Since we're comparing the same symbol, all definitions should be equivalent // Check if any current target matches any of our target definitions.
// We only need to check against the first target definition // This is important for overloaded functions where each overload is a separate
if let Some(first_target) = self.target_definitions.iter().next() { // definition but they all refer to the same logical symbol.
for target_def in self.target_definitions {
for current_target in current_targets { for current_target in current_targets {
if current_target.file == first_target.file if current_target.file == target_def.file
&& current_target.focus_range == first_target.focus_range && current_target.focus_range == target_def.focus_range
{ {
return true; return true;
} }

View File

@ -1486,20 +1486,26 @@ result = func(10, y=20)
) )
.build(); .build();
// TODO(submodule-imports): this is incorrect, we should rename the `subpkg` int // The rename correctly identifies:
// and the RHS of the import statement (but *not* rename the LHS). // 1. The RHS of the import (`from .subpkg import subpkg`)
// 2. The usage (`x = subpkg`)
// 3. The original definition in the subpackage (`subpkg: int = 10`)
// //
// However us being cautious here *would* be good as the rename will actually // Note: The LHS submodule name is NOT renamed, which is correct.
// result in a `subpkg` variable still existing in this code, as the import's LHS
// `DefinitionKind::ImportFromSubmodule` would stop being overwritten by the RHS!
assert_snapshot!(test.rename("mypkg"), @r" assert_snapshot!(test.rename("mypkg"), @r"
info[rename]: Rename symbol (found 1 locations) info[rename]: Rename symbol (found 3 locations)
--> mypackage/__init__.py:4:5 --> mypackage/__init__.py:2:21
| |
2 | from .subpkg import subpkg 2 | from .subpkg import subpkg
| ^^^^^^
3 | 3 |
4 | x = subpkg 4 | x = subpkg
| ^^^^^^ | ------
|
::: mypackage/subpkg/__init__.py:2:1
|
2 | subpkg: int = 10
| ------
| |
"); ");
} }
@ -1534,7 +1540,7 @@ result = func(10, y=20)
.build(); .build();
assert_snapshot!(test.rename("better_name"), @r#" assert_snapshot!(test.rename("better_name"), @r#"
info[rename]: Rename symbol (found 3 locations) info[rename]: Rename symbol (found 6 locations)
--> lib.py:5:5 --> lib.py:5:5
| |
4 | @overload 4 | @overload
@ -1542,6 +1548,14 @@ result = func(10, y=20)
| ^^^^ | ^^^^
6 | @overload 6 | @overload
7 | def test(a: str) -> str: ... 7 | def test(a: str) -> str: ...
| ----
8 | @overload
9 | def test(a: int) -> int: ...
| ----
10 |
11 | def test(a: Any) -> Any:
| ----
12 | return a
| |
::: main.py:2:17 ::: main.py:2:17
| |
@ -1586,7 +1600,7 @@ result = func(10, y=20)
.build(); .build();
assert_snapshot!(test.rename("better_name"), @r#" assert_snapshot!(test.rename("better_name"), @r#"
info[rename]: Rename symbol (found 2 locations) info[rename]: Rename symbol (found 5 locations)
--> lib.py:6:9 --> lib.py:6:9
| |
4 | class Test: 4 | class Test:
@ -1595,6 +1609,14 @@ result = func(10, y=20)
| ^^^^ | ^^^^
7 | @overload 7 | @overload
8 | def test(a: str) -> str: ... 8 | def test(a: str) -> str: ...
| ----
9 | @overload
10 | def test(a: int) -> int: ...
| ----
11 |
12 | def test(a: Any) -> Any:
| ----
13 | return a
| |
::: main.py:4:8 ::: main.py:4:8
| |
@ -1636,7 +1658,7 @@ result = func(10, y=20)
.build(); .build();
assert_snapshot!(test.rename("better_name"), @r#" assert_snapshot!(test.rename("better_name"), @r#"
info[rename]: Rename symbol (found 3 locations) info[rename]: Rename symbol (found 6 locations)
--> main.py:2:17 --> main.py:2:17
| |
2 | from lib import test 2 | from lib import test
@ -1652,6 +1674,14 @@ result = func(10, y=20)
| ---- | ----
6 | @overload 6 | @overload
7 | def test(a: str) -> str: ... 7 | def test(a: str) -> str: ...
| ----
8 | @overload
9 | def test(a: int) -> int: ...
| ----
10 |
11 | def test(a: Any) -> Any:
| ----
12 | return a
| |
"#); "#);
} }
@ -1728,7 +1758,7 @@ result = func(10, y=20)
.build(); .build();
assert_snapshot!(test.rename("better_name"), @r" assert_snapshot!(test.rename("better_name"), @r"
info[rename]: Rename symbol (found 4 locations) info[rename]: Rename symbol (found 5 locations)
--> lib.py:4:9 --> lib.py:4:9
| |
2 | class Foo: 2 | class Foo:
@ -1740,6 +1770,7 @@ result = func(10, y=20)
7 | @my_property.setter 7 | @my_property.setter
| ----------- | -----------
8 | def my_property(self, value: int) -> None: 8 | def my_property(self, value: int) -> None:
| -----------
9 | pass 9 | pass
| |
::: main.py:4:13 ::: main.py:4:13
@ -1754,8 +1785,6 @@ result = func(10, y=20)
"); ");
} }
// TODO: this should rename the name of the function decorated with
// `@my_property.deleter` as well as the getter function name
#[test] #[test]
fn rename_property_with_deleter() { fn rename_property_with_deleter() {
let test = CursorTest::builder() let test = CursorTest::builder()
@ -1784,7 +1813,7 @@ result = func(10, y=20)
.build(); .build();
assert_snapshot!(test.rename("better_name"), @r" assert_snapshot!(test.rename("better_name"), @r"
info[rename]: Rename symbol (found 4 locations) info[rename]: Rename symbol (found 5 locations)
--> lib.py:4:9 --> lib.py:4:9
| |
2 | class Foo: 2 | class Foo:
@ -1796,6 +1825,7 @@ result = func(10, y=20)
7 | @my_property.deleter 7 | @my_property.deleter
| ----------- | -----------
8 | def my_property(self) -> None: 8 | def my_property(self) -> None:
| -----------
9 | pass 9 | pass
| |
::: main.py:4:13 ::: main.py:4:13
@ -1809,10 +1839,6 @@ result = func(10, y=20)
| |
"); ");
} }
// TODO: this should rename the name of the functions decorated with
// `@my_property.deleter` and `@my_property.deleter` as well as the
// getter function name
#[test] #[test]
fn rename_property_with_setter_and_deleter() { fn rename_property_with_setter_and_deleter() {
let test = CursorTest::builder() let test = CursorTest::builder()
@ -1846,7 +1872,7 @@ result = func(10, y=20)
.build(); .build();
assert_snapshot!(test.rename("better_name"), @r" assert_snapshot!(test.rename("better_name"), @r"
info[rename]: Rename symbol (found 6 locations) info[rename]: Rename symbol (found 8 locations)
--> lib.py:4:9 --> lib.py:4:9
| |
2 | class Foo: 2 | class Foo:
@ -1858,11 +1884,13 @@ result = func(10, y=20)
7 | @my_property.setter 7 | @my_property.setter
| ----------- | -----------
8 | def my_property(self, value: int) -> None: 8 | def my_property(self, value: int) -> None:
| -----------
9 | pass 9 | pass
10 | 10 |
11 | @my_property.deleter 11 | @my_property.deleter
| ----------- | -----------
12 | def my_property(self) -> None: 12 | def my_property(self) -> None:
| -----------
13 | pass 13 | pass
| |
::: main.py:4:13 ::: main.py:4:13

View File

@ -30,8 +30,8 @@ pub use suppression::create_suppression_fix;
pub use types::DisplaySettings; pub use types::DisplaySettings;
pub use types::ide_support::{ pub use types::ide_support::{
ImportAliasResolution, ResolvedDefinition, definitions_for_attribute, definitions_for_bin_op, ImportAliasResolution, ResolvedDefinition, definitions_for_attribute, definitions_for_bin_op,
definitions_for_imported_symbol, definitions_for_name, definitions_for_unary_op, definitions_for_imported_symbol, definitions_for_name, definitions_for_name_in_scope,
map_stub_definition, definitions_for_unary_op, map_stub_definition,
}; };
pub mod ast_node_ref; pub mod ast_node_ref;

View File

@ -33,7 +33,7 @@ pub struct Definition<'db> {
pub file: File, pub file: File,
/// The scope in which the definition occurs. /// The scope in which the definition occurs.
pub(crate) file_scope: FileScopeId, pub file_scope: FileScopeId,
/// The place ID of the definition. /// The place ID of the definition.
pub(crate) place: ScopedPlaceId, pub(crate) place: ScopedPlaceId,

View File

@ -190,6 +190,53 @@ pub fn definitions_for_name<'db>(
} }
} }
/// Returns all definitions for a name in a specific scope.
/// This is useful when you have a Definition and want to find all other definitions
/// with the same name in its scope (e.g., for overloaded functions).
pub fn definitions_for_name_in_scope<'db>(
db: &'db dyn Db,
file: ruff_db::files::File,
file_scope_id: crate::semantic_index::scope::FileScopeId,
name_str: &str,
alias_resolution: ImportAliasResolution,
) -> Vec<ResolvedDefinition<'db>> {
let index = semantic_index(db, file);
let place_table = index.place_table(file_scope_id);
let Some(symbol_id) = place_table.symbol_id(name_str) else {
return vec![];
};
let use_def_map = index.use_def_map(file_scope_id);
let mut all_definitions = FxIndexSet::default();
// Get all definitions (both bindings and declarations) for this symbol
let bindings = use_def_map.all_reachable_symbol_bindings(symbol_id);
let declarations = use_def_map.all_reachable_symbol_declarations(symbol_id);
for binding in bindings {
if let Some(def) = binding.binding.definition() {
all_definitions.insert(def);
}
}
for declaration in declarations {
if let Some(def) = declaration.declaration.definition() {
all_definitions.insert(def);
}
}
// Resolve import definitions to their targets
let mut resolved_definitions = Vec::new();
for definition in &all_definitions {
let resolved = resolve_definition(db, *definition, Some(name_str), alias_resolution);
resolved_definitions.extend(resolved);
}
resolved_definitions
}
fn is_float_or_complex_annotation(db: &dyn Db, ty: UnionType, name: &str) -> bool { fn is_float_or_complex_annotation(db: &dyn Db, ty: UnionType, name: &str) -> bool {
let float_or_complex_ty = match name { let float_or_complex_ty = match name {
"float" => UnionType::from_elements( "float" => UnionType::from_elements(