-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Implement patterns and typevars in the LSP #21671
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
| let (symbol_name, is_reexported) = if let Some(asname) = &alias.asname { | ||
| self.scopes_by_expression | ||
| .record_expression(asname, self.current_scope()); |
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 actually useless I think (I added it while thrashing and trying to get imports rename/reference working... it did not work).
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.
Should we remove it then? Seems easy enough to add once we know why we need it
MichaReiser
left a comment
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.
Nice, this actually streamlines things :)
crates/ty_ide/src/goto.rs
Outdated
| ) -> Option<DefinitionsOrTargets<'db>> { | ||
| use crate::NavigationTarget; | ||
| ) -> Option<Definitions<'db>> { | ||
| match self { |
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.
Nit: You could reduce the boilerplate here, I think, by doing something like:
let definitions = match self {
GotoTarget::Expression(expression) => {
definitions_for_expression(model, *expression)
},
GotoTarget::FunctionDef(function) => {
Some(vec![ResolvedDefinition::Definition(
function.definition(model),
)])
},
...
};
definitions.map(Definitions)
| self.scopes_by_expression | ||
| .record_expression(name, self.current_scope()); |
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.
Should we add a record_name method or at least rename the struct (or update its documentation) to explain that, yes, it's mostly expressions but sometimes it's names too
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 probably a good idea, I'll integrate it into some followup work I have planned that will involve more reckoning about ""expressions""
| let (symbol_name, is_reexported) = if let Some(asname) = &alias.asname { | ||
| self.scopes_by_expression | ||
| .record_expression(asname, self.current_scope()); |
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.
Should we remove it then? Seems easy enough to add once we know why we need it
Summary
This is the final goto-targets with missing goto-definition/declaration implementations!
You can now theoretically click on all the user-defined names in all the syntax. 🎉
This adds:
*restpatternsThis notably does not add:
Also I realized we were at the precipice of one of the great GotoTarget sins being resolved, and so I made import aliases also resolve to a ResolvedDefinition. This removes a ton of cruft and prevents further backsliding.
Note however that import aliases are, in general, completely jacked up when it comes to find-references/renames (both before and after this PR). Previously you could try to rename an import alias and it just wouldn't do anything. With this change we instead refuse to even let you try to rename it.
Sorting out why import aliases are jacked up is an ongoing thing I hope to handle in a followup.
Test Plan
You'll surely not regret checking in 86 snapshot tests