From 29c2bb51c0b82bed1322a18566fb659c1846d136 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 7 Feb 2022 22:37:32 -0800 Subject: [PATCH] rework borrowck errors so that it's harder to not set tainted --- .../rustc_borrowck/src/borrowck_errors.rs | 2 +- .../src/diagnostics/conflict_errors.rs | 14 +- compiler/rustc_borrowck/src/lib.rs | 159 ++++++++++++------ compiler/rustc_borrowck/src/nll.rs | 6 +- .../src/const_eval/eval_queries.rs | 4 +- .../src/interpret/eval_context.rs | 6 +- compiler/rustc_middle/src/mir/query.rs | 2 +- 7 files changed, 119 insertions(+), 74 deletions(-) diff --git a/compiler/rustc_borrowck/src/borrowck_errors.rs b/compiler/rustc_borrowck/src/borrowck_errors.rs index 5702203d7c4ff..7140cda8e4e51 100644 --- a/compiler/rustc_borrowck/src/borrowck_errors.rs +++ b/compiler/rustc_borrowck/src/borrowck_errors.rs @@ -327,7 +327,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> { verb: &str, optional_adverb_for_moved: &str, moved_path: Option, - ) -> DiagnosticBuilder<'cx> { + ) -> DiagnosticBuilder<'tcx> { let moved_path = moved_path.map(|mp| format!(": `{}`", mp)).unwrap_or_default(); struct_span_err!( diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index e32963faa7a4b..7b8b5974fe758 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -77,7 +77,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { if move_out_indices.is_empty() { let root_place = PlaceRef { projection: &[], ..used_place }; - self.set_tainted_by_errors(); if !self.uninitialized_error_reported.insert(root_place) { debug!( "report_use_of_moved_or_uninitialized place: error about {:?} suppressed", @@ -107,7 +106,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { self.buffer_error(err); } else { - if let Some((reported_place, _)) = self.move_error_reported.get(&move_out_indices) { + if let Some((reported_place, _)) = self.has_move_error(&move_out_indices) { if self.prefixes(*reported_place, PrefixSet::All).any(|p| p == used_place) { debug!( "report_use_of_moved_or_uninitialized place: error suppressed \ @@ -217,7 +216,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { place_name, partially_str, loop_message ), ); - self.set_tainted_by_errors(); if self.fn_self_span_reported.insert(fn_span) { err.span_note( // Check whether the source is accessible @@ -299,7 +297,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { } // Avoid pointing to the same function in multiple different // error messages. - self.set_tainted_by_errors(); if span != DUMMY_SP && self.fn_self_span_reported.insert(self_arg.span) { err.span_note( @@ -452,13 +449,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { } } - self.set_tainted_by_errors(); - if let Some((_, mut old_err)) = - self.move_error_reported.insert(move_out_indices, (used_place, err)) - { - // Cancel the old error so it doesn't ICE. - old_err.cancel(); - } + self.buffer_move_error(move_out_indices, (used_place, err)); } } @@ -1016,7 +1007,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { return; } - self.set_tainted_by_errors(); self.access_place_error_reported.insert(( Place { local: root_place.local, projection: root_place_projection }, borrow_span, diff --git a/compiler/rustc_borrowck/src/lib.rs b/compiler/rustc_borrowck/src/lib.rs index 4368a894f219e..459b03b0fad65 100644 --- a/compiler/rustc_borrowck/src/lib.rs +++ b/compiler/rustc_borrowck/src/lib.rs @@ -175,10 +175,13 @@ fn do_mir_borrowck<'a, 'tcx>( } } + let mut errors = error::BorrowckErrors::new(); + // Gather the upvars of a closure, if any. let tables = tcx.typeck_opt_const_arg(def); if let Some(ErrorReported) = tables.tainted_by_errors { infcx.set_tainted_by_errors(); + errors.set_tainted_by_errors(); } let upvars: Vec<_> = tables .closure_min_captures_flattened(def.did.to_def_id()) @@ -205,7 +208,6 @@ fn do_mir_borrowck<'a, 'tcx>( let location_table_owned = LocationTable::new(body); let location_table = &location_table_owned; - let mut errors_buffer = Vec::new(); let (move_data, move_errors): (MoveData<'tcx>, Vec<(Place<'tcx>, MoveError<'tcx>)>) = match MoveData::gather_moves(&body, tcx, param_env) { Ok(move_data) => (move_data, Vec::new()), @@ -263,7 +265,7 @@ fn do_mir_borrowck<'a, 'tcx>( ®ioncx, &opt_closure_req, &opaque_type_values, - &mut errors_buffer, + &mut errors, ); // The various `flow_*` structures can be large. We drop `flow_inits` here @@ -310,10 +312,7 @@ fn do_mir_borrowck<'a, 'tcx>( access_place_error_reported: Default::default(), reservation_error_reported: Default::default(), reservation_warnings: Default::default(), - move_error_reported: BTreeMap::new(), uninitialized_error_reported: Default::default(), - errors_buffer, - tainted_by_errors: false, regioncx: regioncx.clone(), used_mut: Default::default(), used_mut_upvars: SmallVec::new(), @@ -324,9 +323,10 @@ fn do_mir_borrowck<'a, 'tcx>( region_names: RefCell::default(), next_region_name: RefCell::new(1), polonius_output: None, + errors, }; promoted_mbcx.report_move_errors(move_errors); - errors_buffer = promoted_mbcx.errors_buffer; + errors = promoted_mbcx.errors; }; } @@ -344,10 +344,7 @@ fn do_mir_borrowck<'a, 'tcx>( access_place_error_reported: Default::default(), reservation_error_reported: Default::default(), reservation_warnings: Default::default(), - move_error_reported: BTreeMap::new(), uninitialized_error_reported: Default::default(), - errors_buffer, - tainted_by_errors: false, regioncx: Rc::clone(®ioncx), used_mut: Default::default(), used_mut_upvars: SmallVec::new(), @@ -358,6 +355,7 @@ fn do_mir_borrowck<'a, 'tcx>( region_names: RefCell::default(), next_region_name: RefCell::new(1), polonius_output, + errors, }; // Compute and report region errors, if any. @@ -462,24 +460,13 @@ fn do_mir_borrowck<'a, 'tcx>( }) } - // Buffer any move errors that we collected and de-duplicated. - for (_, (_, diag)) in std::mem::take(&mut mbcx.move_error_reported) { - mbcx.buffer_error(diag); - } - - if !mbcx.errors_buffer.is_empty() { - mbcx.errors_buffer.sort_by_key(|diag| diag.sort_span); - - for diag in mbcx.errors_buffer.drain(..) { - mbcx.infcx.tcx.sess.diagnostic().emit_diagnostic(&diag); - } - } + let tainted_by_errors = mbcx.emit_errors(); let result = BorrowCheckResult { concrete_opaque_types: opaque_type_values, closure_requirements: opt_closure_req, used_mut_upvars: mbcx.used_mut_upvars, - tainted_by_errors: mbcx.tainted_by_errors, + tainted_by_errors, }; let body_with_facts = if return_body_with_facts { @@ -556,28 +543,9 @@ struct MirBorrowckCtxt<'cx, 'tcx> { /// for the activation of the borrow. reservation_warnings: FxHashMap, Span, Location, BorrowKind, BorrowData<'tcx>)>, - /// This field keeps track of move errors that are to be reported for given move indices. - /// - /// There are situations where many errors can be reported for a single move out (see #53807) - /// and we want only the best of those errors. - /// - /// The `report_use_of_moved_or_uninitialized` function checks this map and replaces the - /// diagnostic (if there is one) if the `Place` of the error being reported is a prefix of the - /// `Place` of the previous most diagnostic. This happens instead of buffering the error. Once - /// all move errors have been reported, any diagnostics in this map are added to the buffer - /// to be emitted. - /// - /// `BTreeMap` is used to preserve the order of insertions when iterating. This is necessary - /// when errors in the map are being re-added to the error buffer so that errors with the - /// same primary span come out in a consistent order. - move_error_reported: BTreeMap, (PlaceRef<'tcx>, DiagnosticBuilder<'cx>)>, /// This field keeps track of errors reported in the checking of uninitialized variables, /// so that we don't report seemingly duplicate errors. uninitialized_error_reported: FxHashSet>, - /// Errors to be reported buffer - errors_buffer: Vec, - /// Set to true if we emit an error during borrowck - tainted_by_errors: bool, /// This field keeps track of all the local variables that are declared mut and are mutated. /// Used for the warning issued by an unused mutable local variable. used_mut: FxHashSet, @@ -609,6 +577,8 @@ struct MirBorrowckCtxt<'cx, 'tcx> { /// Results of Polonius analysis. polonius_output: Option>, + + errors: error::BorrowckErrors<'tcx>, } // Check that: @@ -1032,8 +1002,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { if conflict_error || mutability_error { debug!("access_place: logging error place_span=`{:?}` kind=`{:?}`", place_span, kind); - - self.set_tainted_by_errors(); self.access_place_error_reported.insert((place_span.0, place_span.1)); } } @@ -2055,10 +2023,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { | WriteKind::MutableBorrow(BorrowKind::Shared) | WriteKind::MutableBorrow(BorrowKind::Shallow), ) => { - if let (Err(_), true) = ( - self.is_mutable(place.as_ref(), is_local_mutation_allowed), - self.errors_buffer.is_empty(), - ) { + if self.is_mutable(place.as_ref(), is_local_mutation_allowed).is_err() + && !self.has_buffered_errors() + { // rust-lang/rust#46908: In pure NLL mode this code path should be // unreachable, but we use `delay_span_bug` because we can hit this when // dereferencing a non-Copy raw pointer *and* have `-Ztreat-err-as-bug` @@ -2308,14 +2275,102 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { fn is_upvar_field_projection(&self, place_ref: PlaceRef<'tcx>) -> Option { path_utils::is_upvar_field_projection(self.infcx.tcx, &self.upvars, place_ref, self.body()) } +} + +mod error { + use super::*; + + pub struct BorrowckErrors<'tcx> { + /// This field keeps track of move errors that are to be reported for given move indices. + /// + /// There are situations where many errors can be reported for a single move out (see #53807) + /// and we want only the best of those errors. + /// + /// The `report_use_of_moved_or_uninitialized` function checks this map and replaces the + /// diagnostic (if there is one) if the `Place` of the error being reported is a prefix of the + /// `Place` of the previous most diagnostic. This happens instead of buffering the error. Once + /// all move errors have been reported, any diagnostics in this map are added to the buffer + /// to be emitted. + /// + /// `BTreeMap` is used to preserve the order of insertions when iterating. This is necessary + /// when errors in the map are being re-added to the error buffer so that errors with the + /// same primary span come out in a consistent order. + buffered_move_errors: + BTreeMap, (PlaceRef<'tcx>, DiagnosticBuilder<'tcx>)>, + /// Errors to be reported buffer + buffered: Vec, + /// Set to Some if we emit an error during borrowck + tainted_by_errors: Option, + } + + impl BorrowckErrors<'_> { + pub fn new() -> Self { + BorrowckErrors { + buffered_move_errors: BTreeMap::new(), + buffered: Default::default(), + tainted_by_errors: None, + } + } + + pub fn buffer_error(&mut self, t: DiagnosticBuilder<'_>) { + self.tainted_by_errors = Some(ErrorReported {}); + t.buffer(&mut self.buffered); + } - pub fn buffer_error(&mut self, t: DiagnosticBuilder<'_>) { - self.tainted_by_errors = true; - t.buffer(&mut self.errors_buffer); + pub fn set_tainted_by_errors(&mut self) { + self.tainted_by_errors = Some(ErrorReported {}); + } } - pub fn set_tainted_by_errors(&mut self) { - self.tainted_by_errors = true; + impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { + pub fn buffer_error(&mut self, t: DiagnosticBuilder<'_>) { + self.errors.buffer_error(t); + } + + pub fn buffer_move_error( + &mut self, + move_out_indices: Vec, + place_and_err: (PlaceRef<'tcx>, DiagnosticBuilder<'tcx>), + ) -> bool { + if let Some((_, mut diag)) = + self.errors.buffered_move_errors.insert(move_out_indices, place_and_err) + { + // Cancel the old diagnostic so we don't ICE + diag.cancel(); + false + } else { + true + } + } + + pub fn emit_errors(&mut self) -> Option { + // Buffer any move errors that we collected and de-duplicated. + for (_, (_, diag)) in std::mem::take(&mut self.errors.buffered_move_errors) { + // We have already set tainted for this error, so just buffer it. + diag.buffer(&mut self.errors.buffered); + } + + if !self.errors.buffered.is_empty() { + self.errors.buffered.sort_by_key(|diag| diag.sort_span); + + for diag in self.errors.buffered.drain(..) { + self.infcx.tcx.sess.diagnostic().emit_diagnostic(&diag); + } + } + + self.errors.tainted_by_errors + } + + pub fn has_buffered_errors(&self) -> bool { + self.errors.buffered.is_empty() + } + + pub fn has_move_error( + &self, + move_out_indices: &[MoveOutIndex], + ) -> Option<&(PlaceRef<'tcx>, DiagnosticBuilder<'cx>)> { + self.errors.buffered_move_errors.get(move_out_indices) + } } } diff --git a/compiler/rustc_borrowck/src/nll.rs b/compiler/rustc_borrowck/src/nll.rs index ac505a6071ded..7fc1fe1130b14 100644 --- a/compiler/rustc_borrowck/src/nll.rs +++ b/compiler/rustc_borrowck/src/nll.rs @@ -1,7 +1,6 @@ //! The entry point of the NLL borrow checker. use rustc_data_structures::vec_map::VecMap; -use rustc_errors::Diagnostic; use rustc_index::vec::IndexVec; use rustc_infer::infer::InferCtxt; use rustc_middle::mir::{create_dump_file, dump_enabled, dump_mir, PassWhere}; @@ -373,7 +372,7 @@ pub(super) fn dump_annotation<'a, 'tcx>( regioncx: &RegionInferenceContext<'tcx>, closure_region_requirements: &Option>, opaque_type_values: &VecMap, Ty<'tcx>>, - errors_buffer: &mut Vec, + errors: &mut crate::error::BorrowckErrors<'tcx>, ) { let tcx = infcx.tcx; let base_def_id = tcx.typeck_root_def_id(body.source.def_id()); @@ -418,8 +417,7 @@ pub(super) fn dump_annotation<'a, 'tcx>( err.note(&format!("Inferred opaque type values:\n{:#?}", opaque_type_values)); } - // FIXME(compiler-errors): Maybe we need to set tainted here - err.buffer(errors_buffer); + errors.buffer_error(err); } fn for_each_region_constraint( diff --git a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs index 7235b8416ad65..df08b541801dd 100644 --- a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs +++ b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs @@ -287,8 +287,8 @@ pub fn eval_to_allocation_raw_provider<'tcx>( if let Some(error_reported) = tcx.typeck_opt_const_arg(def).tainted_by_errors { return Err(ErrorHandled::Reported(error_reported)); } - if tcx.mir_borrowck_opt_const_arg(def).tainted_by_errors { - return Err(ErrorHandled::Reported(ErrorReported {})); + if let Some(error_reported) = tcx.mir_borrowck_opt_const_arg(def).tainted_by_errors { + return Err(ErrorHandled::Reported(error_reported)); } } if !tcx.is_mir_available(def.did) { diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 00c066a2851c3..022127de65a67 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -516,8 +516,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { if let Some(error_reported) = self.tcx.typeck_opt_const_arg(def).tainted_by_errors { throw_inval!(AlreadyReported(error_reported)); } - if self.tcx.mir_borrowck_opt_const_arg(def).tainted_by_errors { - throw_inval!(AlreadyReported(rustc_errors::ErrorReported {})); + if let Some(error_reported) = + self.tcx.mir_borrowck_opt_const_arg(def).tainted_by_errors + { + throw_inval!(AlreadyReported(error_reported)); } } } diff --git a/compiler/rustc_middle/src/mir/query.rs b/compiler/rustc_middle/src/mir/query.rs index b1aad01118ad4..5c22bfd09a396 100644 --- a/compiler/rustc_middle/src/mir/query.rs +++ b/compiler/rustc_middle/src/mir/query.rs @@ -214,7 +214,7 @@ pub struct BorrowCheckResult<'tcx> { pub concrete_opaque_types: VecMap, Ty<'tcx>>, pub closure_requirements: Option>, pub used_mut_upvars: SmallVec<[Field; 8]>, - pub tainted_by_errors: bool, + pub tainted_by_errors: Option, } /// The result of the `mir_const_qualif` query.