From b9161ab880cb4f58b2b2e74b7a8bed2514f8a753 Mon Sep 17 00:00:00 2001 From: marmeladema Date: Mon, 13 Apr 2020 17:40:06 +0100 Subject: [PATCH 1/2] Do not use `DUMMY_HIR_ID` as placeholder value in node_id_to_hir_id table 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. --- src/librustc_ast_lowering/lib.rs | 23 ++++++++++------------ src/librustc_hir/definitions.rs | 20 ++++++++++++++++--- src/librustc_middle/hir/map/mod.rs | 10 ++++++++++ src/librustc_middle/ty/context.rs | 17 +++++++++------- src/librustc_privacy/lib.rs | 7 ++++++- src/librustc_save_analysis/dump_visitor.rs | 13 +++++++----- src/librustc_typeck/check/mod.rs | 6 +++++- 7 files changed, 66 insertions(+), 30 deletions(-) diff --git a/src/librustc_ast_lowering/lib.rs b/src/librustc_ast_lowering/lib.rs index 9bb1f57a52457..c2c7de9d21b28 100644 --- 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, - node_id_to_hir_id: IndexVec, + node_id_to_hir_id: IndexVec>, allow_try_trait: Option>, allow_gen_future: Option>, @@ -522,7 +522,7 @@ 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); @@ -530,7 +530,8 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> { 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 index 58f34787613be..5e57bcdc85cfa 100644 --- a/src/librustc_hir/definitions.rs +++ b/src/librustc_hir/definitions.rs @@ -87,7 +87,7 @@ pub struct Definitions { node_id_to_def_id: FxHashMap, def_id_to_node_id: IndexVec, - pub(super) node_id_to_hir_id: IndexVec, + pub(super) node_id_to_hir_id: IndexVec>, /// The reverse mapping of `node_id_to_hir_id`. pub(super) hir_id_to_node_id: FxHashMap, @@ -359,11 +359,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 { 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 { let node_id = self.def_id_to_node_id[id]; self.node_id_to_hir_id[node_id] } @@ -470,7 +481,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) { + pub fn init_node_id_to_hir_id_mapping( + &mut self, + mapping: IndexVec>, + ) { assert!( self.node_id_to_hir_id.is_empty(), "trying to initialize `NodeId` -> `HirId` mapping twice" @@ -481,7 +495,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_middle/hir/map/mod.rs b/src/librustc_middle/hir/map/mod.rs index 3eaacb54d5b42..ead8529fad8be 100644 --- 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 { + 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 { + self.tcx.definitions.opt_local_def_id_to_hir_id(def_id) + } + pub fn def_kind(&self, hir_id: HirId) -> Option { let node = self.find(hir_id)?; diff --git a/src/librustc_middle/ty/context.rs b/src/librustc_middle/ty/context.rs index 9fa25a4363789..a49dc105498ed 100644 --- 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_privacy/lib.rs b/src/librustc_privacy/lib.rs index 84f041ee9f87c..51e1588c71c42 100644 --- 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_save_analysis/dump_visitor.rs b/src/librustc_save_analysis/dump_visitor.rs index 5b93c73e07ce2..ba2541bc6c3d9 100644 --- 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/mod.rs b/src/librustc_typeck/check/mod.rs index 2db397d5c7437..750a6532ac9e5 100644 --- 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 From c15e13ae167b3f72270e0b99333e71ba1bef9ffc Mon Sep 17 00:00:00 2001 From: marmeladema Date: Mon, 13 Apr 2020 17:52:40 +0100 Subject: [PATCH 2/2] Remove `DUMMY_HIR_ID` --- src/librustc_hir/definitions.rs | 4 +--- src/librustc_hir/hir_id.rs | 3 --- src/librustc_middle/hir/map/collector.rs | 9 +-------- src/librustc_middle/middle/stability.rs | 16 ++-------------- src/librustc_passes/hir_id_validator.rs | 10 ---------- src/librustc_resolve/late/lifetimes.rs | 8 -------- src/librustc_typeck/check/method/probe.rs | 3 --- src/librustdoc/clean/mod.rs | 18 ++++++++---------- src/librustdoc/clean/utils.rs | 6 +----- 9 files changed, 13 insertions(+), 64 deletions(-) diff --git a/src/librustc_hir/definitions.rs b/src/librustc_hir/definitions.rs index 5e57bcdc85cfa..1ac23677d4739 100644 --- 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; @@ -345,8 +344,7 @@ impl Definitions { #[inline] pub fn as_local_hir_id(&self, def_id: DefId) -> Option { 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 } diff --git a/src/librustc_hir/hir_id.rs b/src/librustc_hir/hir_id.rs index 1c7987e965f85..d782c3dd70a2c 100644 --- 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 index 70ea856498de4..2906da437abac 100644 --- 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/middle/stability.rs b/src/librustc_middle/middle/stability.rs index 46525bdedad35..1dd14b7c4ffda 100644 --- a/src/librustc_middle/middle/stability.rs +++ b/src/librustc_middle/middle/stability.rs @@ -215,7 +215,6 @@ fn late_report_deprecation( suggestion: Option, 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_passes/hir_id_validator.rs b/src/librustc_passes/hir_id_validator.rs index f5611654fc061..1e31b7c74b6f0 100644 --- 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_resolve/late/lifetimes.rs b/src/librustc_resolve/late/lifetimes.rs index f230eeb8fad57..5bfb5aa2440b7 100644 --- 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_typeck/check/method/probe.rs b/src/librustc_typeck/check/method/probe.rs index b5d3f7b55029f..66d3d68c18daa 100644 --- a/src/librustc_typeck/check/method/probe.rs +++ b/src/librustc_typeck/check/method/probe.rs @@ -860,9 +860,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/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 0f52feda2a303..6e50264c098b6 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -375,18 +375,16 @@ impl<'tcx> Clean>> for InternalSubsts<'tcx> { impl Clean 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 index 9e96015d306e4..316cf84152842 100644 --- 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)),