Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Implement Copy/Clone for closures #44551

Merged
merged 2 commits into from
Sep 21, 2017
Merged

Conversation

scalexm
Copy link
Member

@scalexm scalexm commented Sep 13, 2017

Implement RFC #2132 (tracking issue: #44490).

NB: I'm not totally sure about the whole feature gates thing, that's my first PR of this kind...

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@aidanhs
Copy link
Member

aidanhs commented Sep 13, 2017

Thanks for the PR @scalexm, we'll check in now and again to make sure @pnkfelix or another reviewer gets to this soon!

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2017
@@ -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.

@@ -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::Ty<'tcx>>) {
fn tuple_like_shim(&mut self, tys: &[ty::Ty<'tcx>], kind: AggregateKind<'tcx>) {
match kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this assertion? This method works just as well for structs and univariant enums.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, but it won't work for arrays. I guess the match can be extended in the future if needed.

@bors
Copy link
Contributor

bors commented Sep 17, 2017

☔ The latest upstream changes (presumably #44634) made this pull request unmergeable. Please resolve the merge conflicts.

@arielb1 arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2017
@arielb1
Copy link
Contributor

arielb1 commented Sep 20, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Sep 20, 2017

📌 Commit 3fa3fe0 has been approved by arielb1

@arielb1 arielb1 added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 20, 2017
bors added a commit that referenced this pull request Sep 21, 2017
Implement `Copy`/`Clone` for closures

Implement RFC [#2132](rust-lang/rfcs#2132) (tracking issue: #44490).

NB: I'm not totally sure about the whole feature gates thing, that's my first PR of this kind...
@bors
Copy link
Contributor

bors commented Sep 21, 2017

⌛ Testing commit 3fa3fe0 with merge e2504cf...

@bors
Copy link
Contributor

bors commented Sep 21, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing e2504cf to master...

@bors bors merged commit 3fa3fe0 into rust-lang:master Sep 21, 2017
@scalexm scalexm deleted the copy-clone-closures branch October 22, 2018 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants