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

Tracking issue for RFC 2132: copy_closures/clone_closures #44490

Closed
aturon opened this issue Sep 11, 2017 · 21 comments

Comments

Projects
None yet
@aturon
Copy link
Member

commented Sep 11, 2017

This is a tracking issue for the RFC "Copy/Clone Closures" (rust-lang/rfcs#2132).

Steps:

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

I believe that @scalexm has implemented this RFC already. =)

@arielb1

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

@nmatsakis

That was for tuples, not for closures.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2017

@arielb1 I was referring to #44551

bors added a commit that referenced this issue Sep 21, 2017

Auto merge of #44551 - scalexm:copy-clone-closures, r=arielb1
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...
@scalexm

This comment has been minimized.

Copy link
Member

commented Sep 25, 2017

(I think we can tick the first box)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2017

What do we want to do around stabilization here? @cramertj was floating the idea of stabilizing only the clone impl for closures, not the copy one, at least until such time as we have a workable "unexpected copy" lint that would push you to invoke clone when copying closures (but that lint probably wants a bit of design).

Personally, I think I'd be ok stabilizing copy/clone closures in their entirety, but we probably want to see a bit more usage in the wild.

Can anyone report on using this feature?

@cramertj

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

I've been messing with it in some personal stuff, and I've really enjoyed having the feature. I'd be in favor of stabilizing both Copy and Clone.

@mbrubeck mbrubeck changed the title Tracking issue for RFC 2132: Copy/Clone Closures Tracking issue for RFC 2132: copy_closures/clone_closures Nov 17, 2017

@mbrubeck mbrubeck added the B-unstable label Nov 17, 2017

@tuxmark5

This comment has been minimized.

Copy link

commented Nov 24, 2017

Any plans to extend this to support cloning generators as well?

@solson

This comment has been minimized.

Copy link
Member

commented Dec 9, 2017

I haven't used this directly yet, but I can definitely report that people have complained to me that we don't have this feature on stable yet because it's unnecessarily forced them to change their design. It solves one of those irritating "when you want it, you really want it" problems, imo. :)

@alercah

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2018

What's the status on this? Is it ready to stabilize once docs are ready? I'm planning a rewrite of some closure-related docs in the reference, so if this is going in, I'll include that.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

@alercah yes basically. @cramertj -- would you be up for doing a quick stabilization report? (showing the tests in place and highlighting how any unresolved questions were resolved)

@cramertj

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

@rfcbot fcp merge

I propose to stabilize the clone_closures and copy_closures features, first implemented in #44551.

The following examples demonstrate that the feature is working as intended.

The RFC specifies that non-move closures which do not mutate captured variables are always Copy and Clone: Playground link

The RFC specifies that non-move closures which mutate captured variables are neither Copy nor Clone: Playground link

The RFC specifies that move closures are only Copy or Clone if the values they capture are Copy or Clone: Playground link

One mentioned drawback of the RFC is that cloned move closures which mutate variables will mutate independently of the other copies of the same closure. This is intuitive if you think about each copy of the closure as being its own state machine, but it can be surprising. In order to mitigate this, there is ongoing work on an MIR-based Copy lint to detect cases where copies might cause surprising changes here. I don't believe this issue will arise often enough in practice that we should block stabilization on it.

@rfcbot

This comment has been minimized.

Copy link

commented Feb 22, 2018

Team member @cramertj has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@alercah

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2018

"The RFC specifies that non-move closures which do not mutate captured variables are always Copy and Clone"

"The RFC specifies that non-move closures which mutate captured variables are neither Copy nor Clone"

This omits the case where a capture of a Clone type is made by move into a non-move closure. In this case, the closure will be Clone but not Copy. See https://play.rust-lang.org/?gist=710b5c376ae8338867869c0e62f579d2&version=nightly.

I believe this is the correct behaviour, but wanted to draw attention to it anyway.

@cramertj

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

Yes, you're correct-- I should have said non-move closures which do not mutate or move captured variables are copy.

@rfcbot

This comment has been minimized.

Copy link

commented Feb 23, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

commented Mar 5, 2018

The final comment period is now complete.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

I marked as E-mentor -- looks like we're ready for a stabilization PR here, right? There are instructions on the forge.

@alercah

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

The reference is already updated. RBE probably doesn't need updating, the stdlib could maybe use mention on Copy and Clone traits, and I'm unsure about the book. @steveklabnik?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2018

@alercah sounds right

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

Stabilization PR (with mention in docs for Clone and Copy): #49299

kennytm added a commit to kennytm/rust that referenced this issue Mar 24, 2018

Rollup merge of rust-lang#49299 - SimonSapin:ubiquity, r=nikomatsakis
Stabilize the copy_closures and clone_closures features

In addition to the `Fn*` family of traits, closures now implement `Copy` (and similarly `Clone`) if all of the captures do.

Tracking issue: rust-lang#44490

kennytm added a commit to kennytm/rust that referenced this issue Mar 24, 2018

Rollup merge of rust-lang#49299 - SimonSapin:ubiquity, r=nikomatsakis
Stabilize the copy_closures and clone_closures features

In addition to the `Fn*` family of traits, closures now implement `Copy` (and similarly `Clone`) if all of the captures do.

Tracking issue: rust-lang#44490
@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2018

#49299 is merged, I think this is it. Let me know if I forgot something relevant to this tracking issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.