From aa701154f0a54b6a594f45fa228c482d604ab357 Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 10 Oct 2018 21:56:17 +0200 Subject: [PATCH] Extend closure special-casing for generators. This commit extends existing special-casing of closures to highlight the use of variables within generators that are causing the generator to borrow them. --- src/librustc_borrowck/borrowck/check_loans.rs | 4 +- .../borrow_check/error_reporting.rs | 278 +++++++++++------- src/librustc_mir/util/borrowck_errors.rs | 6 +- src/test/ui/generator/borrowing.nll.stderr | 31 +- src/test/ui/generator/dropck.nll.stderr | 24 +- .../yield-while-iterating.nll.stderr | 20 +- .../yield-while-ref-reborrowed.nll.stderr | 20 +- 7 files changed, 228 insertions(+), 155 deletions(-) diff --git a/src/librustc_borrowck/borrowck/check_loans.rs b/src/librustc_borrowck/borrowck/check_loans.rs index 34ee03b895f..d9b64527700 100644 --- a/src/librustc_borrowck/borrowck/check_loans.rs +++ b/src/librustc_borrowck/borrowck/check_loans.rs @@ -609,12 +609,12 @@ pub fn report_error_if_loan_conflicts_with_restriction(&self, new_loan.span, &nl, old_loan.span, previous_end_span, Origin::Ast), (ty::UniqueImmBorrow, _) => self.bccx.cannot_uniquely_borrow_by_one_closure( - new_loan.span, &nl, &new_loan_msg, + new_loan.span, "closure", &nl, &new_loan_msg, old_loan.span, &ol_pronoun, &old_loan_msg, previous_end_span, Origin::Ast), (_, ty::UniqueImmBorrow) => { let new_loan_str = &new_loan.kind.to_user_str(); self.bccx.cannot_reborrow_already_uniquely_borrowed( - new_loan.span, &nl, &new_loan_msg, new_loan_str, + new_loan.span, "closure", &nl, &new_loan_msg, new_loan_str, old_loan.span, &old_loan_msg, previous_end_span, Origin::Ast) } (..) => diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 759b842e9df..99077ce8b09 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -103,7 +103,7 @@ pub(super) fn report_use_of_moved_or_uninitialized( use_spans.var_span_label( &mut err, - format!("{} occurs due to use in closure", desired_action.as_noun()), + format!("{} occurs due to use{}", desired_action.as_noun(), use_spans.describe()), ); err.buffer(&mut self.errors_buffer); @@ -161,13 +161,16 @@ pub(super) fn report_use_of_moved_or_uninitialized( ); } else { err.span_label(move_span, format!("value moved{} here", move_msg)); - move_spans.var_span_label(&mut err, "variable moved due to use in closure"); + move_spans.var_span_label( + &mut err, + format!("variable moved due to use{}", move_spans.describe()), + ); }; } use_spans.var_span_label( &mut err, - format!("{} occurs due to use in closure", desired_action.as_noun()), + format!("{} occurs due to use{}", desired_action.as_noun(), use_spans.describe()), ); if !is_loop_move { @@ -226,9 +229,13 @@ pub(super) fn report_use_of_moved_or_uninitialized( pub(super) fn report_move_out_while_borrowed( &mut self, context: Context, - (place, _span): (&Place<'tcx>, Span), + (place, span): (&Place<'tcx>, Span), borrow: &BorrowData<'tcx>, ) { + debug!( + "report_move_out_while_borrowed: context={:?} place={:?} span={:?} borrow={:?}", + context, place, span, borrow + ); let tcx = self.infcx.tcx; let value_msg = match self.describe_place(place) { Some(name) => format!("`{}`", name), @@ -253,9 +260,15 @@ pub(super) fn report_move_out_while_borrowed( err.span_label(borrow_span, format!("borrow of {} occurs here", borrow_msg)); err.span_label(span, format!("move out of {} occurs here", value_msg)); - borrow_spans.var_span_label(&mut err, "borrow occurs due to use in closure"); + borrow_spans.var_span_label( + &mut err, + format!("borrow occurs due to use{}", borrow_spans.describe()) + ); - move_spans.var_span_label(&mut err, "move occurs due to use in closure"); + move_spans.var_span_label( + &mut err, + format!("move occurs due to use{}", move_spans.describe()) + ); self.explain_why_borrow_contains_point(context, borrow, None) .add_explanation_to_diagnostic(self.infcx.tcx, self.mir, &mut err, ""); @@ -291,7 +304,7 @@ pub(super) fn report_use_while_mutably_borrowed( let place = &borrow.borrowed_place; let desc_place = self.describe_place(place).unwrap_or("_".to_owned()); - format!("borrow occurs due to use of `{}` in closure", desc_place) + format!("borrow occurs due to use of `{}`{}", desc_place, borrow_spans.describe()) }); self.explain_why_borrow_contains_point(context, borrow, None) @@ -312,6 +325,12 @@ pub(super) fn report_conflicting_borrow( let borrow_spans = self.borrow_spans(span, context.loc); let span = borrow_spans.args_or_use(); + let container_name = if issued_spans.for_generator() || borrow_spans.for_generator() { + "generator" + } else { + "closure" + }; + let desc_place = self.describe_place(place).unwrap_or("_".to_owned()); let tcx = self.infcx.tcx; @@ -392,7 +411,9 @@ pub(super) fn report_conflicting_borrow( ); borrow_spans.var_span_label( &mut err, - format!("borrow occurs due to use of `{}` in closure", desc_place), + format!( + "borrow occurs due to use of `{}`{}", desc_place, borrow_spans.describe() + ), ); err.buffer(&mut self.errors_buffer); @@ -403,6 +424,7 @@ pub(super) fn report_conflicting_borrow( first_borrow_desc = "first "; tcx.cannot_uniquely_borrow_by_one_closure( span, + container_name, &desc_place, "", issued_span, @@ -417,6 +439,7 @@ pub(super) fn report_conflicting_borrow( first_borrow_desc = "first "; tcx.cannot_reborrow_already_uniquely_borrowed( span, + container_name, &desc_place, "", lft, @@ -431,6 +454,7 @@ pub(super) fn report_conflicting_borrow( first_borrow_desc = "first "; tcx.cannot_reborrow_already_uniquely_borrowed( span, + container_name, &desc_place, "", lft, @@ -456,7 +480,7 @@ pub(super) fn report_conflicting_borrow( if issued_spans == borrow_spans { borrow_spans.var_span_label( &mut err, - format!("borrows occur due to use of `{}` in closure", desc_place), + format!("borrows occur due to use of `{}`{}", desc_place, borrow_spans.describe()), ); } else { let borrow_place = &issued_borrow.borrowed_place; @@ -464,16 +488,18 @@ pub(super) fn report_conflicting_borrow( issued_spans.var_span_label( &mut err, format!( - "first borrow occurs due to use of `{}` in closure", - borrow_place_desc + "first borrow occurs due to use of `{}`{}", + borrow_place_desc, + issued_spans.describe(), ), ); borrow_spans.var_span_label( &mut err, format!( - "second borrow occurs due to use of `{}` in closure", - desc_place + "second borrow occurs due to use of `{}`{}", + desc_place, + borrow_spans.describe(), ), ); } @@ -643,7 +669,16 @@ fn report_local_value_does_not_live_long_enough( format!("`{}` dropped here while still borrowed", name), ); - borrow_spans.args_span_label(&mut err, "value captured here"); + let within = if borrow_spans.for_generator() { + " by generator" + } else { + "" + }; + + borrow_spans.args_span_label( + &mut err, + format!("value captured here{}", within), + ); explanation.add_explanation_to_diagnostic(self.infcx.tcx, self.mir, &mut err, ""); } @@ -774,7 +809,16 @@ fn report_temporary_value_does_not_live_long_enough( } explanation.add_explanation_to_diagnostic(self.infcx.tcx, self.mir, &mut err, ""); - borrow_spans.args_span_label(&mut err, "value captured here"); + let within = if borrow_spans.for_generator() { + " by generator" + } else { + "" + }; + + borrow_spans.args_span_label( + &mut err, + format!("value captured here{}", within), + ); err } @@ -906,7 +950,10 @@ pub(super) fn report_illegal_mutation_of_borrowed( ) }; - loan_spans.var_span_label(&mut err, "borrow occurs due to use in closure"); + loan_spans.var_span_label( + &mut err, + format!("borrow occurs due to use{}", loan_spans.describe()), + ); self.explain_why_borrow_contains_point(context, loan, None) .add_explanation_to_diagnostic(self.infcx.tcx, self.mir, &mut err, ""); @@ -1805,6 +1852,8 @@ fn get_region_name_for_ty(&self, ty: ty::Ty<'tcx>, counter: usize) -> String { pub(super) enum UseSpans { // The access is caused by capturing a variable for a closure. ClosureUse { + // This is true if the captured variable was from a generator. + is_generator: bool, // The span of the args of the closure, including the `move` keyword if // it's present. args_span: Span, @@ -1845,10 +1894,31 @@ pub(super) fn var_span_label(self, err: &mut DiagnosticBuilder, message: impl In } } - pub(super) fn for_closure(self) -> bool { - match self { - UseSpans::ClosureUse { .. } => true, - UseSpans::OtherUse(_) => false, + /// Return `false` if this place is not used in a closure. + fn for_closure(&self) -> bool { + match *self { + UseSpans::ClosureUse { is_generator, .. } => !is_generator, + _ => false, + } + } + + /// Return `false` if this place is not used in a generator. + fn for_generator(&self) -> bool { + match *self { + UseSpans::ClosureUse { is_generator, .. } => is_generator, + _ => false, + } + } + + /// Describe the span associated with a use of a place. + fn describe(&self) -> String { + match *self { + UseSpans::ClosureUse { is_generator, .. } => if is_generator { + " in generator".to_string() + } else { + " in closure".to_string() + }, + _ => "".to_string(), } } @@ -1871,53 +1941,37 @@ pub(super) fn move_spans( location: Location, ) -> UseSpans { use self::UseSpans::*; - use rustc::hir::ExprKind::Closure; - use rustc::mir::AggregateKind; - let stmt = match self.mir[location.block] - .statements - .get(location.statement_index) - { + let stmt = match self.mir[location.block].statements.get(location.statement_index) { Some(stmt) => stmt, None => return OtherUse(self.mir.source_info(location).span), }; - if let StatementKind::Assign(_, box Rvalue::Aggregate(ref kind, ref places)) = stmt.kind { - if let AggregateKind::Closure(def_id, _) = **kind { - debug!("find_closure_move_span: found closure {:?}", places); + debug!("move_spans: moved_place={:?} location={:?} stmt={:?}", moved_place, location, stmt); + if let StatementKind::Assign( + _, + box Rvalue::Aggregate(ref kind, ref places) + ) = stmt.kind { + let (def_id, is_generator) = match kind { + box AggregateKind::Closure(def_id, _) => (def_id, false), + box AggregateKind::Generator(def_id, _, _) => (def_id, true), + _ => return OtherUse(stmt.source_info.span), + }; - if let Some(node_id) = self.infcx.tcx.hir.as_local_node_id(def_id) { - if let Closure(_, _, _, args_span, _) = - self.infcx.tcx.hir.expect_expr(node_id).node - { - if let Some(var_span) = self.infcx.tcx.with_freevars(node_id, |freevars| { - for (v, place) in freevars.iter().zip(places) { - match place { - Operand::Copy(place) | Operand::Move(place) - if moved_place == place => - { - debug!( - "find_closure_move_span: found captured local {:?}", - place - ); - return Some(v.span); - } - _ => {} - } - } - None - }) { - return ClosureUse { - args_span, - var_span, - }; - } - } - } + debug!( + "move_spans: def_id={:?} is_generator={:?} places={:?}", + def_id, is_generator, places + ); + if let Some((args_span, var_span)) = self.closure_span(*def_id, moved_place, places) { + return ClosureUse { + is_generator, + args_span, + var_span, + }; } } - return OtherUse(stmt.source_info.span); + OtherUse(stmt.source_info.span) } /// Finds the span of arguments of a closure (within `maybe_closure_span`) @@ -1926,9 +1980,9 @@ pub(super) fn move_spans( /// and originating from `maybe_closure_span`. pub(super) fn borrow_spans(&self, use_span: Span, location: Location) -> UseSpans { use self::UseSpans::*; - use rustc::hir::ExprKind::Closure; + debug!("borrow_spans: use_span={:?} location={:?}", use_span, location); - let local = match self.mir[location.block] + let target = match self.mir[location.block] .statements .get(location.statement_index) { @@ -1939,54 +1993,35 @@ pub(super) fn borrow_spans(&self, use_span: Span, location: Location) -> UseSpan _ => return OtherUse(use_span), }; - if self.mir.local_kind(local) != LocalKind::Temp { + if self.mir.local_kind(target) != LocalKind::Temp { // operands are always temporaries. return OtherUse(use_span); } for stmt in &self.mir[location.block].statements[location.statement_index + 1..] { - if let StatementKind::Assign(_, box Rvalue::Aggregate(ref kind, ref places)) = stmt.kind - { - if let AggregateKind::Closure(def_id, _) = **kind { - debug!("find_closure_borrow_span: found closure {:?}", places); - - return if let Some(node_id) = self.infcx.tcx.hir.as_local_node_id(def_id) { - let args_span = if let Closure(_, _, _, span, _) = - self.infcx.tcx.hir.expect_expr(node_id).node - { - span - } else { - return OtherUse(use_span); - }; + if let StatementKind::Assign( + _, box Rvalue::Aggregate(ref kind, ref places) + ) = stmt.kind { + let (def_id, is_generator) = match kind { + box AggregateKind::Closure(def_id, _) => (def_id, false), + box AggregateKind::Generator(def_id, _, _) => (def_id, true), + _ => continue, + }; - self.infcx - .tcx - .with_freevars(node_id, |freevars| { - for (v, place) in freevars.iter().zip(places) { - match *place { - Operand::Copy(Place::Local(l)) - | Operand::Move(Place::Local(l)) if local == l => - { - debug!( - "find_closure_borrow_span: found captured local \ - {:?}", - l - ); - return Some(v.span); - } - _ => {} - } - } - None - }) - .map(|var_span| ClosureUse { - args_span, - var_span, - }) - .unwrap_or(OtherUse(use_span)) - } else { - OtherUse(use_span) + debug!( + "borrow_spans: def_id={:?} is_generator={:?} places={:?}", + def_id, is_generator, places + ); + if let Some((args_span, var_span)) = self.closure_span( + *def_id, &Place::Local(target), places + ) { + return ClosureUse { + is_generator, + args_span, + var_span, }; + } else { + return OtherUse(use_span); } } @@ -1998,6 +2033,47 @@ pub(super) fn borrow_spans(&self, use_span: Span, location: Location) -> UseSpan OtherUse(use_span) } + /// Finds the span of a captured variable within a closure or generator. + fn closure_span( + &self, + def_id: DefId, + target_place: &Place<'tcx>, + places: &Vec>, + ) -> Option<(Span, Span)> { + debug!( + "closure_span: def_id={:?} target_place={:?} places={:?}", + def_id, target_place, places + ); + let node_id = self.infcx.tcx.hir.as_local_node_id(def_id)?; + let expr = &self.infcx.tcx.hir.expect_expr(node_id).node; + debug!("closure_span: node_id={:?} expr={:?}", node_id, expr); + if let hir::ExprKind::Closure( + .., args_span, _ + ) = expr { + let var_span = self.infcx.tcx.with_freevars( + node_id, + |freevars| { + for (v, place) in freevars.iter().zip(places) { + match place { + Operand::Copy(place) | + Operand::Move(place) if target_place == place => { + debug!("closure_span: found captured local {:?}", place); + return Some(v.span); + }, + _ => {} + } + } + + None + }, + )?; + + Some((*args_span, var_span)) + } else { + None + } + } + /// Helper to retrieve span(s) of given borrow from the current MIR /// representation pub(super) fn retrieve_borrow_spans(&self, borrow: &BorrowData) -> UseSpans { diff --git a/src/librustc_mir/util/borrowck_errors.rs b/src/librustc_mir/util/borrowck_errors.rs index 2616d0cd964..d5c655a3de4 100644 --- a/src/librustc_mir/util/borrowck_errors.rs +++ b/src/librustc_mir/util/borrowck_errors.rs @@ -221,6 +221,7 @@ fn cannot_uniquely_borrow_by_two_closures( fn cannot_uniquely_borrow_by_one_closure( self, new_loan_span: Span, + container_name: &str, desc_new: &str, opt_via: &str, old_loan_span: Span, @@ -241,7 +242,7 @@ fn cannot_uniquely_borrow_by_one_closure( ); err.span_label( new_loan_span, - format!("closure construction occurs here{}", opt_via), + format!("{} construction occurs here{}", container_name, opt_via), ); err.span_label(old_loan_span, format!("borrow occurs here{}", old_opt_via)); if let Some(previous_end_span) = previous_end_span { @@ -253,6 +254,7 @@ fn cannot_uniquely_borrow_by_one_closure( fn cannot_reborrow_already_uniquely_borrowed( self, new_loan_span: Span, + container_name: &str, desc_new: &str, opt_via: &str, kind_new: &str, @@ -275,7 +277,7 @@ fn cannot_reborrow_already_uniquely_borrowed( err.span_label(new_loan_span, format!("borrow occurs here{}", opt_via)); err.span_label( old_loan_span, - format!("closure construction occurs here{}", old_opt_via), + format!("{} construction occurs here{}", container_name, old_opt_via), ); if let Some(previous_end_span) = previous_end_span { err.span_label(previous_end_span, "borrow from closure ends here"); diff --git a/src/test/ui/generator/borrowing.nll.stderr b/src/test/ui/generator/borrowing.nll.stderr index 9d1a52a8335..52b5c9d891b 100644 --- a/src/test/ui/generator/borrowing.nll.stderr +++ b/src/test/ui/generator/borrowing.nll.stderr @@ -1,10 +1,11 @@ error[E0597]: `a` does not live long enough - --> $DIR/borrowing.rs:18:18 + --> $DIR/borrowing.rs:18:29 | LL | unsafe { (|| yield &a).resume() } - | ^^^^^^^^^^^^^ - | | - | borrowed value does not live long enough + | -----------^- + | || | + | || borrowed value does not live long enough + | |value captured here by generator | a temporary with access to the borrow is created here ... LL | //~^ ERROR: `a` does not live long enough LL | }; @@ -15,18 +16,18 @@ LL | }; = note: The temporary is part of an expression at the end of a block. Consider forcing this temporary to be dropped sooner, before the block's local variables are dropped. For example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block. error[E0597]: `a` does not live long enough - --> $DIR/borrowing.rs:24:9 + --> $DIR/borrowing.rs:25:20 | -LL | let _b = { - | -- borrow later stored here -LL | let a = 3; -LL | / || { -LL | | yield &a -LL | | //~^ ERROR: `a` does not live long enough -LL | | } - | |_________^ borrowed value does not live long enough -LL | }; - | - `a` dropped here while still borrowed +LL | let _b = { + | -- borrow later stored here +LL | let a = 3; +LL | || { + | -- value captured here by generator +LL | yield &a + | ^ borrowed value does not live long enough +... +LL | }; + | - `a` dropped here while still borrowed error: aborting due to 2 previous errors diff --git a/src/test/ui/generator/dropck.nll.stderr b/src/test/ui/generator/dropck.nll.stderr index 2b95a5caf6d..078aaf6176a 100644 --- a/src/test/ui/generator/dropck.nll.stderr +++ b/src/test/ui/generator/dropck.nll.stderr @@ -13,21 +13,19 @@ LL | } = note: values in a scope are dropped in the opposite order they are defined error[E0597]: `ref_` does not live long enough - --> $DIR/dropck.rs:22:11 + --> $DIR/dropck.rs:24:18 | -LL | gen = || { - | ___________^ -LL | | // but the generator can use it to drop a `Ref<'a, i32>`. -LL | | let _d = ref_.take(); //~ ERROR `ref_` does not live long enough -LL | | yield; -LL | | }; - | |_____^ borrowed value does not live long enough +LL | gen = || { + | -- value captured here by generator +LL | // but the generator can use it to drop a `Ref<'a, i32>`. +LL | let _d = ref_.take(); //~ ERROR `ref_` does not live long enough + | ^^^^ borrowed value does not live long enough ... -LL | } - | - - | | - | `ref_` dropped here while still borrowed - | borrow might be used here, when `gen` is dropped and runs the destructor for generator +LL | } + | - + | | + | `ref_` dropped here while still borrowed + | borrow might be used here, when `gen` is dropped and runs the destructor for generator | = note: values in a scope are dropped in the opposite order they are defined diff --git a/src/test/ui/generator/yield-while-iterating.nll.stderr b/src/test/ui/generator/yield-while-iterating.nll.stderr index d332df6e4ba..2f0a0589844 100644 --- a/src/test/ui/generator/yield-while-iterating.nll.stderr +++ b/src/test/ui/generator/yield-while-iterating.nll.stderr @@ -9,17 +9,15 @@ LL | yield(); error[E0502]: cannot borrow `x` as immutable because it is also borrowed as mutable --> $DIR/yield-while-iterating.rs:67:20 | -LL | let mut b = || { - | _________________- -LL | | for p in &mut x { -LL | | yield p; -LL | | } -LL | | }; - | |_____- mutable borrow occurs here -LL | println!("{}", x[0]); //~ ERROR - | ^ immutable borrow occurs here -LL | b.resume(); - | - mutable borrow later used here +LL | let mut b = || { + | -- mutable borrow occurs here +LL | for p in &mut x { + | - first borrow occurs due to use of `x` in generator +... +LL | println!("{}", x[0]); //~ ERROR + | ^ immutable borrow occurs here +LL | b.resume(); + | - mutable borrow later used here error: aborting due to 2 previous errors diff --git a/src/test/ui/generator/yield-while-ref-reborrowed.nll.stderr b/src/test/ui/generator/yield-while-ref-reborrowed.nll.stderr index b5c392c51ec..8dabb3c2505 100644 --- a/src/test/ui/generator/yield-while-ref-reborrowed.nll.stderr +++ b/src/test/ui/generator/yield-while-ref-reborrowed.nll.stderr @@ -1,17 +1,15 @@ error[E0501]: cannot borrow `x` as immutable because previous closure requires unique access --> $DIR/yield-while-ref-reborrowed.rs:45:20 | -LL | let mut b = || { - | _________________- -LL | | let a = &mut *x; -LL | | yield(); -LL | | println!("{}", a); -LL | | }; - | |_____- closure construction occurs here -LL | println!("{}", x); //~ ERROR - | ^ borrow occurs here -LL | b.resume(); - | - first borrow later used here +LL | let mut b = || { + | -- generator construction occurs here +LL | let a = &mut *x; + | - first borrow occurs due to use of `x` in generator +... +LL | println!("{}", x); //~ ERROR + | ^ borrow occurs here +LL | b.resume(); + | - first borrow later used here error: aborting due to previous error -- 2.44.0