From caefa6982bc57195687de11137997f1d62d791fe Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 8 Jun 2019 20:42:02 +0300 Subject: remove some hacks from nameresolution for macros --- crates/ra_hir/src/expr.rs | 26 ++++++++++----------- crates/ra_hir/src/nameres.rs | 29 ++--------------------- crates/ra_hir/src/nameres/collector.rs | 38 +++++++++++++++++++++++-------- crates/ra_hir/src/nameres/tests/macros.rs | 13 +++++++---- crates/ra_hir/src/resolve.rs | 17 ++++++++------ crates/ra_hir/src/source_binder.rs | 5 ++-- crates/ra_hir/src/ty/tests.rs | 6 +++-- 7 files changed, 68 insertions(+), 66 deletions(-) (limited to 'crates/ra_hir') diff --git a/crates/ra_hir/src/expr.rs b/crates/ra_hir/src/expr.rs index 9d9769859..012f374ec 100644 --- a/crates/ra_hir/src/expr.rs +++ b/crates/ra_hir/src/expr.rs @@ -827,25 +827,25 @@ where ast::ExprKind::IndexExpr(_e) => self.alloc_expr(Expr::Missing, syntax_ptr), ast::ExprKind::RangeExpr(_e) => self.alloc_expr(Expr::Missing, syntax_ptr), ast::ExprKind::MacroCall(e) => { - // very hacky.FIXME change to use the macro resolution - let path = e.path().and_then(Path::from_ast); - let ast_id = self .db .ast_id_map(self.current_file_id) .ast_id(e) .with_file_id(self.current_file_id); - if let Some(def) = self.resolver.resolve_macro_call(self.db, path) { - let call_id = MacroCallLoc { def, ast_id }.id(self.db); - let file_id = call_id.as_file(MacroFileKind::Expr); - if let Some(node) = self.db.parse_or_expand(file_id) { - if let Some(expr) = ast::Expr::cast(&*node) { - log::debug!("macro expansion {}", expr.syntax().debug_dump()); - let old_file_id = std::mem::replace(&mut self.current_file_id, file_id); - let id = self.collect_expr(&expr); - self.current_file_id = old_file_id; - return id; + if let Some(path) = e.path().and_then(Path::from_ast) { + if let Some(def) = self.resolver.resolve_path_as_macro(self.db, &path) { + let call_id = MacroCallLoc { def: def.id, ast_id }.id(self.db); + let file_id = call_id.as_file(MacroFileKind::Expr); + if let Some(node) = self.db.parse_or_expand(file_id) { + if let Some(expr) = ast::Expr::cast(&*node) { + log::debug!("macro expansion {}", expr.syntax().debug_dump()); + let old_file_id = + std::mem::replace(&mut self.current_file_id, file_id); + let id = self.collect_expr(&expr); + self.current_file_id = old_file_id; + return id; + } } } } diff --git a/crates/ra_hir/src/nameres.rs b/crates/ra_hir/src/nameres.rs index dc0dd23c9..b5938fa03 100644 --- a/crates/ra_hir/src/nameres.rs +++ b/crates/ra_hir/src/nameres.rs @@ -92,7 +92,6 @@ pub struct CrateDefMap { extern_prelude: FxHashMap, root: CrateModuleId, modules: Arena, - public_macros: FxHashMap, /// Some macros are not well-behavior, which leads to infinite loop /// e.g. macro_rules! foo { ($ty:ty) => { foo!($ty); } } @@ -106,7 +105,6 @@ pub struct CrateDefMap { /// However, do we want to put it as a global variable? poison_macros: FxHashSet, - local_macros: FxHashMap, diagnostics: Vec, } @@ -249,9 +247,7 @@ impl CrateDefMap { prelude: None, root, modules, - public_macros: FxHashMap::default(), poison_macros: FxHashSet::default(), - local_macros: FxHashMap::default(), diagnostics: Vec::new(), } }; @@ -313,7 +309,7 @@ impl CrateDefMap { (res.resolved_def.left().unwrap_or_else(PerNs::none), res.segment_index) } - fn resolve_path_with_macro( + pub(crate) fn resolve_path_with_macro( &self, db: &impl DefDatabase, original_module: CrateModuleId, @@ -323,27 +319,6 @@ impl CrateDefMap { (res.resolved_def, res.segment_index) } - // FIXME: This seems to do the same work as `resolve_path_with_macro`, but - // using a completely different code path. Seems bad, huh? - pub(crate) fn find_macro( - &self, - db: &impl DefDatabase, - original_module: CrateModuleId, - path: &Path, - ) -> Option { - let name = path.expand_macro_expr()?; - // search local first - // FIXME: Remove public_macros check when we have a correct local_macors implementation - let local = - self.public_macros.get(&name).or_else(|| self.local_macros.get(&name)).map(|it| *it); - if local.is_some() { - return local; - } - - let res = self.resolve_path_fp_with_macro(db, ResolveMode::Other, original_module, path); - res.resolved_def.right().map(|m| m.id) - } - // Returns Yes if we are sure that additions to `ItemMap` wouldn't change // the result. fn resolve_path_fp_with_macro( @@ -511,7 +486,7 @@ impl CrateDefMap { let from_scope = self[module] .scope .get_item_or_macro(name) - .unwrap_or_else(|| Either::Left(PerNs::none()));; + .unwrap_or_else(|| Either::Left(PerNs::none())); let from_extern_prelude = self.extern_prelude.get(name).map_or(PerNs::none(), |&it| PerNs::types(it)); let from_prelude = self.resolve_in_prelude(db, name); diff --git a/crates/ra_hir/src/nameres/collector.rs b/crates/ra_hir/src/nameres/collector.rs index 3bfef799d..99110d58d 100644 --- a/crates/ra_hir/src/nameres/collector.rs +++ b/crates/ra_hir/src/nameres/collector.rs @@ -138,15 +138,35 @@ where } } - fn define_macro(&mut self, name: Name, macro_id: MacroDefId, export: bool) { + fn define_macro( + &mut self, + module_id: CrateModuleId, + name: Name, + macro_id: MacroDefId, + export: bool, + ) { + // macro-by-example in Rust have completely weird name resolution logic, + // unlike anything else in the language. We'd don't fully implement yet, + // just give a somewhat precise approximation. + // + // Specifically, we store a set of visible macros in each module, just + // like how we do with usual items. This is wrong, however, because + // macros can be shadowed and their scopes are mostly unrelated to + // modules. To paper over the second problem, we also maintain + // `global_macro_scope` which works when we construct `CrateDefMap`, but + // is completely ignored in expressions. + // + // What we should do is that, in CrateDefMap, we should maintain a + // separate tower of macro scopes, with ids. Then, for each item in the + // module, we need to store it's macro scope. + let def = Either::Right(MacroDef { id: macro_id }); + + // In Rust, `#[macro_export]` macros are unconditionally visible at the + // crate root, even if the parent modules is **not** visible. if export { - self.def_map.public_macros.insert(name.clone(), macro_id); - - let def = Either::Right(MacroDef { id: macro_id }); - self.update(self.def_map.root, None, &[(name.clone(), def)]); - } else { - self.def_map.local_macros.insert(name.clone(), macro_id); + self.update(self.def_map.root, None, &[(name.clone(), def.clone())]); } + self.update(module_id, None, &[(name.clone(), def)]); self.global_macro_scope.insert(name, macro_id); } @@ -589,7 +609,7 @@ where if is_macro_rules(&mac.path) { if let Some(name) = &mac.name { let macro_id = MacroDefId(mac.ast_id.with_file_id(self.file_id)); - self.def_collector.define_macro(name.clone(), macro_id, mac.export) + self.def_collector.define_macro(self.module_id, name.clone(), macro_id, mac.export) } return; } @@ -694,9 +714,7 @@ mod tests { prelude: None, root, modules, - public_macros: FxHashMap::default(), poison_macros: FxHashSet::default(), - local_macros: FxHashMap::default(), diagnostics: Vec::new(), } }; diff --git a/crates/ra_hir/src/nameres/tests/macros.rs b/crates/ra_hir/src/nameres/tests/macros.rs index 42241aeff..4e04740eb 100644 --- a/crates/ra_hir/src/nameres/tests/macros.rs +++ b/crates/ra_hir/src/nameres/tests/macros.rs @@ -21,6 +21,7 @@ fn macro_rules_are_globally_visible() { ⋮crate ⋮Foo: t v ⋮nested: t + ⋮structs: m ⋮ ⋮crate::nested ⋮Bar: t v @@ -46,6 +47,7 @@ fn macro_rules_can_define_modules() { ); assert_snapshot_matches!(map, @r###" ⋮crate + ⋮m: m ⋮n1: t ⋮ ⋮crate::n1 @@ -127,8 +129,11 @@ fn unexpanded_macro_should_expand_by_fixedpoint_loop() { "foo": ("/lib.rs", []), }, ); - assert_snapshot_matches!(map, @r###"crate -Foo: t v -bar: m -foo: m"###); + assert_snapshot_matches!(map, @r###" + ⋮crate + ⋮Foo: t v + ⋮bar: m + ⋮baz: m + ⋮foo: m + "###); } diff --git a/crates/ra_hir/src/resolve.rs b/crates/ra_hir/src/resolve.rs index 347bcf27d..1b987c1b6 100644 --- a/crates/ra_hir/src/resolve.rs +++ b/crates/ra_hir/src/resolve.rs @@ -2,11 +2,11 @@ use std::sync::Arc; use rustc_hash::{FxHashMap, FxHashSet}; +use either::Either; use crate::{ - ModuleDef, Trait, + ModuleDef, Trait, MacroDef, code_model::Crate, - MacroDefId, db::HirDatabase, name::{Name, KnownName}, nameres::{PerNs, CrateDefMap, CrateModuleId}, @@ -130,13 +130,16 @@ impl Resolver { resolution } - pub(crate) fn resolve_macro_call( + pub(crate) fn resolve_path_as_macro( &self, db: &impl HirDatabase, - path: Option, - ) -> Option { - let m = self.module()?; - m.0.find_macro(db, m.1, &path?) + path: &Path, + ) -> Option { + let (item_map, module) = self.module()?; + match item_map.resolve_path_with_macro(db, module, path) { + (Either::Right(macro_def), None) => Some(macro_def), + _ => None, + } } /// Returns the resolved path segments diff --git a/crates/ra_hir/src/source_binder.rs b/crates/ra_hir/src/source_binder.rs index 63ec59314..876ebe0e3 100644 --- a/crates/ra_hir/src/source_binder.rs +++ b/crates/ra_hir/src/source_binder.rs @@ -267,9 +267,8 @@ impl SourceAnalyzer { db: &impl HirDatabase, macro_call: &ast::MacroCall, ) -> Option { - let id = - self.resolver.resolve_macro_call(db, macro_call.path().and_then(Path::from_ast))?; - Some(MacroDef { id }) + let path = macro_call.path().and_then(Path::from_ast)?; + self.resolver.resolve_path_as_macro(db, &path) } pub fn resolve_hir_path( diff --git a/crates/ra_hir/src/ty/tests.rs b/crates/ra_hir/src/ty/tests.rs index c34e89af7..54b2a8c16 100644 --- a/crates/ra_hir/src/ty/tests.rs +++ b/crates/ra_hir/src/ty/tests.rs @@ -2481,8 +2481,10 @@ fn main() { } "#), @r###" -[156; 182) '{ ...,2); }': () -[166; 167) 'x': Foo"### + ⋮ + ⋮[156; 182) '{ ...,2); }': () + ⋮[166; 167) 'x': Foo + "### ); } -- cgit v1.2.3