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

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

Open
jonas-schievink opened this issue Feb 7, 2020 · 12 comments
Open
Labels
A-coroutines Area: Coroutines C-enhancement Category: An issue proposing an enhancement or a PR with one. F-coroutines `#![feature(coroutines)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Feb 7, 2020

With #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).

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-coroutines Area: Coroutines F-coroutines `#![feature(coroutines)]` labels Feb 7, 2020
@Nemo157
Copy link
Member

Nemo157 commented Feb 8, 2020

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
Copy link
Contributor Author

@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
Copy link
Member

Nemo157 commented Feb 8, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@spunit262
Copy link
Contributor

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
Copy link
Member

eddyb commented Feb 19, 2020

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
Copy link
Contributor Author

Blocked on #69663

@shepmaster
Copy link
Member

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

  • HRTB
  • Higher Ranked Trait Bounds

@Kixunil
Copy link
Contributor

Kixunil commented Jul 17, 2024

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
Copy link

andrewgazelka commented Aug 24, 2024

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
Copy link
Contributor

Kixunil commented Aug 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-coroutines Area: Coroutines C-enhancement Category: An issue proposing an enhancement or a PR with one. F-coroutines `#![feature(coroutines)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants