Auto merge of #15923 - tamasfe:feat/better-ignored-macros2, r=Veykril

feat: ignored and disabled macro expansion

Supersedes #15117, I was having some conflicts after a rebase and since I didn't remember much of it I started clean instead.

The end result is pretty much the same as the linked PR, but instead of proc macro lookups, I marked the expanders that explicitly cannot be expanded and we shouldn't even attempt to do so.

## Unresolved questions

- [ ] I introduced a `DISABLED_ID` next to `DUMMY_ID` in `hir-expand`'s `ProcMacroExpander`, that is effectively exactly the same thing with slightly different semantics, dummy macros are not (yet) expanded probably due to errors, while not expanding disabled macros is part of the usual flow. I'm not sure if it's the right way to handle this, I also thought of just adding a flag instead of replacing the macro ID, so that the disabled macro can still be expanded for any reason if needed.

Author: bors

crates/base-db/src/input.rs

@@ -243,6 +243,7 @@ impl CrateDisplayName {
         CrateDisplayName { crate_name, canonical_name }
     }
 }
+
 pub type TargetLayoutLoadResult = Result<Arc<str>, Arc<str>>;
 
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]

crates/hir-def/src/data.rs

@@ -634,7 +634,6 @@ impl<'a> AssocItemCollector<'a> {
                     attr,
                 ) {
                     Ok(ResolvedAttr::Macro(call_id)) => {
-                        self.attr_calls.push((ast_id, call_id));
                         // If proc attribute macro expansion is disabled, skip expanding it here
                         if !self.db.expand_proc_attr_macros() {
                             continue 'attrs;
@@ -647,10 +646,21 @@ impl<'a> AssocItemCollector<'a> {
                             // disabled. This is analogous to the handling in
                             // `DefCollector::collect_macros`.
                             if exp.is_dummy() {
+                                self.diagnostics.push(DefDiagnostic::unresolved_proc_macro(
+                                    self.module_id.local_id,
+                                    loc.kind,
+                                    loc.def.krate,
+                                ));
+
+                                continue 'attrs;
+                            }
+                            if exp.is_disabled() {
                                 continue 'attrs;
                             }
                         }
 
+                        self.attr_calls.push((ast_id, call_id));
+
                         let res =
                             self.expander.enter_expand_id::<ast::MacroItems>(self.db, call_id);
                         self.collect_macro_items(res, &|| loc.kind.clone());

crates/hir-def/src/macro_expansion_tests/mod.rs

@@ -58,6 +58,7 @@ pub fn identity_when_valid(_attr: TokenStream, item: TokenStream) -> TokenStream
             name: "identity_when_valid".into(),
             kind: ProcMacroKind::Attr,
             expander: sync::Arc::new(IdentityWhenValidProcMacroExpander),
+            disabled: false,
         },
     )];
     let db = TestDB::with_files_extra_proc_macros(ra_fixture, extra_proc_macros);

crates/hir-def/src/nameres/collector.rs

@@ -11,7 +11,7 @@ use either::Either;
 use hir_expand::{
     ast_id_map::FileAstId,
     attrs::{Attr, AttrId},
-    builtin_attr_macro::find_builtin_attr,
+    builtin_attr_macro::{find_builtin_attr, BuiltinAttrExpander},
     builtin_derive_macro::find_builtin_derive,
     builtin_fn_macro::find_builtin_macro,
     name::{name, AsName, Name},
@@ -98,9 +98,13 @@ pub(super) fn collect_defs(db: &dyn DefDatabase, def_map: DefMap, tree_id: TreeI
                         };
                         (
                             name.as_name(),
-                            CustomProcMacroExpander::new(hir_expand::proc_macro::ProcMacroId(
-                                idx as u32,
-                            )),
+                            if it.disabled {
+                                CustomProcMacroExpander::disabled()
+                            } else {
+                                CustomProcMacroExpander::new(hir_expand::proc_macro::ProcMacroId(
+                                    idx as u32,
+                                ))
+                            },
                         )
                     })
                     .collect())
@@ -604,9 +608,6 @@ impl DefCollector<'_> {
         id: ItemTreeId<item_tree::Function>,
         fn_id: FunctionId,
     ) {
-        if self.def_map.block.is_some() {
-            return;
-        }
         let kind = def.kind.to_basedb_kind();
         let (expander, kind) =
             match self.proc_macros.as_ref().map(|it| it.iter().find(|(n, _)| n == &def.name)) {
@@ -1120,9 +1121,16 @@ impl DefCollector<'_> {
         let mut push_resolved = |directive: &MacroDirective, call_id| {
             resolved.push((directive.module_id, directive.depth, directive.container, call_id));
         };
+
+        #[derive(PartialEq, Eq)]
+        enum Resolved {
+            Yes,
+            No,
+        }
+
         let mut res = ReachedFixedPoint::Yes;
         // Retain unresolved macros after this round of resolution.
-        macros.retain(|directive| {
+        let mut retain = |directive: &MacroDirective| {
             let subns = match &directive.kind {
                 MacroDirectiveKind::FnLike { .. } => MacroSubNs::Bang,
                 MacroDirectiveKind::Attr { .. } | MacroDirectiveKind::Derive { .. } => {
@@ -1156,10 +1164,11 @@ impl DefCollector<'_> {
                         self.def_map.modules[directive.module_id]
                             .scope
                             .add_macro_invoc(ast_id.ast_id, call_id);
+
                         push_resolved(directive, call_id);
 
                         res = ReachedFixedPoint::No;
-                        return false;
+                        return Resolved::Yes;
                     }
                 }
                 MacroDirectiveKind::Derive { ast_id, derive_attr, derive_pos, call_site } => {
@@ -1198,7 +1207,7 @@ impl DefCollector<'_> {
 
                         push_resolved(directive, call_id);
                         res = ReachedFixedPoint::No;
-                        return false;
+                        return Resolved::Yes;
                     }
                 }
                 MacroDirectiveKind::Attr { ast_id: file_ast_id, mod_item, attr, tree } => {
@@ -1221,7 +1230,7 @@ impl DefCollector<'_> {
                         }
                         .collect(&[*mod_item], directive.container);
                         res = ReachedFixedPoint::No;
-                        false
+                        Resolved::Yes
                     };
 
                     if let Some(ident) = path.as_ident() {
@@ -1237,13 +1246,18 @@ impl DefCollector<'_> {
 
                     let def = match resolver_def_id(path.clone()) {
                         Some(def) if def.is_attribute() => def,
-                        _ => return true,
+                        _ => return Resolved::No,
                     };
-                    if matches!(
-                        def,
-                        MacroDefId { kind: MacroDefKind::BuiltInAttr(expander, _),.. }
-                        if expander.is_derive()
-                    ) {
+
+                    if let MacroDefId {
+                        kind:
+                            MacroDefKind::BuiltInAttr(
+                                BuiltinAttrExpander::Derive | BuiltinAttrExpander::DeriveConst,
+                                _,
+                            ),
+                        ..
+                    } = def
+                    {
                         // Resolved to `#[derive]`, we don't actually expand this attribute like
                         // normal (as that would just be an identity expansion with extra output)
                         // Instead we treat derive attributes special and apply them separately.
@@ -1316,16 +1330,6 @@ impl DefCollector<'_> {
                     let call_id =
                         attr_macro_as_call_id(self.db, file_ast_id, attr, self.def_map.krate, def);
 
-                    // If proc attribute macro expansion is disabled, skip expanding it here
-                    if !self.db.expand_proc_attr_macros() {
-                        self.def_map.diagnostics.push(DefDiagnostic::unresolved_proc_macro(
-                            directive.module_id,
-                            self.db.lookup_intern_macro_call(call_id).kind,
-                            def.krate,
-                        ));
-                        return recollect_without(self);
-                    }
-
                     // Skip #[test]/#[bench] expansion, which would merely result in more memory usage
                     // due to duplicating functions into macro expansions
                     if matches!(
@@ -1337,17 +1341,29 @@ impl DefCollector<'_> {
                     }
 
                     if let MacroDefKind::ProcMacro(exp, ..) = def.kind {
-                        if exp.is_dummy() {
-                            // If there's no expander for the proc macro (e.g.
-                            // because proc macros are disabled, or building the
-                            // proc macro crate failed), report this and skip
-                            // expansion like we would if it was disabled
+                        // If proc attribute macro expansion is disabled, skip expanding it here
+                        if !self.db.expand_proc_attr_macros() {
                             self.def_map.diagnostics.push(DefDiagnostic::unresolved_proc_macro(
                                 directive.module_id,
                                 self.db.lookup_intern_macro_call(call_id).kind,
                                 def.krate,
                             ));
+                            return recollect_without(self);
+                        }
 
+                        // If there's no expander for the proc macro (e.g.
+                        // because proc macros are disabled, or building the
+                        // proc macro crate failed), report this and skip
+                        // expansion like we would if it was disabled
+                        if exp.is_dummy() {
+                            self.def_map.diagnostics.push(DefDiagnostic::unresolved_proc_macro(
+                                directive.module_id,
+                                self.db.lookup_intern_macro_call(call_id).kind,
+                                def.krate,
+                            ));
+                            return recollect_without(self);
+                        }
+                        if exp.is_disabled() {
                             return recollect_without(self);
                         }
                     }
@@ -1358,12 +1374,13 @@ impl DefCollector<'_> {
 
                     push_resolved(directive, call_id);
                     res = ReachedFixedPoint::No;
-                    return false;
+                    return Resolved::Yes;
                 }
             }
 
-            true
-        });
+            Resolved::No
+        };
+        macros.retain(|it| retain(it) == Resolved::No);
         // Attribute resolution can add unresolved macro invocations, so concatenate the lists.
         macros.extend(mem::take(&mut self.unresolved_macros));
         self.unresolved_macros = macros;
@@ -1673,7 +1690,11 @@ impl ModCollector<'_, '_> {
                         FunctionLoc { container, id: ItemTreeId::new(self.tree_id, id) }.intern(db);
 
                     let vis = resolve_vis(def_map, &self.item_tree[it.visibility]);
-                    if self.def_collector.is_proc_macro && self.module_id == DefMap::ROOT {
+
+                    if self.def_collector.def_map.block.is_none()
+                        && self.def_collector.is_proc_macro
+                        && self.module_id == DefMap::ROOT
+                    {
                         if let Some(proc_macro) = attrs.parse_proc_macro_decl(&it.name) {
                             self.def_collector.export_proc_macro(
                                 proc_macro,

crates/hir-def/src/nameres/diagnostics.rs

@@ -103,6 +103,9 @@ impl DefDiagnostic {
     }
 
     // FIXME: Whats the difference between this and unresolved_macro_call
+    // FIXME: This is used for a lot of things, unresolved proc macros, disabled proc macros, etc
+    // yet the diagnostic handler in ide-diagnostics has to figure out what happened because this
+    // struct loses all that information!
     pub(crate) fn unresolved_proc_macro(
         container: LocalModuleId,
         ast: MacroCallKind,

crates/hir-expand/src/lib.rs

@@ -129,6 +129,8 @@ pub type ExpandResult<T> = ValueResult<T, ExpandError>;
 #[derive(Debug, PartialEq, Eq, Clone, Hash)]
 pub enum ExpandError {
     UnresolvedProcMacro(CrateId),
+    /// The macro expansion is disabled.
+    MacroDisabled,
     Mbe(mbe::ExpandError),
     RecursionOverflowPoisoned,
     Other(Box<Box<str>>),
@@ -160,6 +162,7 @@ impl fmt::Display for ExpandError {
                 f.write_str(it)
             }
             ExpandError::Other(it) => f.write_str(it),
+            ExpandError::MacroDisabled => f.write_str("macro disabled"),
         }
     }
 }

crates/hir-expand/src/proc_macro.rs

@@ -49,27 +49,43 @@ pub struct ProcMacro {
     pub name: SmolStr,
     pub kind: ProcMacroKind,
     pub expander: sync::Arc<dyn ProcMacroExpander>,
+    pub disabled: bool,
 }
 
 #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
 pub struct CustomProcMacroExpander {
     proc_macro_id: ProcMacroId,
 }
 
-const DUMMY_ID: u32 = !0;
-
 impl CustomProcMacroExpander {
+    const DUMMY_ID: u32 = !0;
+    const DISABLED_ID: u32 = !1;
+
     pub fn new(proc_macro_id: ProcMacroId) -> Self {
-        assert_ne!(proc_macro_id.0, DUMMY_ID);
+        assert_ne!(proc_macro_id.0, Self::DUMMY_ID);
+        assert_ne!(proc_macro_id.0, Self::DISABLED_ID);
         Self { proc_macro_id }
     }
 
-    pub fn dummy() -> Self {
-        Self { proc_macro_id: ProcMacroId(DUMMY_ID) }
+    /// A dummy expander that always errors. This is used for proc-macros that are missing, usually
+    /// due to them not being built yet.
+    pub const fn dummy() -> Self {
+        Self { proc_macro_id: ProcMacroId(Self::DUMMY_ID) }
     }
 
-    pub fn is_dummy(&self) -> bool {
-        self.proc_macro_id.0 == DUMMY_ID
+    /// The macro was not yet resolved.
+    pub const fn is_dummy(&self) -> bool {
+        self.proc_macro_id.0 == Self::DUMMY_ID
+    }
+
+    /// A dummy expander that always errors. This expander is used for macros that have been disabled.
+    pub const fn disabled() -> Self {
+        Self { proc_macro_id: ProcMacroId(Self::DISABLED_ID) }
+    }
+
+    /// The macro is explicitly disabled and cannot be expanded.
+    pub const fn is_disabled(&self) -> bool {
+        self.proc_macro_id.0 == Self::DISABLED_ID
     }
 
     pub fn expand(
@@ -84,10 +100,14 @@ impl CustomProcMacroExpander {
         mixed_site: Span,
     ) -> ExpandResult<tt::Subtree> {
         match self.proc_macro_id {
-            ProcMacroId(DUMMY_ID) => ExpandResult::new(
+            ProcMacroId(Self::DUMMY_ID) => ExpandResult::new(
                 tt::Subtree::empty(tt::DelimSpan { open: call_site, close: call_site }),
                 ExpandError::UnresolvedProcMacro(def_crate),
             ),
+            ProcMacroId(Self::DISABLED_ID) => ExpandResult::new(
+                tt::Subtree::empty(tt::DelimSpan { open: call_site, close: call_site }),
+                ExpandError::MacroDisabled,
+            ),
             ProcMacroId(id) => {
                 let proc_macros = db.proc_macros();
                 let proc_macros = match proc_macros.get(&def_crate) {
@@ -110,7 +130,7 @@ impl CustomProcMacroExpander {
                         );
                         return ExpandResult::new(
                             tt::Subtree::empty(tt::DelimSpan { open: call_site, close: call_site }),
-                            ExpandError::other("Internal error"),
+                            ExpandError::other("Internal error: proc-macro index out of bounds"),
                         );
                     }
                 };