From b238ddd21adf9910769522a21e31c2e14f664396 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Tue, 15 Dec 2020 20:33:05 +0100 Subject: [PATCH] Make macro def krate mandatory Refactors builtin derive support to go through proper name resolution --- crates/hir/src/code_model.rs | 2 +- crates/hir/src/semantics/source_to_def.rs | 2 +- crates/hir_def/src/body/lower.rs | 2 +- crates/hir_def/src/item_scope.rs | 2 +- crates/hir_def/src/nameres/collector.rs | 18 +++------ crates/hir_def/src/nameres/tests/macros.rs | 31 +++++++++++++- crates/hir_expand/src/builtin_derive.rs | 40 ++++++++++++++----- crates/hir_expand/src/builtin_macro.rs | 8 ++-- crates/hir_expand/src/hygiene.rs | 4 +- crates/hir_expand/src/lib.rs | 8 +--- crates/hir_ty/src/tests/macros.rs | 6 +++ crates/ide/src/goto_implementation.rs | 2 + .../test_data/highlighting.html | 5 ++- crates/ide/src/syntax_highlighting/tests.rs | 3 ++ 14 files changed, 91 insertions(+), 42 deletions(-) diff --git a/crates/hir/src/code_model.rs b/crates/hir/src/code_model.rs index 42dc35b762c..9bfcd215a5e 100644 --- a/crates/hir/src/code_model.rs +++ b/crates/hir/src/code_model.rs @@ -970,7 +970,7 @@ impl MacroDef { /// defines this macro. The reasons for this is that macros are expanded /// early, in `hir_expand`, where modules simply do not exist yet. pub fn module(self, db: &dyn HirDatabase) -> Option { - let krate = self.id.krate?; + let krate = self.id.krate; let module_id = db.crate_def_map(krate).root; Some(Module::new(Crate { id: krate }, module_id)) } diff --git a/crates/hir/src/semantics/source_to_def.rs b/crates/hir/src/semantics/source_to_def.rs index d499ae34022..3efca5baa5c 100644 --- a/crates/hir/src/semantics/source_to_def.rs +++ b/crates/hir/src/semantics/source_to_def.rs @@ -158,7 +158,7 @@ impl SourceToDefCtx<'_, '_> { let krate = self.file_to_def(file_id)?.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: Some(krate), ast_id, kind, local_inner: false }) + Some(MacroDefId { krate, ast_id, kind, local_inner: false }) } pub(super) fn find_container(&mut self, src: InFile<&SyntaxNode>) -> Option { diff --git a/crates/hir_def/src/body/lower.rs b/crates/hir_def/src/body/lower.rs index e4bf5603c7e..23e2fd7641f 100644 --- a/crates/hir_def/src/body/lower.rs +++ b/crates/hir_def/src/body/lower.rs @@ -803,7 +803,7 @@ impl ExprCollector<'_> { } Either::Right(e) => { let mac = MacroDefId { - krate: Some(self.expander.module.krate), + krate: self.expander.module.krate, ast_id: Some(self.expander.ast_id(&e)), kind: MacroDefKind::Declarative, local_inner: false, diff --git a/crates/hir_def/src/item_scope.rs b/crates/hir_def/src/item_scope.rs index a8b3fe844a5..62ab3b2bd62 100644 --- a/crates/hir_def/src/item_scope.rs +++ b/crates/hir_def/src/item_scope.rs @@ -363,7 +363,7 @@ impl ItemInNs { ModuleDefId::TypeAliasId(id) => id.lookup(db).module(db).krate, ModuleDefId::BuiltinType(_) => return None, }, - ItemInNs::Macros(id) => return id.krate, + ItemInNs::Macros(id) => return Some(id.krate), }) } } diff --git a/crates/hir_def/src/nameres/collector.rs b/crates/hir_def/src/nameres/collector.rs index c2f741060f5..785895277fa 100644 --- a/crates/hir_def/src/nameres/collector.rs +++ b/crates/hir_def/src/nameres/collector.rs @@ -309,13 +309,13 @@ impl DefCollector<'_> { let macro_def = match self.proc_macros.iter().find(|(n, _)| n == name) { Some((_, expander)) => MacroDefId { ast_id: None, - krate: Some(self.def_map.krate), + krate: self.def_map.krate, kind: MacroDefKind::ProcMacro(*expander), local_inner: false, }, None => MacroDefId { ast_id: None, - krate: Some(self.def_map.krate), + krate: self.def_map.krate, kind: MacroDefKind::ProcMacro(ProcMacroExpander::dummy(self.def_map.krate)), local_inner: false, }, @@ -784,14 +784,6 @@ impl DefCollector<'_> { directive: &DeriveDirective, path: &ModPath, ) -> Option { - if let Some(name) = path.as_ident() { - // FIXME this should actually be handled with the normal name - // resolution; the std lib defines built-in stubs for the derives, - // but these are new-style `macro`s, which we don't support yet - if let Some(def_id) = find_builtin_derive(name) { - return Some(def_id); - } - } let resolved_res = self.def_map.resolve_path_fp_with_macro( self.db, ResolveMode::Other, @@ -984,7 +976,9 @@ impl ModCollector<'_, '_> { // to define builtin macros, so we support at least that part. if mac.is_builtin { let krate = self.def_collector.def_map.krate; - if let Some(macro_id) = find_builtin_macro(&mac.name, krate, ast_id) { + let macro_id = find_builtin_macro(&mac.name, krate, ast_id) + .or_else(|| find_builtin_derive(&mac.name, krate, ast_id)); + if let Some(macro_id) = macro_id { let vis = self .def_collector .def_map @@ -1326,7 +1320,7 @@ impl ModCollector<'_, '_> { // Case 2: normal `macro_rules!` macro let macro_id = MacroDefId { ast_id: Some(ast_id), - krate: Some(self.def_collector.def_map.krate), + krate: self.def_collector.def_map.krate, kind: MacroDefKind::Declarative, local_inner: mac.is_local_inner, }; diff --git a/crates/hir_def/src/nameres/tests/macros.rs b/crates/hir_def/src/nameres/tests/macros.rs index 305fca0f9c1..6fe2ee78a1f 100644 --- a/crates/hir_def/src/nameres/tests/macros.rs +++ b/crates/hir_def/src/nameres/tests/macros.rs @@ -633,14 +633,43 @@ pub struct bar; fn expand_derive() { let map = compute_crate_def_map( " - //- /main.rs + //- /main.rs crate:main deps:core + use core::*; + #[derive(Copy, Clone)] struct Foo; + + //- /core.rs crate:core + #[rustc_builtin_macro] + pub macro Copy {} + + #[rustc_builtin_macro] + pub macro Clone {} ", ); assert_eq!(map.modules[map.root].scope.impls().len(), 2); } +#[test] +fn resolve_builtin_derive() { + check( + r#" +//- /main.rs crate:main deps:core +use core::*; + +//- /core.rs crate:core +#[rustc_builtin_macro] +pub macro Clone {} + +pub trait Clone {} +"#, + expect![[r#" + crate + Clone: t m + "#]], + ); +} + #[test] fn macro_expansion_overflow() { mark::check!(macro_expansion_overflow); diff --git a/crates/hir_expand/src/builtin_derive.rs b/crates/hir_expand/src/builtin_derive.rs index 988a60d56ad..ad378762a1e 100644 --- a/crates/hir_expand/src/builtin_derive.rs +++ b/crates/hir_expand/src/builtin_derive.rs @@ -8,7 +8,7 @@ use syntax::{ match_ast, }; -use crate::{db::AstDatabase, name, quote, LazyMacroId, MacroDefId, MacroDefKind}; +use crate::{db::AstDatabase, name, quote, AstId, CrateId, LazyMacroId, MacroDefId, MacroDefKind}; macro_rules! register_builtin { ( $($trait:ident => $expand:ident),* ) => { @@ -29,16 +29,15 @@ macro_rules! register_builtin { }; expander(db, id, tt) } + + fn find_by_name(name: &name::Name) -> Option { + match name { + $( id if id == &name::name![$trait] => Some(BuiltinDeriveExpander::$trait), )* + _ => None, + } + } } - pub fn find_builtin_derive(ident: &name::Name) -> Option { - let kind = match ident { - $( id if id == &name::name![$trait] => BuiltinDeriveExpander::$trait, )* - _ => return None, - }; - - Some(MacroDefId { krate: None, ast_id: None, kind: MacroDefKind::BuiltInDerive(kind), local_inner: false }) - } }; } @@ -54,6 +53,20 @@ register_builtin! { PartialEq => partial_eq_expand } +pub fn find_builtin_derive( + ident: &name::Name, + krate: CrateId, + ast_id: AstId, +) -> Option { + let expander = BuiltinDeriveExpander::find_by_name(ident)?; + Some(MacroDefId { + krate, + ast_id: Some(ast_id), + kind: MacroDefKind::BuiltInDerive(expander), + local_inner: false, + }) +} + struct BasicAdtInfo { name: tt::Ident, type_params: usize, @@ -261,7 +274,7 @@ mod tests { use super::*; fn expand_builtin_derive(s: &str, name: Name) -> String { - let def = find_builtin_derive(&name).unwrap(); + let expander = BuiltinDeriveExpander::find_by_name(&name).unwrap(); let fixture = format!( r#"//- /main.rs crate:main deps:core <|> @@ -283,7 +296,12 @@ mod tests { let attr_id = AstId::new(file_id.into(), ast_id_map.ast_id(&items[0])); let loc = MacroCallLoc { - def, + def: MacroDefId { + krate: CrateId(0), + ast_id: None, + kind: MacroDefKind::BuiltInDerive(expander), + local_inner: false, + }, krate: CrateId(0), kind: MacroCallKind::Attr(attr_id, name.to_string()), }; diff --git a/crates/hir_expand/src/builtin_macro.rs b/crates/hir_expand/src/builtin_macro.rs index df82cf8e65f..dddbbcdac48 100644 --- a/crates/hir_expand/src/builtin_macro.rs +++ b/crates/hir_expand/src/builtin_macro.rs @@ -69,13 +69,13 @@ pub fn find_builtin_macro( match kind { Either::Left(kind) => Some(MacroDefId { - krate: Some(krate), + krate, ast_id: Some(ast_id), kind: MacroDefKind::BuiltIn(kind), local_inner: false, }), Either::Right(kind) => Some(MacroDefId { - krate: Some(krate), + krate, ast_id: Some(ast_id), kind: MacroDefKind::BuiltInEager(kind), local_inner: false, @@ -534,7 +534,7 @@ mod tests { Either::Left(expander) => { // the first one should be a macro_rules let def = MacroDefId { - krate: Some(CrateId(0)), + krate: CrateId(0), ast_id: Some(AstId::new(file_id.into(), ast_id_map.ast_id(¯o_rules))), kind: MacroDefKind::BuiltIn(expander), local_inner: false, @@ -555,7 +555,7 @@ mod tests { Either::Right(expander) => { // the first one should be a macro_rules let def = MacroDefId { - krate: Some(krate), + krate, ast_id: Some(AstId::new(file_id.into(), ast_id_map.ast_id(¯o_rules))), kind: MacroDefKind::BuiltInEager(expander), local_inner: false, diff --git a/crates/hir_expand/src/hygiene.rs b/crates/hir_expand/src/hygiene.rs index 5d3fa0518f4..7ab0a5e52eb 100644 --- a/crates/hir_expand/src/hygiene.rs +++ b/crates/hir_expand/src/hygiene.rs @@ -29,8 +29,8 @@ impl Hygiene { MacroCallId::LazyMacro(id) => { let loc = db.lookup_intern_macro(id); match loc.def.kind { - MacroDefKind::Declarative => (loc.def.krate, loc.def.local_inner), - MacroDefKind::BuiltIn(_) => (loc.def.krate, false), + MacroDefKind::Declarative => (Some(loc.def.krate), loc.def.local_inner), + MacroDefKind::BuiltIn(_) => (Some(loc.def.krate), false), MacroDefKind::BuiltInDerive(_) => (None, false), MacroDefKind::BuiltInEager(_) => (None, false), MacroDefKind::ProcMacro(_) => (None, false), diff --git a/crates/hir_expand/src/lib.rs b/crates/hir_expand/src/lib.rs index 55f026c7be9..d486186e59d 100644 --- a/crates/hir_expand/src/lib.rs +++ b/crates/hir_expand/src/lib.rs @@ -224,13 +224,7 @@ impl From for MacroCallId { #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct MacroDefId { - // FIXME: krate and ast_id are currently optional because we don't have a - // definition location for built-in derives. There is one, though: the - // standard library defines them. The problem is that it uses the new - // `macro` syntax for this, which we don't support yet. As soon as we do - // (which will probably require touching this code), we can instead use - // that (and also remove the hacks for resolving built-in derives). - pub krate: Option, + pub krate: CrateId, pub ast_id: Option>, pub kind: MacroDefKind, diff --git a/crates/hir_ty/src/tests/macros.rs b/crates/hir_ty/src/tests/macros.rs index de97ec3c206..a7656b86486 100644 --- a/crates/hir_ty/src/tests/macros.rs +++ b/crates/hir_ty/src/tests/macros.rs @@ -686,6 +686,8 @@ mod clone { trait Clone { fn clone(&self) -> Self; } + #[rustc_builtin_macro] + macro Clone {} } "#, ); @@ -702,6 +704,8 @@ mod clone { trait Clone { fn clone(&self) -> Self; } + #[rustc_builtin_macro] + macro Clone {} } #[derive(Clone)] pub struct S; @@ -737,6 +741,8 @@ mod clone { trait Clone { fn clone(&self) -> Self; } + #[rustc_builtin_macro] + macro Clone {} } "#, ); diff --git a/crates/ide/src/goto_implementation.rs b/crates/ide/src/goto_implementation.rs index 529004878d3..68c628d3143 100644 --- a/crates/ide/src/goto_implementation.rs +++ b/crates/ide/src/goto_implementation.rs @@ -221,6 +221,8 @@ struct Foo<|>; mod marker { trait Copy {} } +#[rustc_builtin_macro] +macro Copy {} "#, ); } diff --git a/crates/ide/src/syntax_highlighting/test_data/highlighting.html b/crates/ide/src/syntax_highlighting/test_data/highlighting.html index 0569cf1e5a7..3530a5fdb4b 100644 --- a/crates/ide/src/syntax_highlighting/test_data/highlighting.html +++ b/crates/ide/src/syntax_highlighting/test_data/highlighting.html @@ -38,6 +38,9 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd 
use inner::{self as inner_mod};
 mod inner {}
 
+#[rustc_builtin_macro]
+macro Copy {}
+
 // Needed for function consuming vs normal
 pub mod marker {
     #[lang = "copy"]
@@ -119,7 +122,7 @@ pre                 { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd
     f()
 }
 
-fn foobar() -> impl Copy {}
+fn foobar() -> impl Copy {}
 
 fn foo() {
     let bar = foobar();
diff --git a/crates/ide/src/syntax_highlighting/tests.rs b/crates/ide/src/syntax_highlighting/tests.rs
index 1dc018a167b..f53d2c3ba41 100644
--- a/crates/ide/src/syntax_highlighting/tests.rs
+++ b/crates/ide/src/syntax_highlighting/tests.rs
@@ -12,6 +12,9 @@ fn test_highlighting() {
 use inner::{self as inner_mod};
 mod inner {}
 
+#[rustc_builtin_macro]
+macro Copy {}
+
 // Needed for function consuming vs normal
 pub mod marker {
     #[lang = "copy"]