-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[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
base: main
Are you sure you want to change the base?
Conversation
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(), | ||
) |
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 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}{}", |
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.
Oops.
pub(crate) struct GotoDefinitionRequestHandler; | ||
|
||
impl RequestHandler for GotoDefinitionRequestHandler { | ||
type RequestType = GotoDefinition; | ||
} |
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.
Everything in this file is a copy of goto_type_definition but changed to goto_definition
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) |
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 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.
2 | ab = 1 | ||
3 | if cond: | ||
4 | ab = 2 | ||
| ^^ | ||
5 | print(ab) |
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.
(in this case we point to both ab = 1
(above) and ab = 2
as both bindings can affect the load)
ab: int | ||
print(a<CURSOR>b) | ||
"#, | ||
); | ||
|
||
assert_snapshot!(test.goto_definition(), @"No definitions found"); |
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.
In this case there's no bindings for ab
so we fail to yield anything (suboptimal).
a<CURSOR>b += 2 | ||
print(ab) | ||
"#, | ||
); | ||
|
||
assert_snapshot!(test.goto_definition(), @"No goto target found"); |
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.
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
)
crates/ty_ide/src/goto.rs
Outdated
2 | class AB(val: int): | ||
| ^^ | ||
3 | self.myval = val | ||
| | ||
info: Source | ||
--> main.py:5:17 | ||
| | ||
3 | self.myval = val | ||
4 | | ||
5 | x = AB(5) | ||
| ^^ |
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.
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.
x = AB(5) | ||
print(x.my<CURSOR>val) | ||
"#, | ||
); | ||
|
||
assert_snapshot!(test.goto_definition(), @"No goto target found"); |
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 one is hopeless without type info.
RED = "red" | ||
BLUE = "blue" | ||
x = AB.RE<CURSOR>D |
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.
Similarly hopeless without type info
ab = 1 | ||
def myfunc(): | ||
global ab | ||
print(a<CURSOR>b) | ||
"#, | ||
); | ||
|
||
assert_snapshot!(test.goto_definition(), @"No definitions found"); |
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.
Globals of course fail here without processing of parent scopes.
|
No description provided.