Rollup merge of #71116 - marmeladema:dummy-hir-id-removal, r=eddyb

Entirely remove `DUMMY_HIR_ID` Some helpers functions have been introduced to deal with (buggy) cases where either a `NodeId` or a `DefId` do not have a corresponding `HirId`. Those cases are tracked in issue #71104.
rust-lang · Apr 15, 2020 · 7341cad · 7341cad
2 parents 8da5952 + c15e13a
commit 7341cad
Show file tree

Hide file tree

Showing 15 changed files with 79 additions and 94 deletions.
diff --git a/src/librustc_ast_lowering/lib.rs b/src/librustc_ast_lowering/lib.rs
@@ -168,7 +168,7 @@ struct LoweringContext<'a, 'hir: 'a> {
 
     current_hir_id_owner: Vec<(LocalDefId, u32)>,
     item_local_id_counters: NodeMap<u32>,
-    node_id_to_hir_id: IndexVec<NodeId, hir::HirId>,
+    node_id_to_hir_id: IndexVec<NodeId, Option<hir::HirId>>,
 
     allow_try_trait: Option<Lrc<[Symbol]>>,
     allow_gen_future: Option<Lrc<[Symbol]>>,
@@ -522,15 +522,16 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         }
 
         self.lower_node_id(CRATE_NODE_ID);
-        debug_assert!(self.node_id_to_hir_id[CRATE_NODE_ID] == hir::CRATE_HIR_ID);
+        debug_assert!(self.node_id_to_hir_id[CRATE_NODE_ID] == Some(hir::CRATE_HIR_ID));
 
         visit::walk_crate(&mut MiscCollector { lctx: &mut self, hir_id_owner: None }, c);
         visit::walk_crate(&mut item::ItemLowerer { lctx: &mut self }, c);
 
         let module = self.lower_mod(&c.module);
         let attrs = self.lower_attrs(&c.attrs);
         let body_ids = body_ids(&self.bodies);
-        let proc_macros = c.proc_macros.iter().map(|id| self.node_id_to_hir_id[*id]).collect();
+        let proc_macros =
+            c.proc_macros.iter().map(|id| self.node_id_to_hir_id[*id].unwrap()).collect();
 
         self.resolver.definitions().init_node_id_to_hir_id_mapping(self.node_id_to_hir_id);
 
@@ -571,26 +572,22 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         ast_node_id: NodeId,
         alloc_hir_id: impl FnOnce(&mut Self) -> hir::HirId,
     ) -> hir::HirId {
-        if ast_node_id == DUMMY_NODE_ID {
-            return hir::DUMMY_HIR_ID;
-        }
+        assert_ne!(ast_node_id, DUMMY_NODE_ID);
 
         let min_size = ast_node_id.as_usize() + 1;
 
         if min_size > self.node_id_to_hir_id.len() {
-            self.node_id_to_hir_id.resize(min_size, hir::DUMMY_HIR_ID);
+            self.node_id_to_hir_id.resize(min_size, None);
         }
 
-        let existing_hir_id = self.node_id_to_hir_id[ast_node_id];
-
-        if existing_hir_id == hir::DUMMY_HIR_ID {
+        if let Some(existing_hir_id) = self.node_id_to_hir_id[ast_node_id] {
+            existing_hir_id
+        } else {
             // Generate a new `HirId`.
             let hir_id = alloc_hir_id(self);
-            self.node_id_to_hir_id[ast_node_id] = hir_id;
+            self.node_id_to_hir_id[ast_node_id] = Some(hir_id);
 
             hir_id
-        } else {
-            existing_hir_id
         }
     }
 

diff --git a/src/librustc_hir/definitions.rs b/src/librustc_hir/definitions.rs
@@ -7,7 +7,6 @@
 pub use crate::def_id::DefPathHash;
 use crate::def_id::{CrateNum, DefId, DefIndex, LocalDefId, CRATE_DEF_INDEX, LOCAL_CRATE};
 use crate::hir;
-use crate::hir_id::DUMMY_HIR_ID;
 
 use rustc_ast::ast;
 use rustc_ast::crate_disambiguator::CrateDisambiguator;
@@ -87,7 +86,7 @@ pub struct Definitions {
     node_id_to_def_id: FxHashMap<ast::NodeId, LocalDefId>,
     def_id_to_node_id: IndexVec<LocalDefId, ast::NodeId>,
 
-    pub(super) node_id_to_hir_id: IndexVec<ast::NodeId, hir::HirId>,
+    pub(super) node_id_to_hir_id: IndexVec<ast::NodeId, Option<hir::HirId>>,
     /// The reverse mapping of `node_id_to_hir_id`.
     pub(super) hir_id_to_node_id: FxHashMap<hir::HirId, ast::NodeId>,
 
@@ -345,8 +344,7 @@ impl Definitions {
     #[inline]
     pub fn as_local_hir_id(&self, def_id: DefId) -> Option<hir::HirId> {
         if let Some(def_id) = def_id.as_local() {
-            let hir_id = self.local_def_id_to_hir_id(def_id);
-            if hir_id != DUMMY_HIR_ID { Some(hir_id) } else { None }
+            Some(self.local_def_id_to_hir_id(def_id))
         } else {
             None
         }
@@ -359,11 +357,22 @@ impl Definitions {
 
     #[inline]
     pub fn node_id_to_hir_id(&self, node_id: ast::NodeId) -> hir::HirId {
+        self.node_id_to_hir_id[node_id].unwrap()
+    }
+
+    #[inline]
+    pub fn opt_node_id_to_hir_id(&self, node_id: ast::NodeId) -> Option<hir::HirId> {
         self.node_id_to_hir_id[node_id]
     }
 
     #[inline]
     pub fn local_def_id_to_hir_id(&self, id: LocalDefId) -> hir::HirId {
+        let node_id = self.def_id_to_node_id[id];
+        self.node_id_to_hir_id[node_id].unwrap()
+    }
+
+    #[inline]
+    pub fn opt_local_def_id_to_hir_id(&self, id: LocalDefId) -> Option<hir::HirId> {
         let node_id = self.def_id_to_node_id[id];
         self.node_id_to_hir_id[node_id]
     }
@@ -470,7 +479,10 @@ impl Definitions {
 
     /// Initializes the `ast::NodeId` to `HirId` mapping once it has been generated during
     /// AST to HIR lowering.
-    pub fn init_node_id_to_hir_id_mapping(&mut self, mapping: IndexVec<ast::NodeId, hir::HirId>) {
+    pub fn init_node_id_to_hir_id_mapping(
+        &mut self,
+        mapping: IndexVec<ast::NodeId, Option<hir::HirId>>,
+    ) {
         assert!(
             self.node_id_to_hir_id.is_empty(),
             "trying to initialize `NodeId` -> `HirId` mapping twice"
@@ -481,7 +493,7 @@ impl Definitions {
         self.hir_id_to_node_id = self
             .node_id_to_hir_id
             .iter_enumerated()
-            .map(|(node_id, &hir_id)| (hir_id, node_id))
+            .filter_map(|(node_id, &hir_id)| hir_id.map(|hir_id| (hir_id, node_id)))
             .collect();
     }
 

diff --git a/src/librustc_hir/hir_id.rs b/src/librustc_hir/hir_id.rs
@@ -45,7 +45,4 @@ pub const CRATE_HIR_ID: HirId = HirId {
     local_id: ItemLocalId::from_u32(0),
 };
 
-pub const DUMMY_HIR_ID: HirId =
-    HirId { owner: LocalDefId { local_def_index: CRATE_DEF_INDEX }, local_id: DUMMY_ITEM_LOCAL_ID };
-
 pub const DUMMY_ITEM_LOCAL_ID: ItemLocalId = ItemLocalId::MAX;
diff --git a/src/librustc_middle/hir/map/collector.rs b/src/librustc_middle/hir/map/collector.rs
@@ -250,23 +250,16 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
                     None => format!("{:?}", node),
                 };
 
-                let forgot_str = if hir_id == hir::DUMMY_HIR_ID {
-                    format!("\nMaybe you forgot to lower the node id {:?}?", node_id)
-                } else {
-                    String::new()
-                };
-
                 span_bug!(
                     span,
                     "inconsistent DepNode at `{:?}` for `{}`: \
-                     current_dep_node_owner={} ({:?}), hir_id.owner={} ({:?}){}",
+                     current_dep_node_owner={} ({:?}), hir_id.owner={} ({:?})",
                     self.source_map.span_to_string(span),
                     node_str,
                     self.definitions.def_path(self.current_dep_node_owner).to_string_no_crate(),
                     self.current_dep_node_owner,
                     self.definitions.def_path(hir_id.owner).to_string_no_crate(),
                     hir_id.owner,
-                    forgot_str,
                 )
             }
         }

diff --git a/src/librustc_middle/hir/map/mod.rs b/src/librustc_middle/hir/map/mod.rs
@@ -214,11 +214,21 @@ impl<'hir> Map<'hir> {
         self.tcx.definitions.node_id_to_hir_id(node_id)
     }
 
+    #[inline]
+    pub fn opt_node_id_to_hir_id(&self, node_id: NodeId) -> Option<HirId> {
+        self.tcx.definitions.opt_node_id_to_hir_id(node_id)
+    }
+
     #[inline]
     pub fn local_def_id_to_hir_id(&self, def_id: LocalDefId) -> HirId {
         self.tcx.definitions.local_def_id_to_hir_id(def_id)
     }
 
+    #[inline]
+    pub fn opt_local_def_id_to_hir_id(&self, def_id: LocalDefId) -> Option<HirId> {
+        self.tcx.definitions.opt_local_def_id_to_hir_id(def_id)
+    }
+
     pub fn def_kind(&self, hir_id: HirId) -> Option<DefKind> {
         let node = self.find(hir_id)?;
 

diff --git a/src/librustc_middle/middle/stability.rs b/src/librustc_middle/middle/stability.rs
@@ -215,7 +215,6 @@ fn late_report_deprecation(
     suggestion: Option<Symbol>,
     lint: &'static Lint,
     span: Span,
-    def_id: DefId,
     hir_id: HirId,
 ) {
     if span.in_derive_expansion() {
@@ -229,9 +228,6 @@ fn late_report_deprecation(
         }
         diag.emit()
     });
-    if hir_id == hir::DUMMY_HIR_ID {
-        span_bug!(span, "emitted a {} lint with dummy HIR id: {:?}", lint.name, def_id);
-    }
 }
 
 /// Result of `TyCtxt::eval_stability`.
@@ -296,7 +292,7 @@ impl<'tcx> TyCtxt<'tcx> {
                 if !skip {
                     let (message, lint) =
                         deprecation_message(&depr_entry.attr, &self.def_path_str(def_id));
-                    late_report_deprecation(self, &message, None, lint, span, def_id, id);
+                    late_report_deprecation(self, &message, None, lint, span, id);
                 }
             };
         }
@@ -319,15 +315,7 @@ impl<'tcx> TyCtxt<'tcx> {
                 if let Some(depr) = &stability.rustc_depr {
                     let (message, lint) =
                         rustc_deprecation_message(depr, &self.def_path_str(def_id));
-                    late_report_deprecation(
-                        self,
-                        &message,
-                        depr.suggestion,
-                        lint,
-                        span,
-                        def_id,
-                        id,
-                    );
+                    late_report_deprecation(self, &message, depr.suggestion, lint, span, id);
                 }
             }
         }

diff --git a/src/librustc_middle/ty/context.rs b/src/librustc_middle/ty/context.rs
@@ -1126,13 +1126,16 @@ impl<'tcx> TyCtxt<'tcx> {
 
         let mut trait_map: FxHashMap<_, FxHashMap<_, _>> = FxHashMap::default();
         for (k, v) in resolutions.trait_map {
-            let hir_id = definitions.node_id_to_hir_id(k);
-            let map = trait_map.entry(hir_id.owner).or_default();
-            let v = v
-                .into_iter()
-                .map(|tc| tc.map_import_ids(|id| definitions.node_id_to_hir_id(id)))
-                .collect();
-            map.insert(hir_id.local_id, StableVec::new(v));
+            // FIXME(#71104) Should really be using just `node_id_to_hir_id` but
+            // some `NodeId` do not seem to have a corresponding HirId.
+            if let Some(hir_id) = definitions.opt_node_id_to_hir_id(k) {
+                let map = trait_map.entry(hir_id.owner).or_default();
+                let v = v
+                    .into_iter()
+                    .map(|tc| tc.map_import_ids(|id| definitions.node_id_to_hir_id(id)))
+                    .collect();
+                map.insert(hir_id.local_id, StableVec::new(v));
+            }
         }
 
         GlobalCtxt {

diff --git a/src/librustc_passes/hir_id_validator.rs b/src/librustc_passes/hir_id_validator.rs
@@ -143,16 +143,6 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirIdValidator<'a, 'hir> {
     fn visit_id(&mut self, hir_id: HirId) {
         let owner = self.owner.expect("no owner");
 
-        if hir_id == hir::DUMMY_HIR_ID {
-            self.error(|| {
-                format!(
-                    "HirIdValidator: HirId {:?} is invalid",
-                    self.hir_map.node_to_string(hir_id)
-                )
-            });
-            return;
-        }
-
         if owner != hir_id.owner {
             self.error(|| {
                 format!(

diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs
@@ -928,7 +928,12 @@ impl Visitor<'tcx> for EmbargoVisitor<'tcx> {
 
         let macro_module_def_id =
             ty::DefIdTree::parent(self.tcx, self.tcx.hir().local_def_id(md.hir_id)).unwrap();
-        let mut module_id = match self.tcx.hir().as_local_hir_id(macro_module_def_id) {
+        // FIXME(#71104) Should really be using just `as_local_hir_id` but
+        // some `DefId` do not seem to have a corresponding HirId.
+        let hir_id = macro_module_def_id
+            .as_local()
+            .and_then(|def_id| self.tcx.hir().opt_local_def_id_to_hir_id(def_id));
+        let mut module_id = match hir_id {
             Some(module_id) if self.tcx.hir().is_hir_id_module(module_id) => module_id,
             // `module_id` doesn't correspond to a `mod`, return early (#63164, #65252).
             _ => return,

diff --git a/src/librustc_resolve/late/lifetimes.rs b/src/librustc_resolve/late/lifetimes.rs
@@ -2704,14 +2704,6 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
     }
 
     fn insert_lifetime(&mut self, lifetime_ref: &'tcx hir::Lifetime, def: Region) {
-        if lifetime_ref.hir_id == hir::DUMMY_HIR_ID {
-            span_bug!(
-                lifetime_ref.span,
-                "lifetime reference not renumbered, \
-                 probably a bug in rustc_ast::fold"
-            );
-        }
-
         debug!(
             "insert_lifetime: {} resolved to {:?} span={:?}",
             self.tcx.hir().node_to_string(lifetime_ref.hir_id),

diff --git a/src/librustc_save_analysis/dump_visitor.rs b/src/librustc_save_analysis/dump_visitor.rs
@@ -225,11 +225,14 @@ impl<'l, 'tcx> DumpVisitor<'l, 'tcx> {
             collector.visit_pat(&arg.pat);
 
             for (id, ident, ..) in collector.collected_idents {
-                let hir_id = self.tcx.hir().node_id_to_hir_id(id);
-                let typ = match self.save_ctxt.tables.node_type_opt(hir_id) {
-                    Some(s) => s.to_string(),
-                    None => continue,
-                };
+                // FIXME(#71104) Should really be using just `node_id_to_hir_id` but
+                // some `NodeId` do not seem to have a corresponding HirId.
+                let hir_id = self.tcx.hir().opt_node_id_to_hir_id(id);
+                let typ =
+                    match hir_id.and_then(|hir_id| self.save_ctxt.tables.node_type_opt(hir_id)) {
+                        Some(s) => s.to_string(),
+                        None => continue,
+                    };
                 if !self.span.filter_generated(ident.span) {
                     let id = id_from_node_id(id, &self.save_ctxt);
                     let span = self.span_from_span(ident.span);

diff --git a/src/librustc_typeck/check/method/probe.rs b/src/librustc_typeck/check/method/probe.rs
@@ -865,9 +865,6 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
         &mut self,
         expr_hir_id: hir::HirId,
     ) -> Result<(), MethodError<'tcx>> {
-        if expr_hir_id == hir::DUMMY_HIR_ID {
-            return Ok(());
-        }
         let mut duplicates = FxHashSet::default();
         let opt_applicable_traits = self.tcx.in_scope_traits(expr_hir_id);
         if let Some(applicable_traits) = opt_applicable_traits {

diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs
@@ -838,7 +838,11 @@ fn has_typeck_tables(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
         return tcx.has_typeck_tables(outer_def_id);
     }
 
-    if let Some(id) = tcx.hir().as_local_hir_id(def_id) {
+    // FIXME(#71104) Should really be using just `as_local_hir_id` but
+    // some `LocalDefId` do not seem to have a corresponding HirId.
+    if let Some(id) =
+        def_id.as_local().and_then(|def_id| tcx.hir().opt_local_def_id_to_hir_id(def_id))
+    {
         primary_body_of(tcx, id).is_some()
     } else {
         false

diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs
@@ -375,18 +375,16 @@ impl<'tcx> Clean<Option<Vec<GenericBound>>> for InternalSubsts<'tcx> {
 
 impl Clean<Lifetime> for hir::Lifetime {
     fn clean(&self, cx: &DocContext<'_>) -> Lifetime {
-        if self.hir_id != hir::DUMMY_HIR_ID {
-            let def = cx.tcx.named_region(self.hir_id);
-            match def {
-                Some(rl::Region::EarlyBound(_, node_id, _))
-                | Some(rl::Region::LateBound(_, node_id, _))
-                | Some(rl::Region::Free(_, node_id)) => {
-                    if let Some(lt) = cx.lt_substs.borrow().get(&node_id).cloned() {
-                        return lt;
-                    }
+        let def = cx.tcx.named_region(self.hir_id);
+        match def {
+            Some(rl::Region::EarlyBound(_, node_id, _))
+            | Some(rl::Region::LateBound(_, node_id, _))
+            | Some(rl::Region::Free(_, node_id)) => {
+                if let Some(lt) = cx.lt_substs.borrow().get(&node_id).cloned() {
+                    return lt;
                 }
-                _ => {}
             }
+            _ => {}
         }
         Lifetime(self.name.ident().to_string())
     }

diff --git a/src/librustdoc/clean/utils.rs b/src/librustdoc/clean/utils.rs
@@ -580,11 +580,7 @@ pub fn print_const_expr(cx: &DocContext<'_>, body: hir::BodyId) -> String {
 
 /// Given a type Path, resolve it to a Type using the TyCtxt
 pub fn resolve_type(cx: &DocContext<'_>, path: Path, id: hir::HirId) -> Type {
-    if id == hir::DUMMY_HIR_ID {
-        debug!("resolve_type({:?})", path);
-    } else {
-        debug!("resolve_type({:?},{:?})", path, id);
-    }
+    debug!("resolve_type({:?},{:?})", path, id);
 
     let is_generic = match path.res {
         Res::PrimTy(p) => return Primitive(PrimitiveType::from(p)),