From 64b0b2bfebf857f25f826ca9ebb07630287bc4d0 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 23 Aug 2017 12:52:22 -0700 Subject: [PATCH 1/6] rustc_errors: Add the ability to delay as bugs This adds a function to `DiagnosticBuilder` to delay the entire diagnostic as a bug to be emitted at a later time. This'll end up getting used in the compiler in the subsequent commits... --- src/librustc_errors/diagnostic_builder.rs | 22 ++++++++++++++++++++-- src/librustc_errors/lib.rs | 16 ++++++---------- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/librustc_errors/diagnostic_builder.rs b/src/librustc_errors/diagnostic_builder.rs index 0a8119893509c..2c8d8b4691f0a 100644 --- a/src/librustc_errors/diagnostic_builder.rs +++ b/src/librustc_errors/diagnostic_builder.rs @@ -110,6 +110,22 @@ impl<'a> DiagnosticBuilder<'a> { // } } + /// Delay emission of this diagnostic as a bug. + /// + /// This can be useful in contexts where an error indicates a bug but + /// typically this only happens when other compilation errors have already + /// happened. In those cases this can be used to defer emission of this + /// diagnostic as a bug in the compiler only if no other errors have been + /// emitted. + /// + /// In the meantime, though, callsites are required to deal with the "bug" + /// locally in whichever way makes the most sense. + pub fn delay_as_bug(&mut self) { + self.level = Level::Bug; + *self.handler.delayed_span_bug.borrow_mut() = Some(self.diagnostic.clone()); + self.cancel(); + } + /// Add a span/label to be included in the resulting snippet. /// This is pushed onto the `MultiSpan` that was created when the /// diagnostic was first built. If you don't call this function at @@ -182,8 +198,10 @@ impl<'a> DiagnosticBuilder<'a> { DiagnosticBuilder::new_diagnostic(handler, diagnostic) } - /// Creates a new `DiagnosticBuilder` with an already constructed diagnostic. - pub fn new_diagnostic(handler: &'a Handler, diagnostic: Diagnostic) -> DiagnosticBuilder<'a> { + /// Creates a new `DiagnosticBuilder` with an already constructed + /// diagnostic. + pub fn new_diagnostic(handler: &'a Handler, diagnostic: Diagnostic) + -> DiagnosticBuilder<'a> { DiagnosticBuilder { handler, diagnostic } } } diff --git a/src/librustc_errors/lib.rs b/src/librustc_errors/lib.rs index a51e6022350c1..60f9276c0ca86 100644 --- a/src/librustc_errors/lib.rs +++ b/src/librustc_errors/lib.rs @@ -275,7 +275,7 @@ pub struct Handler { pub can_emit_warnings: bool, treat_err_as_bug: bool, continue_after_error: Cell, - delayed_span_bug: RefCell>, + delayed_span_bug: RefCell>, tracked_diagnostics: RefCell>>, } @@ -442,8 +442,9 @@ impl Handler { if self.treat_err_as_bug { self.span_bug(sp, msg); } - let mut delayed = self.delayed_span_bug.borrow_mut(); - *delayed = Some((sp.into(), msg.to_string())); + let mut diagnostic = Diagnostic::new(Level::Bug, msg); + diagnostic.set_span(sp.into()); + *self.delayed_span_bug.borrow_mut() = Some(diagnostic); } pub fn span_bug_no_panic>(&self, sp: S, msg: &str) { self.emit(&sp.into(), msg, Bug); @@ -510,14 +511,9 @@ impl Handler { let s; match self.err_count.get() { 0 => { - let delayed_bug = self.delayed_span_bug.borrow(); - match *delayed_bug { - Some((ref span, ref errmsg)) => { - self.span_bug(span.clone(), errmsg); - } - _ => {} + if let Some(bug) = self.delayed_span_bug.borrow_mut().take() { + DiagnosticBuilder::new_diagnostic(self, bug).emit(); } - return; } 1 => s = "aborting due to previous error".to_string(), From 97f2c37435b76c59ff60164b30a02f09641f798f Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 23 Aug 2017 12:53:29 -0700 Subject: [PATCH 2/6] rustc: Change the return of a query's `try_get` This alters the return value of the `try_get` function so the error contains a diagnostic rather than a `CycleError`. This way consumers are forced to take *some* action (else they get a bug to an un-emitted diagnostic). This action could be to emit the error itself, or in some cases delay the diagnostic as a bug and continue. --- src/librustc/ty/maps.rs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/librustc/ty/maps.rs b/src/librustc/ty/maps.rs index f1c624a94e307..18202c96cf52b 100644 --- a/src/librustc/ty/maps.rs +++ b/src/librustc/ty/maps.rs @@ -8,6 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use errors::DiagnosticBuilder; use dep_graph::{DepConstructor, DepNode, DepNodeIndex}; use errors::{Diagnostic, DiagnosticBuilder}; use hir::def_id::{CrateNum, DefId, LOCAL_CRATE}; @@ -218,7 +219,9 @@ pub struct CycleError<'a, 'tcx: 'a> { } impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { - pub fn report_cycle(self, CycleError { span, cycle }: CycleError) { + pub fn report_cycle(self, CycleError { span, cycle }: CycleError) + -> DiagnosticBuilder<'a> + { // Subtle: release the refcell lock before invoking `describe()` // below by dropping `cycle`. let stack = cycle.to_vec(); @@ -247,8 +250,8 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { err.note(&format!("...which then again requires {}, completing the cycle.", stack[0].1.describe(self))); - err.emit(); - }); + return err + }) } fn cycle_check(self, span: Span, query: Query<'gcx>, compute: F) @@ -704,8 +707,11 @@ macro_rules! define_maps { } pub fn try_get(tcx: TyCtxt<'a, $tcx, 'lcx>, span: Span, key: $K) - -> Result<$V, CycleError<'a, $tcx>> { - Self::try_get_with(tcx, span, key, Clone::clone) + -> Result<$V, DiagnosticBuilder<'a>> { + match Self::try_get_with(tcx, span, key, Clone::clone) { + Ok(e) => Ok(e), + Err(e) => Err(tcx.report_cycle(e)), + } } pub fn force(tcx: TyCtxt<'a, $tcx, 'lcx>, span: Span, key: $K) { @@ -714,7 +720,7 @@ macro_rules! define_maps { match Self::try_get_with(tcx, span, key, |_| ()) { Ok(()) => {} - Err(e) => tcx.report_cycle(e) + Err(e) => tcx.report_cycle(e).emit(), } } })* @@ -751,8 +757,8 @@ macro_rules! define_maps { impl<'a, $tcx, 'lcx> TyCtxtAt<'a, $tcx, 'lcx> { $($(#[$attr])* pub fn $name(self, key: $K) -> $V { - queries::$name::try_get(self.tcx, self.span, key).unwrap_or_else(|e| { - self.report_cycle(e); + queries::$name::try_get(self.tcx, self.span, key).unwrap_or_else(|mut e| { + e.emit(); Value::from_cycle_error(self.global_tcx()) }) })* From ecb29c11bbcfac40218b6b35e9ccf26b4a7db151 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 23 Aug 2017 12:54:36 -0700 Subject: [PATCH 3/6] rustc: Fix two instances of `try_get` The `sized_constraint` and `needs_drop_raw` queries both use `try_get` to detect cycles, but in both of these cases the cycle indicates an error has happened elsewhere in compilation. In these cases we can just delay the diagnostic to get emitted as a bug later if we ended up forgetting to emit the error diagnostic. --- src/librustc/ty/mod.rs | 7 +++++-- src/librustc/ty/util.rs | 10 +++++++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index f9bbcc1bbe086..852bd48a5eeed 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -1684,12 +1684,15 @@ impl<'a, 'gcx, 'tcx> AdtDef { pub fn sized_constraint(&self, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> &'tcx [Ty<'tcx>] { match queries::adt_sized_constraint::try_get(tcx, DUMMY_SP, self.did) { Ok(tys) => tys, - Err(_) => { + Err(mut bug) => { debug!("adt_sized_constraint: {:?} is recursive", self); // This should be reported as an error by `check_representable`. // // Consider the type as Sized in the meanwhile to avoid - // further errors. + // further errors. Delay our `bug` diagnostic here to get + // emitted later as well in case we accidentally otherwise don't + // emit an error. + bug.delay_as_bug(); tcx.intern_type_list(&[tcx.types.err]) } } diff --git a/src/librustc/ty/util.rs b/src/librustc/ty/util.rs index 9cd6aa2111873..bbbb8611f98a5 100644 --- a/src/librustc/ty/util.rs +++ b/src/librustc/ty/util.rs @@ -1069,11 +1069,15 @@ fn needs_drop_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, let needs_drop = |ty: Ty<'tcx>| -> bool { match ty::queries::needs_drop_raw::try_get(tcx, DUMMY_SP, param_env.and(ty)) { Ok(v) => v, - Err(_) => { + Err(mut bug) => { // Cycles should be reported as an error by `check_representable`. // - // Consider the type as not needing drop in the meanwhile to avoid - // further errors. + // Consider the type as not needing drop in the meanwhile to + // avoid further errors. + // + // In case we forgot to emit a bug elsewhere, delay our + // diagnostic to get emitted as a compiler bug. + bug.delay_as_bug(); false } } From 82bad957c0314bb841757d8260f0f7f8f5050c57 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 24 Aug 2017 07:00:44 -0700 Subject: [PATCH 4/6] rustc: Add a FIXME for `try_get` in MIR inlining It sounds like this is being handled elsewhere, so for now just preserve the existing behavior of ignoring th error. --- src/librustc_mir/transform/inline.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/inline.rs b/src/librustc_mir/transform/inline.rs index 28aedc8d67497..53b46dd2683fc 100644 --- a/src/librustc_mir/transform/inline.rs +++ b/src/librustc_mir/transform/inline.rs @@ -115,8 +115,13 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> { Ok(ref callee_mir) if self.should_inline(callsite, callee_mir) => { callee_mir.subst(self.tcx, callsite.substs) } + Ok(_) => continue, - _ => continue, + Err(mut bug) => { + // FIXME(#43542) shouldn't have to cancel an error + bug.cancel(); + continue + } }; let start = caller_mir.basic_blocks().len(); From c77213d0cc4c536af70d2ca232ec481394fac808 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 24 Aug 2017 07:37:09 -0700 Subject: [PATCH 5/6] rustc: Skip cyclic checks in `item_path` This seems like it may be likely to cause bugs with `RUST_LOG` and other "interesting" scenarios, but it removes the usage of `try_get` for now! --- src/librustc/ty/item_path.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/librustc/ty/item_path.rs b/src/librustc/ty/item_path.rs index 76a20ed8f3023..5caf513981280 100644 --- a/src/librustc/ty/item_path.rs +++ b/src/librustc/ty/item_path.rs @@ -13,7 +13,6 @@ use hir::def_id::{CrateNum, DefId, CRATE_DEF_INDEX, LOCAL_CRATE}; use ty::{self, Ty, TyCtxt}; use syntax::ast; use syntax::symbol::Symbol; -use syntax_pos::DUMMY_SP; use std::cell::Cell; @@ -222,11 +221,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { let use_types = !self.is_default_impl(impl_def_id) && (!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() - } + !force_no_types }); if !use_types { From c766aa4e3b90c17ed6ded91e964c3537ba5eb14b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 24 Aug 2017 15:13:55 -0700 Subject: [PATCH 6/6] rustc: Make `report_cycle` and `CycleError` private --- src/librustc/ty/maps.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustc/ty/maps.rs b/src/librustc/ty/maps.rs index 18202c96cf52b..042ec49b0bda1 100644 --- a/src/librustc/ty/maps.rs +++ b/src/librustc/ty/maps.rs @@ -8,7 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use errors::DiagnosticBuilder; use dep_graph::{DepConstructor, DepNode, DepNodeIndex}; use errors::{Diagnostic, DiagnosticBuilder}; use hir::def_id::{CrateNum, DefId, LOCAL_CRATE}; @@ -213,13 +212,13 @@ impl QueryMap { } } -pub struct CycleError<'a, 'tcx: 'a> { +struct CycleError<'a, 'tcx: 'a> { span: Span, cycle: RefMut<'a, [(Span, Query<'tcx>)]>, } impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { - pub fn report_cycle(self, CycleError { span, cycle }: CycleError) + fn report_cycle(self, CycleError { span, cycle }: CycleError) -> DiagnosticBuilder<'a> { // Subtle: release the refcell lock before invoking `describe()`