From aa701154f0a54b6a594f45fa228c482d604ab357 Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 10 Oct 2018 21:56:17 +0200 Subject: [PATCH 1/3] 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 34ee03b895f9f..d9b64527700fc 100644 --- a/src/librustc_borrowck/borrowck/check_loans.rs +++ b/src/librustc_borrowck/borrowck/check_loans.rs @@ -609,12 +609,12 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { 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 759b842e9dfee..99077ce8b09be 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -103,7 +103,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ); } 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ); 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { first_borrow_desc = "first "; tcx.cannot_uniquely_borrow_by_one_closure( span, + container_name, &desc_place, "", issued_span, @@ -417,6 +439,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { first_borrow_desc = "first "; tcx.cannot_reborrow_already_uniquely_borrowed( span, + container_name, &desc_place, "", lft, @@ -431,6 +454,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { first_borrow_desc = "first "; tcx.cannot_reborrow_already_uniquely_borrowed( span, + container_name, &desc_place, "", lft, @@ -456,7 +480,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ) }; - 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 @@ impl<'tcx> AnnotatedBorrowFnSignature<'tcx> { 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 @@ impl UseSpans { } } - 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { /// 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { _ => 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 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 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 2616d0cd9640b..d5c655a3de472 100644 --- a/src/librustc_mir/util/borrowck_errors.rs +++ b/src/librustc_mir/util/borrowck_errors.rs @@ -221,6 +221,7 @@ pub trait BorrowckErrors<'cx>: Sized + Copy { 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 @@ pub trait BorrowckErrors<'cx>: Sized + Copy { ); 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 @@ pub trait BorrowckErrors<'cx>: Sized + Copy { 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 @@ pub trait BorrowckErrors<'cx>: Sized + Copy { 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 9d1a52a833543..52b5c9d891bb3 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 2b95a5caf6da0..078aaf6176ab8 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 d332df6e4ba46..2f0a05898444c 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 b5c392c51ece5..8dabb3c2505b6 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 From 375645abb89d1b98490c744dc980fb96aa076e73 Mon Sep 17 00:00:00 2001 From: David Wood Date: Thu, 11 Oct 2018 01:19:55 +0200 Subject: [PATCH 2/3] Add by-value captured variable note on second use. This commit adds a note that was present in the AST borrow checker when closures are invoked more than once and have captured variables by-value. --- .../borrow_check/error_reporting.rs | 136 ++++++++++++++++-- .../ui/closure_context/issue-42065.nll.stderr | 11 -- src/test/ui/issues/issue-12127.nll.stderr | 18 +++ ...losures-infer-fnonce-call-twice.nll.stderr | 11 -- ...es-infer-fnonce-move-call-twice.nll.stderr | 11 -- 5 files changed, 140 insertions(+), 47 deletions(-) delete mode 100644 src/test/ui/closure_context/issue-42065.nll.stderr create mode 100644 src/test/ui/issues/issue-12127.nll.stderr delete mode 100644 src/test/ui/unboxed-closures/unboxed-closures-infer-fnonce-call-twice.nll.stderr delete mode 100644 src/test/ui/unboxed-closures/unboxed-closures-infer-fnonce-move-call-twice.nll.stderr diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 99077ce8b09be..3903506068ed7 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -15,11 +15,11 @@ use rustc::hir; use rustc::hir::def_id::DefId; use rustc::middle::region::ScopeTree; use rustc::mir::{ - self, AggregateKind, BindingForm, BorrowKind, ClearCrossCrate, Field, Local, + self, AggregateKind, BindingForm, BorrowKind, ClearCrossCrate, Constant, Field, Local, LocalDecl, LocalKind, Location, Operand, Place, PlaceProjection, ProjectionElem, Rvalue, Statement, StatementKind, TerminatorKind, VarBindingForm, }; -use rustc::ty; +use rustc::ty::{self, DefIdTree}; use rustc::util::ppaux::with_highlight_region_for_bound_region; use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::sync::Lrc; @@ -131,6 +131,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Origin::Mir, ); + self.add_closure_invoked_twice_with_moved_variable_suggestion( + context.loc, + used_place, + &mut err, + ); + let mut is_loop_move = false; for move_site in &move_site_vec { let move_out = self.move_data.moves[(*move_site).moi]; @@ -1056,16 +1062,118 @@ enum StorageDeadOrDrop<'tcx> { } impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { - // End-user visible description of `place` if one can be found. If the - // place is a temporary for instance, None will be returned. + + /// Adds a suggestion when a closure is invoked twice with a moved variable. + /// + /// ```text + /// note: closure cannot be invoked more than once because it moves the variable `dict` out of + /// its environment + /// --> $DIR/issue-42065.rs:16:29 + /// | + /// LL | for (key, value) in dict { + /// | ^^^^ + /// ``` + pub(super) fn add_closure_invoked_twice_with_moved_variable_suggestion( + &self, + location: Location, + place: &Place<'tcx>, + diag: &mut DiagnosticBuilder<'_>, + ) { + let mut target = place.local(); + debug!( + "add_closure_invoked_twice_with_moved_variable_suggestion: location={:?} place={:?} \ + target={:?}", + location, place, target, + ); + for stmt in &self.mir[location.block].statements[location.statement_index..] { + debug!( + "add_closure_invoked_twice_with_moved_variable_suggestion: stmt={:?} \ + target={:?}", + stmt, target, + ); + if let StatementKind::Assign(into, box Rvalue::Use(from)) = &stmt.kind { + debug!( + "add_closure_invoked_twice_with_moved_variable_suggestion: into={:?} \ + from={:?}", + into, from, + ); + match from { + Operand::Copy(ref place) | + Operand::Move(ref place) if target == place.local() => + target = into.local(), + _ => {}, + } + } + } + + + let terminator = self.mir[location.block].terminator(); + debug!( + "add_closure_invoked_twice_with_moved_variable_suggestion: terminator={:?}", + terminator, + ); + if let TerminatorKind::Call { + func: Operand::Constant(box Constant { + literal: ty::Const { ty: &ty::TyS { sty: ty::TyKind::FnDef(id, _), .. }, .. }, + .. + }), + args, + .. + } = &terminator.kind { + debug!("add_closure_invoked_twice_with_moved_variable_suggestion: id={:?}", id); + if self.infcx.tcx.parent(id) == self.infcx.tcx.lang_items().fn_once_trait() { + let closure = match args.first() { + Some(Operand::Copy(ref place)) | + Some(Operand::Move(ref place)) if target == place.local() => + place.local().unwrap(), + _ => return, + }; + debug!( + "add_closure_invoked_twice_with_moved_variable_suggestion: closure={:?}", + closure, + ); + + if let ty::TyKind::Closure(did, substs) = self.mir.local_decls[closure].ty.sty { + let upvar_tys = substs.upvar_tys(did, self.infcx.tcx); + let node_id = match self.infcx.tcx.hir.as_local_node_id(did) { + Some(node_id) => node_id, + _ => return, + }; + + self.infcx.tcx.with_freevars(node_id, |freevars| { + for (freevar, upvar_ty) in freevars.iter().zip(upvar_tys) { + debug!( + "add_closure_invoked_twice_with_moved_variable_suggestion: \ + freevar={:?} upvar_ty={:?}", + freevar, upvar_ty, + ); + if !upvar_ty.is_region_ptr() { + diag.span_note( + freevar.span, + &format!( + "closure cannot be invoked more than once because it \ + moves the variable `{}` out of its environment", + self.infcx.tcx.hir.name(freevar.var_id()), + ), + ); + } + } + }); + } + } + } + } + + /// End-user visible description of `place` if one can be found. If the + /// place is a temporary for instance, None will be returned. pub(super) fn describe_place(&self, place: &Place<'tcx>) -> Option { self.describe_place_with_options(place, IncludingDowncast(false)) } - // End-user visible description of `place` if one can be found. If the - // place is a temporary for instance, None will be returned. - // `IncludingDowncast` parameter makes the function return `Err` if `ProjectionElem` is - // `Downcast` and `IncludingDowncast` is true + /// End-user visible description of `place` if one can be found. If the + /// place is a temporary for instance, None will be returned. + /// `IncludingDowncast` parameter makes the function return `Err` if `ProjectionElem` is + /// `Downcast` and `IncludingDowncast` is true pub(super) fn describe_place_with_options( &self, place: &Place<'tcx>, @@ -1078,7 +1186,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } - // Appends end-user visible description of `place` to `buf`. + /// Appends end-user visible description of `place` to `buf`. fn append_place_to_string( &self, place: &Place<'tcx>, @@ -1213,8 +1321,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Ok(()) } - // Appends end-user visible description of the `local` place to `buf`. If `local` doesn't have - // a name, then `Err` is returned + /// Appends end-user visible description of the `local` place to `buf`. If `local` doesn't have + /// a name, then `Err` is returned fn append_local_to_string(&self, local_index: Local, buf: &mut String) -> Result<(), ()> { let local = &self.mir.local_decls[local_index]; match local.name { @@ -1226,7 +1334,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } - // End-user visible description of the `field`nth field of `base` + /// End-user visible description of the `field`nth field of `base` fn describe_field(&self, base: &Place, field: Field) -> String { match *base { Place::Local(local) => { @@ -1251,7 +1359,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } - // End-user visible description of the `field_index`nth field of `ty` + /// End-user visible description of the `field_index`nth field of `ty` fn describe_field_from_ty(&self, ty: &ty::Ty, field: Field) -> String { if ty.is_box() { // If the type is a box, the field is described from the boxed type @@ -1294,7 +1402,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } - // Retrieve type of a place for the current MIR representation + /// Retrieve type of a place for the current MIR representation fn retrieve_type_for_place(&self, place: &Place<'tcx>) -> Option { match place { Place::Local(local) => { diff --git a/src/test/ui/closure_context/issue-42065.nll.stderr b/src/test/ui/closure_context/issue-42065.nll.stderr deleted file mode 100644 index bda8a3b85f758..0000000000000 --- a/src/test/ui/closure_context/issue-42065.nll.stderr +++ /dev/null @@ -1,11 +0,0 @@ -error[E0382]: use of moved value: `debug_dump_dict` - --> $DIR/issue-42065.rs:21:5 - | -LL | debug_dump_dict(); - | --------------- value moved here -LL | debug_dump_dict(); - | ^^^^^^^^^^^^^^^ value used here after move - -error: aborting due to previous error - -For more information about this error, try `rustc --explain E0382`. diff --git a/src/test/ui/issues/issue-12127.nll.stderr b/src/test/ui/issues/issue-12127.nll.stderr new file mode 100644 index 0000000000000..51dd2e72832b6 --- /dev/null +++ b/src/test/ui/issues/issue-12127.nll.stderr @@ -0,0 +1,18 @@ +error[E0382]: use of moved value: `f` + --> $DIR/issue-12127.rs:21:9 + | +LL | f(); + | - value moved here +LL | f(); + | ^ value used here after move + | +note: closure cannot be invoked more than once because it moves the variable `x` out of its environment + --> $DIR/issue-12127.rs:18:39 + | +LL | let f = to_fn_once(move|| do_it(&*x)); + | ^ + = note: move occurs because `f` has type `[closure@$DIR/issue-12127.rs:18:24: 18:41 x:std::boxed::Box]`, which does not implement the `Copy` trait + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0382`. diff --git a/src/test/ui/unboxed-closures/unboxed-closures-infer-fnonce-call-twice.nll.stderr b/src/test/ui/unboxed-closures/unboxed-closures-infer-fnonce-call-twice.nll.stderr deleted file mode 100644 index f4756696b6b06..0000000000000 --- a/src/test/ui/unboxed-closures/unboxed-closures-infer-fnonce-call-twice.nll.stderr +++ /dev/null @@ -1,11 +0,0 @@ -error[E0382]: use of moved value: `tick` - --> $DIR/unboxed-closures-infer-fnonce-call-twice.rs:20:5 - | -LL | tick(); - | ---- value moved here -LL | tick(); //~ ERROR use of moved value: `tick` - | ^^^^ value used here after move - -error: aborting due to previous error - -For more information about this error, try `rustc --explain E0382`. diff --git a/src/test/ui/unboxed-closures/unboxed-closures-infer-fnonce-move-call-twice.nll.stderr b/src/test/ui/unboxed-closures/unboxed-closures-infer-fnonce-move-call-twice.nll.stderr deleted file mode 100644 index 95ed673627877..0000000000000 --- a/src/test/ui/unboxed-closures/unboxed-closures-infer-fnonce-move-call-twice.nll.stderr +++ /dev/null @@ -1,11 +0,0 @@ -error[E0382]: use of moved value: `tick` - --> $DIR/unboxed-closures-infer-fnonce-move-call-twice.rs:20:5 - | -LL | tick(); - | ---- value moved here -LL | tick(); //~ ERROR use of moved value: `tick` - | ^^^^ value used here after move - -error: aborting due to previous error - -For more information about this error, try `rustc --explain E0382`. From d088edc5310518f740df973b6f85d078684152f6 Mon Sep 17 00:00:00 2001 From: David Wood Date: Tue, 16 Oct 2018 19:37:01 +0200 Subject: [PATCH 3/3] Improve check to consider how value is used. --- .../borrow_check/error_reporting.rs | 37 ++++++++----------- src/test/ui/issues/issue-12127.nll.stderr | 18 --------- 2 files changed, 15 insertions(+), 40 deletions(-) delete mode 100644 src/test/ui/issues/issue-12127.nll.stderr diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 3903506068ed7..baf9e032270a6 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -1133,32 +1133,25 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { closure, ); - if let ty::TyKind::Closure(did, substs) = self.mir.local_decls[closure].ty.sty { - let upvar_tys = substs.upvar_tys(did, self.infcx.tcx); + if let ty::TyKind::Closure(did, _substs) = self.mir.local_decls[closure].ty.sty { let node_id = match self.infcx.tcx.hir.as_local_node_id(did) { Some(node_id) => node_id, _ => return, }; - - self.infcx.tcx.with_freevars(node_id, |freevars| { - for (freevar, upvar_ty) in freevars.iter().zip(upvar_tys) { - debug!( - "add_closure_invoked_twice_with_moved_variable_suggestion: \ - freevar={:?} upvar_ty={:?}", - freevar, upvar_ty, - ); - if !upvar_ty.is_region_ptr() { - diag.span_note( - freevar.span, - &format!( - "closure cannot be invoked more than once because it \ - moves the variable `{}` out of its environment", - self.infcx.tcx.hir.name(freevar.var_id()), - ), - ); - } - } - }); + let hir_id = self.infcx.tcx.hir.node_to_hir_id(node_id); + + if let Some(( + span, name + )) = self.infcx.tcx.typeck_tables_of(did).closure_kind_origins().get(hir_id) { + diag.span_note( + *span, + &format!( + "closure cannot be invoked more than once because it \ + moves the variable `{}` out of its environment", + name, + ), + ); + } } } } diff --git a/src/test/ui/issues/issue-12127.nll.stderr b/src/test/ui/issues/issue-12127.nll.stderr deleted file mode 100644 index 51dd2e72832b6..0000000000000 --- a/src/test/ui/issues/issue-12127.nll.stderr +++ /dev/null @@ -1,18 +0,0 @@ -error[E0382]: use of moved value: `f` - --> $DIR/issue-12127.rs:21:9 - | -LL | f(); - | - value moved here -LL | f(); - | ^ value used here after move - | -note: closure cannot be invoked more than once because it moves the variable `x` out of its environment - --> $DIR/issue-12127.rs:18:39 - | -LL | let f = to_fn_once(move|| do_it(&*x)); - | ^ - = note: move occurs because `f` has type `[closure@$DIR/issue-12127.rs:18:24: 18:41 x:std::boxed::Box]`, which does not implement the `Copy` trait - -error: aborting due to previous error - -For more information about this error, try `rustc --explain E0382`.