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

An async closure may implement FnMut/Fn if it has no self-borrows #125259

Merged
merged 1 commit into from
May 22, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented May 18, 2024

There's no reason that async closures may not implement FnMut or Fn if they don't actually borrow anything with the closure's env lifetime. Specifically, #123660 made it so that we don't always need to borrow captures from the closure's env.

See the doc comment on should_reborrow_from_env_of_parent_coroutine_closure:

/// Determines whether a child capture that is derived from a parent capture
/// should be borrowed with the lifetime of the parent coroutine-closure's env.
///
/// There are two cases when this needs to happen:
///
/// (1.) Are we borrowing data owned by the parent closure? We can determine if
/// that is the case by checking if the parent capture is by move, EXCEPT if we
/// apply a deref projection, which means we're reborrowing a reference that we
/// captured by move.
///
/// ```rust
/// #![feature(async_closure)]
/// let x = &1i32; // Let's call this lifetime `'1`.
/// let c = async move || {
/// println!("{:?}", *x);
/// // Even though the inner coroutine borrows by ref, we're only capturing `*x`,
/// // not `x`, so the inner closure is allowed to reborrow the data for `'1`.
/// };
/// ```
///
/// (2.) If a coroutine is mutably borrowing from a parent capture, then that
/// mutable borrow cannot live for longer than either the parent *or* the borrow
/// that we have on the original upvar. Therefore we always need to borrow the
/// child capture with the lifetime of the parent coroutine-closure's env.
///
/// ```rust
/// #![feature(async_closure)]
/// let mut x = 1i32;
/// let c = async || {
/// x = 1;
/// // The parent borrows `x` for some `&'1 mut i32`.
/// // However, when we call `c()`, we implicitly autoref for the signature of
/// // `AsyncFnMut::async_call_mut`. Let's call that lifetime `'call`. Since
/// // the maximum that `&'call mut &'1 mut i32` can be reborrowed is `&'call mut i32`,
/// // the inner coroutine should capture w/ the lifetime of the coroutine-closure.
/// };
/// ```
///
/// If either of these cases apply, then we should capture the borrow with the
/// lifetime of the parent coroutine-closure's env. Luckily, if this function is
/// not correct, then the program is not unsound, since we still borrowck and validate
/// the choices made from this function -- the only side-effect is that the user
/// may receive unnecessary borrowck errors.
fn should_reborrow_from_env_of_parent_coroutine_closure<'tcx>(
parent_capture: &ty::CapturedPlace<'tcx>,
child_capture: &ty::CapturedPlace<'tcx>,
) -> bool {

If there are no such borrows, then we are free to implement FnMut and Fn as permitted by our closure's inferred ClosureKind.

As far as I can tell, this change makes async || {} work in precisely the set of places they used to work before #120361.
Fixes #125247.

r? oli-obk

@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 May 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 18, 2024

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@oli-obk
Copy link
Contributor

oli-obk commented May 22, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 22, 2024

📌 Commit 2e97dae 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 May 22, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 22, 2024
… r=oli-obk

An async closure may implement `FnMut`/`Fn` if it has no self-borrows

There's no reason that async closures may not implement `FnMut` or `Fn` if they don't actually borrow anything with the closure's env lifetime. Specifically, rust-lang#123660 made it so that we don't always need to borrow captures from the closure's env.

See the doc comment on `should_reborrow_from_env_of_parent_coroutine_closure`:

https://github.com/rust-lang/rust/blob/c00957a3e269219413041a4e3565f33b1f9d0779/compiler/rustc_hir_typeck/src/upvar.rs#L1777-L1823

If there are no such borrows, then we are free to implement `FnMut` and `Fn` as permitted by our closure's inferred `ClosureKind`.

As far as I can tell, this change makes `async || {}` work in precisely the set of places they used to work before rust-lang#120361.
Fixes rust-lang#125247.

r? oli-obk
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#124227 (Make sure that the method resolution matches in `note_source_of_type_mismatch_constraint`)
 - rust-lang#124896 (miri: rename intrinsic_fallback_checks_ub to intrinsic_fallback_is_spec)
 - rust-lang#125015 (Pattern types: Prohibit generic args on const params)
 - rust-lang#125043 (reference type safety invariant docs: clarification)
 - rust-lang#125259 (An async closure may implement `FnMut`/`Fn` if it has no self-borrows)
 - rust-lang#125306 (Force the inner coroutine of an async closure to `move` if the outer closure is `move` and `FnOnce`)
 - rust-lang#125378 (remove tracing tree indent lines)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#124896 (miri: rename intrinsic_fallback_checks_ub to intrinsic_fallback_is_spec)
 - rust-lang#125015 (Pattern types: Prohibit generic args on const params)
 - rust-lang#125049 (Disallow cast with trailing braced macro in let-else)
 - rust-lang#125259 (An async closure may implement `FnMut`/`Fn` if it has no self-borrows)
 - rust-lang#125296 (Fix `unexpected_cfgs` lint on std)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 44c7a2d into rust-lang:master May 22, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
Rollup merge of rust-lang#125259 - compiler-errors:fn-mut-as-a-treat, r=oli-obk

An async closure may implement `FnMut`/`Fn` if it has no self-borrows

There's no reason that async closures may not implement `FnMut` or `Fn` if they don't actually borrow anything with the closure's env lifetime. Specifically, rust-lang#123660 made it so that we don't always need to borrow captures from the closure's env.

See the doc comment on `should_reborrow_from_env_of_parent_coroutine_closure`:

https://github.com/rust-lang/rust/blob/c00957a3e269219413041a4e3565f33b1f9d0779/compiler/rustc_hir_typeck/src/upvar.rs#L1777-L1823

If there are no such borrows, then we are free to implement `FnMut` and `Fn` as permitted by our closure's inferred `ClosureKind`.

As far as I can tell, this change makes `async || {}` work in precisely the set of places they used to work before rust-lang#120361.
Fixes rust-lang#125247.

r? oli-obk
@fmease
Copy link
Member

fmease commented May 22, 2024

bors sleepy @bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 22, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 2, 2024
…v-shim, r=oli-obk

Fix `FnMut::call_mut`/`Fn::call` shim for async closures that capture references

I adjusted async closures to be able to implement `Fn` and `FnMut` *even if* they capture references, as long as those references did not need to borrow data from the closure captures themselves. See rust-lang#125259.

However, when I did this, I didn't actually relax an assertion in the `build_construct_coroutine_by_move_shim` shim code, which builds the `Fn`/`FnMut`/`FnOnce` implementations for async closures. Therefore, if we actually tried to *call* `FnMut`/`Fn` on async closures, it would ICE.

This PR adjusts this assertion to ensure that we only capture immutable references in closures if they implement `Fn`/`FnMut`. It also adds a bunch of tests and makes more of the async-closure tests into `build-pass` since we often care about these tests actually generating the right closure shims and stuff. I think it might be excessive to *always* use build-pass here, but 🤷 it's not that big of a deal.

Fixes rust-lang#127019
Fixes rust-lang#127012

r? oli-obk
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 2, 2024
…v-shim, r=oli-obk

Fix `FnMut::call_mut`/`Fn::call` shim for async closures that capture references

I adjusted async closures to be able to implement `Fn` and `FnMut` *even if* they capture references, as long as those references did not need to borrow data from the closure captures themselves. See rust-lang#125259.

However, when I did this, I didn't actually relax an assertion in the `build_construct_coroutine_by_move_shim` shim code, which builds the `Fn`/`FnMut`/`FnOnce` implementations for async closures. Therefore, if we actually tried to *call* `FnMut`/`Fn` on async closures, it would ICE.

This PR adjusts this assertion to ensure that we only capture immutable references in closures if they implement `Fn`/`FnMut`. It also adds a bunch of tests and makes more of the async-closure tests into `build-pass` since we often care about these tests actually generating the right closure shims and stuff. I think it might be excessive to *always* use build-pass here, but 🤷 it's not that big of a deal.

Fixes rust-lang#127019
Fixes rust-lang#127012

r? oli-obk
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 2, 2024
…env-shim, r=oli-obk

Fix `FnMut::call_mut`/`Fn::call` shim for async closures that capture references

I adjusted async closures to be able to implement `Fn` and `FnMut` *even if* they capture references, as long as those references did not need to borrow data from the closure captures themselves. See rust-lang#125259.

However, when I did this, I didn't actually relax an assertion in the `build_construct_coroutine_by_move_shim` shim code, which builds the `Fn`/`FnMut`/`FnOnce` implementations for async closures. Therefore, if we actually tried to *call* `FnMut`/`Fn` on async closures, it would ICE.

This PR adjusts this assertion to ensure that we only capture immutable references in closures if they implement `Fn`/`FnMut`. It also adds a bunch of tests and makes more of the async-closure tests into `build-pass` since we often care about these tests actually generating the right closure shims and stuff. I think it might be excessive to *always* use build-pass here, but 🤷 it's not that big of a deal.

Fixes rust-lang#127019
Fixes rust-lang#127012

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-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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
5 participants