Merge #10284

10284: internal: definition based hover functions r=Veykril a=HKalbasi

This is part of #10181 but since it is blocked and `hover.rs` is moving quickly so will cause conflicts, I submitted this PR.

This PR extract some parts of `hover` to `find_definition` (maybe this need to be moved to some other file?) and `hover_for_definition`, with those functions I will be able to calculate definition of every token, and calculate hover (and probably other queries) for each definition only once.

Co-authored-by: hamidreza kalbasi <hamidrezakalbasi@protonmail.com>

crates/ide/src/goto_definition.rs

@@ -1,21 +1,19 @@
-use std::{convert::TryInto, iter};
+use std::convert::TryInto;
-
-use either::Either;
-use hir::{AsAssocItem, InFile, ModuleDef, Semantics};
-use ide_db::{
-    base_db::{AnchoredPath, FileId, FileLoader},
-    defs::{Definition, NameClass, NameRefClass},
-    helpers::{pick_best_token, try_resolve_derive_input_at},
-    RootDatabase,
-};
-use itertools::Itertools;
-use syntax::{ast, match_ast, AstNode, AstToken, SyntaxKind::*, SyntaxToken, TextRange, T};
 
 use crate::{
-    display::{ToNav, TryToNav},
+    display::TryToNav,
     doc_links::{doc_attributes, extract_definitions_from_docs, resolve_doc_path_for_def},
     FilePosition, NavigationTarget, RangeInfo,
 };
+use hir::{AsAssocItem, InFile, ModuleDef, Semantics};
+use ide_db::{
+    base_db::{AnchoredPath, FileId, FileLoader},
+    defs::Definition,
+    helpers::pick_best_token,
+    RootDatabase,
+};
+use itertools::Itertools;
+use syntax::{ast, AstNode, AstToken, SyntaxKind::*, SyntaxToken, TextRange, T};
 
 // Feature: Go to Definition
 //
@@ -58,39 +56,22 @@ pub(crate) fn goto_definition(
         .into_iter()
         .filter_map(|token| {
             let parent = token.parent()?;
-            let navs = match_ast! {
+            if let Some(tt) = ast::TokenTree::cast(parent.clone()) {
-                match parent {
+                if let x @ Some(_) =
-                    ast::NameRef(name_ref) => {
+                    try_lookup_include_path(&sema, tt, token.clone(), position.file_id)
-                        reference_definition(&sema, Either::Right(&name_ref))
+                {
-                    },
+                    return x;
-                    ast::Name(name) => {
-                        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) => {
-                        match NameClass::classify_lifetime(&sema, &lt) {
-                            Some(name_class) => {
-                                match name_class {
-                                    NameClass::Definition(def) => def_to_nav(sema.db, def),
-                                    _ => return None,
-                                }
-                            }
-                            None => reference_definition(&sema, Either::Left(&lt)),
-                        }
-                    },
-                    ast::TokenTree(tt) =>
-                        try_lookup_include_path_or_derive(&sema, tt, token, position.file_id)?,
-                    _ => return None,
                 }
-            };
+            }
-            Some(navs)
+            Some(
+                Definition::from_node(&sema, &token)
+                    .into_iter()
+                    .flat_map(|def| {
+                        try_find_trait_item_definition(sema.db, &def)
+                            .unwrap_or_else(|| def_to_nav(sema.db, def))
+                    })
+                    .collect::<Vec<_>>(),
+            )
         })
         .flatten()
         .unique()
@@ -99,41 +80,31 @@ pub(crate) fn goto_definition(
     Some(RangeInfo::new(original_token.text_range(), navs))
 }
 
-fn try_lookup_include_path_or_derive(
+fn try_lookup_include_path(
     sema: &Semantics<RootDatabase>,
     tt: ast::TokenTree,
     token: SyntaxToken,
     file_id: FileId,
 ) -> Option<Vec<NavigationTarget>> {
-    match ast::String::cast(token.clone()) {
+    let token = ast::String::cast(token.clone())?;
-        Some(token) => {
+    let path = token.value()?.into_owned();
-            let path = token.value()?.into_owned();
+    let macro_call = tt.syntax().parent().and_then(ast::MacroCall::cast)?;
-            let macro_call = tt.syntax().parent().and_then(ast::MacroCall::cast)?;
+    let name = macro_call.path()?.segment()?.name_ref()?;
-            let name = macro_call.path()?.segment()?.name_ref()?;
+    if !matches!(&*name.text(), "include" | "include_str" | "include_bytes") {
-            if !matches!(&*name.text(), "include" | "include_str" | "include_bytes") {
+        return None;
-                return None;
-            }
-            let file_id = sema.db.resolve_path(AnchoredPath { anchor: file_id, path: &path })?;
-            let size = sema.db.file_text(file_id).len().try_into().ok()?;
-            Some(vec![NavigationTarget {
-                file_id,
-                full_range: TextRange::new(0.into(), size),
-                name: path.into(),
-                focus_range: None,
-                kind: None,
-                container_name: None,
-                description: None,
-                docs: None,
-            }])
-        }
-        None => try_resolve_derive_input_at(
-            sema,
-            &tt.syntax().ancestors().nth(2).and_then(ast::Attr::cast)?,
-            &token,
-        )
-        .and_then(|it| it.try_to_nav(sema.db))
-        .map(|it| vec![it]),
     }
+    let file_id = sema.db.resolve_path(AnchoredPath { anchor: file_id, path: &path })?;
+    let size = sema.db.file_text(file_id).len().try_into().ok()?;
+    Some(vec![NavigationTarget {
+        file_id,
+        full_range: TextRange::new(0.into(), size),
+        name: path.into(),
+        focus_range: None,
+        kind: None,
+        container_name: None,
+        description: None,
+        docs: None,
+    }])
 }
 
 /// finds the trait definition of an impl'd item
@@ -168,37 +139,10 @@ fn try_find_trait_item_definition(
         .map(|it| vec![it])
 }
 
-pub(crate) fn reference_definition(
-    sema: &Semantics<RootDatabase>,
-    name_ref: Either<&ast::Lifetime, &ast::NameRef>,
-) -> Vec<NavigationTarget> {
-    let name_kind = match name_ref.either(
-        |lifetime| NameRefClass::classify_lifetime(sema, lifetime),
-        |name_ref| NameRefClass::classify(sema, name_ref),
-    ) {
-        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;

crates/ide/src/hover.rs

@@ -4,10 +4,10 @@ use either::Either;
 use hir::{AsAssocItem, HasAttrs, HasSource, HirDisplay, Semantics, TypeInfo};
 use ide_db::{
     base_db::{FileRange, SourceDatabase},
-    defs::{Definition, NameClass, NameRefClass},
+    defs::Definition,
     helpers::{
         generated_lints::{CLIPPY_LINTS, DEFAULT_LINTS, FEATURES},
-        pick_best_token, try_resolve_derive_input_at, FamousDefs,
+        pick_best_token, FamousDefs,
     },
     RootDatabase,
 };
@@ -107,7 +107,7 @@ pub(crate) fn hover(
     }
     let offset = range.start();
 
-    let token = pick_best_token(file.token_at_offset(offset), |kind| match kind {
+    let original_token = pick_best_token(file.token_at_offset(offset), |kind| match kind {
         IDENT | INT_NUMBER | LIFETIME_IDENT | T![self] | T![super] | T![crate] => 3,
         T!['('] | T![')'] => 2,
         kind if kind.is_trivia() => 0,
@@ -117,11 +117,35 @@ pub(crate) fn hover(
     let mut seen = HashSet::default();
 
     let mut fallback = None;
-    sema.descend_into_macros_many(token.clone())
+    // attributes, require special machinery as they are mere ident tokens
+
+    let descend_macros = sema.descend_into_macros_many(original_token.clone());
+
+    for token in &descend_macros {
+        if token.kind() != COMMENT {
+            if let Some(attr) = token.ancestors().find_map(ast::Attr::cast) {
+                // lints
+                if let Some(res) = try_hover_for_lint(&attr, &token) {
+                    return Some(res);
+                }
+            }
+        }
+    }
+
+    descend_macros
         .iter()
         .filter_map(|token| match token.parent() {
             Some(node) => {
-                match find_hover_result(&sema, file_id, offset, config, token, &node, &mut seen) {
+                match find_hover_result(
+                    &sema,
+                    file_id,
+                    offset,
+                    config,
+                    &original_token,
+                    token,
+                    &node,
+                    &mut seen,
+                ) {
                     Some(res) => match res {
                         ControlFlow::Break(inner) => Some(inner),
                         ControlFlow::Continue(_) => {
@@ -159,6 +183,7 @@ fn find_hover_result(
     file_id: FileId,
     offset: TextSize,
     config: &HoverConfig,
+    original_token: &SyntaxToken,
     token: &SyntaxToken,
     node: &SyntaxNode,
     seen: &mut HashSet<Definition>,
@@ -169,71 +194,39 @@ fn find_hover_result(
     // so don't add them to the `seen` duplicate check
     let mut add_to_seen_definitions = true;
 
-    let definition = match_ast! {
+    let definition = Definition::from_node(sema, token).into_iter().next().or_else(|| {
-        match node {
+        // intra-doc links
-            ast::Name(name) => NameClass::classify(sema, &name).map(|class| match class {
+        // FIXME: move comment + attribute special cases somewhere else to simplify control flow,
-                NameClass::Definition(it) | NameClass::ConstReference(it) => it,
+        // hopefully simplifying the return type of this function in the process
-                NameClass::PatFieldShorthand { local_def, field_ref: _ } => Definition::Local(local_def),
+        // (the `Break`/`Continue` distinction is needed to decide whether to use fallback hovers)
-            }),
+        //
-            ast::NameRef(name_ref) => NameRefClass::classify(sema, &name_ref).map(|class| match class {
+        // FIXME: hovering the intra doc link to `Foo` not working:
-                NameRefClass::Definition(def) => def,
+        //
-                NameRefClass::FieldShorthand { local_ref: _, field_ref } => {
+        // #[identity]
-                    Definition::Field(field_ref)
+        // trait Foo {
-                }
+        //    /// [`Foo`]
-            }),
+        // fn foo() {}
-            ast::Lifetime(lifetime) => NameClass::classify_lifetime(&sema, &lifetime).map_or_else(
+        if token.kind() == COMMENT {
-                || {
+            add_to_seen_definitions = false;
-                    NameRefClass::classify_lifetime(&sema, &lifetime).and_then(|class| match class {
+            cov_mark::hit!(no_highlight_on_comment_hover);
-                        NameRefClass::Definition(it) => Some(it),
+            let (attributes, def) = doc_attributes(sema, node)?;
-                        _ => None,
+            let (docs, doc_mapping) = attributes.docs_with_rangemap(sema.db)?;
-                    })
+            let (idl_range, link, ns) = extract_definitions_from_docs(&docs).into_iter().find_map(
+                |(range, link, ns)| {
+                    let mapped = doc_mapping.map(range)?;
+                    (mapped.file_id == file_id.into() && mapped.value.contains(offset))
+                        .then(|| (mapped.value, link, ns))
                 },
-                NameClass::defined,
+            )?;
-            ),
+            range_override = Some(idl_range);
-            _ => {
+            Some(match resolve_doc_path_for_def(sema.db, def, &link, ns)? {
-                // intra-doc links
+                Either::Left(it) => Definition::ModuleDef(it),
-                // FIXME: move comment + attribute special cases somewhere else to simplify control flow,
+                Either::Right(it) => Definition::Macro(it),
-                // hopefully simplifying the return type of this function in the process
+            })
-                // (the `Break`/`Continue` distinction is needed to decide whether to use fallback hovers)
+        } else {
-                //
+            None
-                // FIXME: hovering the intra doc link to `Foo` not working:
-                //
-                // #[identity]
-                // trait Foo {
-                //    /// [`Foo`]
-                // fn foo() {}
-                if token.kind() == COMMENT {
-                    add_to_seen_definitions = false;
-                    cov_mark::hit!(no_highlight_on_comment_hover);
-                    let (attributes, def) = doc_attributes(sema, node)?;
-                    let (docs, doc_mapping) = attributes.docs_with_rangemap(sema.db)?;
-                    let (idl_range, link, ns) =
-                        extract_definitions_from_docs(&docs).into_iter().find_map(|(range, link, ns)| {
-                            let mapped = doc_mapping.map(range)?;
-                            (mapped.file_id == file_id.into() && mapped.value.contains(offset)).then(||(mapped.value, link, ns))
-                        })?;
-                    range_override = Some(idl_range);
-                    Some(match resolve_doc_path_for_def(sema.db,def, &link,ns)? {
-                        Either::Left(it) => Definition::ModuleDef(it),
-                        Either::Right(it) => Definition::Macro(it),
-                    })
-                // attributes, require special machinery as they are mere ident tokens
-                } else if let Some(attr) = token.ancestors().find_map(ast::Attr::cast) {
-                    add_to_seen_definitions = false;
-                    // lints
-                    if let Some(res) = try_hover_for_lint(&attr, &token) {
-                        return Some(ControlFlow::Break(res));
-                    // derives
-                    } else {
-                        range_override = Some(token.text_range());
-                        try_resolve_derive_input_at(&sema, &attr, &token).map(Definition::Macro)
-                    }
-                } else {
-                    None
-                }
-            },
         }
-    };
+    });
 
     if let Some(definition) = definition {
         // skip duplicates
@@ -243,34 +236,8 @@ fn find_hover_result(
         if add_to_seen_definitions {
             seen.insert(definition);
         }
-        let famous_defs = match &definition {
+        if let Some(res) = hover_for_definition(sema, file_id, definition, &node, config) {
-            Definition::ModuleDef(hir::ModuleDef::BuiltinType(_)) => {
+            let range = range_override.unwrap_or_else(|| original_token.text_range());
-                Some(FamousDefs(&sema, sema.scope(&node).krate()))
-            }
-            _ => None,
-        };
-        if let Some(markup) =
-            hover_for_definition(sema.db, definition, famous_defs.as_ref(), config)
-        {
-            let mut res = HoverResult::default();
-            res.markup = process_markup(sema.db, definition, &markup, config);
-            if let Some(action) = show_implementations_action(sema.db, definition) {
-                res.actions.push(action);
-            }
-
-            if let Some(action) = show_fn_references_action(sema.db, definition) {
-                res.actions.push(action);
-            }
-
-            if let Some(action) = runnable_action(&sema, definition, file_id) {
-                res.actions.push(action);
-            }
-
-            if let Some(action) = goto_type_action_for_def(sema.db, definition) {
-                res.actions.push(action);
-            }
-
-            let range = range_override.unwrap_or_else(|| sema.original_range(&node).range);
             return Some(ControlFlow::Break(RangeInfo::new(range, res)));
         }
     }
@@ -283,6 +250,9 @@ fn type_hover(
     config: &HoverConfig,
     token: &SyntaxToken,
 ) -> Option<RangeInfo<HoverResult>> {
+    if token.kind() == COMMENT {
+        return None;
+    }
     let node = token
         .ancestors()
         .take_while(|it| !ast::Item::can_cast(it.kind()))
@@ -749,7 +719,43 @@ fn definition_mod_path(db: &RootDatabase, def: &Definition) -> Option<String> {
     def.module(db).map(|module| render_path(db, module, definition_owner_name(db, def)))
 }
 
-fn hover_for_definition(
+pub(crate) fn hover_for_definition(
+    sema: &Semantics<RootDatabase>,
+    file_id: FileId,
+    definition: Definition,
+    node: &SyntaxNode,
+    config: &HoverConfig,
+) -> Option<HoverResult> {
+    let famous_defs = match &definition {
+        Definition::ModuleDef(hir::ModuleDef::BuiltinType(_)) => {
+            Some(FamousDefs(&sema, sema.scope(&node).krate()))
+        }
+        _ => None,
+    };
+    if let Some(markup) = markup_for_definition(sema.db, definition, famous_defs.as_ref(), config) {
+        let mut res = HoverResult::default();
+        res.markup = process_markup(sema.db, definition, &markup, config);
+        if let Some(action) = show_implementations_action(sema.db, definition) {
+            res.actions.push(action);
+        }
+
+        if let Some(action) = show_fn_references_action(sema.db, definition) {
+            res.actions.push(action);
+        }
+
+        if let Some(action) = runnable_action(&sema, definition, file_id) {
+            res.actions.push(action);
+        }
+
+        if let Some(action) = goto_type_action_for_def(sema.db, definition) {
+            res.actions.push(action);
+        }
+        return Some(res);
+    }
+    None
+}
+
+fn markup_for_definition(
     db: &RootDatabase,
     def: Definition,
     famous_defs: Option<&FamousDefs>,
@@ -885,7 +891,7 @@ mod tests {
                 FileRange { file_id: position.file_id, range: TextRange::empty(position.offset) },
             )
             .unwrap();
-        assert!(hover.is_none());
+        assert!(hover.is_none(), "hover not expected but found: {:?}", hover.unwrap());
     }
 
     fn check(ra_fixture: &str, expect: Expect) {
@@ -4529,6 +4535,36 @@ use crate as foo$0;
         );
     }
 
+    #[test]
+    fn hover_attribute_in_macro() {
+        check(
+            r#"
+macro_rules! identity {
+    ($struct:item) => {
+        $struct
+    };
+}
+#[rustc_builtin_macro]
+pub macro Copy {}
+identity!{
+    #[derive(Copy$0)]
+    struct Foo;
+}
+"#,
+            expect![[r#"
+                *Copy*
+
+                ```rust
+                test
+                ```
+
+                ```rust
+                pub macro Copy
+                ```
+            "#]],
+        );
+    }
+
     #[test]
     fn hover_derive_input() {
         check(

crates/ide_db/src/defs.rs

@@ -11,10 +11,10 @@ use hir::{
 };
 use syntax::{
     ast::{self, AstNode},
-    match_ast, SyntaxKind,
+    match_ast, SyntaxKind, SyntaxToken,
 };
 
-use crate::RootDatabase;
+use crate::{helpers::try_resolve_derive_input_at, RootDatabase};
 
 // FIXME: a more precise name would probably be `Symbol`?
 #[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)]
@@ -29,6 +29,62 @@ pub enum Definition {
 }
 
 impl Definition {
+    pub fn from_node(sema: &Semantics<RootDatabase>, token: &SyntaxToken) -> Vec<Definition> {
+        let node = if let Some(x) = token.parent() {
+            x
+        } else {
+            return vec![];
+        };
+        if token.kind() != SyntaxKind::COMMENT {
+            if let Some(attr) = token.ancestors().find_map(ast::Attr::cast) {
+                // derives
+                let def = try_resolve_derive_input_at(&sema, &attr, &token).map(Definition::Macro);
+                if let Some(def) = def {
+                    return vec![def];
+                }
+            }
+        }
+        match_ast! {
+            match node {
+                ast::Name(name) => {
+                    let class = if let Some(x) = NameClass::classify(&sema, &name) {
+                        x
+                    } else {
+                        return vec![];
+                    };
+                    match class {
+                        NameClass::Definition(it) | NameClass::ConstReference(it) => vec![it],
+                        NameClass::PatFieldShorthand { local_def, field_ref } => vec![Definition::Local(local_def), Definition::Field(field_ref)],
+                    }
+                },
+                ast::NameRef(name_ref) => {
+                    let class = if let Some(x) = NameRefClass::classify(sema, &name_ref) {
+                        x
+                    } else {
+                        return vec![];
+                    };
+                    match class {
+                        NameRefClass::Definition(def) => vec![def],
+                        NameRefClass::FieldShorthand { local_ref, field_ref } => {
+                            vec![Definition::Field(field_ref), Definition::Local(local_ref)]
+                        }
+                    }
+                },
+                ast::Lifetime(lifetime) => {
+                    (if let Some(x) = NameClass::classify_lifetime(&sema, &lifetime) {
+                        NameClass::defined(x)
+                    } else {
+                        NameRefClass::classify_lifetime(&sema, &lifetime).and_then(|class| match class {
+                            NameRefClass::Definition(it) => Some(it),
+                            _ => None,
+                        })
+                    }).into_iter().collect()
+                },
+                _ => vec![],
+            }
+        }
+    }
+
     pub fn module(&self, db: &RootDatabase) -> Option<Module> {
         match self {
             Definition::Macro(it) => it.module(db),