Merge #1386

1386: Remove one of the two different algorithms for name resolution of macros :D r=edwin0cheng a=matklad

r? @edwin0cheng 

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

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) {
+                if let Some(path) = e.path().and_then(Path::from_ast) {
-                    let call_id = MacroCallLoc { def, ast_id }.id(self.db);
+                    if let Some(def) = self.resolver.resolve_path_as_macro(self.db, &path) {
-                    let file_id = call_id.as_file(MacroFileKind::Expr);
+                        let call_id = MacroCallLoc { def: def.id, ast_id }.id(self.db);
-                    if let Some(node) = self.db.parse_or_expand(file_id) {
+                        let file_id = call_id.as_file(MacroFileKind::Expr);
-                        if let Some(expr) = ast::Expr::cast(&*node) {
+                        if let Some(node) = self.db.parse_or_expand(file_id) {
-                            log::debug!("macro expansion {}", expr.syntax().debug_dump());
+                            if let Some(expr) = ast::Expr::cast(&*node) {
-                            let old_file_id = std::mem::replace(&mut self.current_file_id, file_id);
+                                log::debug!("macro expansion {}", expr.syntax().debug_dump());
-                            let id = self.collect_expr(&expr);
+                                let old_file_id =
-                            self.current_file_id = old_file_id;
+                                    std::mem::replace(&mut self.current_file_id, file_id);
-                            return id;
+                                let id = self.collect_expr(&expr);
+                                self.current_file_id = old_file_id;
+                                return id;
+                            }
                         }
                     }
                 }

crates/ra_hir/src/nameres.rs

@@ -92,7 +92,6 @@ pub struct CrateDefMap {
     extern_prelude: FxHashMap<Name, ModuleDef>,
     root: CrateModuleId,
     modules: Arena<CrateModuleId, ModuleData>,
-    public_macros: FxHashMap<Name, MacroDefId>,
 
     /// 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<MacroDefId>,
 
-    local_macros: FxHashMap<Name, MacroDefId>,
     diagnostics: Vec<DefDiagnostic>,
 }
 
@@ -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<MacroDefId> {
-        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);

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(
-        if export {
+        &mut self,
-            self.def_map.public_macros.insert(name.clone(), macro_id);
+        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 });
 
-            let def = Either::Right(MacroDef { id: macro_id });
+        // In Rust, `#[macro_export]` macros are unconditionally visible at the
-            self.update(self.def_map.root, None, &[(name.clone(), def)]);
+        // crate root, even if the parent modules is **not** visible.
-        } else {
+        if export {
-            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(),
             }
         };

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
+    assert_snapshot_matches!(map, @r###"
-Foo: t v
+   ⋮crate
-bar: m
+   ⋮Foo: t v
-foo: m"###);
+   ⋮bar: m
+   ⋮baz: m
+   ⋮foo: m
+    "###);
 }

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<Path>,
+        path: &Path,
-    ) -> Option<MacroDefId> {
+    ) -> Option<MacroDef> {
-        let m = self.module()?;
+        let (item_map, module) = self.module()?;
-        m.0.find_macro(db, m.1, &path?)
+        match item_map.resolve_path_with_macro(db, module, path) {
+            (Either::Right(macro_def), None) => Some(macro_def),
+            _ => None,
+        }
     }
 
     /// Returns the resolved path segments
@@ -165,7 +168,11 @@ impl Resolver {
 
     /// Returns the fully resolved path if we were able to resolve it.
     /// otherwise returns `PerNs::none`
-    pub(crate) fn resolve_path(&self, db: &impl HirDatabase, path: &Path) -> PerNs<Resolution> {
+    pub(crate) fn resolve_path_without_assoc_items(
+        &self,
+        db: &impl HirDatabase,
+        path: &Path,
+    ) -> PerNs<Resolution> {
         // into_fully_resolved() returns the fully resolved path or PerNs::none() otherwise
         self.resolve_path_segments(db, path).into_fully_resolved()
     }

crates/ra_hir/src/source_binder.rs

@@ -267,9 +267,8 @@ impl SourceAnalyzer {
         db: &impl HirDatabase,
         macro_call: &ast::MacroCall,
     ) -> Option<MacroDef> {
-        let id =
+        let path = macro_call.path().and_then(Path::from_ast)?;
-            self.resolver.resolve_macro_call(db, macro_call.path().and_then(Path::from_ast))?;
+        self.resolver.resolve_path_as_macro(db, &path)
-        Some(MacroDef { id })
     }
 
     pub fn resolve_hir_path(
@@ -277,7 +276,7 @@ impl SourceAnalyzer {
         db: &impl HirDatabase,
         path: &crate::Path,
     ) -> PerNs<crate::Resolution> {
-        self.resolver.resolve_path(db, path)
+        self.resolver.resolve_path_without_assoc_items(db, path)
     }
 
     pub fn resolve_path(&self, db: &impl HirDatabase, path: &ast::Path) -> Option<PathResolution> {
@@ -294,7 +293,7 @@ impl SourceAnalyzer {
             }
         }
         let hir_path = crate::Path::from_ast(path)?;
-        let res = self.resolver.resolve_path(db, &hir_path);
+        let res = self.resolver.resolve_path_without_assoc_items(db, &hir_path);
         let res = res.clone().take_types().or_else(|| res.take_values())?;
         let res = match res {
             crate::Resolution::Def(it) => PathResolution::Def(it),

crates/ra_hir/src/ty/infer.rs

@@ -610,23 +610,26 @@ impl<'a, D: HirDatabase> InferenceContext<'a, D> {
             None => return (Ty::Unknown, None),
         };
         let resolver = &self.resolver;
-        let typable: Option<TypableDef> = match resolver.resolve_path(self.db, &path).take_types() {
+        let typable: Option<TypableDef> =
-            Some(Resolution::Def(def)) => def.into(),
+            // FIXME: this should resolve assoc items as well, see this example:
-            Some(Resolution::LocalBinding(..)) => {
+            // https://play.rust-lang.org/?gist=087992e9e22495446c01c0d4e2d69521
-                // this cannot happen
+            match resolver.resolve_path_without_assoc_items(self.db, &path).take_types() {
-                log::error!("path resolved to local binding in type ns");
+                Some(Resolution::Def(def)) => def.into(),
-                return (Ty::Unknown, None);
+                Some(Resolution::LocalBinding(..)) => {
-            }
+                    // this cannot happen
-            Some(Resolution::GenericParam(..)) => {
+                    log::error!("path resolved to local binding in type ns");
-                // generic params can't be used in struct literals
+                    return (Ty::Unknown, None);
-                return (Ty::Unknown, None);
+                }
-            }
+                Some(Resolution::GenericParam(..)) => {
-            Some(Resolution::SelfType(..)) => {
+                    // generic params can't be used in struct literals
-                // FIXME this is allowed in an impl for a struct, handle this
+                    return (Ty::Unknown, None);
-                return (Ty::Unknown, None);
+                }
-            }
+                Some(Resolution::SelfType(..)) => {
-            None => return (Ty::Unknown, None),
+                    // FIXME this is allowed in an impl for a struct, handle this
-        };
+                    return (Ty::Unknown, None);
+                }
+                None => return (Ty::Unknown, None),
+            };
         let def = match typable {
             None => return (Ty::Unknown, None),
             Some(it) => it,

crates/ra_hir/src/ty/lower.rs

@@ -65,7 +65,7 @@ impl Ty {
 
     pub(crate) fn from_hir_path(db: &impl HirDatabase, resolver: &Resolver, path: &Path) -> Self {
         // Resolve the path (in type namespace)
-        let resolution = resolver.resolve_path(db, path).take_types();
+        let resolution = resolver.resolve_path_without_assoc_items(db, path).take_types();
 
         let def = match resolution {
             Some(Resolution::Def(def)) => def,
@@ -216,7 +216,7 @@ impl TraitRef {
         path: &Path,
         explicit_self_ty: Option<Ty>,
     ) -> Option<Self> {
-        let resolved = match resolver.resolve_path(db, &path).take_types()? {
+        let resolved = match resolver.resolve_path_without_assoc_items(db, &path).take_types()? {
             Resolution::Def(ModuleDef::Trait(tr)) => tr,
             _ => return None,
         };

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
+    "###
     );
 }