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 async closures #128201

Merged
merged 2 commits into from
Jul 27, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jul 25, 2024

We can do so in the same cases that regular closures do.

For the purposes of cloning, coroutine-closures are actually precisely the same as regular closures, specifically in the aspect that Clone impls care about which is the upvars. The only difference b/w coroutine-closures and regular closures is the type that they return, but this type has not been created yet, so we don't really have a problem.

IDK why I didn't add this impl initially -- I went back and forth a bit on the internal representation for coroutine-closures before settling on a design which largely models regular closures. Previous (not published) iterations of coroutine-closures used to be represented as a special (read: cursed) kind of coroutine, which would probably suffer from the pitfalls that coroutines have that oli mentioned below in #128201 (comment).

r? oli-obk

@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2024

Could not assign reviewer from: oli-obk.
User(s) oli-obk are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Jul 25, 2024
@oli-obk oli-obk assigned oli-obk and unassigned fmease Jul 25, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Jul 25, 2024

Please add a test where a coroutine closure can't be cloned, but clone is called on it.

Just thinking out loud, but lmk if anything is wrong in my reasoning: Coroutine closures are slightly different from regular closures due to the lending behaviour, but since that is handled by borrowck, I don't see any issues. It's very different from coroutines that can't be cloned soundly anymore after ending up with self-borrows.

@compiler-errors
Copy link
Member Author

For the purposes of cloning, coroutine-closures are actually precisely the same as regular closures, specifically in the aspect that Clone impls care about which is the upvars. The only difference b/w coroutine-closures and regular closures is the type that they return, but this type has not been created yet, so we don't really have a problem.

I'll add a few test.

@rustbot
Copy link
Collaborator

rustbot commented Jul 26, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@compiler-errors
Copy link
Member Author

I added a "can't clone closure" test, and fleshed out the PR descr.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 26, 2024

bors r plus

@bors
Copy link
Contributor

bors commented Jul 26, 2024

📌 Commit 5a9959f has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 26, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 27, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#124941 (Stabilize const `{integer}::from_str_radix` i.e. `const_int_from_str`)
 - rust-lang#128201 (Implement `Copy`/`Clone` for async closures)
 - rust-lang#128210 (rustdoc: change title of search results)
 - rust-lang#128223 (Refactor complex conditions in `collect_tokens_trailing_token`)
 - rust-lang#128224 (Remove unnecessary range replacements)
 - rust-lang#128226 (Remove redundant option that was just encoding that a slice was empty)
 - rust-lang#128227 (CI: do not respect custom try jobs for unrolled perf builds)
 - rust-lang#128229 (Improve `extern "<abi>" unsafe fn()` error message)
 - rust-lang#128235 (Fix `Iterator::filter` docs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bd18f88 into rust-lang:master Jul 27, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 27, 2024
Rollup merge of rust-lang#128201 - compiler-errors:closure-clone, r=oli-obk

Implement `Copy`/`Clone` for async closures

We can do so in the same cases that regular closures do.

For the purposes of cloning, coroutine-closures are actually precisely the same as regular closures, specifically in the aspect that `Clone` impls care about which is the upvars. The only difference b/w coroutine-closures and regular closures is the type that they *return*, but this type has not been *created* yet, so we don't really have a problem.

IDK why I didn't add this impl initially -- I went back and forth a bit on the internal representation for coroutine-closures before settling on a design which largely models regular closures. Previous (not published) iterations of coroutine-closures used to be represented as a special (read: cursed) kind of coroutine, which would probably suffer from the pitfalls that coroutines have that oli mentioned below in rust-lang#128201 (comment).

r? oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants