From ea3ab731a3c762829e3f2bec5bfbaa3a3a84689f Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Sat, 4 Oct 2014 14:04:10 -0700 Subject: [PATCH 1/4] Track kind of closure in upvar categorization Keep track of the kind of closure responsible for an upvar --- .../borrowck/gather_loans/gather_moves.rs | 13 +++-- .../borrowck/gather_loans/move_error.rs | 11 +++- .../borrowck/gather_loans/restrictions.rs | 2 +- src/librustc/middle/borrowck/mod.rs | 12 +++-- src/librustc/middle/mem_categorization.rs | 50 +++++++++++++------ src/librustc/middle/typeck/check/regionck.rs | 6 +-- 6 files changed, 61 insertions(+), 33 deletions(-) diff --git a/src/librustc/middle/borrowck/gather_loans/gather_moves.rs b/src/librustc/middle/borrowck/gather_loans/gather_moves.rs index 1ae512a244c0f..cd67169ff5690 100644 --- a/src/librustc/middle/borrowck/gather_loans/gather_moves.rs +++ b/src/librustc/middle/borrowck/gather_loans/gather_moves.rs @@ -133,16 +133,15 @@ fn check_and_get_illegal_move_origin(bccx: &BorrowckCtxt, mc::cat_deref(_, _, mc::BorrowedPtr(..)) | mc::cat_deref(_, _, mc::Implicit(..)) | mc::cat_deref(_, _, mc::UnsafePtr(..)) | - mc::cat_upvar(..) | mc::cat_static_item | - mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => { + mc::cat_upvar(..) | mc::cat_static_item => { Some(cmt.clone()) } - // Can move out of captured upvars only if the destination closure - // type is 'once'. 1-shot stack closures emit the copied_upvar form - // (see mem_categorization.rs). - mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Once, .. }) => { - None + mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. }) => { + match kind.onceness() { + ast::Once => None, + ast::Many => Some(cmt.clone()) + } } mc::cat_rvalue(..) | diff --git a/src/librustc/middle/borrowck/gather_loans/move_error.rs b/src/librustc/middle/borrowck/gather_loans/move_error.rs index 1b18d07f59053..29677cf897144 100644 --- a/src/librustc/middle/borrowck/gather_loans/move_error.rs +++ b/src/librustc/middle/borrowck/gather_loans/move_error.rs @@ -115,8 +115,15 @@ fn report_cannot_move_out_of(bccx: &BorrowckCtxt, move_from: mc::cmt) { mc::cat_deref(_, _, mc::BorrowedPtr(..)) | mc::cat_deref(_, _, mc::Implicit(..)) | mc::cat_deref(_, _, mc::UnsafePtr(..)) | - mc::cat_upvar(..) | mc::cat_static_item | - mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => { + mc::cat_upvar(..) | mc::cat_static_item => { + bccx.span_err( + move_from.span, + format!("cannot move out of {}", + bccx.cmt_to_string(&*move_from)).as_slice()); + } + + mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. }) + if kind.onceness() == ast::Many => { bccx.span_err( move_from.span, format!("cannot move out of {}", diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index 067a73fcc9b7e..f30a370d06852 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -72,7 +72,7 @@ impl<'a, 'tcx> RestrictionsContext<'a, 'tcx> { SafeIf(lp.clone(), vec![lp]) } - mc::cat_upvar(upvar_id, _) => { + mc::cat_upvar(upvar_id, _, _) => { // R-Variable, captured into closure let lp = Rc::new(LpUpvar(upvar_id)); SafeIf(lp.clone(), vec![lp]) diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 234afc7ae7a83..710193188eed3 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -354,8 +354,12 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option> { match cmt.cat { mc::cat_rvalue(..) | - mc::cat_static_item | - mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => { + mc::cat_static_item => { + None + } + + mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. }) + if kind.onceness() == ast::Many => { None } @@ -363,9 +367,9 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option> { Some(Rc::new(LpVar(id))) } - mc::cat_upvar(ty::UpvarId {var_id: id, closure_expr_id: proc_id}, _) | + mc::cat_upvar(ty::UpvarId {var_id: id, closure_expr_id: proc_id}, _, _) | mc::cat_copied_upvar(mc::CopiedUpvar { upvar_id: id, - onceness: _, + kind: _, capturing_proc: proc_id }) => { let upvar_id = ty::UpvarId{ var_id: id, closure_expr_id: proc_id }; Some(Rc::new(LpUpvar(upvar_id))) diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs index 605d3f6d6ced0..207722eba00e4 100644 --- a/src/librustc/middle/mem_categorization.rs +++ b/src/librustc/middle/mem_categorization.rs @@ -83,7 +83,7 @@ pub enum categorization { cat_rvalue(ty::Region), // temporary val, argument is its scope cat_static_item, cat_copied_upvar(CopiedUpvar), // upvar copied into proc env - cat_upvar(ty::UpvarId, ty::UpvarBorrow), // by ref upvar from stack closure + cat_upvar(ty::UpvarId, ty::UpvarBorrow, Option), // by ref upvar from stack or unboxed closure cat_local(ast::NodeId), // local variable cat_deref(cmt, uint, PointerKind), // deref of a ptr cat_interior(cmt, InteriorKind), // something interior: field, tuple, etc @@ -93,10 +93,27 @@ pub enum categorization { // (*1) downcast is only required if the enum has more than one variant } +#[deriving(Clone, PartialEq)] +pub enum CopiedUpvarKind { + Boxed(ast::Onceness), + Unboxed(ty::UnboxedClosureKind) +} + +impl CopiedUpvarKind { + pub fn onceness(&self) -> ast::Onceness { + match *self { + Boxed(onceness) => onceness, + Unboxed(ty::FnUnboxedClosureKind) | + Unboxed(ty::FnMutUnboxedClosureKind) => ast::Many, + Unboxed(ty::FnOnceUnboxedClosureKind) => ast::Once + } + } +} + #[deriving(Clone, PartialEq)] pub struct CopiedUpvar { pub upvar_id: ast::NodeId, - pub onceness: ast::Onceness, + pub kind: CopiedUpvarKind, pub capturing_proc: ast::NodeId, } @@ -571,14 +588,14 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> { }; if var_is_refd { - self.cat_upvar(id, span, var_id, fn_node_id) + self.cat_upvar(id, span, var_id, fn_node_id, None) } else { Ok(Rc::new(cmt_ { id:id, span:span, cat:cat_copied_upvar(CopiedUpvar { upvar_id: var_id, - onceness: closure_ty.onceness, + kind: Boxed(closure_ty.onceness), capturing_proc: fn_node_id, }), mutbl: MutabilityCategory::from_local(self.tcx(), var_id), @@ -591,20 +608,15 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> { .unboxed_closures() .borrow(); let kind = unboxed_closures.get(&closure_id).kind; - let onceness = match kind { - ty::FnUnboxedClosureKind | - ty::FnMutUnboxedClosureKind => ast::Many, - ty::FnOnceUnboxedClosureKind => ast::Once, - }; if self.typer.capture_mode(fn_node_id) == ast::CaptureByRef { - self.cat_upvar(id, span, var_id, fn_node_id) + self.cat_upvar(id, span, var_id, fn_node_id, Some(kind)) } else { Ok(Rc::new(cmt_ { id: id, span: span, cat: cat_copied_upvar(CopiedUpvar { upvar_id: var_id, - onceness: onceness, + kind: Unboxed(kind), capturing_proc: fn_node_id, }), mutbl: MutabilityCategory::from_local(self.tcx(), var_id), @@ -638,7 +650,8 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> { id: ast::NodeId, span: Span, var_id: ast::NodeId, - fn_node_id: ast::NodeId) + fn_node_id: ast::NodeId, + kind: Option) -> McResult { /*! * Upvars through a closure are in fact indirect @@ -666,7 +679,7 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> { let base_cmt = Rc::new(cmt_ { id:id, span:span, - cat:cat_upvar(upvar_id, upvar_borrow), + cat:cat_upvar(upvar_id, upvar_borrow, kind), mutbl:McImmutable, ty:upvar_ty, }); @@ -1287,7 +1300,6 @@ impl cmt_ { b.freely_aliasable(ctxt) } - cat_copied_upvar(CopiedUpvar {onceness: ast::Once, ..}) | cat_rvalue(..) | cat_local(..) | cat_upvar(..) | @@ -1295,8 +1307,14 @@ impl cmt_ { None } - cat_copied_upvar(CopiedUpvar {onceness: ast::Many, ..}) => { - Some(AliasableOther) + cat_copied_upvar(CopiedUpvar {kind: kind, capturing_proc: id, ..}) => { + match kind { + Boxed(ast::Once) | + Unboxed(ty::FnOnceUnboxedClosureKind) | + Unboxed(ty::FnMutUnboxedClosureKind) => None, + Boxed(_) => Some(AliasableOther), + Unboxed(_) => Some(AliasableClosure(id)) + } } cat_static_item(..) => { diff --git a/src/librustc/middle/typeck/check/regionck.rs b/src/librustc/middle/typeck/check/regionck.rs index 058b3ac9e6ec1..f533079be69b3 100644 --- a/src/librustc/middle/typeck/check/regionck.rs +++ b/src/librustc/middle/typeck/check/regionck.rs @@ -1552,7 +1552,7 @@ fn link_reborrowed_region(rcx: &Rcx, // Detect references to an upvar `x`: let cause = match ref_cmt.cat { - mc::cat_upvar(ref upvar_id, _) => { + mc::cat_upvar(ref upvar_id, _, _) => { let mut upvar_borrow_map = rcx.fcx.inh.upvar_borrow_map.borrow_mut(); match upvar_borrow_map.find_mut(upvar_id) { @@ -1686,7 +1686,7 @@ fn adjust_upvar_borrow_kind_for_mut(rcx: &Rcx, mc::cat_deref(base, _, mc::BorrowedPtr(..)) | mc::cat_deref(base, _, mc::Implicit(..)) => { match base.cat { - mc::cat_upvar(ref upvar_id, _) => { + mc::cat_upvar(ref upvar_id, _, _) => { // if this is an implicit deref of an // upvar, then we need to modify the // borrow_kind of the upvar to make sure it @@ -1739,7 +1739,7 @@ fn adjust_upvar_borrow_kind_for_unique(rcx: &Rcx, cmt: mc::cmt) { mc::cat_deref(base, _, mc::BorrowedPtr(..)) | mc::cat_deref(base, _, mc::Implicit(..)) => { match base.cat { - mc::cat_upvar(ref upvar_id, _) => { + mc::cat_upvar(ref upvar_id, _, _) => { // if this is an implicit deref of an // upvar, then we need to modify the // borrow_kind of the upvar to make sure it From f74b1c4ee255ab7cd2706896bced45f14e1ccc1a Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Sat, 4 Oct 2014 14:06:08 -0700 Subject: [PATCH 2/4] Categorize upvars in `Fn` unboxed closures as freely aliasable This causes borrowck to correctly reject mutation or mutable borrows of upvars in `Fn` unboxed closures since the closure environment is aliasable. This also tracks the responsible closure in the aliasability information returned and uses it to give a helpful diagnostic. Closes issue #17780 --- src/librustc/middle/borrowck/check_loans.rs | 6 ++++++ src/librustc/middle/borrowck/mod.rs | 7 +++++++ src/librustc/middle/mem_categorization.rs | 12 ++++++++++-- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index e114eb88705bf..df18ec30f0ee3 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -854,6 +854,12 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { check_for_aliasability_violation(this, span, b.clone()); } + mc::cat_copied_upvar(mc::CopiedUpvar { + kind: mc::Unboxed(ty::FnUnboxedClosureKind), ..}) => { + // Prohibit writes to capture-by-move upvars in non-once closures + check_for_aliasability_violation(this, span, guarantor.clone()); + } + _ => {} } diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 710193188eed3..a86ae42006595 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -728,6 +728,13 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { format!("{} in an aliasable location", prefix).as_slice()); } + mc::AliasableClosure(id) => { + self.tcx.sess.span_err(span, + format!("{} in a free variable from an \ + immutable unboxed closure", prefix).as_slice()); + span_note!(self.tcx.sess, self.tcx.map.span(id), + "consider changing this closure to take self by mutable reference"); + } mc::AliasableStatic(..) | mc::AliasableStaticMut(..) => { self.tcx.sess.span_err( diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs index 207722eba00e4..9a0885ca30135 100644 --- a/src/librustc/middle/mem_categorization.rs +++ b/src/librustc/middle/mem_categorization.rs @@ -83,7 +83,8 @@ pub enum categorization { cat_rvalue(ty::Region), // temporary val, argument is its scope cat_static_item, cat_copied_upvar(CopiedUpvar), // upvar copied into proc env - cat_upvar(ty::UpvarId, ty::UpvarBorrow, Option), // by ref upvar from stack or unboxed closure + cat_upvar(ty::UpvarId, ty::UpvarBorrow, + Option), // by ref upvar from stack or unboxed closure cat_local(ast::NodeId), // local variable cat_deref(cmt, uint, PointerKind), // deref of a ptr cat_interior(cmt, InteriorKind), // something interior: field, tuple, etc @@ -1246,6 +1247,7 @@ pub enum InteriorSafety { pub enum AliasableReason { AliasableBorrowed, + AliasableClosure(ast::NodeId), // Aliasable due to capture by unboxed closure expr AliasableOther, AliasableStatic(InteriorSafety), AliasableStaticMut(InteriorSafety), @@ -1302,7 +1304,6 @@ impl cmt_ { cat_rvalue(..) | cat_local(..) | - cat_upvar(..) | cat_deref(_, _, UnsafePtr(..)) => { // yes, it's aliasable, but... None } @@ -1317,6 +1318,13 @@ impl cmt_ { } } + cat_upvar(ty::UpvarId { closure_expr_id: id, .. }, _, + Some(ty::FnUnboxedClosureKind)) => { + Some(AliasableClosure(id)) + } + + cat_upvar(..) => None, + cat_static_item(..) => { let int_safe = if ty::type_interior_is_unsafe(ctxt, self.ty) { InteriorUnsafe From 16b27bbeadff1c61ce36e09180b18aedc7ec27b2 Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Sat, 4 Oct 2014 15:34:38 -0700 Subject: [PATCH 3/4] Fix unit test that was illegally mutating an upvar --- src/test/run-pass/unboxed-closures-by-ref.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/run-pass/unboxed-closures-by-ref.rs b/src/test/run-pass/unboxed-closures-by-ref.rs index 2ee28d19b75a9..70d41a5c68909 100644 --- a/src/test/run-pass/unboxed-closures-by-ref.rs +++ b/src/test/run-pass/unboxed-closures-by-ref.rs @@ -28,8 +28,8 @@ fn main() { let mut x = 0u; let y = 2u; - call_fn(|&:| x += y); + call_fn(|&:| assert_eq!(x, 0)); call_fn_mut(|&mut:| x += y); call_fn_once(|:| x += y); - assert_eq!(x, y * 3); + assert_eq!(x, y * 2); } From 4d2ff432e45eedef9d15618b0b9af5378994bc46 Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Sat, 4 Oct 2014 15:47:16 -0700 Subject: [PATCH 4/4] Add regression test for issue #17780 --- src/test/compile-fail/issue-17780.rs | 50 ++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 src/test/compile-fail/issue-17780.rs diff --git a/src/test/compile-fail/issue-17780.rs b/src/test/compile-fail/issue-17780.rs new file mode 100644 index 0000000000000..2072b2ee2d2c5 --- /dev/null +++ b/src/test/compile-fail/issue-17780.rs @@ -0,0 +1,50 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(unboxed_closures, overloaded_calls)] + +fn set(x: &mut uint) { *x = 5; } + +fn main() { + // By-ref captures + { + let mut x = 0u; + let _f = |&:| x = 42; + //~^ ERROR cannot assign to data in a free + // variable from an immutable unboxed closure + + let mut y = 0u; + let _g = |&:| set(&mut y); + //~^ ERROR cannot borrow data mutably in a free + // variable from an immutable unboxed closure + + let mut z = 0u; + let _h = |&mut:| { set(&mut z); |&:| z = 42; }; + //~^ ERROR cannot assign to data in a + // free variable from an immutable unboxed closure + } + // By-value captures + { + let mut x = 0u; + let _f = move |&:| x = 42; + //~^ ERROR cannot assign to data in a free + // variable from an immutable unboxed closure + + let mut y = 0u; + let _g = move |&:| set(&mut y); + //~^ ERROR cannot borrow data mutably in a free + // variable from an immutable unboxed closure + + let mut z = 0u; + let _h = move |&mut:| { set(&mut z); move |&:| z = 42; }; + //~^ ERROR cannot assign to data in a free + // variable from an immutable unboxed closure + } +}