From 10af6a2b37bf52e023b892dc2622c19e32f6ebf5 Mon Sep 17 00:00:00 2001 From: David Wood Date: Sun, 9 Sep 2018 19:34:39 +0200 Subject: [PATCH] Refactor explain_borrow to return explanation. Previously, explain_borrow would emit an error with the explanation of the a borrow. Now, it returns a enum with what the explanation for the borrow is and any relevant spans or information such that the calling code can choose to emit the same note/suggestion as before by calling the emit method on the new enum. --- .../borrow_check/error_reporting.rs | 90 ++++---- .../nll/explain_borrow/find_use.rs | 2 - .../borrow_check/nll/explain_borrow/mod.rs | 210 ++++++++---------- 3 files changed, 140 insertions(+), 162 deletions(-) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 82eca4a18c8..c5d86f9743e 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -10,11 +10,14 @@ use borrow_check::{WriteKind, StorageDeadOrDrop}; use borrow_check::prefixes::IsPrefixOf; +use borrow_check::nll::explain_borrow::BorrowExplanation; use rustc::middle::region::ScopeTree; -use rustc::mir::VarBindingForm; -use rustc::mir::{BindingForm, BorrowKind, ClearCrossCrate, Field, Local}; -use rustc::mir::{FakeReadCause, LocalDecl, LocalKind, Location, Operand, Place}; -use rustc::mir::{ProjectionElem, Rvalue, Statement, StatementKind}; +use rustc::mir::{ + BindingForm, BorrowKind, ClearCrossCrate, Field, FakeReadCause, Local, + LocalDecl, LocalKind, Location, Operand, Place, + ProjectionElem, Rvalue, Statement, StatementKind, + VarBindingForm, +}; use rustc::ty; use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::sync::Lrc; @@ -25,7 +28,6 @@ use super::{Context, MirBorrowckCtxt}; use super::{InitializationRequiringAction, PrefixSet}; -use borrow_check::nll::explain_borrow::BorrowContainsPointReason; use dataflow::drop_flag_effects; use dataflow::move_paths::indexes::MoveOutIndex; use dataflow::move_paths::MovePathIndex; @@ -226,7 +228,7 @@ pub(super) fn report_move_out_while_borrowed( move_spans.var_span_label(&mut err, "move occurs due to use in closure"); - self.explain_why_borrow_contains_point(context, borrow, None, &mut err); + self.explain_why_borrow_contains_point(context, borrow, None).emit(self.tcx, &mut err); err.buffer(&mut self.errors_buffer); } @@ -263,7 +265,7 @@ pub(super) fn report_use_while_mutably_borrowed( format!("borrow occurs due to use of `{}` in closure", desc_place) }); - self.explain_why_borrow_contains_point(context, borrow, None, &mut err); + self.explain_why_borrow_contains_point(context, borrow, None).emit(self.tcx, &mut err); err.buffer(&mut self.errors_buffer); } @@ -390,7 +392,8 @@ pub(super) fn report_conflicting_borrow( ); } - self.explain_why_borrow_contains_point(context, issued_borrow, None, &mut err); + self.explain_why_borrow_contains_point(context, issued_borrow, None) + .emit(self.tcx, &mut err); err.buffer(&mut self.errors_buffer); } @@ -445,17 +448,13 @@ pub(super) fn report_borrowed_value_does_not_live_long_enough( self.access_place_error_reported .insert((root_place.clone(), borrow_span)); - let borrow_reason = self.find_why_borrow_contains_point(context, borrow); - - if let Some(WriteKind::StorageDeadOrDrop(StorageDeadOrDrop::Destructor)) = kind - { + if let Some(WriteKind::StorageDeadOrDrop(StorageDeadOrDrop::Destructor)) = kind { // If a borrow of path `B` conflicts with drop of `D` (and // we're not in the uninteresting case where `B` is a // prefix of `D`), then report this as a more interesting // destructor conflict. if !borrow.borrowed_place.is_prefix_of(place_span.0) { - self.report_borrow_conflicts_with_destructor( - context, borrow, borrow_reason, place_span, kind); + self.report_borrow_conflicts_with_destructor(context, borrow, place_span, kind); return; } } @@ -469,7 +468,6 @@ pub(super) fn report_borrowed_value_does_not_live_long_enough( name, &scope_tree, &borrow, - borrow_reason, drop_span, borrow_span, kind.map(|k| (k, place_span.0)), @@ -478,7 +476,6 @@ pub(super) fn report_borrowed_value_does_not_live_long_enough( context, &scope_tree, &borrow, - borrow_reason, drop_span, proper_span, ), @@ -495,16 +492,15 @@ fn report_local_value_does_not_live_long_enough( name: &String, scope_tree: &Lrc, borrow: &BorrowData<'tcx>, - reason: BorrowContainsPointReason<'tcx>, drop_span: Span, borrow_span: Span, kind_place: Option<(WriteKind, &Place<'tcx>)>, ) -> DiagnosticBuilder<'cx> { debug!( "report_local_value_does_not_live_long_enough(\ - {:?}, {:?}, {:?}, {:?}, {:?}, {:?}, {:?}\ + {:?}, {:?}, {:?}, {:?}, {:?}, {:?}\ )", - context, name, scope_tree, borrow, reason, drop_span, borrow_span + context, name, scope_tree, borrow, drop_span, borrow_span ); let mut err = self.tcx.path_does_not_live_long_enough( @@ -519,7 +515,9 @@ fn report_local_value_does_not_live_long_enough( format!("`{}` dropped here while still borrowed", name), ); - self.report_why_borrow_contains_point(&mut err, reason, kind_place); + self.explain_why_borrow_contains_point(context, borrow, kind_place) + .emit(self.tcx, &mut err); + err } @@ -527,15 +525,14 @@ pub(super) fn report_borrow_conflicts_with_destructor( &mut self, context: Context, borrow: &BorrowData<'tcx>, - borrow_reason: BorrowContainsPointReason<'tcx>, - place_span: (&Place<'tcx>, Span), + (place, drop_span): (&Place<'tcx>, Span), kind: Option, ) { debug!( "report_borrow_conflicts_with_destructor(\ - {:?}, {:?}, {:?}, {:?} {:?}\ + {:?}, {:?}, ({:?}, {:?}), {:?}\ )", - context, borrow, borrow_reason, place_span, kind, + context, borrow, place, drop_span, kind, ); let borrow_spans = self.retrieve_borrow_spans(borrow); @@ -543,10 +540,7 @@ pub(super) fn report_borrow_conflicts_with_destructor( let mut err = self.tcx.cannot_borrow_across_destructor(borrow_span, Origin::Mir); - let drop_span = place_span.1; - let (what_was_dropped, dropped_ty) = { - let place = place_span.0; let desc = match self.describe_place(place) { Some(name) => format!("`{}`", name.as_str()), None => format!("temporary value"), @@ -571,17 +565,19 @@ pub(super) fn report_borrow_conflicts_with_destructor( }; err.span_label(drop_span, label); - // Only give this note and suggestion if they could be relevant - match borrow_reason { - BorrowContainsPointReason::Liveness {..} - | BorrowContainsPointReason::DropLiveness {..} => { + // Only give this note and suggestion if they could be relevant. + let explanation = self.explain_why_borrow_contains_point( + context, borrow, kind.map(|k| (k, place)), + ); + match explanation { + BorrowExplanation::UsedLater {..} | + BorrowExplanation::UsedLaterWhenDropped {..} => { err.note("consider using a `let` binding to create a longer lived value"); - } - BorrowContainsPointReason::OutlivesFreeRegion {..} => (), + }, + _ => {}, } - self.report_why_borrow_contains_point( - &mut err, borrow_reason, kind.map(|k| (k, place_span.0))); + explanation.emit(self.tcx, &mut err); err.buffer(&mut self.errors_buffer); } @@ -607,6 +603,7 @@ fn report_thread_local_value_does_not_live_long_enough( "thread-local variables cannot be borrowed beyond the end of the function", ); err.span_label(drop_span, "end of enclosing function is here"); + err } @@ -615,15 +612,14 @@ fn report_temporary_value_does_not_live_long_enough( context: Context, scope_tree: &Lrc, borrow: &BorrowData<'tcx>, - reason: BorrowContainsPointReason<'tcx>, drop_span: Span, proper_span: Span, ) -> DiagnosticBuilder<'cx> { debug!( "report_temporary_value_does_not_live_long_enough(\ - {:?}, {:?}, {:?}, {:?}, {:?}, {:?}\ + {:?}, {:?}, {:?}, {:?}, {:?}\ )", - context, scope_tree, borrow, reason, drop_span, proper_span + context, scope_tree, borrow, drop_span, proper_span ); let tcx = self.tcx; @@ -632,16 +628,18 @@ fn report_temporary_value_does_not_live_long_enough( err.span_label(proper_span, "temporary value does not live long enough"); err.span_label(drop_span, "temporary value only lives until here"); - // Only give this note and suggestion if they could be relevant - match reason { - BorrowContainsPointReason::Liveness {..} - | BorrowContainsPointReason::DropLiveness {..} => { + let explanation = self.explain_why_borrow_contains_point(context, borrow, None); + match explanation { + BorrowExplanation::UsedLater(..) | + BorrowExplanation::UsedLaterInLoop(..) | + BorrowExplanation::UsedLaterWhenDropped(..) => { + // Only give this note and suggestion if it could be relevant. err.note("consider using a `let` binding to create a longer lived value"); - } - BorrowContainsPointReason::OutlivesFreeRegion {..} => (), + }, + _ => {}, } + explanation.emit(self.tcx, &mut err); - self.report_why_borrow_contains_point(&mut err, reason, None); err } @@ -749,7 +747,7 @@ pub(super) fn report_illegal_mutation_of_borrowed( loan_spans.var_span_label(&mut err, "borrow occurs due to use in closure"); - self.explain_why_borrow_contains_point(context, loan, None, &mut err); + self.explain_why_borrow_contains_point(context, loan, None).emit(self.tcx, &mut err); err.buffer(&mut self.errors_buffer); } diff --git a/src/librustc_mir/borrow_check/nll/explain_borrow/find_use.rs b/src/librustc_mir/borrow_check/nll/explain_borrow/find_use.rs index a35117f2e35..d4adff7c443 100644 --- a/src/librustc_mir/borrow_check/nll/explain_borrow/find_use.rs +++ b/src/librustc_mir/borrow_check/nll/explain_borrow/find_use.rs @@ -121,9 +121,7 @@ struct DefUseVisitor<'cx, 'gcx: 'tcx, 'tcx: 'cx> { enum DefUseResult { Def, - UseLive { local: Local }, - UseDrop { local: Local }, } diff --git a/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs b/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs index 61972629e7b..27fb06bdaec 100644 --- a/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs +++ b/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs @@ -11,26 +11,71 @@ use borrow_check::borrow_set::BorrowData; use borrow_check::nll::region_infer::Cause; use borrow_check::{Context, MirBorrowckCtxt, WriteKind}; -use rustc::mir::{FakeReadCause, Local, Location, Place, TerminatorKind}; +use rustc::ty::{Region, TyCtxt}; +use rustc::mir::{FakeReadCause, Location, Place, TerminatorKind}; use rustc_errors::DiagnosticBuilder; -use rustc::ty::Region; +use syntax_pos::Span; +use syntax_pos::symbol::Symbol; mod find_use; -#[derive(Copy, Clone, Debug)] -pub enum BorrowContainsPointReason<'tcx> { - Liveness { - local: Local, - location: Location, - in_loop: bool, - }, - DropLiveness { - local: Local, - location: Location, - }, - OutlivesFreeRegion { - outlived_region: Option>, - }, +pub(in borrow_check) enum BorrowExplanation<'tcx> { + UsedLater(bool, Option, Span), + UsedLaterInLoop(bool, Span), + UsedLaterWhenDropped(Span, Symbol, bool), + MustBeValidFor(Region<'tcx>), + Unexplained, +} + +impl<'tcx> BorrowExplanation<'tcx> { + pub(in borrow_check) fn emit<'cx, 'gcx>( + &self, + tcx: TyCtxt<'cx, 'gcx, 'tcx>, + err: &mut DiagnosticBuilder<'_> + ) { + match *self { + BorrowExplanation::UsedLater(is_in_closure, fake_read_cause, var_or_use_span) => { + let message = if is_in_closure { + "borrow later captured here by closure" + } else if let Some(FakeReadCause::ForLet) = fake_read_cause { + "borrow later stored here" + } else { + "borrow later used here" + }; + err.span_label(var_or_use_span, message); + }, + BorrowExplanation::UsedLaterInLoop(is_in_closure, var_or_use_span) => { + let message = if is_in_closure { + "borrow captured here by closure in later iteration of loop" + } else { + "borrow used here in later iteration of loop" + }; + err.span_label(var_or_use_span, message); + }, + BorrowExplanation::UsedLaterWhenDropped(span, local_name, should_note_order) => { + err.span_label( + span, + format!("borrow later used here, when `{}` is dropped", local_name), + ); + + if should_note_order { + err.note( + "values in a scope are dropped \ + in the opposite order they are defined", + ); + } + }, + BorrowExplanation::MustBeValidFor(region) => { + tcx.note_and_explain_free_region( + err, + "borrowed value must be valid for ", + region, + "...", + ); + }, + _ => {}, + } + } } impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { @@ -53,23 +98,7 @@ pub(in borrow_check) fn explain_why_borrow_contains_point( context: Context, borrow: &BorrowData<'tcx>, kind_place: Option<(WriteKind, &Place<'tcx>)>, - err: &mut DiagnosticBuilder<'_>, - ) { - let reason = self.find_why_borrow_contains_point(context, borrow); - self.report_why_borrow_contains_point(err, reason, kind_place); - } - - /// Finds the reason that [explain_why_borrow_contains_point] will report - /// but doesn't add it to any message. This is a separate function in case - /// the caller wants to change the error they report based on the reason - /// that will be reported. - pub(in borrow_check) fn find_why_borrow_contains_point( - &self, - context: Context, - borrow: &BorrowData<'tcx> - ) -> BorrowContainsPointReason<'tcx> { - use self::BorrowContainsPointReason::*; - + ) -> BorrowExplanation<'tcx> { debug!( "find_why_borrow_contains_point(context={:?}, borrow={:?})", context, borrow, @@ -80,113 +109,67 @@ pub(in borrow_check) fn find_why_borrow_contains_point( let tcx = self.tcx; let borrow_region_vid = regioncx.to_region_vid(borrow.region); - debug!( "explain_why_borrow_contains_point: borrow_region_vid={:?}", borrow_region_vid ); let region_sub = regioncx.find_sub_region_live_at(borrow_region_vid, context.loc); - debug!( "explain_why_borrow_contains_point: region_sub={:?}", region_sub ); - match find_use::find(mir, regioncx, tcx, region_sub, context.loc) { - Some(Cause::LiveVar(local, location)) => Liveness { - local, - location, - in_loop: self.is_borrow_location_in_loop(context.loc), - }, - Some(Cause::DropVar(local, location)) => DropLiveness { - local, - location, - }, - None => OutlivesFreeRegion { - outlived_region: regioncx.to_error_region(region_sub), - }, - } - } - - /// Adds annotations to `err` for the explanation `reason`. This is a - /// separate method so that the caller can change their error message based - /// on the reason that is going to be reported. - pub (in borrow_check) fn report_why_borrow_contains_point( - &self, - err: &mut DiagnosticBuilder, - reason: BorrowContainsPointReason<'tcx>, - kind_place: Option<(WriteKind, &Place<'tcx>)>, - ) { - use self::BorrowContainsPointReason::*; - - debug!( - "find_why_borrow_contains_point(reason={:?}, kind_place={:?})", - reason, kind_place, - ); - - let mir = self.mir; - - match reason { - Liveness { local, location, in_loop } => { + match find_use::find(mir, regioncx, tcx, region_sub, context.loc) { + Some(Cause::LiveVar(local, location)) => { let span = mir.source_info(location).span; let spans = self.move_spans(&Place::Local(local), location) .or_else(|| self.borrow_spans(span, location)); - let message = if in_loop { - if spans.for_closure() { - "borrow captured here by closure in later iteration of loop" - } else { - "borrow used here in later iteration of loop" - } + + if self.is_borrow_location_in_loop(context.loc) { + BorrowExplanation::UsedLaterInLoop(spans.for_closure(), spans.var_or_use()) } else { - if spans.for_closure() { - "borrow later captured here by closure" - } else { - // Check if the location represents a `FakeRead`, and adapt the error - // message to the `FakeReadCause` it is from: in particular, - // the ones inserted in optimized `let var = ` patterns. - match self.retrieve_fake_read_cause_for_location(&location) { - Some(FakeReadCause::ForLet) => "borrow later stored here", - _ => "borrow later used here" - } - } - }; - err.span_label(spans.var_or_use(), message); + // Check if the location represents a `FakeRead`, and adapt the error + // message to the `FakeReadCause` it is from: in particular, + // the ones inserted in optimized `let var = ` patterns. + BorrowExplanation::UsedLater( + spans.for_closure(), + self.retrieve_fake_read_cause_for_location(&location), + spans.var_or_use() + ) + } } - DropLiveness { local, location } => match &mir.local_decls[local].name { - Some(local_name) => { - err.span_label( - mir.source_info(location).span, - format!("borrow later used here, when `{}` is dropped", local_name), - ); + Some(Cause::DropVar(local, location)) => match &mir.local_decls[local].name { + Some(local_name) => { + let mut should_note_order = false; if let Some((WriteKind::StorageDeadOrDrop(_), place)) = kind_place { if let Place::Local(borrowed_local) = place { let dropped_local_scope = mir.local_decls[local].visibility_scope; let borrowed_local_scope = - mir.local_decls[*borrowed_local].visibility_scope; + mir.local_decls[*borrowed_local].visibility_scope; if mir.is_sub_scope(borrowed_local_scope, dropped_local_scope) { - err.note( - "values in a scope are dropped \ - in the opposite order they are defined", - ); + should_note_order = true; } } } - } - None => {} - } - OutlivesFreeRegion { outlived_region: Some(region) } => { - self.tcx.note_and_explain_free_region( - err, - "borrowed value must be valid for ", - region, - "...", - ); - } - OutlivesFreeRegion { outlived_region: None } => (), + BorrowExplanation::UsedLaterWhenDropped( + mir.source_info(location).span, + *local_name, + should_note_order + ) + }, + + None => BorrowExplanation::Unexplained, + }, + + None => if let Some(region) = regioncx.to_error_region(region_sub) { + BorrowExplanation::MustBeValidFor(region) + } else { + BorrowExplanation::Unexplained + }, } } @@ -262,4 +245,3 @@ fn is_borrow_location_in_loop( false } } - -- 2.44.0