Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement Copy/Clone for closures #44551

Merged
merged 2 commits into from
Sep 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/librustc/dep_graph/dep_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,8 @@ define_dep_nodes!( <'tcx>
[] MissingExternCrateItem(CrateNum),
[] UsedCrateSource(CrateNum),
[] PostorderCnums,
[] HasCloneClosures(CrateNum),
[] HasCopyClosures(CrateNum),

[] Freevars(DefId),
[] MaybeUnusedTraitImport(DefId),
Expand Down
29 changes: 22 additions & 7 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1340,7 +1340,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
self.assemble_candidates_from_impls(obligation, &mut candidates)?;

// For other types, we'll use the builtin rules.
let copy_conditions = self.copy_conditions(obligation);
let copy_conditions = self.copy_clone_conditions(obligation);
self.assemble_builtin_bound_candidates(copy_conditions, &mut candidates)?;
} else if lang_items.sized_trait() == Some(def_id) {
// Sized is never implementable by end-users, it is
Expand All @@ -1355,7 +1355,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
// Same builtin conditions as `Copy`, i.e. every type which has builtin support
// for `Copy` also has builtin support for `Clone`, + tuples and arrays of `Clone`
// types have builtin support for `Clone`.
let clone_conditions = self.copy_conditions(obligation);
let clone_conditions = self.copy_clone_conditions(obligation);
self.assemble_builtin_bound_candidates(clone_conditions, &mut candidates)?;
}

Expand Down Expand Up @@ -2050,7 +2050,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
}
}

fn copy_conditions(&mut self, obligation: &TraitObligation<'tcx>)
fn copy_clone_conditions(&mut self, obligation: &TraitObligation<'tcx>)
-> BuiltinImplConditions<'tcx>
{
// NOTE: binder moved to (*)
Expand All @@ -2068,8 +2068,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
Where(ty::Binder(Vec::new()))
}

ty::TyDynamic(..) | ty::TyStr | ty::TySlice(..) |
ty::TyClosure(..) | ty::TyGenerator(..) |
ty::TyDynamic(..) | ty::TyStr | ty::TySlice(..) | ty::TyGenerator(..) |
ty::TyRef(_, ty::TypeAndMut { ty: _, mutbl: hir::MutMutable }) => {
Never
}
Expand All @@ -2084,6 +2083,22 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
Where(ty::Binder(tys.to_vec()))
}

ty::TyClosure(def_id, substs) => {
let trait_id = obligation.predicate.def_id();
let copy_closures =
Copy link
Contributor

@arielb1 arielb1 Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to check the feature-gate status of the crate that defined the closure, to avoid ICEs?

Crate A:

#![feature(clone_closures)]

pub trait Generic {
    fn foo<T: Clone>(self, t: T);
}

pub fn example<G: Generic>(g: G) {
    g.foo(|| {});
}

Crate B:

// no feature flags!

extern crate a;

struct S;
impl Generic for S {
    fn foo<T: Clone>(self, t: T) { let _ = t.clone(); }
}

fn main() {
    // trans will monomorphize this, and will try to evaluate `[closure]: Clone`, which
    // will fail and cause an ICE.
    foo::example(S);
}

Another option would be to make all closures Copy & Clone in trans, but that would be visible through specialization etc. so I'm not sure it's the better idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arielb1 Yes you're right. I'm working on it.

Some(trait_id) == self.tcx().lang_items().copy_trait() &&
self.tcx().has_copy_closures(def_id.krate);
let clone_closures =
Some(trait_id) == self.tcx().lang_items().clone_trait() &&
self.tcx().has_clone_closures(def_id.krate);

if copy_closures || clone_closures {
Where(ty::Binder(substs.upvar_tys(def_id, self.tcx()).collect()))
} else {
Never
}
}

ty::TyAdt(..) | ty::TyProjection(..) | ty::TyParam(..) | ty::TyAnon(..) => {
// Fallback to whatever user-defined impls exist in this case.
None
Expand Down Expand Up @@ -2370,10 +2385,10 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
self.sized_conditions(obligation)
}
_ if Some(trait_def) == lang_items.copy_trait() => {
self.copy_conditions(obligation)
self.copy_clone_conditions(obligation)
}
_ if Some(trait_def) == lang_items.clone_trait() => {
self.copy_conditions(obligation)
self.copy_clone_conditions(obligation)
}
_ => bug!("unexpected builtin trait {:?}", trait_def)
};
Expand Down
8 changes: 8 additions & 0 deletions src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2247,4 +2247,12 @@ pub fn provide(providers: &mut ty::maps::Providers) {
assert_eq!(cnum, LOCAL_CRATE);
tcx.output_filenames.clone()
};
providers.has_copy_closures = |tcx, cnum| {
assert_eq!(cnum, LOCAL_CRATE);
tcx.sess.features.borrow().copy_closures
};
providers.has_clone_closures = |tcx, cnum| {
assert_eq!(cnum, LOCAL_CRATE);
tcx.sess.features.borrow().clone_closures
};
}
2 changes: 1 addition & 1 deletion src/librustc/ty/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub enum InstanceDef<'tcx> {
/// drop_in_place::<T>; None for empty drop glue.
DropGlue(DefId, Option<Ty<'tcx>>),

/// Builtin method implementation, e.g. `Clone::clone`.
///`<T as Clone>::clone` shim.
CloneShim(DefId, Ty<'tcx>),
}

Expand Down
12 changes: 12 additions & 0 deletions src/librustc/ty/maps/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,3 +490,15 @@ impl<'tcx> QueryDescription for queries::output_filenames<'tcx> {
format!("output_filenames")
}
}

impl<'tcx> QueryDescription for queries::has_clone_closures<'tcx> {
fn describe(_tcx: TyCtxt, _: CrateNum) -> String {
format!("seeing if the crate has enabled `Clone` closures")
}
}

impl<'tcx> QueryDescription for queries::has_copy_closures<'tcx> {
fn describe(_tcx: TyCtxt, _: CrateNum) -> String {
format!("seeing if the crate has enabled `Copy` closures")
}
}
3 changes: 3 additions & 0 deletions src/librustc/ty/maps/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,9 @@ define_maps! { <'tcx>
[] fn compile_codegen_unit: CompileCodegenUnit(InternedString) -> Stats,
[] fn output_filenames: output_filenames_node(CrateNum)
-> Arc<OutputFilenames>,

[] fn has_copy_closures: HasCopyClosures(CrateNum) -> bool,
[] fn has_clone_closures: HasCloneClosures(CrateNum) -> bool,
}

//////////////////////////////////////////////////////////////////////
Expand Down
10 changes: 10 additions & 0 deletions src/librustc_metadata/cstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,16 @@ impl CrateMetadata {
attr::contains_name(&attrs, "no_builtins")
}

pub fn has_copy_closures(&self) -> bool {
let attrs = self.get_item_attrs(CRATE_DEF_INDEX);
attr::contains_feature_attr(&attrs, "copy_closures")
}

pub fn has_clone_closures(&self) -> bool {
let attrs = self.get_item_attrs(CRATE_DEF_INDEX);
attr::contains_feature_attr(&attrs, "clone_closures")
}

pub fn panic_strategy(&self) -> PanicStrategy {
self.root.panic_strategy.clone()
}
Expand Down
3 changes: 3 additions & 0 deletions src/librustc_metadata/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,9 @@ provide! { <'tcx> tcx, def_id, other, cdata,
}

used_crate_source => { Rc::new(cdata.source.clone()) }

has_copy_closures => { cdata.has_copy_closures() }
has_clone_closures => { cdata.has_clone_closures() }
}

pub fn provide_local<'tcx>(providers: &mut Providers<'tcx>) {
Expand Down
23 changes: 17 additions & 6 deletions src/librustc_mir/shim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,15 @@ fn build_clone_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
let len = len.val.to_const_int().unwrap().to_u64().unwrap();
builder.array_shim(ty, len)
}
ty::TyTuple(tys, _) => builder.tuple_shim(tys),
ty::TyClosure(def_id, substs) => {
builder.tuple_like_shim(
&substs.upvar_tys(def_id, tcx).collect::<Vec<_>>(),
AggregateKind::Closure(def_id, substs)
)
}
ty::TyTuple(tys, _) => builder.tuple_like_shim(&**tys, AggregateKind::Tuple),
_ => {
bug!("clone shim for `{:?}` which is not `Copy` and is not an aggregate", self_ty);
bug!("clone shim for `{:?}` which is not `Copy` and is not an aggregate", self_ty)
}
};

Expand Down Expand Up @@ -613,7 +619,12 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> {
self.block(vec![], TerminatorKind::Resume, true);
}

fn tuple_shim(&mut self, tys: &ty::Slice<Ty<'tcx>>) {
fn tuple_like_shim(&mut self, tys: &[ty::Ty<'tcx>], kind: AggregateKind<'tcx>) {
match kind {
AggregateKind::Tuple | AggregateKind::Closure(..) => (),
_ => bug!("only tuples and closures are accepted"),
};

let rcvr = Lvalue::Local(Local::new(1+0)).deref();

let mut returns = Vec::new();
Expand Down Expand Up @@ -646,17 +657,17 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> {
}
}

// `return (returns[0], returns[1], ..., returns[tys.len() - 1]);`
// `return kind(returns[0], returns[1], ..., returns[tys.len() - 1]);`
let ret_statement = self.make_statement(
StatementKind::Assign(
Lvalue::Local(RETURN_POINTER),
Rvalue::Aggregate(
box AggregateKind::Tuple,
box kind,
returns.into_iter().map(Operand::Consume).collect()
)
)
);
self.block(vec![ret_statement], TerminatorKind::Return, false);
self.block(vec![ret_statement], TerminatorKind::Return, false);
}
}

Expand Down
14 changes: 14 additions & 0 deletions src/libsyntax/attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,20 @@ pub fn first_attr_value_str_by_name(attrs: &[Attribute], name: &str) -> Option<S
.and_then(|at| at.value_str())
}

/// Check if `attrs` contains an attribute like `#![feature(feature_name)]`.
/// This will not perform any "sanity checks" on the form of the attributes.
pub fn contains_feature_attr(attrs: &[Attribute], feature_name: &str) -> bool {
attrs.iter().any(|item| {
item.check_name("feature") &&
item.meta_item_list().map(|list| {
list.iter().any(|mi| {
mi.word().map(|w| w.name() == feature_name)
.unwrap_or(false)
})
}).unwrap_or(false)
})
}

/* Higher-level applications */

pub fn find_crate_name(attrs: &[Attribute]) -> Option<Symbol> {
Expand Down
31 changes: 27 additions & 4 deletions src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,10 @@ declare_features! (

// allow '|' at beginning of match arms (RFC 1925)
(active, match_beginning_vert, "1.21.0", Some(44101)),

// Copy/Clone closures (RFC 2132)
(active, clone_closures, "1.22.0", Some(44490)),
(active, copy_closures, "1.22.0", Some(44490)),
);

declare_features! (
Expand Down Expand Up @@ -1573,7 +1577,7 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
pub fn get_features(span_handler: &Handler, krate_attrs: &[ast::Attribute]) -> Features {
let mut features = Features::new();

let mut feature_checker = MutexFeatureChecker::default();
let mut feature_checker = FeatureChecker::default();

for attr in krate_attrs {
if !attr.check_name("feature") {
Expand Down Expand Up @@ -1622,14 +1626,16 @@ pub fn get_features(span_handler: &Handler, krate_attrs: &[ast::Attribute]) -> F
features
}

// A collector for mutually-exclusive features and their flag spans
/// A collector for mutually exclusive and interdependent features and their flag spans.
#[derive(Default)]
struct MutexFeatureChecker {
struct FeatureChecker {
proc_macro: Option<Span>,
custom_attribute: Option<Span>,
copy_closures: Option<Span>,
clone_closures: Option<Span>,
}

impl MutexFeatureChecker {
impl FeatureChecker {
// If this method turns out to be a hotspot due to branching,
// the branching can be eliminated by modifying `set!()` to set these spans
// only for the features that need to be checked for mutual exclusion.
Expand All @@ -1642,6 +1648,14 @@ impl MutexFeatureChecker {
if features.custom_attribute {
self.custom_attribute = self.custom_attribute.or(Some(span));
}

if features.copy_closures {
self.copy_closures = self.copy_closures.or(Some(span));
}

if features.clone_closures {
self.clone_closures = self.clone_closures.or(Some(span));
}
}

fn check(self, handler: &Handler) {
Expand All @@ -1653,6 +1667,15 @@ impl MutexFeatureChecker {

panic!(FatalError);
}

if let (Some(span), None) = (self.copy_closures, self.clone_closures) {
handler.struct_span_err(span, "`#![feature(copy_closures)]` can only be used with \
`#![feature(clone_closures)]`")
.span_note(span, "`#![feature(copy_closures)]` declared here")
.emit();

panic!(FatalError);
}
}
}

Expand Down
21 changes: 21 additions & 0 deletions src/test/compile-fail/feature-gate-clone-closures.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2017 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[derive(Clone)]
struct S(i32);

fn main() {
let a = S(5);
let hello = move || {
println!("Hello {}", a.0);
};

let hello = hello.clone(); //~ ERROR no method named `clone` found for type
}
19 changes: 19 additions & 0 deletions src/test/compile-fail/feature-gate-copy-closures.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright 2017 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn main() {
let a = 5;
let hello = || {
println!("Hello {}", a);
};

let b = hello;
let c = hello; //~ ERROR use of moved value: `hello` [E0382]
}
24 changes: 24 additions & 0 deletions src/test/compile-fail/not-clone-closure.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2017 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Check that closures do not implement `Clone` if their environment is not `Clone`.

#![feature(clone_closures)]

struct S(i32);

fn main() {
let a = S(5);
let hello = move || {
println!("Hello {}", a.0);
};

let hello = hello.clone(); //~ ERROR the trait bound `S: std::clone::Clone` is not satisfied
}
24 changes: 24 additions & 0 deletions src/test/compile-fail/not-copy-closure.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2017 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Check that closures do not implement `Copy` if their environment is not `Copy`.

#![feature(copy_closures)]
#![feature(clone_closures)]

fn main() {
let mut a = 5;
let hello = || {
a += 1;
};

let b = hello;
let c = hello; //~ ERROR use of moved value: `hello` [E0382]
}
Loading