internal: Explicitly check for reference locals or fields in Name classification #9569
Conversation
|
Guess I'll rebase on top of #9567 once its merged |
|
bors r+ |
9569: Explicitly check for reference locals or fields in Name classification r=Veykril a=Veykril Closes #7524 Co-authored-by: Lukas Wirth <[email protected]>
crates/ide_db/src/defs.rs
Outdated
| /// `Definition`, which this name refers to. | ||
| pub fn referenced(self) -> Definition { | ||
| /// `Definition`, which this name refers to with a preference for the field reference in case of a field shorthand. | ||
| pub fn referenced_field(self) -> Definition { |
There was a problem hiding this comment.
Hm, not sure if that's the best API: or_field feels too specific. I wonder if the best solution here is maybe not to have any methods at all, and just write explicit matches at every call-site? That's more code, but it makes it clear when we ignore some cases...
There was a problem hiding this comment.
Ye the main reason I didn't feel like doing that was due to the extern crate variant which you conveniently got rid of now. So inling all of these might be fine now.
| @@ -878,10 +877,11 @@ fn main() { | |||
| r#" | |||
| enum Foo { | |||
| Bar { x: i32 } | |||
| } //^ | |||
| } | |||
| fn baz(foo: Foo) { | |||
| match foo { | |||
| Foo::Bar { x$0 } => x | |||
There was a problem hiding this comment.
Hm, think we shouldn't change the test, the previous behavior feels more helpful.
There was a problem hiding this comment.
But that would be inconsistent in my eyes.
If you were to use goto_defintion on a reference to a local you would go to the pattern definition here, invoking it on there again I would expect to see all references of the local instead of going to the field definiton.
There was a problem hiding this comment.
Yeah, it seems like this actually regresses behavior, let's try avoid breaking this
bors r-
There was a problem hiding this comment.
What I was considering though would be to have goto_declaration instead to go to the field definition in that case while having goto_definiton stick to the local.
There was a problem hiding this comment.
Hm, good point about find references...
I guess, there are two different IDE actions: gotodef and findref. If gotodef is invoked, then I think it is clear that it's more helpful to show field definition. If findref is invoked, then it's more helpful to look for usages of local items.
However, the editor also implements a fallback behavior, where invoking gotodef on the def iteslf then invokes findref (i guess the editor just checkst that nav-target range intersects with current cursor position to detect this?).
If we implement gotodef in the more helpful way, than we break editor's fallback.
Seems like fundamentally we just have two different things at the same cursor position, and it's impossible to cover all cases 100% correctly without showing some kind of multiresolve dialog to the user 🤔
Hm, it seems that LSP actually allows us to return both definitions in this case?
If that's the case, I think the ideal behavior here is to return two locatins for gotodef, and, for findref, look only for local variables.
There was a problem hiding this comment.
Ye if we can return multiple definitions for goto_def that seems like the correct solution then.
|
Canceled. |
|
bors r+ |
Veykril
changed the title
Closes #7524
Inlines all the calls to reference related name classification as well as emits both goto definition targets for field shorthands.