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

lowering: extend temporary lifetimes around await #64292

Merged

Conversation

@davidtwco
Copy link
Member

commented Sep 8, 2019

Fixes #63832.

r? @matthewjasper
cc @nikomatsakis

Copy link
Member

left a comment

Could you please also update the doc comment on fn lower_expr_await to accurately reflect the new desugaring and then follow up with a PR against the reference (See rust-lang-nursery/reference#635 for @nikomatsakis's initial PR.)?

davidtwco added a commit to davidtwco/reference that referenced this pull request Sep 8, 2019
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco davidtwco force-pushed the davidtwco:issue-63832-await-temporary-lifetimes branch from 78ea5f5 to 2f3c957 Sep 8, 2019
@davidtwco davidtwco referenced this pull request Sep 8, 2019
@davidtwco

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2019

Thanks @Centril, fixed those comments and opened rust-lang-nursery/reference#674.

@Centril
Centril approved these changes Sep 8, 2019
@davidtwco davidtwco force-pushed the davidtwco:issue-63832-await-temporary-lifetimes branch from 2f3c957 to 8e3fb22 Sep 9, 2019
This commit changes the HIR lowering around `await` so that temporary
lifetimes are extended. Previously, await was lowered as:

```rust
{
    let mut pinned = future;
    loop {
        match ::std::future::poll_with_tls_context(unsafe {
            <::std::pin::Pin>::new_unchecked(&mut pinned)
        }) {
            ::std::task::Poll::Ready(result) => break result,
            ::std::task::Poll::Pending => {}
        }
        yield ();
    }
}
```

With this commit, await is lowered as:

```rust
match future {
    mut pinned => loop {
        match ::std::future::poll_with_tls_context(unsafe {
            <::std::pin::Pin>::new_unchecked(&mut pinned)
        }) {
            ::std::task::Poll::Ready(result) => break result,
            ::std::task::Poll::Pending => {}
        }
        yield ();
    }
}
```

However, this change has the following side-effects:

- All temporaries in future will be considered to live across a
  yield for the purpose of auto-traits.
- Borrowed temporaries in future are likely to be considered to be live
  across the yield for the purpose of the generator transform.

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco davidtwco force-pushed the davidtwco:issue-63832-await-temporary-lifetimes branch from 8e3fb22 to 63fad69 Sep 10, 2019
@matthewjasper

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

📌 Commit 63fad69 has been approved by matthewjasper

Centril added a commit to Centril/rust that referenced this pull request Sep 10, 2019
…ry-lifetimes, r=matthewjasper

lowering: extend temporary lifetimes around await

Fixes rust-lang#63832.

r? @matthewjasper
cc @nikomatsakis
bors added a commit that referenced this pull request Sep 10, 2019
Rollup of 8 pull requests

Successful merges:

 - #63786 (Make `abs`, `wrapping_abs`, `overflowing_abs` const functions)
 - #63989 (Add Yaah to clippy toolstain notification list)
 - #64256 (test/c-variadic: Fix patterns on powerpc64)
 - #64292 (lowering: extend temporary lifetimes around await)
 - #64311 (lldb: avoid mixing "Hit breakpoint" message with other output.)
 - #64330 (Clarify E0507 to note Fn/FnMut relationship to borrowing)
 - #64331 (Changed instant is earlier to instant is later)
 - #64344 (rustc_mir: buffer -Zdump-mir output instead of pestering the kernel constantly.)

Failed merges:

r? @ghost
@bors bors merged commit 63fad69 into rust-lang:master Sep 10, 2019
4 checks passed
4 checks passed
pr Build #20190910.16 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
jebrosen added a commit to SergioBenitez/Rocket that referenced this pull request Sep 11, 2019
@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Is it expected that this is a breaking change? A Miri testcase is failing now:

2019-09-10T19:17:04.0564321Z normalized stderr:
2019-09-10T19:17:04.0564553Z error[E0597]: `x` does not live long enough
2019-09-10T19:17:04.0565934Z   --> $DIR/async-fn.rs:18:13
2019-09-10T19:17:04.0566063Z    |
2019-09-10T19:17:04.0566130Z 18 |     async { x + y }.await
2019-09-10T19:17:04.0566458Z    |     --------^------
2019-09-10T19:17:04.0566539Z    |     |     | |
2019-09-10T19:17:04.0566641Z    |     |     | borrowed value does not live long enough
2019-09-10T19:17:04.0566727Z    |     |     value captured here by generator
2019-09-10T19:17:04.0566873Z    |     a temporary with access to the borrow is created here ...
2019-09-10T19:17:04.0566971Z 19 | }
2019-09-10T19:17:04.0567254Z    | -
2019-09-10T19:17:04.0567323Z    | |
2019-09-10T19:17:04.0567409Z    | `x` dropped here while still borrowed
2019-09-10T19:17:04.0567524Z    | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `impl std::future::Future`
2019-09-10T19:17:04.0567653Z    |
2019-09-10T19:17:04.0568341Z    = note: The temporary is part of an expression at the end of a block. Consider forcing this temporary to be dropped sooner, before the block's local variables are dropped. For example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block.
2019-09-10T19:17:04.0568497Z 
2019-09-10T19:17:04.0568622Z error[E0597]: `y` does not live long enough
2019-09-10T19:17:04.0568922Z   --> $DIR/async-fn.rs:18:17
2019-09-10T19:17:04.0569038Z    |
2019-09-10T19:17:04.0569103Z 18 |     async { x + y }.await
2019-09-10T19:17:04.0569731Z    |     ------------^--
2019-09-10T19:17:04.0569811Z    |     |     |     |
2019-09-10T19:17:04.0569908Z    |     |     |     borrowed value does not live long enough
2019-09-10T19:17:04.0569999Z    |     |     value captured here by generator
2019-09-10T19:17:04.0570108Z    |     a temporary with access to the borrow is created here ...
2019-09-10T19:17:04.0570189Z 19 | }
2019-09-10T19:17:04.0570464Z    | -
2019-09-10T19:17:04.0570534Z    | |
2019-09-10T19:17:04.0570619Z    | `y` dropped here while still borrowed
2019-09-10T19:17:04.0570726Z    | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `impl std::future::Future`
2019-09-10T19:17:04.0570849Z    |
2019-09-10T19:17:04.0571489Z    = note: The temporary is part of an expression at the end of a block. Consider forcing this temporary to be dropped sooner, before the block's local variables are dropped. For example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block.
2019-09-10T19:17:04.0571685Z 
2019-09-10T19:17:04.0571753Z error: aborting due to 2 previous errors
@RalfJung RalfJung referenced this pull request Sep 11, 2019
bors added a commit to rust-lang/miri that referenced this pull request Sep 11, 2019
fix async test

Test got broken by rust-lang/rust#64292.
@yoshuawuyts

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Not sure if this was intended to cause breakage, but we had to patch our code in async-std likely because of this PR: https://github.com/async-rs/async-std/pull/182/files.

before

 peer.send(format!("from {}: {}\n", from, msg)).await?

after

let msg = format!("from {}: {}\n", from, msg);
peer.send(msg).await?
@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

We needed a similar patch in Miri.

@seanmonstar

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

So are the above examples a regression?

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Depends... can you create a minimal reproducer for before/after?

@tmandry

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

This causes errors to crop up all over the Fuchsia code base. Based on #63832 (comment), it seems like this might have been expected, however.

@seanmonstar

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@Centril The change in miri is quite succinct, and the "before" doesn't appear to be bad... while the "after" seems odd? https://github.com/rust-lang/miri/pull/946/files#diff-ecc42d2008d431ab52ee698995abd41b

Is something more needed?

Inlined:

Before:

async fn add(x: u32, y: u32) -> u32 {
    async { x + y }.await
}

After:

async fn add(x: u32, y: u32) -> u32 {
    let a = async { x + y };
    a.await
}
@davidtwco

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

I've confirmed locally that this patch did cause these regressions. I'm not sure what the best way to proceed here is, @nikomatsakis @Centril @matthewjasper?

@pimeys

This comment has been minimized.

Copy link

commented Sep 12, 2019

Seems that this patch broke my test server with Tide.

https://github.com/pimeys/blocking_test/

Works with 2019-09-10, broken in 2019-09-11.

@davidtwco davidtwco deleted the davidtwco:issue-63832-await-temporary-lifetimes branch Sep 17, 2019
jebrosen added a commit to SergioBenitez/Rocket that referenced this pull request Sep 21, 2019
@bstrie bstrie referenced this pull request Sep 23, 2019
10 of 11 tasks complete
tmandry added a commit that referenced this pull request Oct 6, 2019
- 771c5d10cf944bf7d221f5d6cb7abd2a06c400e4 Add macros in extern blocks and new proc-macro support.
- 8caabd62ef5fbe99e6be6aa9e76f55bbb8433d95 Update for "modern" `meta` matcher.
- 1b44947d36ccf7eba2b3bd245769eff68abf6d4d Update await desugaring after #64292
@tmandry tmandry referenced this pull request Oct 6, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 7, 2019
Update reference

- Add macros in extern blocks and new proc-macro support.
- Update for "modern" `meta` matcher.
- Update await desugaring after rust-lang#64292
Centril added a commit to Centril/rust that referenced this pull request Oct 7, 2019
Update reference

- Add macros in extern blocks and new proc-macro support.
- Update for "modern" `meta` matcher.
- Update await desugaring after rust-lang#64292
XiangQingW added a commit to XiangQingW/rust that referenced this pull request Oct 13, 2019
- 771c5d10cf944bf7d221f5d6cb7abd2a06c400e4 Add macros in extern blocks and new proc-macro support.
- 8caabd62ef5fbe99e6be6aa9e76f55bbb8433d95 Update for "modern" `meta` matcher.
- 1b44947d36ccf7eba2b3bd245769eff68abf6d4d Update await desugaring after rust-lang#64292
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.