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

temporary lifetime around await is (maybe) unexpectedly short #63832

Closed
nikomatsakis opened this issue Aug 23, 2019 · 8 comments
Closed

temporary lifetime around await is (maybe) unexpectedly short #63832

nikomatsakis opened this issue Aug 23, 2019 · 8 comments

Comments

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 23, 2019

So @sfackler pointed out something to me. The problem @sfackler reported was that temporary lifetimes around await were too short. The effect was that foo(&[123]).await would give an error where foo(&[123]) would not (in sync code). My assumption was that this was caused by our temporary lifetimes being derived from the desugared version of await, which doesn't seem like what one might expect.

Here is an example showing the problem:

async version, playground

sync version, playground

@nikomatsakis
Copy link
Contributor Author

@nikomatsakis nikomatsakis commented Aug 23, 2019

Loading

@eddyb
Copy link
Member

@eddyb eddyb commented Aug 23, 2019

I assume this would also apply to foo("123").await, i.e. the input is definitely &'static _, and that lifetime is not propagated to the output?

Loading

@matthewjasper
Copy link
Contributor

@matthewjasper matthewjasper commented Aug 25, 2019

So we could get the expected temporary lifetime by changing the HIR lowering from

{
    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 ();
    }
}

to

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 ();
    }
}

There is a trade off for a larger temporary lifetime:

  • 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.

Loading

@Centril
Copy link
Contributor

@Centril Centril commented Aug 25, 2019

This does seem quite unexpected; cc also #63778.

Loading

@nikomatsakis
Copy link
Contributor Author

@nikomatsakis nikomatsakis commented Aug 27, 2019

Another example, copied from #63778

struct Test(String);

impl Test {
    async fn borrow_async(&self) {}

    fn borrow(&self) {}

    fn with(&mut self, s: &str) -> &mut Self {
        self.0 = s.into();
        self
    }
}

async fn test() {
    // error[E0716]: temporary value dropped while borrowed
    Test("".to_string()).with("123").borrow_async().await;
}

fn main() {
    // Temporary outlives the borrow() call
    Test("".to_string()).with("123").borrow();
}

https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=2649e4f172c54090d759b2f9484b31e1

Loading

@nikomatsakis
Copy link
Contributor Author

@nikomatsakis nikomatsakis commented Aug 27, 2019

I've nominated for discussion at the @rust-lang/lang meeting -- as @matthewjasper points out, longer temporary lifetimes are in some sense less efficient, but I still think the correct thing is to be consistent with the behavior of sync code, and I would advocate for that (seems analogous to us deciding not to alter the drop order).

Loading

@nikomatsakis
Copy link
Contributor Author

@nikomatsakis nikomatsakis commented Sep 3, 2019

We discussed this in the lang-team meeting last Thursday. We decided that we want to align the semantics of sync/async code (i.e., this is indeed a bug).

There are some downsides: temporaries that live across the await will cause larger generators, and fixing that will be non-trivial in that it affects when destructors run which is visible to end users.

Nonetheless, the "surprise factor" -- and the fact that eronomic APIs are often dependent on the existing temporary lifetime rules -- seems to dictate that the semantics should align.

(It may be possible in the future to enable destructors to sometimes run earlier -- but that would be a design we can pursue independently and uniformly for both sync/async code.)

Loading

@davidtwco
Copy link
Member

@davidtwco davidtwco commented Sep 8, 2019

I've opened #64292 for this, sorry about the delay!

Loading

@bors bors closed this in a1755df Sep 10, 2019
vsrinivas pushed a commit to vsrinivas/fuchsia that referenced this issue Nov 2, 2019
A change to the language async/await semantics caused these tests to
hang. In particular, the lock guard returned by aux() is being saved
across `.await` when it wasn't before.

See rust-lang/rust#63832

Fixes fxb/40072

Change-Id: I456282b6a2e66166cfe65784c04468ed3a395303
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants