From a69eaf62c5e325c96c9924102829a42cbfd37424 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Fri, 11 Aug 2017 11:56:26 +0200 Subject: [PATCH] Improve validation of TypeckTables keys. --- src/librustc/infer/mod.rs | 2 +- src/librustc/lint/context.rs | 4 +- src/librustc/middle/dead.rs | 2 +- src/librustc/middle/effect.rs | 3 +- src/librustc/middle/reachable.rs | 2 +- src/librustc/ty/context.rs | 52 ++++++++++++++++---------- src/librustc_driver/pretty.rs | 3 +- src/librustc_passes/consts.rs | 2 +- src/librustc_privacy/lib.rs | 2 +- src/librustc_save_analysis/lib.rs | 2 +- src/librustc_typeck/check/mod.rs | 2 +- src/librustc_typeck/check/writeback.rs | 17 ++++++--- 12 files changed, 55 insertions(+), 38 deletions(-) diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs index 1a8d570bde03..186eab724db3 100644 --- a/src/librustc/infer/mod.rs +++ b/src/librustc/infer/mod.rs @@ -359,7 +359,7 @@ impl<'a, 'gcx, 'tcx> InferCtxtBuilder<'a, 'gcx, 'tcx> { /// Used only by `rustc_typeck` during body type-checking/inference, /// will initialize `in_progress_tables` with fresh `TypeckTables`. pub fn with_fresh_in_progress_tables(mut self, table_owner: DefId) -> Self { - self.fresh_tables = Some(RefCell::new(ty::TypeckTables::empty(table_owner))); + self.fresh_tables = Some(RefCell::new(ty::TypeckTables::empty(Some(table_owner)))); self } diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index cd06806ca601..930e8e750908 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -43,7 +43,7 @@ use syntax::ast; use syntax_pos::{MultiSpan, Span}; use errors::DiagnosticBuilder; use hir; -use hir::def_id::{DefId, LOCAL_CRATE}; +use hir::def_id::LOCAL_CRATE; use hir::intravisit as hir_visit; use syntax::visit as ast_visit; @@ -986,7 +986,7 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { let mut cx = LateContext { tcx, - tables: &ty::TypeckTables::empty(DefId::invalid()), + tables: &ty::TypeckTables::empty(None), param_env: ty::ParamEnv::empty(Reveal::UserFacing), access_levels, lint_sess: LintSession::new(&tcx.sess.lint_store), diff --git a/src/librustc/middle/dead.rs b/src/librustc/middle/dead.rs index 0081555095b2..ed04186eb286 100644 --- a/src/librustc/middle/dead.rs +++ b/src/librustc/middle/dead.rs @@ -427,7 +427,7 @@ fn find_live<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, let mut symbol_visitor = MarkSymbolVisitor { worklist, tcx, - tables: &ty::TypeckTables::empty(DefId::invalid()), + tables: &ty::TypeckTables::empty(None), live_symbols: box FxHashSet(), struct_has_extern_repr: false, ignore_non_const_paths: false, diff --git a/src/librustc/middle/effect.rs b/src/librustc/middle/effect.rs index 6b7280cf0ac4..98934d607032 100644 --- a/src/librustc/middle/effect.rs +++ b/src/librustc/middle/effect.rs @@ -19,7 +19,6 @@ use syntax::ast; use syntax_pos::Span; use hir::{self, PatKind}; use hir::def::Def; -use hir::def_id::DefId; use hir::intravisit::{self, FnKind, Visitor, NestedVisitorMap}; #[derive(Copy, Clone)] @@ -263,7 +262,7 @@ impl<'a, 'tcx> Visitor<'tcx> for EffectCheckVisitor<'a, 'tcx> { pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { let mut visitor = EffectCheckVisitor { tcx, - tables: &ty::TypeckTables::empty(DefId::invalid()), + tables: &ty::TypeckTables::empty(None), body_id: hir::BodyId { node_id: ast::CRATE_NODE_ID }, unsafe_context: UnsafeContext::new(SafeContext), }; diff --git a/src/librustc/middle/reachable.rs b/src/librustc/middle/reachable.rs index bd9a413ca617..1e2bb6627afc 100644 --- a/src/librustc/middle/reachable.rs +++ b/src/librustc/middle/reachable.rs @@ -375,7 +375,7 @@ fn reachable_set<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, crate_num: CrateNum) -> }); let mut reachable_context = ReachableContext { tcx, - tables: &ty::TypeckTables::empty(DefId::invalid()), + tables: &ty::TypeckTables::empty(None), reachable_symbols: NodeSet(), worklist: Vec::new(), any_library, diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index ac851f182a0f..d93750ec04e0 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -212,7 +212,7 @@ pub struct CommonTypes<'tcx> { } pub struct LocalTableInContext<'a, V: 'a> { - local_id_root: DefId, + local_id_root: Option, data: &'a ItemLocalMap } @@ -223,11 +223,13 @@ pub struct LocalTableInContext<'a, V: 'a> { /// would be in a different frame of reference and using its `local_id` /// would result in lookup errors, or worse, in silently wrong data being /// stored/returned. -fn validate_hir_id_for_typeck_tables(table_id_root: DefId, hir_id: hir::HirId) { +fn validate_hir_id_for_typeck_tables(local_id_root: Option, + hir_id: hir::HirId, + mut_access: bool) { #[cfg(debug_assertions)] { - if table_id_root.is_local() { - if hir_id.owner != table_id_root.index { + if let Some(local_id_root) = local_id_root { + if hir_id.owner != local_id_root.index { ty::tls::with(|tcx| { let node_id = tcx.hir .definitions() @@ -237,21 +239,30 @@ fn validate_hir_id_for_typeck_tables(table_id_root: DefId, hir_id: hir::HirId) { TypeckTables with local_id_root {:?}", tcx.hir.node_to_string(node_id), DefId::local(hir_id.owner), - table_id_root) + local_id_root) }); } + } else { + // We use "Null Object" TypeckTables in some of the analysis passes. + // These are just expected to be empty and their `local_id_root` is + // `None`. Therefore we cannot verify whether a given `HirId` would + // be a valid key for the given table. Instead we make sure that + // nobody tries to write to such a Null Object table. + if mut_access { + bug!("access to invalid TypeckTables") + } } } } impl<'a, V> LocalTableInContext<'a, V> { pub fn contains_key(&self, id: hir::HirId) -> bool { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, false); self.data.contains_key(&id.local_id) } pub fn get(&self, id: hir::HirId) -> Option<&V> { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, false); self.data.get(&id.local_id) } @@ -269,29 +280,29 @@ impl<'a, V> ::std::ops::Index for LocalTableInContext<'a, V> { } pub struct LocalTableInContextMut<'a, V: 'a> { - local_id_root: DefId, + local_id_root: Option, data: &'a mut ItemLocalMap } impl<'a, V> LocalTableInContextMut<'a, V> { pub fn get_mut(&mut self, id: hir::HirId) -> Option<&mut V> { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, true); self.data.get_mut(&id.local_id) } pub fn entry(&mut self, id: hir::HirId) -> Entry { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, true); self.data.entry(id.local_id) } pub fn insert(&mut self, id: hir::HirId, val: V) -> Option { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, true); self.data.insert(id.local_id, val) } pub fn remove(&mut self, id: hir::HirId) -> Option { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, true); self.data.remove(&id.local_id) } } @@ -299,7 +310,7 @@ impl<'a, V> LocalTableInContextMut<'a, V> { #[derive(RustcEncodable, RustcDecodable)] pub struct TypeckTables<'tcx> { /// The HirId::owner all ItemLocalIds in this table are relative to. - pub local_id_root: DefId, + pub local_id_root: Option, /// Resolved definitions for `::X` associated paths and /// method calls, including those of overloaded operators. @@ -363,7 +374,7 @@ pub struct TypeckTables<'tcx> { } impl<'tcx> TypeckTables<'tcx> { - pub fn empty(local_id_root: DefId) -> TypeckTables<'tcx> { + pub fn empty(local_id_root: Option) -> TypeckTables<'tcx> { TypeckTables { local_id_root, type_dependent_defs: ItemLocalMap(), @@ -388,7 +399,7 @@ impl<'tcx> TypeckTables<'tcx> { match *qpath { hir::QPath::Resolved(_, ref path) => path.def, hir::QPath::TypeRelative(..) => { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, false); self.type_dependent_defs.get(&id.local_id).cloned().unwrap_or(Def::Err) } } @@ -436,7 +447,7 @@ impl<'tcx> TypeckTables<'tcx> { } pub fn node_id_to_type_opt(&self, id: hir::HirId) -> Option> { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, false); self.node_types.get(&id.local_id).cloned() } @@ -448,12 +459,12 @@ impl<'tcx> TypeckTables<'tcx> { } pub fn node_substs(&self, id: hir::HirId) -> &'tcx Substs<'tcx> { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, false); self.node_substs.get(&id.local_id).cloned().unwrap_or(Substs::empty()) } pub fn node_substs_opt(&self, id: hir::HirId) -> Option<&'tcx Substs<'tcx>> { - validate_hir_id_for_typeck_tables(self.local_id_root, id); + validate_hir_id_for_typeck_tables(self.local_id_root, id, false); self.node_substs.get(&id.local_id).cloned() } @@ -502,7 +513,7 @@ impl<'tcx> TypeckTables<'tcx> { pub fn expr_adjustments(&self, expr: &hir::Expr) -> &[ty::adjustment::Adjustment<'tcx>] { - validate_hir_id_for_typeck_tables(self.local_id_root, expr.hir_id); + validate_hir_id_for_typeck_tables(self.local_id_root, expr.hir_id, false); self.adjustments.get(&expr.hir_id.local_id).map_or(&[], |a| &a[..]) } @@ -663,6 +674,9 @@ impl<'a, 'gcx, 'tcx> HashStable> for Typeck closure_expr_id } = *up_var_id; + let local_id_root = + local_id_root.expect("trying to hash invalid TypeckTables"); + let var_def_id = DefId { krate: local_id_root.krate, index: var_id, diff --git a/src/librustc_driver/pretty.rs b/src/librustc_driver/pretty.rs index 51f5cc4f249f..828136d6b7e4 100644 --- a/src/librustc_driver/pretty.rs +++ b/src/librustc_driver/pretty.rs @@ -45,7 +45,6 @@ use std::option; use std::path::Path; use std::str::FromStr; -use rustc::hir::def_id::DefId; use rustc::hir::map as hir_map; use rustc::hir::map::blocks; use rustc::hir; @@ -233,7 +232,7 @@ impl PpSourceMode { arenas, id, |tcx, _, _, _| { - let empty_tables = ty::TypeckTables::empty(DefId::invalid()); + let empty_tables = ty::TypeckTables::empty(None); let annotation = TypedAnnotation { tcx: tcx, tables: Cell::new(&empty_tables) diff --git a/src/librustc_passes/consts.rs b/src/librustc_passes/consts.rs index a390448f43a9..9185f73974cd 100644 --- a/src/librustc_passes/consts.rs +++ b/src/librustc_passes/consts.rs @@ -471,7 +471,7 @@ fn check_adjustments<'a, 'tcx>(v: &mut CheckCrateVisitor<'a, 'tcx>, e: &hir::Exp pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { tcx.hir.krate().visit_all_item_likes(&mut CheckCrateVisitor { tcx: tcx, - tables: &ty::TypeckTables::empty(DefId::invalid()), + tables: &ty::TypeckTables::empty(None), in_fn: false, promotable: false, mut_rvalue_borrows: NodeSet(), diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index 6f8c6a5ed3b0..373d7911cf93 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -1656,7 +1656,7 @@ fn privacy_access_levels<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, let krate = tcx.hir.krate(); - let empty_tables = ty::TypeckTables::empty(DefId::invalid()); + let empty_tables = ty::TypeckTables::empty(None); // Check privacy of names not checked in previous compilation stages. diff --git a/src/librustc_save_analysis/lib.rs b/src/librustc_save_analysis/lib.rs index 8baea0fd83db..c3d071d27d6e 100644 --- a/src/librustc_save_analysis/lib.rs +++ b/src/librustc_save_analysis/lib.rs @@ -977,7 +977,7 @@ pub fn process_crate<'l, 'tcx, H: SaveHandler>(tcx: TyCtxt<'l, 'tcx, 'tcx>, let save_ctxt = SaveContext { tcx: tcx, - tables: &ty::TypeckTables::empty(DefId::invalid()), + tables: &ty::TypeckTables::empty(None), analysis: analysis, span_utils: SpanUtils::new(&tcx.sess), config: find_config(config), diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 483b5bb10209..826e8de860b9 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -901,7 +901,7 @@ fn typeck_tables_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, // Consistency check our TypeckTables instance can hold all ItemLocalIds // it will need to hold. assert_eq!(tables.local_id_root, - DefId::local(tcx.hir.definitions().node_to_hir_id(id).owner)); + Some(DefId::local(tcx.hir.definitions().node_to_hir_id(id).owner))); tables } diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index 0e7083b72f5f..a363e47a14f2 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -81,7 +81,7 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { WritebackCx { fcx: fcx, - tables: ty::TypeckTables::empty(DefId::local(owner.owner)), + tables: ty::TypeckTables::empty(Some(DefId::local(owner.owner))), body: body } } @@ -229,10 +229,11 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { fn visit_closures(&mut self) { let fcx_tables = self.fcx.tables.borrow(); debug_assert_eq!(fcx_tables.local_id_root, self.tables.local_id_root); + let common_local_id_root = fcx_tables.local_id_root.unwrap(); for (&id, closure_ty) in fcx_tables.closure_tys().iter() { let hir_id = hir::HirId { - owner: fcx_tables.local_id_root.index, + owner: common_local_id_root.index, local_id: id, }; let closure_ty = self.resolve(closure_ty, &hir_id); @@ -241,7 +242,7 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { for (&id, &closure_kind) in fcx_tables.closure_kinds().iter() { let hir_id = hir::HirId { - owner: fcx_tables.local_id_root.index, + owner: common_local_id_root.index, local_id: id, }; self.tables.closure_kinds_mut().insert(hir_id, closure_kind); @@ -251,11 +252,13 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { fn visit_cast_types(&mut self) { let fcx_tables = self.fcx.tables.borrow(); let fcx_cast_kinds = fcx_tables.cast_kinds(); + debug_assert_eq!(fcx_tables.local_id_root, self.tables.local_id_root); let mut self_cast_kinds = self.tables.cast_kinds_mut(); + let common_local_id_root = fcx_tables.local_id_root.unwrap(); for (&local_id, &cast_kind) in fcx_cast_kinds.iter() { let hir_id = hir::HirId { - owner: fcx_tables.local_id_root.index, + owner: common_local_id_root.index, local_id, }; self_cast_kinds.insert(hir_id, cast_kind); @@ -357,10 +360,11 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { fn visit_liberated_fn_sigs(&mut self) { let fcx_tables = self.fcx.tables.borrow(); debug_assert_eq!(fcx_tables.local_id_root, self.tables.local_id_root); + let common_local_id_root = fcx_tables.local_id_root.unwrap(); for (&local_id, fn_sig) in fcx_tables.liberated_fn_sigs().iter() { let hir_id = hir::HirId { - owner: fcx_tables.local_id_root.index, + owner: common_local_id_root.index, local_id, }; let fn_sig = self.resolve(fn_sig, &hir_id); @@ -371,10 +375,11 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { fn visit_fru_field_types(&mut self) { let fcx_tables = self.fcx.tables.borrow(); debug_assert_eq!(fcx_tables.local_id_root, self.tables.local_id_root); + let common_local_id_root = fcx_tables.local_id_root.unwrap(); for (&local_id, ftys) in fcx_tables.fru_field_types().iter() { let hir_id = hir::HirId { - owner: fcx_tables.local_id_root.index, + owner: common_local_id_root.index, local_id, }; let ftys = self.resolve(ftys, &hir_id);