-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Make the computation of coroutine_captures_by_ref_ty
more sophisticated
#123660
Conversation
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
field_used_at_least_once = true; | ||
break for_each((parent_field_idx, parent_capture), (child_field_idx, child_capture)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing this fails to do is assert that the parents captures are all used at least once. I'm tempted to use a coroutine for this and just use iter::from_coroutine
...
7dbc68b
to
76b84f5
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #123708) made this pull request unmergeable. Please resolve the merge conflicts. |
3784660
to
c5efc2d
Compare
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
r? oli-obk |
This comment has been minimized.
This comment has been minimized.
c5efc2d
to
a183b48
Compare
☔ The latest upstream changes (presumably #123725) made this pull request unmergeable. Please resolve the merge conflicts. |
a183b48
to
599d456
Compare
Nice (and great docs, thanks!) @bors r+ rollup |
…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#123660 (Make the computation of `coroutine_captures_by_ref_ty` more sophisticated) - rust-lang#123738 (Call lower_const_param instead of duplicating the code) - rust-lang#123774 (Fix typo MaybeUnit -> MaybeUninit) - rust-lang#123790 (correct the handling of `bootstrap-cache-path` option) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#123660 - compiler-errors:coroutine-closure-env, r=oli-obk Make the computation of `coroutine_captures_by_ref_ty` more sophisticated Currently, we treat all the by-(mut/)ref borrows of a coroutine-closure as having a "closure env" borrowed lifetime. When we have the given code: ```rust let x: &'a i32 = ...; let c = async || { let _x = *x; }; ``` Then when we call: ```rust c() // which, because `AsyncFn` takes a `&self`, we insert an autoref: (&c /* &'env {coroutine-closure} */)() ``` We will return a future whose captures contain `&'env i32` instead of `&'a i32`, which is way more restrictive than necessary. We should be able to drop `c` while the future is alive since it's not actually borrowing any data *originating from within* the closure's captures, but since the capture has that `'env` lifetime, this is not possible. This wouldn't be true, for example, if the closure captured `i32` instead of `&'a i32`, because the `'env` lifetime is actually *necessary* since the data (`i32`) is owned by the closure. This PR identifies two criteria where we *need* to take the borrow with the closure env lifetime: 1. If the closure borrows data from inside the closure's captures. This is not true if the parent capture is by-ref, OR if the parent capture is by-move and the child capture begins with a deref projection. This is the example described above. 2. If we're dealing with mutable references, since we cannot reborrow `&'env mut &'a mut i32` into `&'a mut i32`, *only* `&'env mut i32`. See the documentation on `should_reborrow_from_env_of_parent_coroutine_closure` for more info. **important:** As disclaimer states on that function, luckily, if this heuristic 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. Fixes rust-lang#123241
… 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
… 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
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
Currently, we treat all the by-(mut/)ref borrows of a coroutine-closure as having a "closure env" borrowed lifetime.
When we have the given code:
Then when we call:
We will return a future whose captures contain
&'env i32
instead of&'a i32
, which is way more restrictive than necessary. We should be able to dropc
while the future is alive since it's not actually borrowing any data originating from within the closure's captures, but since the capture has that'env
lifetime, this is not possible.This wouldn't be true, for example, if the closure captured
i32
instead of&'a i32
, because the'env
lifetime is actually necessary since the data (i32
) is owned by the closure.This PR identifies two criteria where we need to take the borrow with the closure env lifetime:
&'env mut &'a mut i32
into&'a mut i32
, only&'env mut i32
.See the documentation on
should_reborrow_from_env_of_parent_coroutine_closure
for more info.important: As disclaimer states on that function, luckily, if this heuristic 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.
Fixes #123241