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.

Sign up for GitHub

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

Jump to bottom

Allow generators to impl Clone/Copy #95137

Closed
wants to merge 12 commits into from
Closed

Allow generators to impl Clone/Copy #95137

wants to merge 12 commits into from

Conversation

canndrew
Copy link
Contributor

This PR allows generators to implement Clone/Copy if all their upvars and all their locals which are held across a yield implement Clone/Copy. This only applies to non-static generators, and so excludes generators which contain self-references and the generators that async blocks/fns desugar to,

r? @oli-obk

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 20, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 20, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 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 Mar 20, 2022
@canndrew canndrew mentioned this pull request Mar 27, 2022
Open
3 tasks
oli-obk
oli-obk requested changes Mar 28, 2022
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

I'd like to see some tests on various kinds of async blocks, too. Even if we're just testing the diagnostics that way.

compiler/rustc_mir_transform/src/shim.rs Show resolved Hide resolved
compiler/rustc_mir_transform/src/shim.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved
@canndrew
Copy link
Contributor Author

Sorry for the delay. I've done the requested changes.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@canndrew
Copy link
Contributor Author

I've also added a test which checks that feature(generator_clone) doesn't interfere with the clonability of async blocks and doesn't effect the error message of trying to clone an async block. And I've also rebased off of master in order to fix the ui tests which were failing due to minor changes in diagnostics.

oli-obk
oli-obk reviewed Apr 12, 2022
View reviewed changes
compiler/rustc_trait_selection/src/traits/select/mod.rs
Comment on lines +1909 to +1913
let all = substs
.as_generator()
.upvar_tys()
.chain(iter::once(substs.as_generator().witness()))
.collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not due to this PR, but I really wish we had ways to track where an inference var got resolved and then bubble this span up here. In the current design there is no way to point at the local that causes the generator to stop being clone/copy, all we can do is keep pointing at the entire generator :(

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 8, 2022
@bors
Copy link
Contributor

bors commented May 15, 2022

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

@oli-obk oli-obk removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 11, 2022
@oli-obk oli-obk added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jun 11, 2022

Oof I forgot about this. Changed the label so it shows up in my queue

@oli-obk
Copy link
Contributor

oli-obk commented Jun 21, 2022

ok, did another pass. This lgtm now. r=me after a rebase

@oli-obk oli-obk changed the title WIP: Allow generators to impl Clone/Copy Allow generators to impl Clone/Copy Jun 21, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jun 21, 2022

@rustbot author

@rustbot rustbot 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 Jun 21, 2022
@JohnCSimon
Copy link
Member

ok, did another pass. This lgtm now. r=me after a rebase

ping from triage:
@canndrew
Can you please rebase so we can move forward?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@JohnCSimon
Copy link
Member

@canndrew

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Sep 11, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Sep 11, 2022
@crlf0710 crlf0710 mentioned this pull request Sep 11, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2022
@bors
Auto merge of rust-lang#101673 - crlf0710:generator_clone, r=oli-obk
6f0c4a6 
Allow generators to impl Clone/Copy

Revives rust-lang#95137. It's a pity that the original pr didn't land because the implementation is almost complete! All credits goes to `@canndrew,` and i just resolved the merge conflicts and updated the feature gate version number.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk requested changes

Assignees

@oli-obk oli-obk

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@canndrew @rust-log-analyzer @bors @oli-obk @JohnCSimon @rust-highfive @bjorn3 @rustbot