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

async/await: “the requirement for<'r> 'r : 'static is not satisfied” with 'static across await point #53548

Closed
Ekleog opened this issue Aug 21, 2018 · 5 comments · Fixed by #59132
Assignees
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines AsyncAwait-Polish Async-await issues that are part of the "polish" area T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Ekleog
Copy link

Ekleog commented Aug 21, 2018

(continuation of rust-lang/futures-rs#1199 as it looks like a compiler bug after all, important points repeated here)

Problem

There appear to be an issue in handling traits that have a parent 'static bound boxed through an await point: the following example fails to compile with the requirement for<'r> 'r : 'static is not satisfied (note: I'm using generators for simplicity of reproduction, see below for a futures-only example)

trait Trait: 'static {}
async fn foo(b: Box<Trait + 'static>) -> () {
    let bar = move || { b; () };
    yield
}

(playground link)

However, when removing the 'static bound on Trait, it appears to compile correctly, even though the Box itself still has the + 'static bound.

trait Trait {}
async fn foo(b: Box<Trait + 'static>) -> () {
    let bar = move || { b; () };
    yield
}

(playground link)

Futures-only version

The error originated in code that looked like:

use futures::future;                                  
use std::any::Any;                                    
async fn foo(b: Box<Any + Send + 'static>) -> () { 
    await!(future::lazy(move |_| {                    
        b;                                          
        ()                                            
    }))                                               
}    

The Any being here the trait that has the 'static bound. This was tested with futures-rs at commit rust-lang/futures-rs@c02ec75 and nightly 2018-08-14.

No generator-only version

I couldn't manage to get a generator-only version to fail: this compiles.

use std::any::Any;
use std::ops::Generator;
fn send(msg: Box<Any + 'static>) -> impl Generator + 'static {
    || {
        let mut pinned = move || { msg; () };
        yield
    }
}

(playground link)

Potentially related issue

This is potentially related to #53259, as the same for<'r> appears there?

@cramertj cramertj added the A-coroutines Area: Coroutines label Aug 21, 2018
Ekleog added a commit to Ekleog/rust that referenced this issue Aug 27, 2018
This makes the code similar to the handling of `ty::Ref` in
`src/librustc/ty/wf.rs`, except checking for non-escaping-regions
somewhere down the call chain instead of at the root.

Fixes rust-lang#53548
@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 15, 2018
@Ekleog
Copy link
Author

Ekleog commented Nov 26, 2018

Some work has started on #53736 ; the information there will likely be helpful to someone trying to fix this.

@pandaman64
Copy link

I think I've hit this with a generator-only code.

#![feature(generators)]

use std::cell::RefCell;
use std::rc::Rc;

trait Trait: 'static {}

struct Store<C> {
    inner: Rc<RefCell<Option<C>>>,
}

#[test]
fn test_compose() {
    Box::new(static move || {
        let store = Store::<Box<dyn Trait>> {
            inner: Default::default(),
        };
        yield ();
    });
}

(playground link)

@nikomatsakis nikomatsakis added the A-async-await Area: Async & Await label Feb 22, 2019
@nikomatsakis nikomatsakis added the AsyncAwait-Polish Async-await issues that are part of the "polish" area label Mar 5, 2019
@nikomatsakis
Copy link
Contributor

I'm marking this issue as blocking the async-await stabilization -- if nothing else, we need to investigate it and figure out what is going on.

@nikomatsakis nikomatsakis self-assigned this Mar 5, 2019
@nikomatsakis
Copy link
Contributor

I did some more digging into this problem. This code will actually ICE if using a version of rustc built with debug assertions. Moreover, the follow example code -- which doesn't use generators at all -- will ICE in the same way:

use std::cell::RefCell;
use std::rc::Rc;

trait Trait: 'static {}

struct Store<C> {
    inner: Rc<RefCell<Option<C>>>,
}

fn main() {
    let store = Store::<Box<for<'a> fn(&(dyn Trait + 'a))>> {
        inner: Default::default(),
    };
}

the problem here ultimately has to do with the way that the WF code works. I left some obscure notes on Zulip, but the bottom line is something like this:

  • The WF code presently descends through binders but (generally speaking) opts not to prove WF conditions that involve higher-ranked regions.
  • The strategy was described as "lazy" in the relevant RFC, but with the new universes code we can generally try to pursue something better, so I hope to get rid of it sooner or later.
  • Anyway the idea is that whenever a higher-region is "expanded" with a particular value, we'll prove the WF at that point.

So in this particular case we wind up with a type for<'a> (dyn Trait + 'a) -- for this type to be WF, the region bound 'a: 'static must hold. We are producing this constraint. If debug assertions are enabled, we ICE out because the constraint isn't supposed to include higher-ranked regions (here, 'a). But without debug assertions, we fail to detect that, and then we just wind up failing to prove it.

It's kind of hard to stumble across this ICE normally, because dyn Trait defaults to dyn Trait + 'static since we see the 'static bound declared on Trait. However, with generators, it occurs more readily because when constructing the generator witness, we replace the 'static with a bound region (we replace all the internal regions in this way).

Pursuant with the existing lazy strategy, I think the right fix is just for the wf code to ignore the 'a: 'static constraint -- and that is a small patch to do. I will probably open a PR around this shortly.

Centril added a commit to Centril/rust that referenced this issue Mar 13, 2019
…bound, r=pnkfelix

ignore higher-ranked object bound conditions created by WF

In the `issue-53548` test added in this PR, the `Box<dyn Trait>` type is expanded to `Box<dyn Trait + 'static>`, but the generator "witness" that results is `for<'r> { Box<dyn Trait + 'r> }`. The WF code was encountering an ICE (when debug-assertions were enabled) and an unexpected compilation error (without debug-asserions) when trying to process this `'r` region bound. In particular, to be WF, the region bound must meet the requirements of the trait, and hence we got `for<'r> { 'r: 'static }`. This would ICE because the `Binder` constructor we were using was assering that no higher-ranked regions were involved (because the WF code is supposed to skip those). The error (if debug-asserions were disabled) came because we obviously cannot prove that `'r: 'static` for any region `'r`.  Pursuant with
our "lazy WF" strategy for higher-ranked regions, the fix is not to require that `for<'r> { 'r: 'static }` holds (this is also analogous to what we would do for higher-ranked regions appearing within the trait in other positions).

Fixes rust-lang#53548

r? @pnkfelix
@Ekleog
Copy link
Author

Ekleog commented Mar 16, 2019

Great, thank you! I can confirm that on latest nightly the issue is no longer there :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines AsyncAwait-Polish Async-await issues that are part of the "polish" area T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants