Avoid cyclic queries in name resolution when processing enums

3 files changed, 48 insertions, 15 deletions

diff --git a/crates/ra_hir_def/src/adt.rs b/crates/ra_hir_def/src/adt.rs
index c7c3c0bc7..7c0d93691 100644
--- a/crates/ra_hir_def/src/adt.rs
+++ b/crates/ra_hir_def/src/adt.rs
@@ -12,7 +12,7 @@ use ra_prof::profile;
 use ra_syntax::ast::{self, NameOwner, TypeAscriptionOwner, VisibilityOwner};
 
 use crate::{
-    body::Expander, db::DefDatabase, src::HasChildSource, src::HasSource, trace::Trace,
+    body::CfgExpander, db::DefDatabase, src::HasChildSource, src::HasSource, trace::Trace,
     type_ref::TypeRef, visibility::RawVisibility, EnumId, HasModule, LocalEnumVariantId,
     LocalStructFieldId, Lookup, ModuleId, StructId, UnionId, VariantId,
 };
@@ -124,7 +124,7 @@ fn lower_enum(
 
 impl VariantData {
     fn new(db: &dyn DefDatabase, flavor: InFile<ast::StructKind>, module_id: ModuleId) -> Self {
-        let mut expander = Expander::new(db, flavor.file_id, module_id);
+        let mut expander = CfgExpander::new(db, flavor.file_id, module_id.krate);
         let mut trace = Trace::new_for_arena();
         match lower_struct(db, &mut expander, &mut trace, &flavor) {
             StructKind::Tuple => VariantData::Tuple(trace.into_arena()),
@@ -178,7 +178,7 @@ impl HasChildSource for VariantId {
                 it.lookup(db).container.module(db),
             ),
         };
-        let mut expander = Expander::new(db, src.file_id, module_id);
+        let mut expander = CfgExpander::new(db, src.file_id, module_id.krate);
         let mut trace = Trace::new_for_map();
         lower_struct(db, &mut expander, &mut trace, &src);
         src.with_value(trace.into_map())
@@ -194,7 +194,7 @@ pub enum StructKind {
 
 fn lower_struct(
     db: &dyn DefDatabase,
-    expander: &mut Expander,
+    expander: &mut CfgExpander,
     trace: &mut Trace<StructFieldData, Either<ast::TupleFieldDef, ast::RecordFieldDef>>,
     ast: &InFile<ast::StructKind>,
 ) -> StructKind {
diff --git a/crates/ra_hir_def/src/body.rs b/crates/ra_hir_def/src/body.rs
index 7fe5403c0..7fac6ce66 100644
--- a/crates/ra_hir_def/src/body.rs
+++ b/crates/ra_hir_def/src/body.rs
@@ -25,32 +25,57 @@ use crate::{
     AsMacroCall, DefWithBodyId, HasModule, Lookup, ModuleId,
 };
 use ra_cfg::CfgOptions;
+use ra_db::CrateId;
+
+/// A subser of Exander that only deals with cfg attributes. We only need it to
+/// avoid cyclic queries in crate def map during enum processing.
+pub(crate) struct CfgExpander {
+    cfg_options: CfgOptions,
+    hygiene: Hygiene,
+}
 
 pub(crate) struct Expander {
+    cfg_expander: CfgExpander,
     crate_def_map: Arc<CrateDefMap>,
-    cfg_options: CfgOptions,
     current_file_id: HirFileId,
-    hygiene: Hygiene,
     ast_id_map: Arc<AstIdMap>,
     module: ModuleId,
     recursive_limit: usize,
 }
 
+impl CfgExpander {
+    pub(crate) fn new(
+        db: &dyn DefDatabase,
+        current_file_id: HirFileId,
+        krate: CrateId,
+    ) -> CfgExpander {
+        let hygiene = Hygiene::new(db.upcast(), current_file_id);
+        let cfg_options = db.crate_graph()[krate].cfg_options.clone();
+        CfgExpander { cfg_options, hygiene }
+    }
+
+    pub(crate) fn parse_attrs(&self, owner: &dyn ast::AttrsOwner) -> Attrs {
+        Attrs::new(owner, &self.hygiene)
+    }
+
+    pub(crate) fn is_cfg_enabled(&self, attrs: &Attrs) -> bool {
+        attrs.is_cfg_enabled(&self.cfg_options)
+    }
+}
+
 impl Expander {
     pub(crate) fn new(
         db: &dyn DefDatabase,
         current_file_id: HirFileId,
         module: ModuleId,
     ) -> Expander {
+        let cfg_expander = CfgExpander::new(db, current_file_id, module.krate);
         let crate_def_map = db.crate_def_map(module.krate);
-        let hygiene = Hygiene::new(db.upcast(), current_file_id);
         let ast_id_map = db.ast_id_map(current_file_id);
-        let cfg_options = db.crate_graph()[module.krate].cfg_options.clone();
         Expander {
+            cfg_expander,
             crate_def_map,
-            cfg_options,
             current_file_id,
-            hygiene,
             ast_id_map,
             module,
             recursive_limit: 0,
@@ -87,7 +112,7 @@ impl Expander {
                         ast_id_map: mem::take(&mut self.ast_id_map),
                         bomb: DropBomb::new("expansion mark dropped"),
                     };
-                    self.hygiene = Hygiene::new(db.upcast(), file_id);
+                    self.cfg_expander.hygiene = Hygiene::new(db.upcast(), file_id);
                     self.current_file_id = file_id;
                     self.ast_id_map = db.ast_id_map(file_id);
                     self.recursive_limit += 1;
@@ -103,7 +128,7 @@ impl Expander {
     }
 
     pub(crate) fn exit(&mut self, db: &dyn DefDatabase, mut mark: Mark) {
-        self.hygiene = Hygiene::new(db.upcast(), mark.file_id);
+        self.cfg_expander.hygiene = Hygiene::new(db.upcast(), mark.file_id);
         self.current_file_id = mark.file_id;
         self.ast_id_map = mem::take(&mut mark.ast_id_map);
         self.recursive_limit -= 1;
@@ -115,15 +140,15 @@ impl Expander {
     }
 
     pub(crate) fn parse_attrs(&self, owner: &dyn ast::AttrsOwner) -> Attrs {
-        Attrs::new(owner, &self.hygiene)
+        self.cfg_expander.parse_attrs(owner)
     }
 
     pub(crate) fn is_cfg_enabled(&self, attrs: &Attrs) -> bool {
-        attrs.is_cfg_enabled(&self.cfg_options)
+        self.cfg_expander.is_cfg_enabled(attrs)
     }
 
     fn parse_path(&mut self, path: ast::Path) -> Option<Path> {
-        Path::from_src(path, &self.hygiene)
+        Path::from_src(path, &self.cfg_expander.hygiene)
     }
 
     fn resolve_path_as_macro(&self, db: &dyn DefDatabase, path: &ModPath) -> Option<MacroDefId> {
diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs
index 1c33a6520..98c74fe25 100644
--- a/crates/ra_hir_def/src/nameres/collector.rs
+++ b/crates/ra_hir_def/src/nameres/collector.rs
@@ -462,6 +462,14 @@ impl DefCollector<'_> {
                 Some(ModuleDefId::AdtId(AdtId::EnumId(e))) => {
                     tested_by!(glob_enum);
                     // glob import from enum => just import all the variants
+
+                    // XXX: urgh, so this works by accident! Here, we look at
+                    // the enum data, and, in theory, this might require us to
+                    // look back at the crate_def_map, creating a cycle. For
+                    // example, `enum E { crate::some_macro!(); }`. Luckely, the
+                    // only kind of macro that is allowed inside enum is a
+                    // `cfg_macro`, and we don't need to run name resolution for
+                    // it, but this is sheer luck!
                     let enum_data = self.db.enum_data(e);
                     let resolutions = enum_data
                         .variants