Merge #9582

9582: internal: remove erroneous default impl r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <[email protected]>

Author: bors[bot]

crates/hir_def/src/nameres.rs

@@ -145,12 +145,6 @@ pub enum ModuleOrigin {
     },
 }
 
-impl Default for ModuleOrigin {
-    fn default() -> Self {
-        ModuleOrigin::CrateRoot { definition: FileId(0) }
-    }
-}
-
 impl ModuleOrigin {
     fn declaration(&self) -> Option<AstId<ast::Module>> {
         match self {
@@ -196,7 +190,7 @@ impl ModuleOrigin {
     }
 }
 
-#[derive(Default, Debug, PartialEq, Eq)]
+#[derive(Debug, PartialEq, Eq)]
 pub struct ModuleData {
     pub parent: Option<LocalModuleId>,
     pub children: FxHashMap<Name, LocalModuleId>,
@@ -211,9 +205,14 @@ impl DefMap {
         let _p = profile::span("crate_def_map_query").detail(|| {
             db.crate_graph()[krate].display_name.as_deref().unwrap_or_default().to_string()
         });
-        let edition = db.crate_graph()[krate].edition;
-        let def_map = DefMap::empty(krate, edition);
+
+        let crate_graph = db.crate_graph();
+
+        let edition = crate_graph[krate].edition;
+        let origin = ModuleOrigin::CrateRoot { definition: crate_graph[krate].root_file_id };
+        let def_map = DefMap::empty(krate, edition, origin);
         let def_map = collector::collect_defs(db, def_map, None);
+
         Arc::new(def_map)
     }
 
@@ -231,16 +230,20 @@ impl DefMap {
         let block_info = BlockInfo { block: block_id, parent: block.module };
 
         let parent_map = block.module.def_map(db);
-        let mut def_map = DefMap::empty(block.module.krate, parent_map.edition);
+        let mut def_map = DefMap::empty(
+            block.module.krate,
+            parent_map.edition,
+            ModuleOrigin::BlockExpr { block: block.ast_id },
+        );
         def_map.block = Some(block_info);
 
         let def_map = collector::collect_defs(db, def_map, Some(block.ast_id));
         Some(Arc::new(def_map))
     }
 
-    fn empty(krate: CrateId, edition: Edition) -> DefMap {
+    fn empty(krate: CrateId, edition: Edition, root_module_origin: ModuleOrigin) -> DefMap {
         let mut modules: Arena<ModuleData> = Arena::default();
-        let root = modules.alloc(ModuleData::default());
+        let root = modules.alloc(ModuleData::new(root_module_origin));
         DefMap {
             _c: Count::new(),
             block: None,
@@ -445,6 +448,15 @@ impl DefMap {
 }
 
 impl ModuleData {
+    pub(crate) fn new(origin: ModuleOrigin) -> Self {
+        ModuleData {
+            parent: None,
+            children: FxHashMap::default(),
+            scope: ItemScope::default(),
+            origin,
+        }
+    }
+
     /// Returns a node which defines this module. That is, a file or a `mod foo {}` with items.
     pub fn definition_source(&self, db: &dyn DefDatabase) -> InFile<ModuleSource> {
         self.origin.definition_source(db)

crates/hir_def/src/nameres/collector.rs

@@ -272,7 +272,6 @@ impl DefCollector<'_> {
         let file_id = self.db.crate_graph()[self.def_map.krate].root_file_id;
         let item_tree = self.db.file_item_tree(file_id.into());
         let module_id = self.def_map.root;
-        self.def_map.modules[module_id].origin = ModuleOrigin::CrateRoot { definition: file_id };
 
         let attrs = item_tree.top_level_attrs(self.db, self.def_map.krate);
         if attrs.cfg().map_or(true, |cfg| self.cfg_options.check(&cfg) != Some(false)) {
@@ -323,7 +322,6 @@ impl DefCollector<'_> {
     fn seed_with_inner(&mut self, block: AstId<ast::BlockExpr>) {
         let item_tree = self.db.file_item_tree(block.file_id);
         let module_id = self.def_map.root;
-        self.def_map.modules[module_id].origin = ModuleOrigin::BlockExpr { block };
         if item_tree
             .top_level_attrs(self.db, self.def_map.krate)
             .cfg()
@@ -1625,14 +1623,14 @@ impl ModCollector<'_, '_> {
             .resolve_visibility(self.def_collector.db, self.module_id, visibility)
             .unwrap_or(Visibility::Public);
         let modules = &mut self.def_collector.def_map.modules;
-        let res = modules.alloc(ModuleData::default());
-        modules[res].parent = Some(self.module_id);
-        modules[res].origin = match definition {
+        let origin = match definition {
             None => ModuleOrigin::Inline { definition: declaration },
             Some((definition, is_mod_rs)) => {
                 ModuleOrigin::File { declaration, definition, is_mod_rs }
             }
         };
+        let res = modules.alloc(ModuleData::new(origin));
+        modules[res].parent = Some(self.module_id);
         for (name, mac) in modules[self.module_id].scope.collect_legacy_macros() {
             modules[res].scope.define_legacy_macro(name, mac)
         }
@@ -2005,11 +2003,12 @@ mod tests {
     }
 
     fn do_resolve(not_ra_fixture: &str) -> DefMap {
-        let (db, _file_id) = TestDB::with_single_file(not_ra_fixture);
+        let (db, file_id) = TestDB::with_single_file(not_ra_fixture);
         let krate = db.test_crate();
 
         let edition = db.crate_graph()[krate].edition;
-        let def_map = DefMap::empty(krate, edition);
+        let module_origin = ModuleOrigin::CrateRoot { definition: file_id };
+        let def_map = DefMap::empty(krate, edition, module_origin);
         do_collect_defs(&db, def_map)
     }
 

docs/dev/style.md

@@ -312,7 +312,7 @@ Even when generality is not required, consistency is important.
 
 ## Constructors
 
-Prefer `Default` to zero-argument `new` function
+Prefer `Default` to zero-argument `new` function.
 
 ```rust
 // GOOD
@@ -341,6 +341,10 @@ Use `Vec::new` rather than `vec![]`.
 
 **Rationale:** uniformity, strength reduction.
 
+Avoid using "dummy" states to implement a `Default`.
+If a type doesn't have a sensible default, empty value, don't hide it.
+Let the caller explicitly decide what's the right initial state is.
+
 ## Functions Over Objects
 
 Avoid creating "doer" objects.