From 2c21b5807a64878e5ab5eb5c599ccac6776a19e5 Mon Sep 17 00:00:00 2001 From: Ben Blum Date: Thu, 13 Jun 2013 19:37:18 -0400 Subject: [PATCH 1/3] Check closure freevar kinds against destination environment bounds (#3569) --- src/librustc/middle/kind.rs | 77 +++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 21 deletions(-) diff --git a/src/librustc/middle/kind.rs b/src/librustc/middle/kind.rs index 70ad0e1c3a9f5..074bdfdd91d11 100644 --- a/src/librustc/middle/kind.rs +++ b/src/librustc/middle/kind.rs @@ -81,8 +81,6 @@ pub fn check_crate(tcx: ty::ctxt, tcx.sess.abort_if_errors(); } -type check_fn = @fn(Context, @freevar_entry); - fn check_struct_safe_for_destructor(cx: Context, span: span, struct_did: def_id) { @@ -162,30 +160,43 @@ fn check_item(item: @item, (cx, visitor): (Context, visit::vt)) { // Yields the appropriate function to check the kind of closed over // variables. `id` is the node_id for some expression that creates the // closure. -fn with_appropriate_checker(cx: Context, id: node_id, b: &fn(check_fn)) { - fn check_for_uniq(cx: Context, fv: @freevar_entry) { +fn with_appropriate_checker(cx: Context, id: node_id, + b: &fn(checker: &fn(Context, @freevar_entry))) { + fn check_for_uniq(cx: Context, fv: @freevar_entry, bounds: ty::BuiltinBounds) { // all captured data must be owned, regardless of whether it is // moved in or copied in. let id = ast_util::def_id_of_def(fv.def).node; let var_t = ty::node_id_to_type(cx.tcx, id); + + // FIXME(#3569): Once closure capabilities are restricted based on their + // incoming bounds, make this check conditional based on the bounds. if !check_owned(cx, var_t, fv.span) { return; } // check that only immutable variables are implicitly copied in check_imm_free_var(cx, fv.def, fv.span); + + check_freevar_bounds(cx, fv.span, var_t, bounds); } - fn check_for_box(cx: Context, fv: @freevar_entry) { + fn check_for_box(cx: Context, fv: @freevar_entry, bounds: ty::BuiltinBounds) { // all captured data must be owned let id = ast_util::def_id_of_def(fv.def).node; let var_t = ty::node_id_to_type(cx.tcx, id); + + // FIXME(#3569): Once closure capabilities are restricted based on their + // incoming bounds, make this check conditional based on the bounds. if !check_durable(cx.tcx, var_t, fv.span) { return; } // check that only immutable variables are implicitly copied in check_imm_free_var(cx, fv.def, fv.span); + + check_freevar_bounds(cx, fv.span, var_t, bounds); } - fn check_for_block(_cx: Context, _fv: @freevar_entry) { - // no restrictions + fn check_for_block(cx: Context, fv: @freevar_entry, bounds: ty::BuiltinBounds) { + let id = ast_util::def_id_of_def(fv.def).node; + let var_t = ty::node_id_to_type(cx.tcx, id); + check_freevar_bounds(cx, fv.span, var_t, bounds); } fn check_for_bare(cx: Context, fv: @freevar_entry) { @@ -196,14 +207,14 @@ fn with_appropriate_checker(cx: Context, id: node_id, b: &fn(check_fn)) { let fty = ty::node_id_to_type(cx.tcx, id); match ty::get(fty).sty { - ty::ty_closure(ty::ClosureTy {sigil: OwnedSigil, _}) => { - b(check_for_uniq) + ty::ty_closure(ty::ClosureTy {sigil: OwnedSigil, bounds: bounds, _}) => { + b(|cx, fv| check_for_uniq(cx, fv, bounds)) } - ty::ty_closure(ty::ClosureTy {sigil: ManagedSigil, _}) => { - b(check_for_box) + ty::ty_closure(ty::ClosureTy {sigil: ManagedSigil, bounds: bounds, _}) => { + b(|cx, fv| check_for_box(cx, fv, bounds)) } - ty::ty_closure(ty::ClosureTy {sigil: BorrowedSigil, _}) => { - b(check_for_block) + ty::ty_closure(ty::ClosureTy {sigil: BorrowedSigil, bounds: bounds, _}) => { + b(|cx, fv| check_for_block(cx, fv, bounds)) } ty::ty_bare_fn(_) => { b(check_for_bare) @@ -271,7 +282,7 @@ pub fn check_expr(e: @expr, (cx, v): (Context, visit::vt)) { type_param_defs.repr(cx.tcx)); } for ts.iter().zip(type_param_defs.iter()).advance |(&ty, type_param_def)| { - check_bounds(cx, type_parameter_id, e.span, ty, type_param_def) + check_typaram_bounds(cx, type_parameter_id, e.span, ty, type_param_def) } } } @@ -314,7 +325,7 @@ fn check_ty(aty: @Ty, (cx, v): (Context, visit::vt)) { let type_param_defs = ty::lookup_item_type(cx.tcx, did).generics.type_param_defs; for ts.iter().zip(type_param_defs.iter()).advance |(&ty, type_param_def)| { - check_bounds(cx, aty.id, aty.span, ty, type_param_def) + check_typaram_bounds(cx, aty.id, aty.span, ty, type_param_def) } } } @@ -323,19 +334,26 @@ fn check_ty(aty: @Ty, (cx, v): (Context, visit::vt)) { visit::visit_ty(aty, (cx, v)); } -pub fn check_bounds(cx: Context, - _type_parameter_id: node_id, - sp: span, - ty: ty::t, - type_param_def: &ty::TypeParameterDef) +pub fn check_builtin_bounds(cx: Context, ty: ty::t, bounds: ty::BuiltinBounds) + -> ty::BuiltinBounds // returns the missing bounds { let kind = ty::type_contents(cx.tcx, ty); let mut missing = ty::EmptyBuiltinBounds(); - for type_param_def.bounds.builtin_bounds.each |bound| { + for bounds.each |bound| { if !kind.meets_bound(cx.tcx, bound) { missing.add(bound); } } + missing +} + +pub fn check_typaram_bounds(cx: Context, + _type_parameter_id: node_id, + sp: span, + ty: ty::t, + type_param_def: &ty::TypeParameterDef) +{ + let missing = check_builtin_bounds(cx, ty, type_param_def.bounds.builtin_bounds); if !missing.is_empty() { cx.tcx.sess.span_err( sp, @@ -346,6 +364,23 @@ pub fn check_bounds(cx: Context, } } +pub fn check_freevar_bounds(cx: Context, sp: span, ty: ty::t, + bounds: ty::BuiltinBounds) +{ + let missing = check_builtin_bounds(cx, ty, bounds); + if !missing.is_empty() { + cx.tcx.sess.span_err( + sp, + fmt!("cannot capture variable of type `%s`, which does not fulfill \ + `%s`, in a bounded closure", + ty_to_str(cx.tcx, ty), missing.user_string(cx.tcx))); + cx.tcx.sess.span_note( + sp, + fmt!("this closure's environment must satisfy `%s`", + bounds.user_string(cx.tcx))); + } +} + fn is_nullary_variant(cx: Context, ex: @expr) -> bool { match ex.node { expr_path(_) => { From 24bd5ba0aee309e31ea66e7cc7d719dc8de7fcb4 Mon Sep 17 00:00:00 2001 From: Ben Blum Date: Thu, 13 Jun 2013 19:38:36 -0400 Subject: [PATCH 2/3] Add basic test cases for closure bounds. (#3569) --- ...bounds-cant-promote-superkind-in-struct.rs | 20 ++++++++++++++ ...re-bounds-copy-cant-capture-noncopyable.rs | 26 +++++++++++++++++++ ...ure-bounds-static-cant-capture-borrowed.rs | 21 +++++++++++++++ .../compile-fail/closure-bounds-subtype.rs | 7 ++++- .../closure-bounds-can-capture-chan.rs | 23 ++++++++++++++++ 5 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 src/test/compile-fail/closure-bounds-cant-promote-superkind-in-struct.rs create mode 100644 src/test/compile-fail/closure-bounds-copy-cant-capture-noncopyable.rs create mode 100644 src/test/compile-fail/closure-bounds-static-cant-capture-borrowed.rs create mode 100644 src/test/run-pass/closure-bounds-can-capture-chan.rs diff --git a/src/test/compile-fail/closure-bounds-cant-promote-superkind-in-struct.rs b/src/test/compile-fail/closure-bounds-cant-promote-superkind-in-struct.rs new file mode 100644 index 0000000000000..c3c8467233c4d --- /dev/null +++ b/src/test/compile-fail/closure-bounds-cant-promote-superkind-in-struct.rs @@ -0,0 +1,20 @@ +// Copyright 2013 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. + +struct X { + field: @fn:Copy(), +} + +fn foo(blk: @fn()) -> X { + return X { field: blk }; //~ ERROR expected bounds `Copy` but found no bounds +} + +fn main() { +} diff --git a/src/test/compile-fail/closure-bounds-copy-cant-capture-noncopyable.rs b/src/test/compile-fail/closure-bounds-copy-cant-capture-noncopyable.rs new file mode 100644 index 0000000000000..0b11da14e71a9 --- /dev/null +++ b/src/test/compile-fail/closure-bounds-copy-cant-capture-noncopyable.rs @@ -0,0 +1,26 @@ +// Copyright 2013 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. + +use std::comm; + +// If this were legal you could use it to copy captured noncopyables. +// Issue (#2828) + +fn foo(blk: ~fn:Copy()) { + blk(); +} + +fn main() { + let (p,c) = comm::stream(); + do foo { + c.send(()); //~ ERROR does not fulfill `Copy` + } + p.recv(); +} diff --git a/src/test/compile-fail/closure-bounds-static-cant-capture-borrowed.rs b/src/test/compile-fail/closure-bounds-static-cant-capture-borrowed.rs new file mode 100644 index 0000000000000..cac1244a56092 --- /dev/null +++ b/src/test/compile-fail/closure-bounds-static-cant-capture-borrowed.rs @@ -0,0 +1,21 @@ +// Copyright 2013 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. + +fn bar(blk: &fn:'static()) { +} + +fn foo(x: &()) { + do bar { + let _ = x; //~ ERROR does not fulfill `'static` + } +} + +fn main() { +} diff --git a/src/test/compile-fail/closure-bounds-subtype.rs b/src/test/compile-fail/closure-bounds-subtype.rs index ebec113cedc53..a975349e73022 100644 --- a/src/test/compile-fail/closure-bounds-subtype.rs +++ b/src/test/compile-fail/closure-bounds-subtype.rs @@ -1,3 +1,4 @@ + fn take_any(_: &fn()) { } @@ -7,6 +8,9 @@ fn take_copyable(_: &fn:Copy()) { fn take_copyable_owned(_: &fn:Copy+Owned()) { } +fn take_const_owned(_: &fn:Const+Owned()) { +} + fn give_any(f: &fn()) { take_any(f); take_copyable(f); //~ ERROR expected bounds `Copy` but found no bounds @@ -29,6 +33,7 @@ fn give_copyable_owned(f: &fn:Copy+Owned()) { take_any(f); take_copyable(f); take_copyable_owned(f); + take_const_owned(f); //~ ERROR expected bounds `Owned+Const` but found bounds `Copy+Owned` } -fn main() {} \ No newline at end of file +fn main() {} diff --git a/src/test/run-pass/closure-bounds-can-capture-chan.rs b/src/test/run-pass/closure-bounds-can-capture-chan.rs new file mode 100644 index 0000000000000..26bea0e514151 --- /dev/null +++ b/src/test/run-pass/closure-bounds-can-capture-chan.rs @@ -0,0 +1,23 @@ +// Copyright 2013 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. + +use std::comm; + +fn foo(blk: ~fn:Owned()) { + blk(); +} + +fn main() { + let (p,c) = comm::stream(); + do foo { + c.send(()); + } + p.recv(); +} From df7be94bc6324650876a1844e837a3f37a6e1d36 Mon Sep 17 00:00:00 2001 From: Ben Blum Date: Thu, 13 Jun 2013 20:57:23 -0400 Subject: [PATCH 3/3] Allow ~fn:Copy() to be copied. --- src/librustc/middle/ty.rs | 27 ++++++++++++++----- ...losure-bounds-copyable-squiggle-closure.rs | 23 ++++++++++++++++ ...ds-squiggle-closure-as-copyable-typaram.rs | 26 ++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) create mode 100644 src/test/run-pass/closure-bounds-copyable-squiggle-closure.rs create mode 100644 src/test/run-pass/closure-bounds-squiggle-closure-as-copyable-typaram.rs diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 4fc431e0a54ca..6ffd1d3debd94 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -1850,7 +1850,7 @@ impl TypeContents { } pub fn noncopyable(_cx: ctxt) -> TypeContents { - TC_DTOR + TC_BORROWED_MUT + TC_ONCE_CLOSURE + TC_OWNED_CLOSURE + + TC_DTOR + TC_BORROWED_MUT + TC_ONCE_CLOSURE + TC_NONCOPY_TRAIT + TC_EMPTY_ENUM } @@ -1899,13 +1899,19 @@ impl TypeContents { } pub fn needs_drop(&self, cx: ctxt) -> bool { + if self.intersects(TC_NONCOPY_TRAIT) { + // Currently all noncopyable existentials are 2nd-class types + // behind owned pointers. With dynamically-sized types, remove + // this assertion. + assert!(self.intersects(TC_OWNED_POINTER)); + } let tc = TC_MANAGED + TC_DTOR + TypeContents::owned(cx); self.intersects(tc) } pub fn owned(_cx: ctxt) -> TypeContents { //! Any kind of owned contents. - TC_OWNED_CLOSURE + TC_OWNED_POINTER + TC_OWNED_VEC + TC_OWNED_POINTER + TC_OWNED_VEC } } @@ -1939,8 +1945,8 @@ static TC_OWNED_POINTER: TypeContents = TypeContents{bits: 0b0000_0000_0010}; /// Contains an owned vector ~[] or owned string ~str static TC_OWNED_VEC: TypeContents = TypeContents{bits: 0b0000_0000_0100}; -/// Contains a ~fn() or a ~Trait, which is non-copyable. -static TC_OWNED_CLOSURE: TypeContents = TypeContents{bits: 0b0000_0000_1000}; +/// Contains a non-copyable ~fn() or a ~Trait (NOT a ~fn:Copy() or ~Trait:Copy). +static TC_NONCOPY_TRAIT: TypeContents = TypeContents{bits: 0b0000_0000_1000}; /// Type with a destructor static TC_DTOR: TypeContents = TypeContents{bits: 0b0000_0001_0000}; @@ -2055,7 +2061,8 @@ pub fn type_contents(cx: ctxt, ty: t) -> TypeContents { } ty_trait(_, _, UniqTraitStore, _) => { - TC_OWNED_CLOSURE + // FIXME(#3569): Make this conditional on the trait's bounds. + TC_NONCOPY_TRAIT + TC_OWNED_POINTER } ty_trait(_, _, BoxTraitStore, mutbl) => { @@ -2179,7 +2186,9 @@ pub fn type_contents(cx: ctxt, ty: t) -> TypeContents { match sigil { ast::BorrowedSigil => TC_BORROWED_POINTER, ast::ManagedSigil => TC_MANAGED, - ast::OwnedSigil => TC_OWNED_CLOSURE + // FIXME(#3569): Looks like noncopyability should depend + // on the bounds, but I don't think this case ever comes up. + ast::OwnedSigil => TC_NONCOPY_TRAIT + TC_OWNED_POINTER, } } @@ -2253,7 +2262,11 @@ pub fn type_contents(cx: ctxt, ty: t) -> TypeContents { let st = match cty.sigil { ast::BorrowedSigil => TC_BORROWED_POINTER, ast::ManagedSigil => TC_MANAGED, - ast::OwnedSigil => TC_OWNED_CLOSURE + ast::OwnedSigil => if cty.bounds.contains_elem(BoundCopy) { + TC_OWNED_POINTER + } else { + TC_OWNED_POINTER + TC_NONCOPY_TRAIT + } }; let rt = borrowed_contents(cty.region, m_imm); let ot = match cty.onceness { diff --git a/src/test/run-pass/closure-bounds-copyable-squiggle-closure.rs b/src/test/run-pass/closure-bounds-copyable-squiggle-closure.rs new file mode 100644 index 0000000000000..28add8e9d54bf --- /dev/null +++ b/src/test/run-pass/closure-bounds-copyable-squiggle-closure.rs @@ -0,0 +1,23 @@ +// Copyright 2013 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. + +// Tests correct copying of heap closures' environments. + +fn foo(x: ~fn:Copy()) -> (~fn(), ~fn()) { + (copy x, x) +} +fn main() { + let v = ~[~[1,2,3],~[4,5,6]]; // shouldn't get double-freed + let (f1,f2) = do foo { + assert!(v.len() == 2); + }; + f1(); + f2(); +} diff --git a/src/test/run-pass/closure-bounds-squiggle-closure-as-copyable-typaram.rs b/src/test/run-pass/closure-bounds-squiggle-closure-as-copyable-typaram.rs new file mode 100644 index 0000000000000..149f7fee586bd --- /dev/null +++ b/src/test/run-pass/closure-bounds-squiggle-closure-as-copyable-typaram.rs @@ -0,0 +1,26 @@ +// Copyright 2013 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. + +// Tests correct copying of heap closures' environments. + +fn bar(x: T) -> (T, T) { + (copy x, x) +} +fn foo(x: ~fn:Copy()) -> (~fn(), ~fn()) { + bar(x) +} +fn main() { + let v = ~[~[1,2,3],~[4,5,6]]; // shouldn't get double-freed + let (f1,f2) = do foo { + assert!(v.len() == 2); + }; + f1(); + f2(); +}