From 4e5666eace14651539ca2a1f5eed8cdf0ccfb130 Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Wed, 1 Oct 2014 22:24:10 -0700 Subject: [PATCH 1/6] Fix missing entries in upvar borrows map for capture-by-ref unboxed closures This prevents a later ICE in borrowck. Closes issue #17655 --- src/librustc/middle/typeck/check/regionck.rs | 42 ++++++++++++-------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/librustc/middle/typeck/check/regionck.rs b/src/librustc/middle/typeck/check/regionck.rs index 9e20028569b..3a14b90f789 100644 --- a/src/librustc/middle/typeck/check/regionck.rs +++ b/src/librustc/middle/typeck/check/regionck.rs @@ -844,7 +844,7 @@ fn check_expr_fn_block(rcx: &mut Rcx, // has static lifetime. } else { // Variables being referenced must outlive closure. - constrain_free_variables_in_stack_closure( + constrain_free_variables_in_by_ref_closure( rcx, bounds.region_bound, expr, freevars); // Closure is stack allocated and hence cannot @@ -856,20 +856,17 @@ fn check_expr_fn_block(rcx: &mut Rcx, }); } ty::ty_unboxed_closure(_, region) => { - ty::with_freevars(tcx, expr.id, |freevars| { - // No free variables means that there is no environment and - // hence the closure has static lifetime. Otherwise, the - // closure must not outlive the variables it closes over - // by-reference. - // - // NDM -- this seems wrong, discuss with pcwalton, should - // be straightforward enough. - if !freevars.is_empty() { - let bounds = ty::region_existential_bound(region); - ensure_free_variable_types_outlive_closure_bound( - rcx, bounds, expr, freevars); - } - }) + let bounds = ty::region_existential_bound(region); + if tcx.capture_modes.borrow().get_copy(&expr.id) == ast::CaptureByRef { + ty::with_freevars(tcx, expr.id, |freevars| { + if !freevars.is_empty() { + // Variables being referenced must be constrained and registered + // in the upvar borrow map + constrain_free_variables_in_by_ref_closure( + rcx, bounds.region_bound, expr, freevars); + } + }) + } } _ => { } } @@ -884,6 +881,13 @@ fn check_expr_fn_block(rcx: &mut Rcx, propagate_upupvar_borrow_kind(rcx, expr, freevars); }) } + ty::ty_unboxed_closure(..) => { + if tcx.capture_modes.borrow().get_copy(&expr.id) == ast::CaptureByRef { + ty::with_freevars(tcx, expr.id, |freevars| { + propagate_upupvar_borrow_kind(rcx, expr, freevars); + }); + } + } _ => {} } @@ -893,6 +897,12 @@ fn check_expr_fn_block(rcx: &mut Rcx, ensure_free_variable_types_outlive_closure_bound(rcx, bounds, expr, freevars); }) } + ty::ty_unboxed_closure(_, region) => { + ty::with_freevars(tcx, expr.id, |freevars| { + let bounds = ty::region_existential_bound(region); + ensure_free_variable_types_outlive_closure_bound(rcx, bounds, expr, freevars); + }) + } _ => {} } @@ -959,7 +969,7 @@ fn ensure_free_variable_types_outlive_closure_bound( } } - fn constrain_free_variables_in_stack_closure( + fn constrain_free_variables_in_by_ref_closure( rcx: &mut Rcx, region_bound: ty::Region, expr: &ast::Expr, From 931f59f214f68bf2a5656d605cb200f22d47cdf8 Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Wed, 1 Oct 2014 23:52:19 -0700 Subject: [PATCH 2/6] Fix categorization of upvars of capture-by-reference unboxed closures In particular, this causes mutation of an upvar to correctly mark it as mutable during adjustment. This makes borrowck correctly flag conflicting borrows, etc. We still seem to generate incorrect code in trans which copies the upvar by value into the closure. This remains to be fixed. --- src/librustc/middle/mem_categorization.rs | 26 +++++++++++++---------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs index bb44b1f55cb..b62884c057a 100644 --- a/src/librustc/middle/mem_categorization.rs +++ b/src/librustc/middle/mem_categorization.rs @@ -604,17 +604,21 @@ pub fn cat_def(&self, ty::FnMutUnboxedClosureKind => ast::Many, ty::FnOnceUnboxedClosureKind => ast::Once, }; - Ok(Rc::new(cmt_ { - id: id, - span: span, - cat: cat_copied_upvar(CopiedUpvar { - upvar_id: var_id, - onceness: onceness, - capturing_proc: fn_node_id, - }), - mutbl: MutabilityCategory::from_local(self.tcx(), var_id), - ty: expr_ty - })) + if self.typer.capture_mode(fn_node_id) == ast::CaptureByRef { + self.cat_upvar(id, span, var_id, fn_node_id) + } else { + Ok(Rc::new(cmt_ { + id: id, + span: span, + cat: cat_copied_upvar(CopiedUpvar { + upvar_id: var_id, + onceness: onceness, + capturing_proc: fn_node_id, + }), + mutbl: MutabilityCategory::from_local(self.tcx(), var_id), + ty: expr_ty + })) + } } _ => { self.tcx().sess.span_bug( From 72dc0f5f82efcf891581fdc3ec15eb46f38cd718 Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Thu, 2 Oct 2014 19:41:42 -0700 Subject: [PATCH 3/6] Return correct types for capture-by-ref unboxed closure upvars Treat upvars of capture-by-reference unboxed closures as references with appropriate regions and mutability. --- src/librustc/middle/ty.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index b38f362dcf1..e771c3d967a 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -4668,15 +4668,27 @@ pub struct UnboxedClosureUpvar { pub fn unboxed_closure_upvars(tcx: &ctxt, closure_id: ast::DefId) -> Vec { if closure_id.krate == ast::LOCAL_CRATE { + let capture_mode = tcx.capture_modes.borrow().get_copy(&closure_id.node); match tcx.freevars.borrow().find(&closure_id.node) { None => vec![], Some(ref freevars) => { freevars.iter().map(|freevar| { let freevar_def_id = freevar.def.def_id(); + let mut freevar_ty = node_id_to_type(tcx, freevar_def_id.node); + if capture_mode == ast::CaptureByRef { + let borrow = tcx.upvar_borrow_map.borrow().get_copy(&ty::UpvarId { + var_id: freevar_def_id.node, + closure_expr_id: closure_id.node + }); + freevar_ty = mk_rptr(tcx, borrow.region, ty::mt { + ty: freevar_ty, + mutbl: borrow.kind.to_mutbl_lossy() + }); + } UnboxedClosureUpvar { def: freevar.def, span: freevar.span, - ty: node_id_to_type(tcx, freevar_def_id.node), + ty: freevar_ty } }).collect() } From c4c19fe96003bb5bdaa6ca35603eb4220b1d5ca8 Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Thu, 2 Oct 2014 19:43:16 -0700 Subject: [PATCH 4/6] Correctly trans capture-by-ref unboxed closures Store references to the freevars instead of copies when constructing the environment and insert an additional load when reading them from the environment. --- src/librustc/middle/trans/closure.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/librustc/middle/trans/closure.rs b/src/librustc/middle/trans/closure.rs index f08736335c3..1e2e8c589c6 100644 --- a/src/librustc/middle/trans/closure.rs +++ b/src/librustc/middle/trans/closure.rs @@ -301,6 +301,7 @@ fn load_environment<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, fn load_unboxed_closure_environment<'blk, 'tcx>( bcx: Block<'blk, 'tcx>, arg_scope_id: ScopeId, + freevar_mode: ast::CaptureClause, freevars: &Vec, closure_id: ast::DefId) -> Block<'blk, 'tcx> { @@ -326,11 +327,14 @@ fn load_unboxed_closure_environment<'blk, 'tcx>( }; for (i, freevar) in freevars.iter().enumerate() { - let upvar_ptr = GEPi(bcx, llenv, [0, i]); + let mut upvar_ptr = GEPi(bcx, llenv, [0, i]); + if freevar_mode == ast::CaptureByRef { + upvar_ptr = Load(bcx, upvar_ptr); + } let def_id = freevar.def.def_id(); bcx.fcx.llupvars.borrow_mut().insert(def_id.node, upvar_ptr); - if kind == ty::FnOnceUnboxedClosureKind { + if kind == ty::FnOnceUnboxedClosureKind && freevar_mode == ast::CaptureByValue { bcx.fcx.schedule_drop_mem(arg_scope_id, upvar_ptr, node_id_type(bcx, def_id.node)) @@ -477,6 +481,7 @@ pub fn trans_unboxed_closure<'blk, 'tcx>( let freevars: Vec = ty::with_freevars(bcx.tcx(), id, |fv| fv.iter().map(|&fv| fv).collect()); let freevars_ptr = &freevars; + let freevar_mode = bcx.tcx().capture_mode(id); trans_closure(bcx.ccx(), decl, @@ -493,6 +498,7 @@ pub fn trans_unboxed_closure<'blk, 'tcx>( |bcx, arg_scope| { load_unboxed_closure_environment(bcx, arg_scope, + freevar_mode, freevars_ptr, closure_id) }); @@ -518,7 +524,14 @@ pub fn trans_unboxed_closure<'blk, 'tcx>( dest_addr, 0, i); - bcx = datum.store_to(bcx, upvar_slot_dest); + match freevar_mode { + ast::CaptureByValue => { + bcx = datum.store_to(bcx, upvar_slot_dest); + } + ast::CaptureByRef => { + Store(bcx, datum.to_llref(), upvar_slot_dest); + } + } } adt::trans_set_discr(bcx, &*repr, dest_addr, 0); From 39344c2d7ec6fdacb7085aefda3e440891e0855a Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Thu, 2 Oct 2014 19:48:07 -0700 Subject: [PATCH 5/6] Rehabilitate an unboxed closure test This test works as a regression test for issue #17655. It also exercises mutation of by-ref upvars. --- src/test/run-pass/capture-clauses-unboxed-closures.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/test/run-pass/capture-clauses-unboxed-closures.rs b/src/test/run-pass/capture-clauses-unboxed-closures.rs index 99e6d6e7a4f..a826f4df5b3 100644 --- a/src/test/run-pass/capture-clauses-unboxed-closures.rs +++ b/src/test/run-pass/capture-clauses-unboxed-closures.rs @@ -8,13 +8,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-test -// -// This is ignored because it depends on #16122. - #![feature(overloaded_calls, unboxed_closures)] -fn each<'a,T,F:|&mut: &'a T|>(x: &'a [T], mut f: F) { +fn each<'a,T,F:FnMut(&'a T)>(x: &'a [T], mut f: F) { for val in x.iter() { f(val) } @@ -23,7 +19,6 @@ fn each<'a,T,F:|&mut: &'a T|>(x: &'a [T], mut f: F) { fn main() { let mut sum = 0u; let elems = [ 1u, 2, 3, 4, 5 ]; - each(elems, ref |&mut: val: &uint| sum += *val); + each(elems, |&mut: val: &uint| sum += *val); assert_eq!(sum, 15); } - From 521ca31071c01b21de40ce3477a81972103b3db9 Mon Sep 17 00:00:00 2001 From: Brian Koropoff Date: Fri, 3 Oct 2014 00:57:21 -0700 Subject: [PATCH 6/6] Add some more test coverage of by-ref unboxed closures --- .../compile-fail/unboxed-closure-region.rs | 20 +++++++++++ .../unboxed-closures-borrow-conflict.rs | 20 +++++++++++ src/test/run-pass/unboxed-closures-by-ref.rs | 35 +++++++++++++++++++ 3 files changed, 75 insertions(+) create mode 100644 src/test/compile-fail/unboxed-closure-region.rs create mode 100644 src/test/compile-fail/unboxed-closures-borrow-conflict.rs create mode 100644 src/test/run-pass/unboxed-closures-by-ref.rs diff --git a/src/test/compile-fail/unboxed-closure-region.rs b/src/test/compile-fail/unboxed-closure-region.rs new file mode 100644 index 00000000000..2a71aeaca5f --- /dev/null +++ b/src/test/compile-fail/unboxed-closure-region.rs @@ -0,0 +1,20 @@ +// 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)] + +// Test that an unboxed closure that captures a free variable by +// reference cannot escape the region of that variable. +fn main() { + let _f = { + let x = 0u; + |:| x //~ ERROR cannot infer an appropriate lifetime due to conflicting requirements + }; +} diff --git a/src/test/compile-fail/unboxed-closures-borrow-conflict.rs b/src/test/compile-fail/unboxed-closures-borrow-conflict.rs new file mode 100644 index 00000000000..baf7f3f5e58 --- /dev/null +++ b/src/test/compile-fail/unboxed-closures-borrow-conflict.rs @@ -0,0 +1,20 @@ +// 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)] + +// Test that an unboxed closure that mutates a free variable will +// cause borrow conflicts. + +fn main() { + let mut x = 0u; + let f = |:| x += 1; + let _y = x; //~ ERROR cannot use `x` because it was mutably borrowed +} diff --git a/src/test/run-pass/unboxed-closures-by-ref.rs b/src/test/run-pass/unboxed-closures-by-ref.rs new file mode 100644 index 00000000000..2ee28d19b75 --- /dev/null +++ b/src/test/run-pass/unboxed-closures-by-ref.rs @@ -0,0 +1,35 @@ +// 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(overloaded_calls, unboxed_closures)] + +// Test by-ref capture of environment in unboxed closure types + +fn call_fn(f: F) { + f() +} + +fn call_fn_mut(mut f: F) { + f() +} + +fn call_fn_once(f: F) { + f() +} + +fn main() { + let mut x = 0u; + let y = 2u; + + call_fn(|&:| x += y); + call_fn_mut(|&mut:| x += y); + call_fn_once(|:| x += y); + assert_eq!(x, y * 3); +}