Skip to content

Implement Copy/Clone for async closures #128201

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

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 (-Znext-solver) 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
Collaborator

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
# for free to join this conversation on GitHub. Already have an account? # 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 (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants