Implement goto-definition and find-references for global/nonlocal statements (#21616)

## Summary

The implementation here is to just record the idents of these statements
in `scopes_by_expression` (which already supported idents but only ones
that happened to appear in expressions), so that `definitions_for_name`
Just Works.

goto-type (and therefore hover) notably does not work on these
statements because the typechecker does not record info for them. I am
tempted to just introduce `type_for_name` which runs
`definitions_for_name` to find other expressions and queries the
inferred type... but that's a bit whack because it won't be the computed
type at the right point in the code. It probably wouldn't be
particularly expensive to just compute/record the type at those nodes,
as if they were a load, because global/nonlocal is so scarce?

## Test Plan

Snapshot tests added/re-enabled.
This commit is contained in:
Aria Desires 2025-11-25 08:56:57 -05:00 committed by GitHub
parent 88bfc32dfc
commit 209ea06592
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 342 additions and 34 deletions

View File

@ -331,15 +331,6 @@ impl GotoTarget<'_> {
let module = import_name(module_name, *component_index); let module = import_name(module_name, *component_index);
model.resolve_module_type(Some(module), *level)? model.resolve_module_type(Some(module), *level)?
} }
// TODO: Support identifier targets
GotoTarget::PatternMatchRest(_)
| GotoTarget::PatternKeywordArgument(_)
| GotoTarget::PatternMatchStarName(_)
| GotoTarget::PatternMatchAsName(_)
| GotoTarget::TypeParamParamSpecName(_)
| GotoTarget::TypeParamTypeVarTupleName(_)
| GotoTarget::NonLocal { .. }
| GotoTarget::Globals { .. } => return None,
GotoTarget::StringAnnotationSubexpr { GotoTarget::StringAnnotationSubexpr {
string_expr, string_expr,
subrange, subrange,
@ -366,6 +357,15 @@ impl GotoTarget<'_> {
let (_, ty) = ty_python_semantic::definitions_for_unary_op(model, expression)?; let (_, ty) = ty_python_semantic::definitions_for_unary_op(model, expression)?;
ty ty
} }
// TODO: Support identifier targets
GotoTarget::PatternMatchRest(_)
| GotoTarget::PatternKeywordArgument(_)
| GotoTarget::PatternMatchStarName(_)
| GotoTarget::PatternMatchAsName(_)
| GotoTarget::TypeParamParamSpecName(_)
| GotoTarget::TypeParamTypeVarTupleName(_)
| GotoTarget::NonLocal { .. }
| GotoTarget::Globals { .. } => return None,
}; };
Some(ty) Some(ty)
@ -402,9 +402,8 @@ impl GotoTarget<'_> {
) -> Option<DefinitionsOrTargets<'db>> { ) -> Option<DefinitionsOrTargets<'db>> {
use crate::NavigationTarget; use crate::NavigationTarget;
match self { match self {
GotoTarget::Expression(expression) => { GotoTarget::Expression(expression) => definitions_for_expression(model, *expression)
definitions_for_expression(model, expression).map(DefinitionsOrTargets::Definitions) .map(DefinitionsOrTargets::Definitions),
}
// For already-defined symbols, they are their own definitions // For already-defined symbols, they are their own definitions
GotoTarget::FunctionDef(function) => Some(DefinitionsOrTargets::Definitions(vec![ GotoTarget::FunctionDef(function) => Some(DefinitionsOrTargets::Definitions(vec![
ResolvedDefinition::Definition(function.definition(model)), ResolvedDefinition::Definition(function.definition(model)),
@ -511,7 +510,7 @@ impl GotoTarget<'_> {
GotoTarget::Call { callable, call } => { GotoTarget::Call { callable, call } => {
let mut definitions = definitions_for_callable(model, call); let mut definitions = definitions_for_callable(model, call);
let expr_definitions = let expr_definitions =
definitions_for_expression(model, callable).unwrap_or_default(); definitions_for_expression(model, *callable).unwrap_or_default();
definitions.extend(expr_definitions); definitions.extend(expr_definitions);
if definitions.is_empty() { if definitions.is_empty() {
@ -545,18 +544,23 @@ impl GotoTarget<'_> {
let subexpr = covering_node(subast.syntax().into(), *subrange) let subexpr = covering_node(subast.syntax().into(), *subrange)
.node() .node()
.as_expr_ref()?; .as_expr_ref()?;
definitions_for_expression(&submodel, &subexpr) definitions_for_expression(&submodel, subexpr)
.map(DefinitionsOrTargets::Definitions) .map(DefinitionsOrTargets::Definitions)
} }
GotoTarget::NonLocal { identifier } | GotoTarget::Globals { identifier } => {
Some(DefinitionsOrTargets::Definitions(definitions_for_name(
model,
identifier.as_str(),
AnyNodeRef::Identifier(identifier),
)))
}
// TODO: implement these // TODO: implement these
GotoTarget::PatternKeywordArgument(..) GotoTarget::PatternKeywordArgument(..)
| GotoTarget::PatternMatchStarName(..) | GotoTarget::PatternMatchStarName(..)
| GotoTarget::TypeParamTypeVarName(..) | GotoTarget::TypeParamTypeVarName(..)
| GotoTarget::TypeParamParamSpecName(..) | GotoTarget::TypeParamParamSpecName(..)
| GotoTarget::TypeParamTypeVarTupleName(..) | GotoTarget::TypeParamTypeVarTupleName(..) => None,
| GotoTarget::NonLocal { .. }
| GotoTarget::Globals { .. } => None,
} }
} }
@ -916,9 +920,9 @@ impl Ranged for GotoTarget<'_> {
} }
/// Converts a collection of `ResolvedDefinition` items into `NavigationTarget` items. /// Converts a collection of `ResolvedDefinition` items into `NavigationTarget` items.
fn convert_resolved_definitions_to_targets( fn convert_resolved_definitions_to_targets<'db>(
db: &dyn ty_python_semantic::Db, db: &'db dyn ty_python_semantic::Db,
definitions: Vec<ty_python_semantic::ResolvedDefinition<'_>>, definitions: Vec<ty_python_semantic::ResolvedDefinition<'db>>,
) -> Vec<crate::NavigationTarget> { ) -> Vec<crate::NavigationTarget> {
definitions definitions
.into_iter() .into_iter()
@ -953,10 +957,14 @@ fn convert_resolved_definitions_to_targets(
/// Shared helper to get definitions for an expr (that is presumably a name/attr) /// Shared helper to get definitions for an expr (that is presumably a name/attr)
fn definitions_for_expression<'db>( fn definitions_for_expression<'db>(
model: &SemanticModel<'db>, model: &SemanticModel<'db>,
expression: &ruff_python_ast::ExprRef<'_>, expression: ruff_python_ast::ExprRef<'_>,
) -> Option<Vec<ResolvedDefinition<'db>>> { ) -> Option<Vec<ResolvedDefinition<'db>>> {
match expression { match expression {
ast::ExprRef::Name(name) => Some(definitions_for_name(model, name)), ast::ExprRef::Name(name) => Some(definitions_for_name(
model,
name.id.as_str(),
expression.into(),
)),
ast::ExprRef::Attribute(attribute) => Some(ty_python_semantic::definitions_for_attribute( ast::ExprRef::Attribute(attribute) => Some(ty_python_semantic::definitions_for_attribute(
model, attribute, model, attribute,
)), )),

View File

@ -1254,6 +1254,45 @@ def outer():
"#); "#);
} }
#[test]
fn goto_declaration_nonlocal_stmt() {
let test = cursor_test(
r#"
def outer():
xy = "outer_value"
def inner():
nonlocal x<CURSOR>y
xy = "modified"
return x # Should find the nonlocal x declaration in outer scope
return inner
"#,
);
// Should find the variable declaration in the outer scope, not the nonlocal statement
assert_snapshot!(test.goto_declaration(), @r#"
info[goto-declaration]: Declaration
--> main.py:3:5
|
2 | def outer():
3 | xy = "outer_value"
| ^^
4 |
5 | def inner():
|
info: Source
--> main.py:6:18
|
5 | def inner():
6 | nonlocal xy
| ^^
7 | xy = "modified"
8 | return x # Should find the nonlocal x declaration in outer scope
|
"#);
}
#[test] #[test]
fn goto_declaration_global_binding() { fn goto_declaration_global_binding() {
let test = cursor_test( let test = cursor_test(
@ -1288,6 +1327,41 @@ def function():
"#); "#);
} }
#[test]
fn goto_declaration_global_stmt() {
let test = cursor_test(
r#"
global_var = "global_value"
def function():
global global_<CURSOR>var
global_var = "modified"
return global_var # Should find the global variable declaration
"#,
);
// Should find the global variable declaration, not the global statement
assert_snapshot!(test.goto_declaration(), @r#"
info[goto-declaration]: Declaration
--> main.py:2:1
|
2 | global_var = "global_value"
| ^^^^^^^^^^
3 |
4 | def function():
|
info: Source
--> main.py:5:12
|
4 | def function():
5 | global global_var
| ^^^^^^^^^^
6 | global_var = "modified"
7 | return global_var # Should find the global variable declaration
|
"#);
}
#[test] #[test]
fn goto_declaration_inherited_attribute() { fn goto_declaration_inherited_attribute() {
let test = cursor_test( let test = cursor_test(

View File

@ -149,7 +149,6 @@ result = calculate_sum(value=42)
} }
#[test] #[test]
#[ignore] // TODO: Enable when nonlocal support is fully implemented in goto.rs
fn test_nonlocal_variable_references() { fn test_nonlocal_variable_references() {
let test = cursor_test( let test = cursor_test(
" "
@ -273,7 +272,6 @@ def outer_function():
} }
#[test] #[test]
#[ignore] // TODO: Enable when global support is fully implemented in goto.rs
fn test_global_variable_references() { fn test_global_variable_references() {
let test = cursor_test( let test = cursor_test(
" "

View File

@ -1062,6 +1062,118 @@ f(**kwargs<CURSOR>)
"#); "#);
} }
#[test]
fn goto_type_nonlocal_binding() {
let test = cursor_test(
r#"
def outer():
x = "outer_value"
def inner():
nonlocal x
x = "modified"
return x<CURSOR> # Should find the nonlocal x declaration in outer scope
return inner
"#,
);
// Should find the variable declaration in the outer scope, not the nonlocal statement
assert_snapshot!(test.goto_type_definition(), @r#"
info[goto-type-definition]: Type definition
--> stdlib/builtins.pyi:915:7
|
914 | @disjoint_base
915 | class str(Sequence[str]):
| ^^^
916 | """str(object='') -> str
917 | str(bytes_or_buffer[, encoding[, errors]]) -> str
|
info: Source
--> main.py:8:16
|
6 | nonlocal x
7 | x = "modified"
8 | return x # Should find the nonlocal x declaration in outer scope
| ^
9 |
10 | return inner
|
"#);
}
#[test]
fn goto_type_nonlocal_stmt() {
let test = cursor_test(
r#"
def outer():
xy = "outer_value"
def inner():
nonlocal x<CURSOR>y
xy = "modified"
return x # Should find the nonlocal x declaration in outer scope
return inner
"#,
);
// Should find the variable declaration in the outer scope, not the nonlocal statement
assert_snapshot!(test.goto_type_definition(), @"No goto target found");
}
#[test]
fn goto_type_global_binding() {
let test = cursor_test(
r#"
global_var = "global_value"
def function():
global global_var
global_var = "modified"
return global_<CURSOR>var # Should find the global variable declaration
"#,
);
// Should find the global variable declaration, not the global statement
assert_snapshot!(test.goto_type_definition(), @r#"
info[goto-type-definition]: Type definition
--> stdlib/builtins.pyi:915:7
|
914 | @disjoint_base
915 | class str(Sequence[str]):
| ^^^
916 | """str(object='') -> str
917 | str(bytes_or_buffer[, encoding[, errors]]) -> str
|
info: Source
--> main.py:7:12
|
5 | global global_var
6 | global_var = "modified"
7 | return global_var # Should find the global variable declaration
| ^^^^^^^^^^
|
"#);
}
#[test]
fn goto_type_global_stmt() {
let test = cursor_test(
r#"
global_var = "global_value"
def function():
global global_<CURSOR>var
global_var = "modified"
return global_var # Should find the global variable declaration
"#,
);
// Should find the global variable declaration, not the global statement
assert_snapshot!(test.goto_type_definition(), @"No goto target found");
}
#[test] #[test]
fn goto_type_of_expression_with_builtin() { fn goto_type_of_expression_with_builtin() {
let test = cursor_test( let test = cursor_test(

View File

@ -1648,6 +1648,117 @@ def ab(a: int, *, c: int):
"); ");
} }
#[test]
fn hover_nonlocal_binding() {
let test = cursor_test(
r#"
def outer():
x = "outer_value"
def inner():
nonlocal x
x = "modified"
return x<CURSOR> # Should find the nonlocal x declaration in outer scope
return inner
"#,
);
// Should find the variable declaration in the outer scope, not the nonlocal statement
assert_snapshot!(test.hover(), @r#"
Literal["modified"]
---------------------------------------------
```python
Literal["modified"]
```
---------------------------------------------
info[hover]: Hovered content is
--> main.py:8:16
|
6 | nonlocal x
7 | x = "modified"
8 | return x # Should find the nonlocal x declaration in outer scope
| ^- Cursor offset
| |
| source
9 |
10 | return inner
|
"#);
}
#[test]
fn hover_nonlocal_stmt() {
let test = cursor_test(
r#"
def outer():
xy = "outer_value"
def inner():
nonlocal x<CURSOR>y
xy = "modified"
return x # Should find the nonlocal x declaration in outer scope
return inner
"#,
);
// Should find the variable declaration in the outer scope, not the nonlocal statement
assert_snapshot!(test.hover(), @"Hover provided no content");
}
#[test]
fn hover_global_binding() {
let test = cursor_test(
r#"
global_var = "global_value"
def function():
global global_var
global_var = "modified"
return global_<CURSOR>var # Should find the global variable declaration
"#,
);
// Should find the global variable declaration, not the global statement
assert_snapshot!(test.hover(), @r#"
Literal["modified"]
---------------------------------------------
```python
Literal["modified"]
```
---------------------------------------------
info[hover]: Hovered content is
--> main.py:7:12
|
5 | global global_var
6 | global_var = "modified"
7 | return global_var # Should find the global variable declaration
| ^^^^^^^-^^
| | |
| | Cursor offset
| source
|
"#);
}
#[test]
fn hover_global_stmt() {
let test = cursor_test(
r#"
global_var = "global_value"
def function():
global global_<CURSOR>var
global_var = "modified"
return global_var # Should find the global variable declaration
"#,
);
// Should find the global variable declaration, not the global statement
assert_snapshot!(test.hover(), @"Hover provided no content");
}
#[test] #[test]
fn hover_module_import() { fn hover_module_import() {
let mut test = cursor_test( let mut test = cursor_test(

View File

@ -2255,6 +2255,8 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> {
names, names,
}) => { }) => {
for name in names { for name in names {
self.scopes_by_expression
.record_expression(name, self.current_scope());
let symbol_id = self.add_symbol(name.id.clone()); let symbol_id = self.add_symbol(name.id.clone());
let symbol = self.current_place_table().symbol(symbol_id); let symbol = self.current_place_table().symbol(symbol_id);
// Check whether the variable has already been accessed in this scope. // Check whether the variable has already been accessed in this scope.
@ -2290,6 +2292,8 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> {
names, names,
}) => { }) => {
for name in names { for name in names {
self.scopes_by_expression
.record_expression(name, self.current_scope());
let symbol_id = self.add_symbol(name.id.clone()); let symbol_id = self.add_symbol(name.id.clone());
let symbol = self.current_place_table().symbol(symbol_id); let symbol = self.current_place_table().symbol(symbol_id);
// Check whether the variable has already been accessed in this scope. // Check whether the variable has already been accessed in this scope.

View File

@ -22,7 +22,7 @@ use crate::{Db, DisplaySettings, HasType, NameKind, SemanticModel};
use ruff_db::files::FileRange; use ruff_db::files::FileRange;
use ruff_db::parsed::parsed_module; use ruff_db::parsed::parsed_module;
use ruff_python_ast::name::Name; use ruff_python_ast::name::Name;
use ruff_python_ast::{self as ast}; use ruff_python_ast::{self as ast, AnyNodeRef};
use ruff_text_size::{Ranged, TextRange}; use ruff_text_size::{Ranged, TextRange};
use rustc_hash::FxHashSet; use rustc_hash::FxHashSet;
@ -515,7 +515,7 @@ pub fn definition_for_name<'db>(
model: &SemanticModel<'db>, model: &SemanticModel<'db>,
name: &ast::ExprName, name: &ast::ExprName,
) -> Option<Definition<'db>> { ) -> Option<Definition<'db>> {
let definitions = definitions_for_name(model, name); let definitions = definitions_for_name(model, name.id.as_str(), name.into());
// Find the first valid definition and return its kind // Find the first valid definition and return its kind
for declaration in definitions { for declaration in definitions {
@ -531,15 +531,15 @@ pub fn definition_for_name<'db>(
/// are resolved (recursively) to the original definitions or module files. /// are resolved (recursively) to the original definitions or module files.
pub fn definitions_for_name<'db>( pub fn definitions_for_name<'db>(
model: &SemanticModel<'db>, model: &SemanticModel<'db>,
name: &ast::ExprName, name_str: &str,
node: AnyNodeRef<'_>,
) -> Vec<ResolvedDefinition<'db>> { ) -> Vec<ResolvedDefinition<'db>> {
let db = model.db(); let db = model.db();
let file = model.file(); let file = model.file();
let index = semantic_index(db, file); let index = semantic_index(db, file);
let name_str = name.id.as_str();
// Get the scope for this name expression // Get the scope for this name expression
let Some(file_scope) = model.scope(name.into()) else { let Some(file_scope) = model.scope(node) else {
return vec![]; return vec![];
}; };
@ -648,7 +648,8 @@ pub fn definitions_for_name<'db>(
// //
// https://typing.python.org/en/latest/spec/special-types.html#special-cases-for-float-and-complex // https://typing.python.org/en/latest/spec/special-types.html#special-cases-for-float-and-complex
if matches!(name_str, "float" | "complex") if matches!(name_str, "float" | "complex")
&& let Some(union) = name.inferred_type(&SemanticModel::new(db, file)).as_union() && let Some(expr) = node.expr_name()
&& let Some(union) = expr.inferred_type(&SemanticModel::new(db, file)).as_union()
&& is_float_or_complex_annotation(db, union, name_str) && is_float_or_complex_annotation(db, union, name_str)
{ {
return union return union