Skip to content

[ty] Add initial implementation of goto definition for loads of local names #19123

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Jul 3, 2025

No description provided.

@Gankra Gankra added server Related to the LSP server ty Multi-file analysis & type inference labels Jul 3, 2025
Comment on lines 164 to 175
let index = semantic_index(model.db, model.file);
let file_scope = index.expression_scope_id(*self);
let scope = file_scope.to_scope_id(model.db, model.file);
let use_def = index.use_def_map(file_scope);
let use_id = self.scoped_use_id(model.db, scope);

Some(
use_def
.bindings_at_use(use_id)
.filter_map(|binding| binding.binding.definition())
.collect(),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically the happy path of TypeInferenceBuilder::infer_place_load. It's extremely limited because it can't walk into parent scopes, and because it never engages type inference, it can't reason about "oh this is MyClass.myfield".

I feel like the more Correct solution we want here is to augment infer_place_load to also yield Definition info, and then run the whole TypeInferenceBuilder machinery just like inferred_type does.

@@ -2667,7 +2667,7 @@ impl<'db> BindingError<'db> {
if let Some(builder) = context.report_lint(&MISSING_ARGUMENT, node) {
let s = if parameters.0.len() == 1 { "" } else { "s" };
let mut diag = builder.into_diagnostic(format_args!(
"No argument{s} provided for required parameter{s} {parameters}{}",
"No argument{s} provided for required parameterzz{s} {parameters}{}",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.

Comment on lines +15 to +19
pub(crate) struct GotoDefinitionRequestHandler;

impl RequestHandler for GotoDefinitionRequestHandler {
type RequestType = GotoDefinition;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything in this file is a copy of goto_type_definition but changed to goto_definition

Comment on lines +938 to +946
assert_snapshot!(test.goto_definition(), @r"
info[goto-type-definition]: Type definition
--> main.py:4:13
|
2 | ab = 1
3 | ab = 2
4 | ab = 3
| ^^
5 | print(ab)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first non-trivial property of the current implementation: the "definitions" we yield are the bindings that dominate the load. This is a useful answer but obviously not the only possible one.

Comment on lines +991 to +995
2 | ab = 1
3 | if cond:
4 | ab = 2
| ^^
5 | print(ab)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(in this case we point to both ab = 1 (above) and ab = 2 as both bindings can affect the load)

Comment on lines +1063 to +1068
ab: int
print(a<CURSOR>b)
"#,
);

assert_snapshot!(test.goto_definition(), @"No definitions found");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case there's no bindings for ab so we fail to yield anything (suboptimal).

Comment on lines +1186 to +1191
a<CURSOR>b += 2
print(ab)
"#,
);

assert_snapshot!(test.goto_definition(), @"No goto target found");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any kind of Store completely falls over right now -- it seems like most of the code gives up when it sees Store, even if it's also a Load, but maybe it's handled by different paths? (in the playground ab += 2 does seem to support getting the type of ab)

Comment on lines 1209 to 1221
2 | class AB(val: int):
| ^^
3 | self.myval = val
|
info: Source
--> main.py:5:17
|
3 | self.myval = val
4 |
5 | x = AB(5)
| ^^
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is only working because the class is defined in the same scope. If x = AB(5) was in a function it would fail to resolve, because we don't look into parent scopes. yet.

Comment on lines +1231 to +1256
x = AB(5)
print(x.my<CURSOR>val)
"#,
);

assert_snapshot!(test.goto_definition(), @"No goto target found");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is hopeless without type info.

RED = "red"
BLUE = "blue"
x = AB.RE<CURSOR>D
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly hopeless without type info

Comment on lines +1335 to +1363
ab = 1
def myfunc():
global ab
print(a<CURSOR>b)
"#,
);

assert_snapshot!(test.goto_definition(), @"No definitions found");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Globals of course fail here without processing of parent scopes.

Copy link
Contributor

github-actions bot commented Jul 3, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Related to the LSP server ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant