Merge #10789

10789: internal: Check for derive attributes by item path, not `derive` identifier r=Veykril a=Veykril

Prior we only checked if an attribute is the exact `derive` identifier and nothing else to collect derive attributes. That means when we encounter the following:
```rs
#[::core::macros::builtin::derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct ModPath {
    pub kind: PathKind,
    segments: Vec<Name>,
}
```
We won't actually expand the derive attributes, but instead we just expand the `derive` attribute with our dummy identity expander.

The changes here make it so we actually lookup the attribute path, check if it is the derive attribute and then collect the derives.


Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

crates/hir_def/src/attr.rs

@@ -8,7 +8,7 @@ use either::Either;
 use hir_expand::{hygiene::Hygiene, name::AsName, AstId, InFile};
 use itertools::Itertools;
 use la_arena::ArenaMap;
-use mbe::{syntax_node_to_token_tree, DelimiterKind};
+use mbe::{syntax_node_to_token_tree, DelimiterKind, Punct};
 use smallvec::{smallvec, SmallVec};
 use syntax::{
     ast::{self, AstNode, HasAttrs, IsString},
@@ -722,25 +722,20 @@ impl Attr {
     /// Parses this attribute as a `#[derive]`, returns an iterator that yields all contained paths
     /// to derive macros.
     ///
-    /// Returns `None` when the attribute is not a well-formed `#[derive]` attribute.
+    /// Returns `None` when the attribute does not have a well-formed `#[derive]` attribute input.
     pub(crate) fn parse_derive(&self) -> Option<impl Iterator<Item = ModPath>> {
-        if self.path.as_ident() != Some(&hir_expand::name![derive]) {
+        if let Some(AttrInput::TokenTree(args, _)) = self.input.as_deref() {
+            if args.delimiter_kind() != Some(DelimiterKind::Parenthesis) {
                 return None;
             }
-
-        match self.input.as_deref() {
-            Some(AttrInput::TokenTree(args, _)) => {
             let mut counter = 0;
             let paths = args
                 .token_trees
                 .iter()
                 .group_by(move |tt| {
-                        match tt {
+                    if let tt::TokenTree::Leaf(tt::Leaf::Punct(Punct { char: ',', .. })) = tt {
-                            tt::TokenTree::Leaf(tt::Leaf::Punct(p)) if p.char == ',' => {
                         counter += 1;
                     }
-                            _ => {}
-                        }
                     counter
                 })
                 .into_iter()
@@ -753,10 +748,9 @@ impl Attr {
                 })
                 .collect::<Vec<_>>();
 
-                Some(paths.into_iter())
+            return Some(paths.into_iter());
-            }
-            _ => None,
         }
+        None
     }
 
     pub fn string_value(&self) -> Option<&SmolStr> {

crates/hir_def/src/lib.rs

@@ -776,13 +776,10 @@ fn attr_macro_as_call_id(
     macro_attr: &Attr,
     db: &dyn db::DefDatabase,
     krate: CrateId,
-    resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
+    def: Option<MacroDefId>,
 ) -> Result<MacroCallId, UnresolvedMacro> {
     let attr_path = &item_attr.path;
-
+    let def = def.ok_or_else(|| UnresolvedMacro { path: attr_path.clone() })?;
-    let def = resolver(attr_path.clone())
-        .filter(MacroDefId::is_attribute)
-        .ok_or_else(|| UnresolvedMacro { path: attr_path.clone() })?;
     let last_segment =
         attr_path.segments().last().ok_or_else(|| UnresolvedMacro { path: attr_path.clone() })?;
     let mut arg = match macro_attr.input.as_deref() {

crates/hir_def/src/macro_expansion_tests/builtin_derive_macro.rs

@@ -26,12 +26,16 @@ fn test_copy_expand_in_core() {
     check(
         r#"
 #[rustc_builtin_macro]
+macro derive {}
+#[rustc_builtin_macro]
 macro Copy {}
 #[derive(Copy)]
 struct Foo;
 "#,
         expect![[r##"
 #[rustc_builtin_macro]
+macro derive {}
+#[rustc_builtin_macro]
 macro Copy {}
 #[derive(Copy)]
 struct Foo;

crates/hir_def/src/macro_expansion_tests/proc_macros.rs

@@ -31,6 +31,7 @@ fn derive_censoring() {
     check(
         r#"
 //- proc_macros: derive_identity
+//- minicore:derive
 #[attr1]
 #[derive(Foo)]
 #[derive(proc_macros::DeriveIdentity)]

crates/hir_def/src/nameres/collector.rs

@@ -9,7 +9,7 @@ use base_db::{CrateId, Edition, FileId, ProcMacroId};
 use cfg::{CfgExpr, CfgOptions};
 use hir_expand::{
     ast_id_map::FileAstId,
-    builtin_attr_macro::{find_builtin_attr, is_builtin_test_or_bench_attr},
+    builtin_attr_macro::find_builtin_attr,
     builtin_derive_macro::find_builtin_derive,
     builtin_fn_macro::find_builtin_macro,
     name::{name, AsName, Name},
@@ -781,7 +781,7 @@ impl DefCollector<'_> {
     }
 
     fn resolve_extern_crate(&self, name: &Name) -> PerNs {
-        if name == &name!(self) {
+        if *name == name!(self) {
             cov_mark::hit!(extern_crate_self_as);
             let root = match self.def_map.block {
                 Some(_) => {
@@ -1054,14 +1054,15 @@ impl DefCollector<'_> {
 
             match &directive.kind {
                 MacroDirectiveKind::FnLike { ast_id, expand_to } => {
-                    match macro_call_as_call_id(
+                    let call_id = macro_call_as_call_id(
                         ast_id,
                         *expand_to,
                         self.db,
                         self.def_map.krate,
                         &resolver,
                         &mut |_err| (),
-                    ) {
+                    );
+                    match call_id {
                         Ok(Ok(call_id)) => {
                             resolved.push((directive.module_id, call_id, directive.depth));
                             res = ReachedFixedPoint::No;
@@ -1071,13 +1072,14 @@ impl DefCollector<'_> {
                     }
                 }
                 MacroDirectiveKind::Derive { ast_id, derive_attr } => {
-                    match derive_macro_as_call_id(
+                    let call_id = derive_macro_as_call_id(
                         ast_id,
                         *derive_attr,
                         self.db,
                         self.def_map.krate,
                         &resolver,
-                    ) {
+                    );
+                    match call_id {
                         Ok(call_id) => {
                             self.def_map.modules[directive.module_id].scope.add_derive_macro_invoc(
                                 ast_id.ast_id,
@@ -1089,73 +1091,103 @@ impl DefCollector<'_> {
                             res = ReachedFixedPoint::No;
                             return false;
                         }
-                        Err(UnresolvedMacro { .. }) => (),
+                        Err(UnresolvedMacro { .. }) => {}
                     }
                 }
                 MacroDirectiveKind::Attr { ast_id, mod_item, attr } => {
+                    let file_id = ast_id.ast_id.file_id;
+                    let mut recollect_without = |collector: &mut Self, item_tree| {
+                        // Remove the original directive since we resolved it.
+                        let mod_dir = collector.mod_dirs[&directive.module_id].clone();
+                        collector.skip_attrs.insert(InFile::new(file_id, *mod_item), attr.id);
+                        ModCollector {
+                            def_collector: collector,
+                            macro_depth: directive.depth,
+                            module_id: directive.module_id,
+                            tree_id: TreeId::new(file_id, None),
+                            item_tree,
+                            mod_dir,
+                        }
+                        .collect(&[*mod_item]);
+                        res = ReachedFixedPoint::No;
+                        false
+                    };
+
                     if let Some(ident) = ast_id.path.as_ident() {
                         if let Some(helpers) = self.derive_helpers_in_scope.get(&ast_id.ast_id) {
                             if helpers.contains(ident) {
                                 cov_mark::hit!(resolved_derive_helper);
-
                                 // Resolved to derive helper. Collect the item's attributes again,
                                 // starting after the derive helper.
+                                let item_tree = self.db.file_item_tree(file_id);
+                                return recollect_without(self, &item_tree);
+                            }
+                        }
+                    }
+
+                    let def = resolver(ast_id.path.clone()).filter(MacroDefId::is_attribute);
+                    if matches!(
+                        def,
+                        Some(MacroDefId {  kind:MacroDefKind::BuiltInAttr(expander, _),.. })
+                        if expander.is_derive()
+                    ) {
+                        // Resolved to `#[derive]`
                         let file_id = ast_id.ast_id.file_id;
                         let item_tree = self.db.file_item_tree(file_id);
-                                let mod_dir = self.mod_dirs[&directive.module_id].clone();
-                                self.skip_attrs.insert(InFile::new(file_id, *mod_item), attr.id);
-                                ModCollector {
-                                    def_collector: &mut *self,
-                                    macro_depth: directive.depth,
-                                    module_id: directive.module_id,
-                                    tree_id: TreeId::new(file_id, None),
-                                    item_tree: &item_tree,
-                                    mod_dir,
-                                }
-                                .collect(&[*mod_item]);
 
-                                // Remove the original directive since we resolved it.
+                        let ast_id: FileAstId<ast::Item> = match *mod_item {
+                            ModItem::Struct(it) => item_tree[it].ast_id.upcast(),
+                            ModItem::Union(it) => item_tree[it].ast_id.upcast(),
+                            ModItem::Enum(it) => item_tree[it].ast_id.upcast(),
+                            _ => {
+                                // Cannot use derive on this item.
+                                // FIXME: diagnose
                                 res = ReachedFixedPoint::No;
                                 return false;
                             }
+                        };
+
+                        match attr.parse_derive() {
+                            Some(derive_macros) => {
+                                for path in derive_macros {
+                                    let ast_id = AstIdWithPath::new(file_id, ast_id, path);
+                                    self.unresolved_macros.push(MacroDirective {
+                                        module_id: directive.module_id,
+                                        depth: directive.depth + 1,
+                                        kind: MacroDirectiveKind::Derive {
+                                            ast_id,
+                                            derive_attr: attr.id,
+                                        },
+                                    });
                                 }
                             }
+                            None => {
+                                // FIXME: diagnose
+                                tracing::debug!("malformed derive: {:?}", attr);
+                            }
+                        }
+
+                        return recollect_without(self, &item_tree);
+                    }
 
                     if !self.db.enable_proc_attr_macros() {
                         return true;
                     }
 
-                    // Not resolved to a derive helper, so try to resolve as a macro.
+                    // Not resolved to a derive helper or the derive attribute, so try to resolve as a normal attribute.
-                    match attr_macro_as_call_id(
+                    match attr_macro_as_call_id(ast_id, attr, self.db, self.def_map.krate, def) {
-                        ast_id,
-                        attr,
-                        self.db,
-                        self.def_map.krate,
-                        &resolver,
-                    ) {
                         Ok(call_id) => {
                             let loc: MacroCallLoc = self.db.lookup_intern_macro_call(call_id);
 
                             // Skip #[test]/#[bench] expansion, which would merely result in more memory usage
                             // due to duplicating functions into macro expansions
-                            if is_builtin_test_or_bench_attr(loc.def) {
+                            if matches!(
-                                let file_id = ast_id.ast_id.file_id;
+                                loc.def.kind,
+                                MacroDefKind::BuiltInAttr(expander, _)
+                                if expander.is_test() || expander.is_bench()
+                            ) {
                                 let item_tree = self.db.file_item_tree(file_id);
-                                let mod_dir = self.mod_dirs[&directive.module_id].clone();
+                                return recollect_without(self, &item_tree);
-                                self.skip_attrs.insert(InFile::new(file_id, *mod_item), attr.id);
-                                ModCollector {
-                                    def_collector: &mut *self,
-                                    macro_depth: directive.depth,
-                                    module_id: directive.module_id,
-                                    tree_id: TreeId::new(file_id, None),
-                                    item_tree: &item_tree,
-                                    mod_dir,
-                                }
-                                .collect(&[*mod_item]);
-
-                                // Remove the original directive since we resolved it.
-                                res = ReachedFixedPoint::No;
-                                return false;
                             }
 
                             if let MacroDefKind::ProcMacro(exp, ..) = loc.def.kind {
@@ -1171,21 +1203,7 @@ impl DefCollector<'_> {
 
                                     let file_id = ast_id.ast_id.file_id;
                                     let item_tree = self.db.file_item_tree(file_id);
-                                    let mod_dir = self.mod_dirs[&directive.module_id].clone();
+                                    return recollect_without(self, &item_tree);
-                                    self.skip_attrs
-                                        .insert(InFile::new(file_id, *mod_item), attr.id);
-                                    ModCollector {
-                                        def_collector: &mut *self,
-                                        macro_depth: directive.depth,
-                                        module_id: directive.module_id,
-                                        tree_id: TreeId::new(file_id, None),
-                                        item_tree: &item_tree,
-                                        mod_dir,
-                                    }
-                                    .collect(&[*mod_item]);
-
-                                    // Remove the macro directive.
-                                    return false;
                                 }
                             }
 
@@ -1281,7 +1299,7 @@ impl DefCollector<'_> {
         for directive in &self.unresolved_macros {
             match &directive.kind {
                 MacroDirectiveKind::FnLike { ast_id, expand_to } => {
-                    match macro_call_as_call_id(
+                    let macro_call_as_call_id = macro_call_as_call_id(
                         ast_id,
                         *expand_to,
                         self.db,
@@ -1297,9 +1315,8 @@ impl DefCollector<'_> {
                             resolved_res.resolved_def.take_macros()
                         },
                         &mut |_| (),
-                    ) {
+                    );
-                        Ok(_) => (),
+                    if let Err(UnresolvedMacro { path }) = macro_call_as_call_id {
-                        Err(UnresolvedMacro { path }) => {
                         self.def_map.diagnostics.push(DefDiagnostic::unresolved_macro_call(
                             directive.module_id,
                             ast_id.ast_id,
@@ -1307,7 +1324,6 @@ impl DefCollector<'_> {
                         ));
                     }
                 }
-                }
                 MacroDirectiveKind::Derive { .. } | MacroDirectiveKind::Attr { .. } => {
                     // FIXME: we might want to diagnose this too
                 }
@@ -1747,11 +1763,9 @@ impl ModCollector<'_, '_> {
             });
 
         for attr in iter {
-            if attr.path.as_ident() == Some(&hir_expand::name![derive]) {
+            if self.is_builtin_or_registered_attr(&attr.path) {
-                self.collect_derive(attr, mod_item);
-            } else if self.is_builtin_or_registered_attr(&attr.path) {
                 continue;
-            } else {
+            }
             tracing::debug!("non-builtin attribute {}", attr.path);
 
             let ast_id = AstIdWithPath::new(
@@ -1767,7 +1781,6 @@ impl ModCollector<'_, '_> {
 
             return Err(());
         }
-        }
 
         Ok(())
     }
@@ -1800,36 +1813,6 @@ impl ModCollector<'_, '_> {
         false
     }
 
-    fn collect_derive(&mut self, attr: &Attr, mod_item: ModItem) {
-        let ast_id: FileAstId<ast::Item> = match mod_item {
-            ModItem::Struct(it) => self.item_tree[it].ast_id.upcast(),
-            ModItem::Union(it) => self.item_tree[it].ast_id.upcast(),
-            ModItem::Enum(it) => self.item_tree[it].ast_id.upcast(),
-            _ => {
-                // Cannot use derive on this item.
-                // FIXME: diagnose
-                return;
-            }
-        };
-
-        match attr.parse_derive() {
-            Some(derive_macros) => {
-                for path in derive_macros {
-                    let ast_id = AstIdWithPath::new(self.file_id(), ast_id, path);
-                    self.def_collector.unresolved_macros.push(MacroDirective {
-                        module_id: self.module_id,
-                        depth: self.macro_depth + 1,
-                        kind: MacroDirectiveKind::Derive { ast_id, derive_attr: attr.id },
-                    });
-                }
-            }
-            None => {
-                // FIXME: diagnose
-                tracing::debug!("malformed derive: {:?}", attr);
-            }
-        }
-    }
-
     /// If `attrs` registers a procedural macro, collects its definition.
     fn collect_proc_macro_def(&mut self, func_name: &Name, ast_id: AstId<ast::Fn>, attrs: &Attrs) {
         // FIXME: this should only be done in the root module of `proc-macro` crates, not everywhere

crates/hir_def/src/nameres/tests/macros.rs

@@ -672,13 +672,14 @@ fn expand_derive() {
 //- /main.rs crate:main deps:core
 use core::Copy;
 
-        #[derive(Copy, core::Clone)]
+#[core::derive(Copy, core::Clone)]
 struct Foo;
 
 //- /core.rs crate:core
 #[rustc_builtin_macro]
+pub macro derive($item:item) {}
+#[rustc_builtin_macro]
 pub macro Copy {}
-
 #[rustc_builtin_macro]
 pub macro Clone {}
 "#,
@@ -713,7 +714,7 @@ fn builtin_derive_with_unresolved_attributes_fall_back() {
     let map = compute_crate_def_map(
         r#"
 //- /main.rs crate:main deps:core
-        use core::Clone;
+use core::{Clone, derive};
 
 #[derive(Clone)]
 #[unresolved]
@@ -721,6 +722,8 @@ fn builtin_derive_with_unresolved_attributes_fall_back() {
 
 //- /core.rs crate:core
 #[rustc_builtin_macro]
+pub macro derive($item:item) {}
+#[rustc_builtin_macro]
 pub macro Clone {}
 "#,
     );
@@ -799,6 +802,9 @@ fn resolves_derive_helper() {
     check(
         r#"
 //- /main.rs crate:main deps:proc
+#[rustc_builtin_macro]
+pub macro derive($item:item) {}
+
 #[derive(proc::Derive)]
 #[helper]
 #[unresolved]
@@ -811,6 +817,7 @@ fn derive() {}
         expect![[r#"
             crate
             S: t v
+            derive: m
         "#]],
     );
 }

crates/hir_expand/src/builtin_attr_macro.rs

@@ -36,6 +36,18 @@ macro_rules! register_builtin {
     };
 }
 
+impl BuiltinAttrExpander {
+    pub fn is_derive(self) -> bool {
+        matches!(self, BuiltinAttrExpander::Derive)
+    }
+    pub fn is_test(self) -> bool {
+        matches!(self, BuiltinAttrExpander::Test)
+    }
+    pub fn is_bench(self) -> bool {
+        matches!(self, BuiltinAttrExpander::Bench)
+    }
+}
+
 register_builtin! {
     (bench, Bench) => dummy_attr_expand,
     (cfg_accessible, CfgAccessible) => dummy_attr_expand,
@@ -46,16 +58,6 @@ register_builtin! {
     (test_case, TestCase) => dummy_attr_expand
 }
 
-pub fn is_builtin_test_or_bench_attr(makro: MacroDefId) -> bool {
-    match makro.kind {
-        MacroDefKind::BuiltInAttr(expander, ..) => {
-            BuiltinAttrExpander::find_by_name(&name!(test)) == Some(expander)
-                || BuiltinAttrExpander::find_by_name(&name!(bench)) == Some(expander)
-        }
-        _ => false,
-    }
-}
-
 pub fn find_builtin_attr(
     ident: &name::Name,
     krate: CrateId,

crates/hir_ty/src/tests/macros.rs

@@ -974,61 +974,12 @@ fn infer_builtin_macros_env() {
 fn infer_derive_clone_simple() {
     check_types(
         r#"
-//- /main.rs crate:main deps:core
+//- minicore: derive, clone
 #[derive(Clone)]
 struct S;
 fn test() {
     S.clone();
 } //^^^^^^^^^ S
-
-//- /lib.rs crate:core
-pub mod prelude {
-    pub mod rust_2018 {
-        #[rustc_builtin_macro]
-        pub macro Clone {}
-        pub use crate::clone::Clone;
-    }
-}
-
-pub mod clone {
-    pub trait Clone {
-        fn clone(&self) -> Self;
-    }
-}
-"#,
-    );
-}
-
-#[test]
-fn infer_derive_clone_in_core() {
-    check_types(
-        r#"
-//- /lib.rs crate:core
-#[prelude_import]
-use prelude::rust_2018::*;
-
-pub mod prelude {
-    pub mod rust_2018 {
-        #[rustc_builtin_macro]
-        pub macro Clone {}
-        pub use crate::clone::Clone;
-    }
-}
-
-pub mod clone {
-    pub trait Clone {
-        fn clone(&self) -> Self;
-    }
-}
-
-#[derive(Clone)]
-pub struct S;
-
-//- /main.rs crate:main deps:core
-use core::S;
-fn test() {
-    S.clone();
-} //^^^^^^^^^ S
 "#,
     );
 }
@@ -1037,7 +988,7 @@ fn test() {
 fn infer_derive_clone_with_params() {
     check_types(
         r#"
-//- /main.rs crate:main deps:core
+//- minicore: clone, derive
 #[derive(Clone)]
 struct S;
 #[derive(Clone)]
@@ -1048,21 +999,6 @@ fn test() {
     x;
   //^ (Wrapper<S>, {unknown})
 }
-
-//- /lib.rs crate:core
-pub mod prelude {
-    pub mod rust_2018 {
-        #[rustc_builtin_macro]
-        pub macro Clone {}
-        pub use crate::clone::Clone;
-    }
-}
-
-pub mod clone {
-    pub trait Clone {
-        fn clone(&self) -> Self;
-    }
-}
 "#,
     );
 }
@@ -1072,7 +1008,7 @@ fn infer_custom_derive_simple() {
     // FIXME: this test current now do nothing
     check_types(
         r#"
-//- /main.rs crate:main
+//- minicore: derive
 use foo::Foo;
 
 #[derive(Foo)]

crates/ide/src/expand_macro.rs

@@ -367,9 +367,7 @@ fn main() {
         check(
             r#"
 //- proc_macros: identity
-
+//- minicore: clone, derive
-#[rustc_builtin_macro]
-pub macro Clone {}
 
 #[proc_macros::identity]
 #[derive(C$0lone)]
@@ -377,7 +375,7 @@ struct Foo {}
 "#,
             expect![[r#"
                 Clone
-                impl< >crate::clone::Clone for Foo< >{}
+                impl< >core::clone::Clone for Foo< >{}
 
             "#]],
         );
@@ -387,10 +385,7 @@ struct Foo {}
     fn macro_expand_derive2() {
         check(
             r#"
-#[rustc_builtin_macro]
+//- minicore: copy, clone, derive
-pub macro Clone {}
-#[rustc_builtin_macro]
-pub macro Copy {}
 
 #[derive(Cop$0y)]
 #[derive(Clone)]
@@ -398,7 +393,7 @@ struct Foo {}
 "#,
             expect![[r#"
                 Copy
-                impl< >crate::marker::Copy for Foo< >{}
+                impl< >core::marker::Copy for Foo< >{}
 
             "#]],
         );
@@ -408,19 +403,16 @@ struct Foo {}
     fn macro_expand_derive_multi() {
         check(
             r#"
-#[rustc_builtin_macro]
+//- minicore: copy, clone, derive
-pub macro Clone {}
-#[rustc_builtin_macro]
-pub macro Copy {}
 
 #[derive(Cop$0y, Clone)]
 struct Foo {}
 "#,
             expect![[r#"
                 Copy, Clone
-                impl< >crate::marker::Copy for Foo< >{}
+                impl< >core::marker::Copy for Foo< >{}
 
-                impl< >crate::clone::Clone for Foo< >{}
+                impl< >core::clone::Clone for Foo< >{}
 
             "#]],
         );

crates/rust-analyzer/tests/slow-tests/main.rs

@@ -805,6 +805,9 @@ bar = {path = "../bar"}
 
 //- /foo/src/main.rs
 use bar::Bar;
+
+#[rustc_builtin_macro]
+macro derive($item:item) {}
 trait Bar {
   fn bar();
 }
@@ -875,7 +878,7 @@ pub fn foo(_input: TokenStream) -> TokenStream {
     let res = server.send_request::<HoverRequest>(HoverParams {
         text_document_position_params: TextDocumentPositionParams::new(
             server.doc_id("foo/src/main.rs"),
-            Position::new(7, 9),
+            Position::new(10, 9),
         ),
         work_done_progress_params: Default::default(),
     });