From 2cca2567d9a39f159e392c47c8be6d94087242c9 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 18 Apr 2017 11:50:21 -0400 Subject: [PATCH] make `ty` and `impl_trait_ref` private This requires copying out the cycle error to avoid a cyclic borrow. Is this a problem? Are there paths where we expect cycles to arise and not result in errors? (In such cases, we could add a faster way to test for cycle.) --- src/librustc/ty/item_path.rs | 43 ++++++++++++++++++++-------- src/librustc/ty/maps.rs | 55 ++++++++++++++++++++++++------------ 2 files changed, 68 insertions(+), 30 deletions(-) diff --git a/src/librustc/ty/item_path.rs b/src/librustc/ty/item_path.rs index eb31dfba4a4..16d5d1187fc 100644 --- a/src/librustc/ty/item_path.rs +++ b/src/librustc/ty/item_path.rs @@ -13,16 +13,19 @@ use ty::{self, Ty, TyCtxt}; use syntax::ast; use syntax::symbol::Symbol; +use syntax_pos::DUMMY_SP; use std::cell::Cell; thread_local! { - static FORCE_ABSOLUTE: Cell = Cell::new(false) + static FORCE_ABSOLUTE: Cell = Cell::new(false); + static FORCE_IMPL_FILENAME_LINE: Cell = Cell::new(false); } -/// Enforces that item_path_str always returns an absolute path. -/// This is useful when building symbols that contain types, -/// where we want the crate name to be part of the symbol. +/// Enforces that item_path_str always returns an absolute path and +/// also enables "type-based" impl paths. This is used when building +/// symbols that contain types, where we want the crate name to be +/// part of the symbol. pub fn with_forced_absolute_paths R, R>(f: F) -> R { FORCE_ABSOLUTE.with(|force| { let old = force.get(); @@ -33,6 +36,20 @@ pub fn with_forced_absolute_paths R, R>(f: F) -> R { }) } +/// Force us to name impls with just the filename/line number. We +/// normally try to use types. But at some points, notably while printing +/// cycle errors, this can result in extra or suboptimal error output, +/// so this variable disables that check. +pub fn with_forced_impl_filename_line R, R>(f: F) -> R { + FORCE_IMPL_FILENAME_LINE.with(|force| { + let old = force.get(); + force.set(true); + let result = f(); + force.set(old); + result + }) +} + impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { /// Returns a string identifying this def-id. This string is /// suitable for user output. It is relative to the current crate @@ -199,14 +216,16 @@ fn push_impl_path(self, { let parent_def_id = self.parent_def_id(impl_def_id).unwrap(); - let use_types = if !impl_def_id.is_local() { - // always have full types available for extern crates - true - } else { - // for local crates, check whether type info is - // available; typeck might not have completed yet - self.maps.impl_trait_ref.borrow().contains_key(&impl_def_id) && - self.maps.type_of.borrow().contains_key(&impl_def_id) + // Always use types for non-local impls, where types are always + // available, and filename/line-number is mostly uninteresting. + let use_types = !impl_def_id.is_local() || { + // Otherwise, use filename/line-number if forced. + let force_no_types = FORCE_IMPL_FILENAME_LINE.with(|f| f.get()); + !force_no_types && { + // Otherwise, use types if we can query them without inducing a cycle. + ty::queries::impl_trait_ref::try_get(self, DUMMY_SP, impl_def_id).is_ok() && + ty::queries::type_of::try_get(self, DUMMY_SP, impl_def_id).is_ok() + } }; if !use_types { diff --git a/src/librustc/ty/maps.rs b/src/librustc/ty/maps.rs index c6e5b5da0d6..93f3916d1b5 100644 --- a/src/librustc/ty/maps.rs +++ b/src/librustc/ty/maps.rs @@ -17,11 +17,13 @@ use mir; use session::CompileResult; use ty::{self, CrateInherentImpls, Ty, TyCtxt}; +use ty::item_path; use ty::subst::Substs; use util::nodemap::NodeSet; use rustc_data_structures::indexed_vec::IndexVec; use std::cell::{RefCell, RefMut}; +use std::mem; use std::ops::Deref; use std::rc::Rc; use syntax_pos::{Span, DUMMY_SP}; @@ -139,24 +141,36 @@ pub struct CycleError<'a, 'tcx: 'a> { impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { pub fn report_cycle(self, CycleError { span, cycle }: CycleError) { - assert!(!cycle.is_empty()); - - let mut err = struct_span_err!(self.sess, span, E0391, - "unsupported cyclic reference between types/traits detected"); - err.span_label(span, &format!("cyclic reference")); - - err.span_note(cycle[0].0, &format!("the cycle begins when {}...", - cycle[0].1.describe(self))); - - for &(span, ref query) in &cycle[1..] { - err.span_note(span, &format!("...which then requires {}...", - query.describe(self))); - } + // Subtle: release the refcell lock before invoking `describe()` + // below by dropping `cycle`. + let stack = cycle.to_vec(); + mem::drop(cycle); + + assert!(!stack.is_empty()); + + // Disable naming impls with types in this path, since that + // sometimes cycles itself, leading to extra cycle errors. + // (And cycle errors around impls tend to occur during the + // collect/coherence phases anyhow.) + item_path::with_forced_impl_filename_line(|| { + let mut err = + struct_span_err!(self.sess, span, E0391, + "unsupported cyclic reference between types/traits detected"); + err.span_label(span, &format!("cyclic reference")); + + err.span_note(stack[0].0, &format!("the cycle begins when {}...", + stack[0].1.describe(self))); + + for &(span, ref query) in &stack[1..] { + err.span_note(span, &format!("...which then requires {}...", + query.describe(self))); + } - err.note(&format!("...which then again requires {}, completing the cycle.", - cycle[0].1.describe(self))); + err.note(&format!("...which then again requires {}, completing the cycle.", + stack[0].1.describe(self))); - err.emit(); + err.emit(); + }); } fn cycle_check(self, span: Span, query: Query<'gcx>, compute: F) @@ -335,6 +349,11 @@ fn try_get_with(tcx: TyCtxt<'a, $tcx, 'lcx>, -> Result> where F: FnOnce(&$V) -> R { + debug!("ty::queries::{}::try_get_with(key={:?}, span={:?})", + stringify!($name), + key, + span); + if let Some(result) = tcx.maps.$name.borrow().get(&key) { return Ok(f(result)); } @@ -441,7 +460,7 @@ fn default() -> Self { // the driver creates (using several `rustc_*` crates). define_maps! { <'tcx> /// Records the type of every item. - [pub] type_of: ItemSignature(DefId) -> Ty<'tcx>, + [] type_of: ItemSignature(DefId) -> Ty<'tcx>, /// Maps from the def-id of an item (trait/struct/enum/fn) to its /// associated generics and predicates. @@ -480,7 +499,7 @@ fn default() -> Self { /// Maps from a trait item to the trait item "descriptor" [] associated_item: AssociatedItems(DefId) -> ty::AssociatedItem, - [pub] impl_trait_ref: ItemSignature(DefId) -> Option>, + [] impl_trait_ref: ItemSignature(DefId) -> Option>, [] impl_polarity: ItemSignature(DefId) -> hir::ImplPolarity, /// Maps a DefId of a type to a list of its inherent impls. -- 2.44.0