rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.78k stars 12.77k forks source link

Coroutines should be able to implement `for<'a> Coroutine<&'a mut T>` #68923

Open jonas-schievink opened 4 years ago

jonas-schievink commented 4 years ago

With https://github.com/rust-lang/rust/pull/68524, the Coroutine trait has gained a type parameter for the resume argument. Currently, this parameter will not be inferred to a type with late-bound regions. In some cases, this is required for soundness. For example, this coroutine stores the resume argument across a yield, which means that it can not accept arbitrary lifetimes in it:

let coro = |arg: &mut bool| {
    yield ();
    *arg = true;
};

This coroutine ends up implementing Coroutine<&'x mut bool> with a specific 'x, not for<'x> Generator<&'x mut bool>.

However, if the resume argument doesn't get stored inside the generator state, it should be fine for the coroutine to implement for<'x> Coroutine<&'x mut bool> instead.

This is already how closures behave, since they can also store their arguments inside upvars, so it shouldn't be terribly difficult to extend this to coroutines. It would be good to come up with lots of test cases beforehand though (perhaps inspired by the tests for closures).

Nemo157 commented 4 years ago

One question is how do you not store the argument in the generator state, it seems like this would require some extra syntax (or inference) to tell the transform not to store it. For example to support something like the async transform you essentially need to construct generators like this:

let gen = |arg: &mut bool| {
    *arg = true;
    let arg = yield ();
    *arg = false
    let arg = yield ();
};

With the current generator transform this will have to allocate storage for the first two instances of arg in the state, being entirely based on scopes.

jonas-schievink commented 4 years ago

@Nemo157 Generators already make sure to only capture variables that are actually live across a yield point. This also seems to work with your example (resulting generator state size is 4, so it only contains the state discriminant).

Nemo157 commented 4 years ago

The value not actually being stored in the state is an optimization, currently during type checking the values are still assumed to be live across the yield point (and making this be based on the actual data flow may not be wanted as it will mean minor changes to the code can have unintended impacts on what is live across the yield)

jonas-schievink commented 4 years ago

Are you sure? I couldn't get the witness type to show up in error messages when I tried your example, so I couldn't see what typeck actually considers live, but I'm pretty sure it's already smart enough to ignore variables that are only accessed between two yields.

jonas-schievink commented 4 years ago

Ah, got it. Looks like you're right: [generator@src/main.rs:8:15: 13:6 for<'r> {&'r mut bool, ()}]

spunit262 commented 4 years ago

Currently only blocks can prevent typeck from considering a value live across yields. Looks like the check is even before moves are checked, based on which errors are gotten. (playground)

eddyb commented 4 years ago

Just to make sure it doesn't get forgotten: type-checking needs to ensure distinct skolemizations for every yield (this is assuming you're forced to get the new value from yield)

That is, you shouldn't be able to e.g.:

for<'a> |first: &mut &'a T| {
    let second = yield;
    mem::swap(first, second);
}

because the 'a in the "generator arg type" should be instantiated to one skolem var for first and another for the yield (and every yield should be different).

If first can't be accessed after the yield, the example would need to be different, but the underlying concern would likely remain.

The other thing is that a yield in a loop shouldn't allow unifying the different values its multiple executions may produce. That is likely even harder, so we might have to ban yield in loops until we can find a way to make iterations distinct.

Wait ugh I forgot the fact that the lifetimes are only guaranteed by the resumer until the next yield, so we'd need to teach NLL borrowck about that.

That means even e.g. (yield, yield) (or (first, second) in my example) shouldn't compile.

I would still try to get separate skolemizations, even if the lifetimes can be "cut" at the yields in some other way, seems safer in terms of limiting potential crosstalk etc.

jonas-schievink commented 4 years ago

Blocked on https://github.com/rust-lang/rust/issues/69663

shepmaster commented 10 months ago

Since I couldn't find this issue via search, let me add some keywords:

Kixunil commented 4 months ago

I've found one issue regarding the let x = yield y; syntax: how do you use it in a loop? Each iteration may potentially give the closure a reference with a different lifetime so assigning it to mut variable doesn't work (and not doing so is specifically desired!)

It feels like for<'a> coroutines should have a different semantics regarding parameters: yield not returning anything and the parameter to the "closure" changing value after each yield. This was previously ruled out but I think it should be revisited. Note that this is kinda how Future works, it's just hidden from users.

andrewgazelka commented 3 months ago

To add on to this, coroutines can currently not impl

for<'a> Coroutine<&'a T>

either? Right? I am not able to get this for the #[coroutine] static move |bot: &Input| { ... } syntax.

Kixunil commented 3 months ago

@andrewgazelka they currently cannot implement it for any type that has a lifetime parameter.

bluebear94 commented 2 months ago

Is there any workaround for this limitation available?

andrewgazelka commented 2 months ago

Is there any workaround for this limitation available?

You can always transmute lifetimes, but I would not recommend.

semtexzv commented 1 month ago

Relevant RFC & Discussion https://github.com/rust-lang/rfcs/pull/2781