Merge #8087

8087: Make MacroDefId's `AstId` mandatory when possible r=jonas-schievink a=jonas-schievink

This makes it clearer (in the type definition) which macros have or don't have an `AstId`

bors r+

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>

crates/hir/src/has_source.rs

@@ -113,7 +113,7 @@ impl HasSource for TypeAlias {
 impl HasSource for MacroDef {
     type Ast = ast::Macro;
     fn source(self, db: &dyn HirDatabase) -> Option<InFile<Self::Ast>> {
-        let ast_id = self.id.ast_id?;
+        let ast_id = self.id.ast_id()?;
         Some(InFile { file_id: ast_id.file_id, value: ast_id.to_node(db.upcast()) })
     }
 }

crates/hir/src/lib.rs

@@ -1154,7 +1154,8 @@ impl MacroDef {
 
     /// Indicate it is a derive macro
     pub fn is_derive_macro(&self) -> bool {
-        matches!(self.id.kind, MacroDefKind::ProcMacro(_) | MacroDefKind::BuiltInDerive(_))
+        // FIXME: wrong for `ProcMacro`
+        matches!(self.id.kind, MacroDefKind::ProcMacro(..) | MacroDefKind::BuiltInDerive(..))
     }
 }
 

crates/hir/src/semantics/source_to_def.rs

@@ -195,12 +195,12 @@ impl SourceToDefCtx<'_, '_> {
         &mut self,
         src: InFile<ast::MacroRules>,
     ) -> Option<MacroDefId> {
-        let kind = MacroDefKind::Declarative;
+        let file_ast_id = self.db.ast_id_map(src.file_id).ast_id(&src.value);
+        let ast_id = AstId::new(src.file_id, file_ast_id.upcast());
+        let kind = MacroDefKind::Declarative(ast_id);
         let file_id = src.file_id.original_file(self.db.upcast());
         let krate = self.file_to_def(file_id).get(0).copied()?.krate();
-        let file_ast_id = self.db.ast_id_map(src.file_id).ast_id(&src.value);
-        let ast_id = Some(AstId::new(src.file_id, file_ast_id.upcast()));
-        Some(MacroDefId { krate, ast_id, kind, local_inner: false })
+        Some(MacroDefId { krate, kind, local_inner: false })
     }
 
     pub(super) fn find_container(&mut self, src: InFile<&SyntaxNode>) -> Option<ChildContainer> {

crates/hir_def/src/attr.rs

@@ -209,7 +209,7 @@ impl Attrs {
             },
             AttrDefId::TraitId(it) => attrs_from_item_tree(it.lookup(db).id, db),
             AttrDefId::MacroDefId(it) => {
-                it.ast_id.map_or_else(Default::default, |ast_id| attrs_from_ast(ast_id, db))
+                it.ast_id().map_or_else(Default::default, |ast_id| attrs_from_ast(ast_id, db))
             }
             AttrDefId::ImplId(it) => attrs_from_item_tree(it.lookup(db).id, db),
             AttrDefId::ConstId(it) => attrs_from_item_tree(it.lookup(db).id, db),

crates/hir_def/src/import_map.rs

@@ -912,10 +912,10 @@ mod tests {
                 dep::fmt (t)
                 dep::format (f)
                 dep::Fmt (v)
-                dep::fmt::Display (t)
+                dep::Fmt (m)
                 dep::Fmt (t)
                 dep::fmt::Display::fmt (a)
-                dep::Fmt (m)
+                dep::fmt::Display (t)
             "#]],
         );
 
@@ -926,9 +926,9 @@ mod tests {
             expect![[r#"
                 dep::fmt (t)
                 dep::Fmt (v)
+                dep::Fmt (m)
                 dep::Fmt (t)
                 dep::fmt::Display::fmt (a)
-                dep::Fmt (m)
             "#]],
         );
 
@@ -939,10 +939,10 @@ mod tests {
             expect![[r#"
                 dep::fmt (t)
                 dep::Fmt (v)
-                dep::fmt::Display (t)
+                dep::Fmt (m)
                 dep::Fmt (t)
                 dep::fmt::Display::fmt (a)
-                dep::Fmt (m)
+                dep::fmt::Display (t)
             "#]],
         );
     }
@@ -980,10 +980,10 @@ mod tests {
             expect![[r#"
                 dep::fmt (t)
                 dep::Fmt (v)
-                dep::fmt::Display (t)
+                dep::Fmt (m)
                 dep::Fmt (t)
                 dep::fmt::Display::fmt (a)
-                dep::Fmt (m)
+                dep::fmt::Display (t)
             "#]],
         );
 
@@ -994,9 +994,9 @@ mod tests {
             expect![[r#"
                 dep::fmt (t)
                 dep::Fmt (v)
+                dep::Fmt (m)
                 dep::Fmt (t)
                 dep::fmt::Display::fmt (a)
-                dep::Fmt (m)
             "#]],
         );
     }
@@ -1058,8 +1058,8 @@ mod tests {
             Query::new("".to_string()).limit(2),
             expect![[r#"
                 dep::fmt (t)
-                dep::Fmt (t)
                 dep::Fmt (m)
+                dep::Fmt (t)
                 dep::Fmt (v)
             "#]],
         );

crates/hir_def/src/lib.rs

@@ -650,7 +650,7 @@ fn macro_call_as_call_id(
 ) -> Result<Result<MacroCallId, ErrorEmitted>, UnresolvedMacro> {
     let def: MacroDefId = resolver(call.path.clone()).ok_or(UnresolvedMacro)?;
 
-    let res = if let MacroDefKind::BuiltInEager(_) = def.kind {
+    let res = if let MacroDefKind::BuiltInEager(..) = def.kind {
         let macro_call = InFile::new(call.ast_id.file_id, call.ast_id.to_node(db.upcast()));
         let hygiene = Hygiene::new(db.upcast(), call.ast_id.file_id);
 

crates/hir_def/src/nameres/collector.rs

@@ -357,13 +357,11 @@ impl DefCollector<'_> {
         self.exports_proc_macros = true;
         let macro_def = match self.proc_macros.iter().find(|(n, _)| n == name) {
             Some((_, expander)) => MacroDefId {
-                ast_id: None,
                 krate: self.def_map.krate,
                 kind: MacroDefKind::ProcMacro(*expander),
                 local_inner: false,
             },
             None => MacroDefId {
-                ast_id: None,
                 krate: self.def_map.krate,
                 kind: MacroDefKind::ProcMacro(ProcMacroExpander::dummy(self.def_map.krate)),
                 local_inner: false,
@@ -1445,9 +1443,8 @@ impl ModCollector<'_, '_> {
 
         // Case 2: normal `macro_rules!` macro
         let macro_id = MacroDefId {
-            ast_id: Some(ast_id),
             krate: self.def_collector.def_map.krate,
-            kind: MacroDefKind::Declarative,
+            kind: MacroDefKind::Declarative(ast_id),
             local_inner: is_local_inner,
         };
         self.def_collector.define_macro(self.module_id, mac.name.clone(), macro_id, is_export);

crates/hir_expand/src/builtin_derive.rs

@@ -61,8 +61,7 @@ pub fn find_builtin_derive(
     let expander = BuiltinDeriveExpander::find_by_name(ident)?;
     Some(MacroDefId {
         krate,
-        ast_id: Some(ast_id),
-        kind: MacroDefKind::BuiltInDerive(expander),
+        kind: MacroDefKind::BuiltInDerive(expander, ast_id),
         local_inner: false,
     })
 }
@@ -314,8 +313,7 @@ $0
         let loc = MacroCallLoc {
             def: MacroDefId {
                 krate: CrateId(0),
-                ast_id: Some(macro_ast_id),
-                kind: MacroDefKind::BuiltInDerive(expander),
+                kind: MacroDefKind::BuiltInDerive(expander, macro_ast_id),
                 local_inner: false,
             },
             krate: CrateId(0),

crates/hir_expand/src/builtin_macro.rs

@@ -71,14 +71,12 @@ pub fn find_builtin_macro(
     match kind {
         Either::Left(kind) => Some(MacroDefId {
             krate,
-            ast_id: Some(ast_id),
-            kind: MacroDefKind::BuiltIn(kind),
+            kind: MacroDefKind::BuiltIn(kind, ast_id),
             local_inner: false,
         }),
         Either::Right(kind) => Some(MacroDefId {
             krate,
-            ast_id: Some(ast_id),
-            kind: MacroDefKind::BuiltInEager(kind),
+            kind: MacroDefKind::BuiltInEager(kind, ast_id),
             local_inner: false,
         }),
     }
@@ -512,6 +510,7 @@ mod tests {
         let macro_call = macro_calls.pop().unwrap();
 
         let expander = find_by_name(&macro_rules.name().unwrap().as_name()).unwrap();
+        let ast_id = AstId::new(file_id.into(), ast_id_map.ast_id(&macro_rules));
 
         let krate = CrateId(0);
         let file_id = match expander {
@@ -519,8 +518,7 @@ mod tests {
                 // the first one should be a macro_rules
                 let def = MacroDefId {
                     krate: CrateId(0),
-                    ast_id: Some(AstId::new(file_id.into(), ast_id_map.ast_id(&macro_rules))),
-                    kind: MacroDefKind::BuiltIn(expander),
+                    kind: MacroDefKind::BuiltIn(expander, ast_id),
                     local_inner: false,
                 };
 
@@ -540,8 +538,7 @@ mod tests {
                 // the first one should be a macro_rules
                 let def = MacroDefId {
                     krate,
-                    ast_id: Some(AstId::new(file_id.into(), ast_id_map.ast_id(&macro_rules))),
-                    kind: MacroDefKind::BuiltInEager(expander),
+                    kind: MacroDefKind::BuiltInEager(expander, ast_id),
                     local_inner: false,
                 };
 

crates/hir_expand/src/db.rs

@@ -130,8 +130,8 @@ fn ast_id_map(db: &dyn AstDatabase, file_id: HirFileId) -> Arc<AstIdMap> {
 
 fn macro_def(db: &dyn AstDatabase, id: MacroDefId) -> Option<Arc<(TokenExpander, mbe::TokenMap)>> {
     match id.kind {
-        MacroDefKind::Declarative => {
-            let macro_rules = match id.ast_id?.to_node(db) {
+        MacroDefKind::Declarative(ast_id) => {
+            let macro_rules = match ast_id.to_node(db) {
                 syntax::ast::Macro::MacroRules(mac) => mac,
                 syntax::ast::Macro::MacroDef(_) => return None,
             };
@@ -150,13 +150,13 @@ fn macro_def(db: &dyn AstDatabase, id: MacroDefId) -> Option<Arc<(TokenExpander,
             };
             Some(Arc::new((TokenExpander::MacroRules(rules), tmap)))
         }
-        MacroDefKind::BuiltIn(expander) => {
+        MacroDefKind::BuiltIn(expander, _) => {
             Some(Arc::new((TokenExpander::Builtin(expander), mbe::TokenMap::default())))
         }
-        MacroDefKind::BuiltInDerive(expander) => {
+        MacroDefKind::BuiltInDerive(expander, _) => {
             Some(Arc::new((TokenExpander::BuiltinDerive(expander), mbe::TokenMap::default())))
         }
-        MacroDefKind::BuiltInEager(_) => None,
+        MacroDefKind::BuiltInEager(..) => None,
         MacroDefKind::ProcMacro(expander) => {
             Some(Arc::new((TokenExpander::ProcMacro(expander), mbe::TokenMap::default())))
         }

crates/hir_expand/src/eager.rs

@@ -140,7 +140,7 @@ pub fn expand_eager_macro(
     let subtree =
         diagnostic_sink.option(to_subtree(&result), || err("failed to parse macro result"))?;
 
-    if let MacroDefKind::BuiltInEager(eager) = def.kind {
+    if let MacroDefKind::BuiltInEager(eager, _) = def.kind {
         let res = eager.expand(db, arg_id, &subtree);
 
         let (subtree, fragment) = diagnostic_sink.expand_result_option(res)?;
@@ -193,7 +193,7 @@ fn eager_macro_recur(
         let def = diagnostic_sink
             .option_with(|| macro_resolver(child.path()?), || err("failed to resolve macro"))?;
         let insert = match def.kind {
-            MacroDefKind::BuiltInEager(_) => {
+            MacroDefKind::BuiltInEager(..) => {
                 let id: MacroCallId = expand_eager_macro(
                     db,
                     krate,
@@ -206,9 +206,9 @@ fn eager_macro_recur(
                 db.parse_or_expand(id.as_file())
                     .expect("successful macro expansion should be parseable")
             }
-            MacroDefKind::Declarative
-            | MacroDefKind::BuiltIn(_)
-            | MacroDefKind::BuiltInDerive(_)
+            MacroDefKind::Declarative(_)
+            | MacroDefKind::BuiltIn(..)
+            | MacroDefKind::BuiltInDerive(..)
             | MacroDefKind::ProcMacro(_) => {
                 let res = lazy_expand(db, &def, curr.with_value(child.clone()), krate);
                 let val = diagnostic_sink.expand_result_option(res)?;

crates/hir_expand/src/hygiene.rs

@@ -145,7 +145,7 @@ fn make_hygiene_info(
 ) -> Option<HygieneInfo> {
     let arg_tt = loc.kind.arg(db)?;
 
-    let def_offset = loc.def.ast_id.and_then(|id| {
+    let def_offset = loc.def.ast_id().and_then(|id| {
         let def_tt = match id.to_node(db) {
             ast::Macro::MacroRules(mac) => mac.token_tree()?.syntax().text_range().start(),
             ast::Macro::MacroDef(_) => return None,
@@ -176,12 +176,12 @@ impl HygieneFrame {
                     let loc = db.lookup_intern_macro(id);
                     let info = make_hygiene_info(db, macro_file, &loc);
                     match loc.def.kind {
-                        MacroDefKind::Declarative => {
+                        MacroDefKind::Declarative(_) => {
                             (info, Some(loc.def.krate), loc.def.local_inner)
                         }
-                        MacroDefKind::BuiltIn(_) => (info, Some(loc.def.krate), false),
-                        MacroDefKind::BuiltInDerive(_) => (info, None, false),
-                        MacroDefKind::BuiltInEager(_) => (info, None, false),
+                        MacroDefKind::BuiltIn(..) => (info, Some(loc.def.krate), false),
+                        MacroDefKind::BuiltInDerive(..) => (info, None, false),
+                        MacroDefKind::BuiltInEager(..) => (info, None, false),
                         MacroDefKind::ProcMacro(_) => (info, None, false),
                     }
                 }

crates/hir_expand/src/lib.rs

@@ -143,7 +143,7 @@ impl HirFileId {
 
                 let arg_tt = loc.kind.arg(db)?;
 
-                let def = loc.def.ast_id.and_then(|id| {
+                let def = loc.def.ast_id().and_then(|id| {
                     let def_tt = match id.to_node(db) {
                         ast::Macro::MacroRules(mac) => mac.token_tree()?,
                         ast::Macro::MacroDef(_) => return None,
@@ -180,7 +180,7 @@ impl HirFileId {
                 };
                 let loc: MacroCallLoc = db.lookup_intern_macro(lazy_id);
                 let item = match loc.def.kind {
-                    MacroDefKind::BuiltInDerive(_) => loc.kind.node(db),
+                    MacroDefKind::BuiltInDerive(..) => loc.kind.node(db),
                     _ => return None,
                 };
                 Some(item.with_value(ast::Item::cast(item.value.clone())?))
@@ -224,7 +224,6 @@ impl From<EagerMacroId> for MacroCallId {
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct MacroDefId {
     pub krate: CrateId,
-    pub ast_id: Option<AstId<ast::Macro>>,
     pub kind: MacroDefKind,
 
     pub local_inner: bool,
@@ -239,15 +238,26 @@ impl MacroDefId {
     ) -> LazyMacroId {
         db.intern_macro(MacroCallLoc { def: self, krate, kind })
     }
+
+    pub fn ast_id(&self) -> Option<AstId<ast::Macro>> {
+        let id = match &self.kind {
+            MacroDefKind::Declarative(id) => id,
+            MacroDefKind::BuiltIn(_, id) => id,
+            MacroDefKind::BuiltInDerive(_, id) => id,
+            MacroDefKind::BuiltInEager(_, id) => id,
+            MacroDefKind::ProcMacro(_) => return None,
+        };
+        Some(*id)
+    }
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub enum MacroDefKind {
-    Declarative,
-    BuiltIn(BuiltinFnLikeExpander),
+    Declarative(AstId<ast::Macro>),
+    BuiltIn(BuiltinFnLikeExpander, AstId<ast::Macro>),
     // FIXME: maybe just Builtin and rename BuiltinFnLikeExpander to BuiltinExpander
-    BuiltInDerive(BuiltinDeriveExpander),
-    BuiltInEager(EagerExpander),
+    BuiltInDerive(BuiltinDeriveExpander, AstId<ast::Macro>),
+    BuiltInEager(EagerExpander, AstId<ast::Macro>),
     ProcMacro(ProcMacroExpander),
 }