Merge #9569

9569: internal: Explicitly check for reference locals or fields in Name classification r=Veykril a=Veykril

Closes #7524
Inlines all the calls to reference related name classification as well as emits both goto definition targets for field shorthands.

Co-authored-by: Lukas Wirth <[email protected]>

Author: bors[bot]

crates/ide/src/doc_links.rs

@@ -112,8 +112,16 @@ pub(crate) fn external_docs(
     let node = token.parent()?;
     let definition = match_ast! {
         match node {
-            ast::NameRef(name_ref) => NameRefClass::classify(&sema, &name_ref).map(|d| d.referenced())?,
-            ast::Name(name) => NameClass::classify(&sema, &name).map(|d| d.referenced_or_defined())?,
+            ast::NameRef(name_ref) => match NameRefClass::classify(&sema, &name_ref)? {
+                NameRefClass::Definition(def) => def,
+                NameRefClass::FieldShorthand { local_ref: _, field_ref } => {
+                    Definition::Field(field_ref)
+                }
+            },
+            ast::Name(name) => match NameClass::classify(&sema, &name)? {
+                NameClass::Definition(it) | NameClass::ConstReference(it) => it,
+                NameClass::PatFieldShorthand { local_def: _, field_ref } => Definition::Field(field_ref),
+            },
             _ => return None,
         }
     };

crates/ide/src/fixture.rs

@@ -51,11 +51,3 @@ pub(crate) fn annotations(ra_fixture: &str) -> (Analysis, FilePosition, Vec<(Fil
         .collect();
     (host.analysis(), FilePosition { file_id, offset }, annotations)
 }
-
-pub(crate) fn nav_target_annotation(ra_fixture: &str) -> (Analysis, FilePosition, FileRange) {
-    let (analysis, position, mut annotations) = annotations(ra_fixture);
-    let (expected, data) = annotations.pop().unwrap();
-    assert!(annotations.is_empty());
-    assert_eq!(data, "");
-    (analysis, position, expected)
-}

crates/ide/src/goto_declaration.rs

@@ -23,45 +23,56 @@ pub(crate) fn goto_declaration(
     let parent = token.parent()?;
     let def = match_ast! {
         match parent {
-            ast::NameRef(name_ref) => {
-                let name_kind = NameRefClass::classify(&sema, &name_ref)?;
-                name_kind.referenced()
+            ast::NameRef(name_ref) => match NameRefClass::classify(&sema, &name_ref)? {
+                NameRefClass::Definition(it) => Some(it),
+                _ => None
             },
-            ast::Name(name) => {
-                NameClass::classify(&sema, &name)?.referenced_or_defined()
+            ast::Name(name) => match NameClass::classify(&sema, &name)? {
+                NameClass::Definition(it) => Some(it),
+                _ => None
             },
-            _ => return None,
+            _ => None,
         }
     };
-    match def {
+    match def? {
         Definition::ModuleDef(hir::ModuleDef::Module(module)) => Some(RangeInfo::new(
             original_token.text_range(),
             vec![NavigationTarget::from_module_to_decl(db, module)],
         )),
-        _ => return None,
+        _ => None,
     }
 }
 
 #[cfg(test)]
 mod tests {
     use ide_db::base_db::FileRange;
+    use itertools::Itertools;
 
     use crate::fixture;
 
     fn check(ra_fixture: &str) {
-        let (analysis, position, expected) = fixture::nav_target_annotation(ra_fixture);
-        let mut navs = analysis
+        let (analysis, position, expected) = fixture::annotations(ra_fixture);
+        let navs = analysis
             .goto_declaration(position)
             .unwrap()
             .expect("no declaration or definition found")
             .info;
         if navs.len() == 0 {
             panic!("unresolved reference")
         }
-        assert_eq!(navs.len(), 1);
 
-        let nav = navs.pop().unwrap();
-        assert_eq!(expected, FileRange { file_id: nav.file_id, range: nav.focus_or_full_range() });
+        let cmp = |&FileRange { file_id, range }: &_| (file_id, range.start());
+        let navs = navs
+            .into_iter()
+            .map(|nav| FileRange { file_id: nav.file_id, range: nav.focus_or_full_range() })
+            .sorted_by_key(cmp)
+            .collect::<Vec<_>>();
+        let expected = expected
+            .into_iter()
+            .map(|(FileRange { file_id, range }, _)| FileRange { file_id, range })
+            .sorted_by_key(cmp)
+            .collect::<Vec<_>>();
+        assert_eq!(expected, navs);
     }
 
     #[test]

crates/ide/src/goto_definition.rs

@@ -1,4 +1,4 @@
-use std::convert::TryInto;
+use std::{convert::TryInto, iter};
 
 use either::Either;
 use hir::{AsAssocItem, InFile, ModuleDef, Semantics};
@@ -11,7 +11,7 @@ use ide_db::{
 use syntax::{ast, match_ast, AstNode, AstToken, SyntaxKind::*, SyntaxToken, TextRange, T};
 
 use crate::{
-    display::TryToNav,
+    display::{ToNav, TryToNav},
     doc_links::{doc_attributes, extract_definitions_from_markdown, resolve_doc_path_for_def},
     FilePosition, NavigationTarget, RangeInfo,
 };
@@ -54,36 +54,44 @@ pub(crate) fn goto_definition(
         let nav = resolve_doc_path_for_def(db, def, &link, ns)?.try_to_nav(db)?;
         return Some(RangeInfo::new(original_token.text_range(), vec![nav]));
     }
-    let nav = match_ast! {
+
+    let navs = match_ast! {
         match parent {
             ast::NameRef(name_ref) => {
                 reference_definition(&sema, Either::Right(&name_ref))
             },
             ast::Name(name) => {
-                let def = NameClass::classify(&sema, &name)?.referenced_or_defined();
-                try_find_trait_item_definition(sema.db, &def)
-                    .or_else(|| def.try_to_nav(sema.db))
+                match NameClass::classify(&sema, &name)? {
+                    NameClass::Definition(def) | NameClass::ConstReference(def) => {
+                        try_find_trait_item_definition(sema.db, &def).unwrap_or_else(|| def_to_nav(sema.db, def))
+                    }
+                    NameClass::PatFieldShorthand { local_def, field_ref } => {
+                        local_and_field_to_nav(sema.db, local_def, field_ref)
+                    },
+                }
             },
             ast::Lifetime(lt) => if let Some(name_class) = NameClass::classify_lifetime(&sema, &lt) {
-                let def = name_class.referenced_or_defined();
-                def.try_to_nav(sema.db)
+                match name_class {
+                    NameClass::Definition(def) => def_to_nav(sema.db, def),
+                    _ => return None,
+                }
             } else {
                 reference_definition(&sema, Either::Left(&lt))
             },
-            ast::TokenTree(tt) => try_lookup_include_path(sema.db, tt, token, position.file_id),
+            ast::TokenTree(tt) => try_lookup_include_path(sema.db, tt, token, position.file_id)?,
             _ => return None,
         }
     };
 
-    Some(RangeInfo::new(original_token.text_range(), nav.into_iter().collect()))
+    Some(RangeInfo::new(original_token.text_range(), navs))
 }
 
 fn try_lookup_include_path(
     db: &RootDatabase,
     tt: ast::TokenTree,
     token: SyntaxToken,
     file_id: FileId,
-) -> Option<NavigationTarget> {
+) -> Option<Vec<NavigationTarget>> {
     let path = ast::String::cast(token)?.value()?.into_owned();
     let macro_call = tt.syntax().parent().and_then(ast::MacroCall::cast)?;
     let name = macro_call.path()?.segment()?.name_ref()?;
@@ -92,7 +100,7 @@ fn try_lookup_include_path(
     }
     let file_id = db.resolve_path(AnchoredPath { anchor: file_id, path: &path })?;
     let size = db.file_text(file_id).len().try_into().ok()?;
-    Some(NavigationTarget {
+    Some(vec![NavigationTarget {
         file_id,
         full_range: TextRange::new(0.into(), size),
         name: path.into(),
@@ -101,7 +109,7 @@ fn try_lookup_include_path(
         container_name: None,
         description: None,
         docs: None,
-    })
+    }])
 }
 
 /// finds the trait definition of an impl'd item
@@ -111,7 +119,10 @@ fn try_lookup_include_path(
 /// struct S;
 /// impl A for S { fn a(); } // <-- on this function, will get the location of a() in the trait
 /// ```
-fn try_find_trait_item_definition(db: &RootDatabase, def: &Definition) -> Option<NavigationTarget> {
+fn try_find_trait_item_definition(
+    db: &RootDatabase,
+    def: &Definition,
+) -> Option<Vec<NavigationTarget>> {
     let name = def.name(db)?;
     let assoc = match def {
         Definition::ModuleDef(ModuleDef::Function(f)) => f.as_assoc_item(db),
@@ -130,37 +141,66 @@ fn try_find_trait_item_definition(db: &RootDatabase, def: &Definition) -> Option
         .items(db)
         .iter()
         .find_map(|itm| (itm.name(db)? == name).then(|| itm.try_to_nav(db)).flatten())
+        .map(|it| vec![it])
 }
 
 pub(crate) fn reference_definition(
     sema: &Semantics<RootDatabase>,
     name_ref: Either<&ast::Lifetime, &ast::NameRef>,
-) -> Option<NavigationTarget> {
-    let name_kind = name_ref.either(
+) -> Vec<NavigationTarget> {
+    let name_kind = match name_ref.either(
         |lifetime| NameRefClass::classify_lifetime(sema, lifetime),
         |name_ref| NameRefClass::classify(sema, name_ref),
-    )?;
-    let def = name_kind.referenced();
-    def.try_to_nav(sema.db)
+    ) {
+        Some(class) => class,
+        None => return Vec::new(),
+    };
+    match name_kind {
+        NameRefClass::Definition(def) => def_to_nav(sema.db, def),
+        NameRefClass::FieldShorthand { local_ref, field_ref } => {
+            local_and_field_to_nav(sema.db, local_ref, field_ref)
+        }
+    }
+}
+
+fn def_to_nav(db: &RootDatabase, def: Definition) -> Vec<NavigationTarget> {
+    def.try_to_nav(db).map(|it| vec![it]).unwrap_or_default()
+}
+
+fn local_and_field_to_nav(
+    db: &RootDatabase,
+    local: hir::Local,
+    field: hir::Field,
+) -> Vec<NavigationTarget> {
+    iter::once(local.to_nav(db)).chain(field.try_to_nav(db)).collect()
 }
 
 #[cfg(test)]
 mod tests {
     use ide_db::base_db::FileRange;
+    use itertools::Itertools;
 
     use crate::fixture;
 
     fn check(ra_fixture: &str) {
-        let (analysis, position, expected) = fixture::nav_target_annotation(ra_fixture);
-        let mut navs =
-            analysis.goto_definition(position).unwrap().expect("no definition found").info;
+        let (analysis, position, expected) = fixture::annotations(ra_fixture);
+        let navs = analysis.goto_definition(position).unwrap().expect("no definition found").info;
         if navs.len() == 0 {
             panic!("unresolved reference")
         }
-        assert_eq!(navs.len(), 1);
 
-        let nav = navs.pop().unwrap();
-        assert_eq!(expected, FileRange { file_id: nav.file_id, range: nav.focus_or_full_range() });
+        let cmp = |&FileRange { file_id, range }: &_| (file_id, range.start());
+        let navs = navs
+            .into_iter()
+            .map(|nav| FileRange { file_id: nav.file_id, range: nav.focus_or_full_range() })
+            .sorted_by_key(cmp)
+            .collect::<Vec<_>>();
+        let expected = expected
+            .into_iter()
+            .map(|(FileRange { file_id, range }, _)| FileRange { file_id, range })
+            .sorted_by_key(cmp)
+            .collect::<Vec<_>>();
+        assert_eq!(expected, navs);
     }
 
     fn check_unresolved(ra_fixture: &str) {
@@ -863,6 +903,7 @@ fn bar() {
         check(
             r#"
 struct Foo { x: i32 }
+           //^
 fn main() {
     let x = 92;
       //^
@@ -878,10 +919,12 @@ fn main() {
             r#"
 enum Foo {
     Bar { x: i32 }
-}       //^
+        //^
+}
 fn baz(foo: Foo) {
     match foo {
         Foo::Bar { x$0 } => x
+                 //^
     };
 }
 "#,
@@ -1126,13 +1169,15 @@ fn foo<'foobar>(_: &'foobar ()) {
     fn goto_lifetime_hrtb() {
         // FIXME: requires the HIR to somehow track these hrtb lifetimes
         check_unresolved(
-            r#"trait Foo<T> {}
+            r#"
+trait Foo<T> {}
 fn foo<T>() where for<'a> T: Foo<&'a$0 (u8, u16)>, {}
                     //^^
 "#,
         );
         check_unresolved(
-            r#"trait Foo<T> {}
+            r#"
+trait Foo<T> {}
 fn foo<T>() where for<'a$0> T: Foo<&'a (u8, u16)>, {}
                     //^^
 "#,

crates/ide/src/goto_implementation.rs

@@ -28,11 +28,19 @@ pub(crate) fn goto_implementation(
 
     let node = sema.find_node_at_offset_with_descend(&syntax, position.offset)?;
     let def = match &node {
-        ast::NameLike::Name(name) => {
-            NameClass::classify(&sema, name).map(|class| class.referenced_or_defined())
-        }
+        ast::NameLike::Name(name) => NameClass::classify(&sema, name).map(|class| match class {
+            NameClass::Definition(it) | NameClass::ConstReference(it) => it,
+            NameClass::PatFieldShorthand { local_def, field_ref: _ } => {
+                Definition::Local(local_def)
+            }
+        }),
         ast::NameLike::NameRef(name_ref) => {
-            NameRefClass::classify(&sema, name_ref).map(|class| class.referenced())
+            NameRefClass::classify(&sema, name_ref).map(|class| match class {
+                NameRefClass::Definition(def) => def,
+                NameRefClass::FieldShorthand { local_ref, field_ref: _ } => {
+                    Definition::Local(local_ref)
+                }
+            })
         }
         ast::NameLike::Lifetime(_) => None,
     }?;

crates/ide/src/hover.rs

@@ -96,18 +96,23 @@ pub(crate) fn hover(
         match node {
             // we don't use NameClass::referenced_or_defined here as we do not want to resolve
             // field pattern shorthands to their definition
-            ast::Name(name) => NameClass::classify(&sema, &name).and_then(|class| match class {
-                NameClass::ConstReference(def) => Some(def),
-                def => def.defined(),
+            ast::Name(name) => NameClass::classify(&sema, &name).map(|class| match class {
+                NameClass::Definition(it) | NameClass::ConstReference(it) => it,
+                NameClass::PatFieldShorthand { local_def, field_ref: _ } => Definition::Local(local_def),
+            }),
+            ast::NameRef(name_ref) => NameRefClass::classify(&sema, &name_ref).map(|class| match class {
+                NameRefClass::Definition(def) => def,
+                NameRefClass::FieldShorthand { local_ref: _, field_ref } => {
+                    Definition::Field(field_ref)
+                }
             }),
-            ast::NameRef(name_ref) => {
-                NameRefClass::classify(&sema, &name_ref).map(|d| d.referenced())
-            },
             ast::Lifetime(lifetime) => NameClass::classify_lifetime(&sema, &lifetime).map_or_else(
-                || NameRefClass::classify_lifetime(&sema, &lifetime).map(|d| d.referenced()),
+                || NameRefClass::classify_lifetime(&sema, &lifetime).and_then(|class| match class {
+                    NameRefClass::Definition(it) => Some(it),
+                    _ => None,
+                }),
                 |d| d.defined(),
             ),
-
             _ => {
                 if ast::Comment::cast(token.clone()).is_some() {
                     cov_mark::hit!(no_highlight_on_comment_hover);