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

FnPinMut: fully generalized stackless semicoroutines #1

Closed
wants to merge 2 commits into from

Conversation

CAD97
Copy link
Owner

@CAD97 CAD97 commented Nov 13, 2019

cc @samsartor

Rendered

I think the building blocks for putting this together as a proper proposal are here; let's put them together!

@CAD97 CAD97 changed the title Create RFC FnPinMut: fully generalized stackless semicoroutines Nov 13, 2019
@samsartor
Copy link

I like it! It make a lot of sense outside of coroutines. My two questions so far:

  • Should we ask for a default impl Future for FnPinMut(&Context) -> Poll?
  • Should we ask for a FnPin as well? It isn't needed for coroutines but it kinda makes sense to have both.

@pcpthm
Copy link

pcpthm commented Nov 13, 2019

  • Should we ask for a FnPin as well? It isn't needed for coroutines but it kinda makes sense to have both.

Assuming it is fn call_pin(self: Pin<&Self>, args: Args) -> Self::Output;,
Pin<&T> is interchangeable to &T (by as_ref and Pin::new) thus FnPin is isomorphic to Fn and not needed.

@CAD97
Copy link
Owner Author

CAD97 commented Nov 13, 2019

FnPin isn't needed because you can't move out of &T anyway. Pin<P<T>> offers a safe conversion to &T.

The rule on specialization is that the standard library can't expose any specializations that aren't totally covered (i.e. the exposed implementations don't require specialization to exist).

So either we put impl<F> Future for F where F: FnPinMut(&Context) -> Poll in the RFC text itself, or put it in future possibilities. (Also note that implementations would necessarily be unnamable as FnPinMut would be subjects to the same restrictions as the normal Fn hiearchy.

On that line, would it make sense to insert FnPinMut into the hiearchy? What order would it be in? Fn: FnMut: FnPinMut: FnOnce seems the most reasonable to me.

@samsartor
Copy link

samsartor commented Nov 13, 2019

Right! I forgot about as_ref. I don't think FnPinMut fits into a linear hierarchy with the other Fns. Like, anything which implements Fn can implement FnOnce and FnPinMut. Anything which implements FnMut can implement FnOnce but not FnPinMut. Or does that mean it fits between Fn and FnMut?

@samsartor
Copy link

Also, I've been thinking a bit more about "coroutines as an extension to closures" idea but I don't want to drop in anywhere major yet. Like, any closure |input| body is a coroutine { loop { yield body; } } already. It is a state machine with one state. yield could just be a way of extending the coroutine-ness that closures already have. A way of adding stops on the subway line if you will. Or am I going nutty after a long day?

@CAD97
Copy link
Owner Author

CAD97 commented Nov 13, 2019

If we didn't have to worry about self captures at all, then I'd agree with Fn is a coroutine. In practice, the pinning requirement gets in the way.

I need to think more about the interaction of FnPinMut and FnMut with Self: ?Unpin before I can say concretely one way or another.

@CAD97
Copy link
Owner Author

CAD97 commented Nov 13, 2019

FnPinMut does imply the capability for FnOnce (stack pin, call once). FnMut and FnPinMut are trickier for ?Unpin types due to the forever requirement of the pin wrapper. Perhaps just blanket implementations without actual inheritance is the answer.

@pcpthm
Copy link

pcpthm commented Nov 13, 2019

Yes, FnPinMut : FnOnce (I'm in favor of adding this bound) but not FnMut : FnPinMut.
An FnPinMut instance can implement FnOnce by doing a stack pinning.
An FnMut instance probably (not proven) cannot implement FnPinMut in general because converting from &mut T to Pin<&mut T> Pin<&mut T> to &mut T requires T: Unpin. There is also an issue of adding a bound being a breaking change (for unstable #[feature(unboxed_closures)] user only in this case? not sure).

So I'm suggesting FnMut : FnOnce and FnPinMut : FnOnce.

@samsartor
Copy link

samsartor commented Nov 14, 2019

The theoretical max is:
fn_hierarchy

In practice we would probably only require FnPinMut: FnOnce and FnPinMut: FnMut where Self: Unpin.

@pcpthm
Copy link

pcpthm commented Nov 14, 2019

Note that we don't require a super-trait bound to use a trait.
Rather than declaring that all FnPinMut + Unpin are FnMut, we can implement all non-self-referencial (Unpin) coroutines to also implement FnMut.

A coroutine desugars to

// Semantically contains `PhantomPinned` when self-referencial
enum Coro1 { /* state_machine */ }
impl FnPinMut for Coro1 { ... }
// `Self: Unpin` is an implementation details but probably it is easier to implement in this way because otherwise we have to do a type checking at the desugaring stage. I don't know current generator implementation.
impl FnMut for Coro1 where Self: Unpin { ... }

@CAD97
Copy link
Owner Author

CAD97 commented Nov 14, 2019

I've added a bit of intro to the explanation that covers yield closures that don't need FnPinMut. That's one of our key advantages over other proposals 😜

(These restrictions may be lifted in the future.)

An implementation of `FnPinMut` is provided for all closures where said implementation is sound.
Practically, this means all currently stable `FnMut` closures, as they are `Unpin`, get the following implementation:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A closure can be !Unpin by capturing a !Unpin value:

use std::marker::{Unpin, PhantomPinned};
fn main() {
    struct Pinned(PhantomPinned);
    impl Pinned { fn method(&mut self) {} }
    let mut pinned = Pinned(PhantomPinned);
    let closure = move || pinned.method();
    fn is_unpin(_: impl Unpin) {}
    is_unpin(closure);  // ERROR: the trait bound `std::marker::PhantomPinned: std::marker::Unpin` is not satisfied 
}

})
```

This closure will implement all four `Fn` traits.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct that this closure can implement Fn?
A call modifies the state by iterating 1..6. It needs a mutable reference.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only type of yield closure that can implement Fn is a yield closure with only 1 state. For example, the yield closure |x| loop { yield x + 1; } could be Fn because it is exactly identical to |x| x + 1.

```

Closures are now allowed to contain **yield expression**s of the form `yield $expr`.
If a closure contains a yield expression, it _may not_ also contain a return expression.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern with disallowing return is that try blocks would then be required to have any kind of reasonable ? story. I would mention in the alternatives section that return could be supported through a desugar like return $x:expr => yield $x; unreachable!() without fundamentally changing the behavior of coroutines.

})
```

This closure will implement all four `Fn` traits.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only type of yield closure that can implement Fn is a yield closure with only 1 state. For example, the yield closure |x| loop { yield x + 1; } could be Fn because it is exactly identical to |x| x + 1.

Discuss prior art, both the good and the bad, in relation to this proposal.
A few examples of what this can include are:

- For language, library, cargo, tools, and compiler proposals: Does this feature exist in other programming languages and what experience have their community had?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my Reddit post, someone brought up yield from in python which allows one coroutine to delegate to another. Python's coroutines can signal that they have reached an end state by throwing StopIteration. When that happens, any delegation ends and yield from evaluates to the last yielded value. That actually means that return x inside a python coroutine is the same as yield x; throw StopIteration which looks a lot like our potential desugar for return! The difference is that Python can then end the coroutine delegation while we would have no obvious way to do so (having gotten rid of GeneratorState). Just an interesting validation + trade-off.

# Unresolved questions
[unresolved-questions]: #unresolved-questions

- Exact naming of `trait FnPinMut`. `FnPinMut` was chosen for this RFC as an

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FnPin could also be an option since (as y'all pointed out) Pin<&mut Self is the only obviously useful way to take self as pinned.

@samsartor
Copy link

More on that dumb "yield as a strict extension to closures" idea that I won't let die. We could specify that when any kind of closure (yield or otherwise) reaches the end of the block, it yields and will resume at the top. If there is only one state, the closure doesn't need a mutable state variable and can implement Fn. If there are no self captures, the closure can implement FnMut. Any bindings get dropped at the end of the closure. We could even treat inputs as shadowing previous inputs across yields instead of mutating them. For example,


|x: usize| x + 1 becomes

coroutine { loop {
    #[bind_input]
    let x: usize; // not mutable
    // state machine is always here
    // x is bound here
    yield x + 1;
} }

|x: String| { yield x + "Hello"; x + "World" } becomes

coroutine { loop {
    #[bind_input]
    let x: String;
    // x is bound here
    yield x + "Hello"; // move x
    // x is shadowed here
    yield x + "World"; // move x
} }

|x: String| { yield 0; 1 } becomes

coroutine { loop {
    #[bind_input]
    let x: String;
    // x is bound here
    yield 0;
    // x is shadowed here
    yield 1; // both xs are dropped before execution is handed back to the caller
} }

|| {
    if test() {
        return 0;
    }
    yield 1;
    2
}

becomes

coroutine { loop {
    #[bind_input]
    let _: ();
    if test() {
        yield 0;
        continue;
    }
    yield 1;
    yield 2;
} }

async { 3 } is the same as |ctx: &mut Context| Poll::Ready(3) and becomes

coroutine { loop {
    #[bind_input]
    let ctx: &mut Context;
    yield Poll::Ready(3);
    // logic error to poll again, could panic instead
} }

async { func().await?; Ok(3) } becomes

coroutine { loop {
    #[bind_input]
    let ctx: &mut Context;
    match await!(func(), ctx) {
        Ok(value) => value,
        Err(error) => {
            yield Poll::Ready(Err(error));
            continue // logic error to poll again, could panic instead
        },
    }
    yield Poll::Ready(Ok(3));
    // logic error to poll again, could panic instead
} }

@samsartor
Copy link

I just realized some really cool things about the loop version:

  • It answers all of our lifetime questions (I'll elaborate on that later when I have time tonight).
  • We can use the loop semantics with the extra rule that "a closure block must evaluate to ! if it contains yield". Then the looping semantics become identical to the current semantics since the loop can never be reached! Then anything that we answer about one applies to the other. We can even release the feature that way and then remove the rule without a breaking change!

@samsartor
Copy link

Woah, there is something even better about end-with-a-bang. It is compatible with and strictly more conservative than other proposals!

Because return is not allowed, we can stabilize this proposal and later decide if adding a return:

  • changes the closure to a FnPin(_) -> GeneratorState<_, _>
  • inserts a yield x; unreachable!() keeping the closure as FnPin(_) -> _
  • inserts a yield x; continue thereby exposing my loop symantics

@pcpthm
Copy link

pcpthm commented Nov 15, 2019

I like the idea return x; as yield x; continue;. I'm excited to see yet another level of generalization.
However, I want to point out that this transformation doesn't work when implemented naively. Consider this:

loop {
    let bomb = DropBomb::new();
    return 1;
}

The closure explodes the bomb before it yields the value 1. Thus, more precise expansion of return $expr; is:

let value = $expr;
$(drop($local_var);)*
yield value;
continue;

Alternatively, something like yield_continue 'label value can be introduced to cover the semantics. yield_continue doesn't have to exposed to the surface language (only exist in HIR / MIR).

Another point is about the Fn trait and "one-state" coroutine.
In general, a coroutine with n yields requires n + 1 states. But it can be less when multiple "continuation points" are the same.
If we want unit state coroutines to implement Fn trait, there have to be rules of precisely when a coroutine is one-state.
But this rule is necessary subtle:

// probably one state
loop {
    process();
    yield 1;
}

// one state?
loop {
    if cond() {
        yield 1;
    } else {
        yield 2;
    }
}

// cannot be one state
loop {
    let bomb = DropBomb::new();
    yield 1;
    // bomb explodes after resumed
}

I find it is confusing that a coroutine loses Fn when a local variable with Drop is introduced.
Therefore, I propose to not implement the Fn trait for all coroutine with one or more yields.

@samsartor
Copy link

I've also been thinking a lot about the drop and unit state cases. My current conclusion is that return x => yield x; continue is a useful mental model but not perfectly correct once drop gets involved. I think your yield_continue operator demonstrates that perfectly! Let's look at this example you gave:

// cannot be one state
loop {
    let bomb = DropBomb::new();
    yield 1;
    // bomb explodes after resumed
}

This could actually be one state but that isn't clear because of the ambiguous syntax. If the user writes:

|| {
    let bomb = DropBomb::new();
    1 // or `return 1;`
}

Then the compiler generates:

coroutine loop {
    let bomb = DropBomb::new();
    yield_continue 1; // drop(bomb); yield 1; continue
}

Which is clearly a unit state. If the user writes:

|| {
    let bomb = DropBomb::new();
    yield 1;
}

Then the compiler will throw a type error because the yield type (u32) does not match the return type (unit). If the user writes:

|| {
    let bomb = DropBomb::new();
    yield 1;
    return 2;
}

Then the compiler generates:

coroutine loop {
    let bomb = DropBomb::new();
    yield 1;
    yield_continue 2;
}

Which clearly has two states.

So I don't think the rule is subtle at all. Every coroutine has the "empty state" (the state before the first resume). The only way to re-enter that state is with an implicit or explicit return (which is semantically identical yield_continue). Thus, if the user ever writes yield, the coroutine must have a second state and will not implement Fn.

@CAD97
Copy link
Owner Author

CAD97 commented Nov 15, 2019

There's another option though, returning !/diverging (which the example did do.)

|| loop {
    yield 5;
}

We could just say that this doesn't implement Fn because it uses yield, but it does only require one state because it's the same effect as || 5.

@samsartor
Copy link

samsartor commented Nov 15, 2019

True. But if doesn't work in every case. For example,

|| {
    some_action();
    loop {
        yield 5;
    }
}

This diverges but does require two states.

I bet there are some interesting optimization opportunities in some diverging cases but I would feel odd having the type system aware of those.

@pcpthm
Copy link

pcpthm commented Nov 15, 2019

Summarizing trait implementations

A coroutine implements

  • FnOnce always.
  • FnPin if it doesn't move out its state (existing FnMut rule for closures).
  • Fn if it doesn't contain yield and its state is not mutated (existing Fn rule for closures).
  • FnMut if it is not self-referential.
  • Unpin if it is not self-referential and doesn't contain !Unpin state.
    Is the later condition really necessary? The current behavior is just a consequence of auto trait.

All values yielded or returned in a coroutine must have the same type. The Output type of Fn* traits is this type.

Yet another return transformation

I found a new way.

$stmts;
return $expr;

is transformed to

yield ('label: {
    $stmts;
    break 'label $expr;
});

This is a valid syntax with #![feature(label_break_value)].
This is clearer than continue because it doesn't have Drop issue. Also the top level loop is now syntactically unnecessary (so it can be "built-in" to coroutine semantics).

@pcpthm
Copy link

pcpthm commented Nov 18, 2019

More thought on Unpin. There are three choices I can think of:

  1. Non-self-referential coroutines implement Unpin.
    a. Non-self-referential-ness is explicitly declared by a syntax like current static ||.
    b. Self-referential-ness is explicitly declared by a syntax (opposite of (a)).
    c. Implicit.
  2. Coroutines with no yield implement Unpin.
  3. Coroutines with no yield and no !Unpin type states implement Unpin.

The current behavior is:

  • Closures rely on the auto implementation of Unpin. It matches (3).
  • All async blocks and async fns don't implement Unpin. It matches (2) or (3).

As it is possible to assert negative trait bound (implementation), adding Unpin for existing is technically a breaking change but I think it is considered as an "acceptable minor breakage".

I propose to choose (1).

Firstly, it is always safe to implement Unpin for a struct unless "pin projection"s are used.
But in a coroutine, a pin projection can always be substituted by a "stack pinning" (pin_mut!).
This pattern is already used in the await desugar.

Secondly, it also means all FnMut closures can implement FnPin. It simplifies the Fn* trait hierarchy to the nice linear shape Fn <: FnMut <: FnPin <: FnOnce.

Thirdly, if (3) is chosen, there is a question of whether a coroutine with yields but no self-reference implements FnMut or not.
On the one hand, FnMut implementation is very useful when used for example std::iter::from_fn.
On the other hand, FnMut + !Unpin is a somewhat inconsistent bound as FnMut cannot be implemented for FnPin + !Unpin types in general.

For (1.c), there is a concern about an implementation change affecting public API.
When a self-reference is introduced to an existing coroutine, it no longer implements Unpin.
However, in my opinion, this trait change is similar to the situation of when a closure is no longer Fn when a state mutation is introduced. I believe it is manageable.

For the choice of (a), (b) or (c), I have no strong opinion currently but my preference is (c).

@CAD97
Copy link
Owner Author

CAD97 commented Nov 18, 2019

FWIW, I think pin_mut! should put a PhantomPinned into local scope.

@samsartor
Copy link

Braindump incoming...


Bind-at-yield, as I originally imagined it, is a footgun. Consider this code:

"    x".chars().for_each(|c| {
  println!("begin");
  while c.is_whitespace() {
    yield;
  }
  println!("end");
})

It seems like it should print "begin" then "end" but it doesn't. Because c is shadowed and dropped inside the while loop, the is_whitespace check is always performed against the first character. The correct implementation is:

"    x".chars().for_each(|c| {
  println!("begin");
  if c.is_whitespace() {
    loop {
      yield;
      if !c.is_whitespace() { break }
    }
  }
  println!("end");
})

That sucks. Thankfully, mutate-at-yield fixes this problem! I originally believed that mutate-at-yield was not consistent with closures since it does not allow corouties to move their resume arguments but this is not actually the case. Rust allows move-and-reassign like this:

let mut x = String::new();
loop {
  let y = x;
  do_stuff(y);
  x = String::new();
}

So the following coroutine would be valid under magic mutation:

|x: String| {
  let y = x;
  do_stuff(y); 
  // x = ...
}

With magic mutation, references to inputs can not live across yields. That is, these two examples are invalid for the same reason:

|x| {
  let y = &x;
  yield;
  dbg!(y);
}
let mut x = ...;
let y = &x;
x = ...;
dbg!(y);

However, this is allowed:

|x| {
  let x0 = x;
  let y = &x0;
  yield;
  dbg!(y);
}

I'm not sure if I have talked about this before but .await can be allowed inside impl FnPin(&mut Context) -> Poll<T> closures under this proposal. The async { ... } syntax is a relatively trivial sugar for creating such a closure. The bind-at-yield semantics made the await desugar difficult for the reasons above but it works very well with mutate-at-yield.

Note that .await can not be used inside coroutines if we use yield-is-an-expression semantics.


Proposals for a dedicated generator syntax often say something like "you should eventually be able to write a single function that is both async and an iterator, or in other words a stream." The idea is that, by making async/await orthogonal to generators, the two will be composable at some point in the future. TBH, I have yet to see a concrete demonstration of that composability. In contrast, our proposal actually supports stream creation today, with no additional syntactic tricks:

let mut bytes = open("some_path.bin"); // impl TryStream<Vec<u8>, Error>

|ctx: &mut Context| {
    let mut buffer = Vec::new();

    while let Some(chunk) = bytes.next().await {
        // ? works because Poll<Option<Result<_>>>: Try
        for byte in chunk? {
            match byte {
                0 => yield Poll::Ready(Some(Ok(buffer.split_off(0)))),
                x => buffer.push(x),
            }
        }
    }

    yield Poll::Ready(Some(Ok(buffer)));
    Poll::Ready(None)
}

Note that the above doesn't even require a definition of Stream from std or core! This is the real advantage of a more general syntax: it can be used to implement types introduced by crates (such as futures-rs) without waiting for the language to catch up.

@samsartor
Copy link

samsartor commented Nov 18, 2019

fn parse_csv<E>(strs: impl TryStream<&str, E>): impl TryStream<Vec<String>, E> {
    let csv_parser = |c: char| {
        let mut line = Vec::new();

        loop {
            while c.is_whitespace() { yield None; }

            let mut part = String::new();
            while c != ',' && c != '\n' {
                part += c;
                yield None;
            }

            part.truncate(part.trim_end().len());
            line.push(part);

            if c == '\n' {
                return Some(line);
            } else {
                yield None;
            }
        }
    };

    FnStream(move |&mut Context| {
        while let Some(item) = strs.next().await {
            for c in item?.chars() {
                match csv_parser(c) {
                    Some(o) => yield Poll::Ready(Some(Ok(o))),
                    None => (),
                }
            }
        }
        Poll::Ready(None)
    })
}

@pcpthm
Copy link

pcpthm commented Nov 19, 2019

FWIW, I think pin_mut! should put a PhantomPinned into local scope.

I don't follow. Could you elaborate more?
My reasoning is that a stack-pinned variable is either:

  • Used across yield points: the containing coroutine is self-referential and thus necessary !Unpin. It has to be pinned to be called and thus the pinned variable doesn't move.
  • Not used across yield points: move is prevented by the mechanism of pin_mut!. It is perfectly safe even if the containing closure is Unpin and moved between calls.

With magic mutation, references to inputs can not live across yields. That is, these two examples are invalid for the same reason:

I thought the same. This should be described in RFC for why "magic mutation" is "right".

@samsartor
Copy link

samsartor commented Nov 20, 2019

I now have some opinions on how we market/teach this feature. I'll probably even write a follow up to my blog post soon. In the mean time, I'm dropping some thoughts on the RFC text here so they aren't lost.

  • The idea that closures are already looping coroutines is surprisingly approachable. I've run it past quite a few people IRL (both rustations and not) without any confusion or objection. The guide-level explanation can totally approach yield closures through that lens.
  • Our support for Iterator and futures-rs Stream/Sink is the most exciting part of this for me and I think others will feel the same way. That needs to be touched on in motivation and with examples throughout the RFC.
  • Yes, allowing .await in FnPin(&mut Context, ...) -> Poll<T> closures (and, by extension, mutate-at-yield) is important to the ergonomics of things like streams. We could allow yield in async blocks instead but that risks breaking the association of async=futures or conflating a future with a stream (they have the same types but different behavior).
  • Some sort of while let Some(_) in stream.next().await sugar like async for is a future possibility but not at all needed right now.
  • The fact that mutate-on-yield allows arguments to move is not immediately obvious (at least it wasn't to me). We should show an example of moving arguments in the guide-level explanation and elaborate on the reasoning/lifetimes in reference-level.
  • I think we are pretty settled on the looping coroutine semantics. However, it could make things go more smoothly (e.g. less arguing in the RFC comment section) if we have the ability to stabilize !-returning yield while feature-gating @pcpthm's return desugar so that the following future changes are allowed:
    • Should unreachable!() be inserted after the outer yield?
    • Should the labeled block be wrapped in GeneratorState::Returned?
  • The interaction of yield closures/streams with ? and the Try trait should be mentioned. We get it for free with any return behavior since Poll<Result> and Poll<Option<Result>> implement Try already!
  • @pcpthm's reasoning on Unpin and the trait hierarchy is awesome but a bit tricky to follow. We need to be careful to point out that the linear hierarchy can not be required by Fn trait inheritance itself; just the implementations generated by the closure syntax. And reinforce that pin_mut/await can prevent Unpin not because they can add pin projections to the struct (they don't) but because they can create internal references which live across yield.

@samsartor
Copy link

samsartor commented Jun 27, 2020

Await is implemented for Stream and Future

I don't really think the verbosity difference between .await and .next().await is large enough to justify this trait. If we need to sugar-up awaiting on the next element of a stream, I'd prefer an async for syntax over shortcutting the next method.

This keeps await tied to async contexts, which I think is important for language accessibility.

I do think this is a valid point! It is one reason I originally intended delegate/yield_from/await(...) to work only on FnPin() -> Poll types. The addition of an extra trait to generalize past Poll stretched await even further past its current limits.

But .await doesn't fundamentally have anything to do with futures. All it truly requires is something that can be polled with some sort of context. That means an impl Future and &mut Context in async blocks but I don't think it is too much of a stretch to see it play a very similar role in relation to any other sorts of non-Future poll functions (e.g. poll_read, poll_write, poll_next, poll_has_my_parser_reached_the_end_of_the_string_literal, etc.).

let macros handle it for now.

I'm fine with having a delegate! macro and only proposing .await(...) as a future extension. The original async/await proposal did something very similar. But I think it is very important to propose some universal way for Poll-returning closures to delegate to other arbitrary poll functions. Streams are an example of that but it turns up constantly in all sorts of contexts. If anything, being able to await an Iterator being None is a rare case by comparison, which I would be totally fine leaving out.

probably no way to make this usefully generalized as it's hard to decide which types this makes sense for

FnPin(...) -> Poll<T>

can't see it being used that often (maybe I'm wrong though)

Again, I can't get 10 seconds into prototyping a pushdown parser or some kind of AsyncRead/AsyncWrite/Stream combinator before needing it. But that could just be my own coding style and preferences.

We may however want to be able to pass resume arguments into stream coroutines just like we can normal coroutines.

I mean yes, but I don't think I understand what you mean. There are plenty of FnPin-like types (e.g. AsyncRead) that are polled with &mut Context plus additional buffers & things but Stream is not one of them.

It could be useful for the implicit context to be available as an additional argument when delegating within an async block. I think that is the problem your code example is trying to solve? But that seems to me like an oddly specific case for coroutine delegation in general. Like, once we figure out how coroutine delegation ever works, we can look into ways of using it more ergonomically in async blocks (beyond the trivial case of old-fashioned .await).


I think we agree that delegation is a very important thing that coroutines need to be able to do. And I think we agree that .await(...) runs the risk of being unacceptably inaccessible if over generalized (although I hope it proves to be intuitive because I really like it). However, I feel like we are hitting the coin from totally opposite sides or something. You may need to elaborate more before I understand where you are coming from.


Edit: Just for the sake of clarity going forward, the expansion of delegate! (as I envision it) is identical to the desugar for .await with the only differences being:

  • Delegate polls FnPin::call_pin instead of Future::poll
  • Delegate allows the user to specify the arguments passed to the poll function, instead of it using the implicit async context.
  • Because it does not rely on the implicit async context, delegate can be used anywhere yield is allowed.

This would make delegate a little less powerful than Python's yield from, since exception hackery makes yield from more useful for poll-unlike functions like Iterator::next. But it is still super important to using the wide range of poll-like functions Rust, as a very type safe language, is stuck with.

@pitaj
Copy link

pitaj commented Jun 30, 2020

Sorry it's taken me so long to respond. A plus is that it's given me more time to think on this.

I think the main issue here is that when I think "generalized yield from" I think of something that works with a generalized coroutine, allowing delegation of the yield sequence to one called within. This version of delegation has nothing to do with await, and would cover iterators and streams. To the contrary, you seem to be focused on a more specific idea of generalized await, which would essentially just cover Fn*(...) -> Poll.

I don't really think the verbosity difference between .await and .next().await is large enough to justify this trait.

Ah I didn't see that in StreamExt. Makes sense that would already be a thing though.

But .await doesn't fundamentally have anything to do with futures.

It doesn't have to be, but I think that await should be fundamentally tied to and only allowed within async blocks/fns/closures. I don't have a problem with the idea of a language feature standardized on Poll (or something like it), I just don't think await should be used for that, as there's a lot of utility in keeping things simple and partitioned (it allows for easier composition).

Essentially, I agree with what @CAD97 said

The way I think it should go, and the way boats described it way back when, is that the two are orthogonal.

However I don't think we should rely on assumptions about any particular function signature. Fn*(&mut Context) -> Option<impl Future> and Fn*(&mut Context) -> impl Future(Output = Option(_)) (any therefore async yield closures) are both not necessarily streams. I think instead we should provide a method into_stream (or just implement Into<FnPinStream>) when we have a closure Fn*(&mut Context) -> Poll<Option<_>> or Fn*(&mut Context) -> impl Future(Output = Option(_)).

Then your previous example becomes

fn map_stream<X, Y>(input: impl Stream<Item=X>, func: FnMut(X) -> Y) -> impl Stream<Item=Y> {
    (async move || {
        while let Some(x) = stream.next().await {
            yield Some(func(x));
        }
        None
    }).into_stream()
}

I think we're getting a little ahead of ourselves here, though. Streams aren't even in std yet, and are far from final IMO.

I'm fine with having a delegate! macro and only proposing .await(...) as a future extension.

I don't think we should even worry about it in this proposal besides a small note about future opportunities.

But I think it is very important to propose some universal way for Poll-returning closures to delegate to other arbitrary poll functions.

Are you sure Poll is right for this? The variants Pending and Ready(T) seem very tailored to be meaningful in an async context, and I'm not sure it is well-suited for more general contexts like you're discussing. Perhaps a different enum Fulfill with Partial and Complete(T) variants or something?

That would certainly be more meaningful in this example of yours:

fn digits(radix: u32) -> impl FnPin(char) -> Fulfill<i32> {
    let mut value = 0;
    |c| {
        match c.to_digit(radix) {
            Some(d) => {
                value *= radix;
                value += d;
                Partial
            },
            None => Complete(value),
        }
    }
}

fn integer(radix: u32) -> impl FnPin(char) -> Fulfill<i32> {
    |c| {
        let multiplier = match c {
            '-' => {
                yield Partial;
                -1
            },
            _ => 1,
        };
        let value = delegate!(digits(radix), c); // or something like `digits(radix).yield(c)`
        Complete(value as i32 * multiplier)
    }
}

This keeps things (IMO) orthogonal, more easily composable, more meaningful, and more understandable.

probably no way to make this usefully generalized as it's hard to decide which types this makes sense for

FnPin(...) -> Poll

I meant if you want to cover iterators.

Again, I can't get 10 seconds into prototyping a pushdown parser or some kind of AsyncRead/AsyncWrite/Stream combinator before needing it. But that could just be my own coding style and preferences.

Here's how I'd write your example with an async yield closure and into_stream

fn byte_stream<R: AsyncRead>(input: R) -> impl TryStream<Item=u8>
    (async mut move || {
        pin_mut!(input);
        let mut buffer = [0u8; 1024];
        loop {
            let num = AsyncReadExt::read(input, &mut buffer).await?;
            if num == 0 {
                return None;
            }
            for &byte in buffer.iter().take(num) {
                yield Some(Ok(byte));
            }
        }
    }).into_stream()
}

I think we agree that delegation is a very important thing that coroutines need to be able to do. And I think we agree that .await(...) runs the risk of being unacceptably inaccessible if over generalized

I think we're on the same page, but I think that capability should be orthogonal to async/await. And that cases which aren't orthogonal (like AsyncRead and stuff) would be better handled by just using async/await instead.

I think making await and Poll dual-purpose for both async stuff and synchronous stuff like your parser example would be super confusing for beginners just because it allows for await outside async contexts, not to mention the composability and other conceptual issues.


Just for the sake of clarity going forward, the expansion of delegate! (as I envision it) is identical to the desugar for .await with the only differences being

I think all of this is fine besides await. Maybe use .yield(...) instead? Only other issue I have is that Poll isn't very meaningful in sync contexts, but I can get over that.

tl;dr

  • agreed delegation is important, but should be solved at a later point
  • async/await should be orthogonal to yield closures
  • Poll might not be the best fit for sync contexts

@samsartor
Copy link

samsartor commented Jul 1, 2020

@pitaj Hmmm, your examples of async yield closures do appear to work well and seem to be valid+intuitive alternative to .await(..) when combined with the obvious *Ext methods. I do need to play around with them more myself.

However, we must understand that yield in an async closure implies the ability to use yield in async blocks and thus decouples async with Future. I actually think async closures (and maybe async functions even) are a really bad idea since they make all kinds of things like this non-obvious. I've actually been thinking of writing a scary clickbaity post like "async fn considered harmful" to further elaborate on that.

Anyway, if we trivially async || { .. } => || async { .. } desugar the body of your map_stream example to:

(move || async move {
    while let Some(x) = stream.next().await {
        yield Some(func(x));
    }
    None
}).into_stream()

then it becomes clear that the closure adds a totally useless level of indirection and we should additionally simplify it to:

(async move {
    while let Some(x) = stream.next().await {
        yield Some(func(x));
    }
    None
}).into_stream()

So yield closures aren't even directly relevant here! Although they'd still exist under the async block sugar, it is a totally different ability to the user. Heck, they might be a totally independent RFC. I actually touched on a version of yield in async at the end of my original "generalized coroutines" post, so it is funny to see it coming back.

On top of that it is now clear that async { .. yield .. } probably should never implement Future. It is compatible with the Future types but has become semantically distinct given it has the power to be Ready multiple times.

So, to get useful streams we must allow one of yield in async blocks or .await in non-async closures. To me it is just a question of which breaks the intuition around Future more.

Edit: Note that the awesome stream! macro uses await outside of implementing Future, which I would see as validating the hypothesis that await is more weakly associated with Future vs async.


You have convinced me that it was a mistake on my part to equate .await(..) to "yield from". If implemented, .await(..) or poll! would have a specific set of use cases when building async-like structures outside of actual Futures. Those cases are relatively broad: I still think Poll makes total sense in pushdown parsers even though the context is a token rather than a std::task::Context. But "yield from" is fundamentally more broad and probably should not be tied to the await keyword.

I still think delegation is an important part of this proposal and should not be left out of the discussion. However, I think non-poll cases (notably iterator delagation) are concisely solved without a sugar:

|| {
    for x in iter_a {
        yield Some(x);
    }
    for x in iter_b {
        yield Some(x);
    }
    None
}

Hence my focus on poll-like cases. But you could still change my mind on that.

Edit: Generalized coroutine delegation actually makes the most sense in a universe where we do GeneratorState-wrapping, which is a reason I've recently become more conflicted on it. However, I think there are still good reasons not to do that.


I've been drafting my version of this complete proposal at samsartor.com/coroutines-2. That isn't because I think this is settled (e.g. you literally just opened me up to the possibility of yield in async instead) but we are at a point where some kind of polished, end-to-end proposal is possible and I want to start working on the verbiage.

@pitaj
Copy link

pitaj commented Jul 2, 2020

However, we must understand that yield in an async closure implies the ability to use yield in async blocks and thus decouples async with Future

I don't see that implication at all. For a normal block, we only allow yield if it is in a closure context, because yield controls the behavior of the closure. For an async block, I don't see why anything would change besides wrapping the yielded values in Poll::Ready.

Sure, yield closures will be the backing of async/await, but the async/await magic doesn't need to expose that to anyone.

if we trivially async || { .. } => || async { .. } desugar the body of your map_stream example ... then it becomes clear that the closure adds a totally useless level of indirection

Except yield doesn't mean anything is the context of just a block. Nothing changes when it becomes an async block.

So, to get useful streams we must allow one of yield in async blocks or .await in non-async closures. To me it is just a question of which breaks the intuition around Future more.

You're going to need to expand more on why having yield in async blocks inside closures necessitates having yield in async blocks generally.


You have convinced me that it was a mistake on my part to equate .await(..) to "yield from". If implemented, .await(..) or poll! would have a specific set of use cases when building async-like structures outside of actual Futures. Those cases are relatively broad: I still think Poll makes total sense in pushdown parsers even though the context is a token rather than a std::task::Context. But "yield from" is fundamentally more broad and probably should not be tied to the await keyword.

👍

I still think delegation is an important part of this proposal and should not be left out of the discussion

Hard disagree. I think delegation is something that will be hard to get right, and I think it's something that many people will disagree on. I think adding too much to this proposal, especially when it could be easily left out for future coverage, could drag down the RFC. We can provide notes about why it would be useful and that we have talked about it, but I think it's better handled in a separate RFC, since it would be essentially a new operator.

tl;dr: Keep the RFC as light as possible while still meeting the necessary features.

@samsartor
Copy link

samsartor commented Jul 2, 2020

You're going to need to expand more on why having yield in async blocks inside closures necessitates having yield in async blocks generally.

Imagine I construct a closure like:

let x = aysnc || thing.await;

and then call it twice:

let a = x();
let b = x();

At this point, nothing has happened. Even though it looks like we might have resumed our closure twice, a, b are in fact identical, brand new impl Futures. We haven't even entered the async context yet. Async functions and closures in Rust are just constructors like some kind of Future::new.

This trips up a lot of Rust users since it is tempting to think that:

let data = async_function();
expensive_action();
data.await

will add parallelism by running async_function in the background. After all, it is what JS and other async languages do. But it doesn't.

So even if we add a yield point:

let x = async || {
    yield first.await;
    second.await
}

it is the same thing:

let a = x();
let b = x();

// The body of x hasn't run yet. We can even drop it and then resume the body via a and b.

drop(x);
spawn(a);
spawn(b);

So clearly that yield must apply to the returned Future, not the closure itself. This is made extra apparent the moment we desugar the async:

let x = || async {
   yield first.await;
   second.await
}

Now it is clear that x itself has no yeild points. It returns an expression that evaluates to a Future (aka an async block) and then restarts.

It would be technically possible to go out go our way to forbid yeild when using the desugared form and only allow it in the sugared form. But that would be extra extra confusing since the yield has nothing at all to do with the closure itself. I would actually recommend forbidding it the other way around: make yield only usable in async blocks and not in async functions (which could be mistaken for yield closures).

Am I making any sense?

Edit: sorry about the typos, I am on a phone right now

tl;dr: Keep the RFC as light as possible while still meeting the necessary features.

I think it is fine being left out of an RFC. I just want to discuss it now in case it turns up new design considerations, which I think it has.

@CAD97
Copy link
Owner Author

CAD97 commented Jul 2, 2020

I highly expect that async || { yield } will not desugar to || async { yield }, and instead be a separate transform that supports yield and await in the same context.

async closures aren't even stable yet. You seem to be assuming that async || { } and || async { } are necessarily the same, but that isn't necessarily true. On the contrary, if async || {} is identical to || async {}, then I feel the former doesn't need to exist at all.

@samsartor
Copy link

samsartor commented Jul 2, 2020

Edit: deleted 2 previous comments since I can make a more cohesive argument here at my PC

I highly expect that async || { yield } will not desugar to || async { yield }, and instead be a separate transform that supports yield and await in the same context.

@CAD97 I'm not sure what else it would desugar to. Imagine if the closure takes arguments, something like:

async |x| { yield x.await; ... }

Now what happens? When should it be resumed with x and when with &mut Context? Or should you have to pass both?

I think needing to pass both is a confusing mix of implicit/explicit arguments. It would be better as:

|ctx, x| { yield poll!(x, ctx); .. }

But if it takes one and then the other, you need some kind of type-level distinction. I don't think it is possible to first take &mut Context and then get x later because the closure could try and use x at any time. What would you do with the &mut Context besides move onto the closure body?

The only other possibility is to construct a closure which takes x and returns a future that takes &mut Context. Which is exactly |x| async { ... }. As far as I can tell, it is also the version of this where @pitaj's examples make most sense.

You could require that the closure takes no arguments. But then what is the point of even using the closure syntax? It would be far less confusing to have something like:

pollable! {
    yield x.await;
    ...
}

I like that a lot actually! I think it is certainly better than allowing yield in async { } itself. But it isn't really a part of this proposal besides being something you could build on top of yield closures.

if async || {} is identical to || async {}, then I feel the former doesn't need to exist at all.

I agree. I personally don't think async || {} should ever be stabilized in its current form because it is just redundant and confusing. And I frankly don't know what else the syntax could mean. If you have neat ideas, please tell me!

@pitaj
Copy link

pitaj commented Jul 3, 2020

So clearly that yield must apply to the returned Future, not the closure itself. This is made extra apparent the moment we desugar the async

I disagree. I think the desugar of

let x = async || {
   yield first.await;
   second.await
};

should be

let x = || {
    yield async { first.await }
    async { second.await }
};
let a = x();
let b = x();

Would be identical to if you were to do

let x1 = async || first.await;
let x2 = async || second.await;

let a = x1();
let b = x2();

Which makes it clear that x itself does retain its yield points.

What you said does make sense with the desugaring you envisioned. I think mine makes a little bit more sense and solves the issues you brought up.

I agree. I personally don't think async || {} should ever be stabilized in its current form because it is just redundant and confusing. And I frankly don't know what else the syntax could mean. If you have neat ideas, please tell me!

from comment on the tracking issue:

There may be differences in how lifetimes in the arguments are treated once they’re supported. Also elaborating the return type is not possible with the closure to async block

It also luckily allows my desugar to be possible.

@pitaj
Copy link

pitaj commented Jul 3, 2020

I'm not sure what else it would desugar to. Imagine if the closure takes arguments, something like:

async |x| { yield x.await; ... }

would desugar as

|x| {
    yield async { x.await };
}

So it yields a future which is resumed with &mut Context.

Now for a more difficult example, one with internal state that each future mutates:

let x = async || {
    let mut i = 0;
    loop {
        yield something_async(&mut i).await;
    }
};

desugars as

let x = || {
    let mut i = 0;
    loop {
        yield async { something_async(&mut i).await };
    }
};

You could execute the futures out of order like so

let a = x();
let b = x();

b.await;
a.await;

which would cause out of order execution. I don't know if that's a problem. I wonder how streams handle this with .next().

@CAD97
Copy link
Owner Author

CAD97 commented Jul 3, 2020

I don't think that desugar is possible in general. Consider instead something like

let x = async || {
    yield something;
    let s: Config = read_config();
    yield do_op_a(&s).await;
    yield do_op_b(&s).await;
    loop { panic!("exhausted") }
}

Borrowing across yield/await points is still a thing that has to be considered.

It's for that reason that I think that async yield closures will need to take on a shape isomorphic to

trait AsyncYieldClosure {
    fn resume(Pin<&mut self>, ...args) -> impl '_ + Future;
}

That is,

  • calling x(...args) does no work, just constructs something that can be polled with await
  • x cannot be called again until the previous return value is used and discarded
  • calling x again, supplying new arguments, without finishing polling the previous return value is a logic error (and a panic)

The resulting state machine would be flat, and something like

entry               (args) -> created,           yield AsyncYieldFuture(&mut self)
created     (&mut context) -> yielded_something, yield Done
yielded_something   (args) -> working_on_a,      yield AsyncYieldFuture(&mut self)
working_on_a(&mut context) -> working_on_a,      yield Pending
working_on_a(&mut context) -> yielded_a,         yield Done
yielded_a           (args) -> working_on_b,      yield AsyncYieldFuture(&mut self)
working_on_b(&mut context) -> working_on_b,      yield Pending
working_on_b(&mut context) -> exhausted,         yield Done
exhausted           (args) -> exhausted,         yield panic

Trying to resume a state expecting to be polled like a future by giving it the semicoroutine's arguments, or vice versa, would cause it to enter a panicking state.

@samsartor
Copy link

samsartor commented Jul 3, 2020

@CAD97 I could be wrong, but that solution feels super overengineered to me. Like, the state machine given by the user is already flat. There is no need to flatten it with runtime checks if you don't try to split it into multiple potentially-independent regions via async blocks (which the user can always do manually if it is important to them). Let's look at your example. If we remove the async, make the context explicit, and flatten the type to x: impl FnPin(&mut Context) -> Poll<_>, we get:

let x = mut |ctx| {
    yield something;
    let s: Config = read_config();
    yield Ready(poll!(do_op_a(&s), ctx));
    yield Ready(poll!(do_op_b(&s), ctx));
    panic!("exhausted")
}

which (thanks to the lack of additional resume arguments) could easily be sugared back to:

let x = pollable! {
    yield something;
    let s: Config = read_config();
    yield do_op_a(&s).await;
    yield do_op_b(&s).await;
    panic!("exhausted")
}

which is totally free of borrow-checked return-value attachment, runtime panics (beyond the obvious), additional scheduler overhead, etc. all while being barely less verbose and significantly less magical. And additional resume arguments don't cause any problems.

I guess I just don't see what we would gain from having a more sophisticated async || { ... } desugar like what @pcpthm is suggesting. Even if it is possible to fully define in every odd little case. Could you sketch out a use case where it is a significant improvement over pollable!/ordinary yield closures?

Is there some major advantage of an impl Fn() -> impl Future style Stream trait over a flat one that I've missed? Using the flat futures::stream/tokio::stream/async_std::stream traits hasn't caused any problems for me so far. In fact, async_std actually switched from a next function with associated impl Future to a flattened poll_next function from version 0.99 to version 1.0.

@CAD97
Copy link
Owner Author

CAD97 commented Jul 3, 2020

The reason is those extra arguments. The closure can take arguments, which should be supplied anew at each yield point but not each await point.

So the flat Stream does make a lot of sense for the case without extra not-async uses of yield. It's when you add yield into the mix that you get this two-layer effect.

I'll add the argument here just to give us something to work from:

let x = async |args: &Args| {
    yield something(args);
    let s: Config = read_config();
    yield do_op_a(&s, args).await;
    yield do_op_b(&s, args).await;
    loop { panic!("exhausted") }
}

Normal usage would look like

let args: Args = Args::new();
let something = x(&args).await;
let op_a_res = x(&args).await;
let op_b_res = x(&args).await;

This is exactly what normal usage would look like for a non-yield async closure as well. And that's the whole point of FnPinMut: yield functions are just a regular FnMut that requires pinning. async closures are FnMut -> impl Future. So async yield closures are also FnPinMut -> impl Future (though with the extra condition to make them better that they can just reborrow the same structure and use it for scratch space).


Once more, to be explicit:

for Stream, which is the async trait Iterator, having a flat fn poll_next(Pin<&mut self>) -> Poll<Self::Item> makes absolute sense. But async yield functions are not Stream in the general case, because they take arguments.

In the case where it's an async || { yield; }, the closure can still implement the flat Stream. The "AsyncYieldClosure::resume" would literally be fn resume(Pin<&mut self>) -> Pin<&mut self> { self } in that case. The orthogonal stacked design is required when it's async |args| { yield; }.

@CAD97
Copy link
Owner Author

CAD97 commented Jul 3, 2020

The state machine doesn't have to be flattened at all, that's just an interesting (maybe) optimization. Not doing the flattening might be better, as you can avoid the cross-contamination of states that require the runtime checking. The returned "awaitable" does, however, have to borrow from the yield closure it was yielded from, for the simple reason that... it can borrow from the stack state in the closure.

@samsartor
Copy link

samsartor commented Jul 3, 2020

@CAD97 That is valid. Sink would be a better example IMO since your &Args case could be easily solved with capturing or just by passing the same reference to both kinds of resumes. You could implement poll_complete via an ordinary yield closure (especially if there is some nice way to "fork" coroutines or at least externally mutate the state, which I absolutely want to think more about) but I admit it is less intuitive.

However, I think that non-trivial async closures should absolutely not be a part of this proposal even if they are feasible.

I think I can explain where I am coming from here. In the async interviews boats said something along the lines of:

The design space for coroutines is so large that we should narrow our focus from "create the perfect, flexible abstraction for suspended functions and coroutines” to “create something that lets you write iterators and streams”.

I've thought about that quote a lot and in the last few days I've become sure this proposal proves him wrong: suspended functions can actually be really simple. In fact, they are so incredibly dumb that our proposal should really be able to stand on its own without any asyncyness at all. yield is just another common way to return from a function which many languages support. We should support it too.

On the other hand, "create[ing] something that lets you write iterators and streams” is really where the design space levels the block. The moment you map suspendable functions onto higher level concepts, the number of possible syntax sugars absolutely explodes. And it is largely because those sugars are really interesting to think about! I myself am guilty of being tempted away from the true path by .await(). I even included it as the 4th horseman in my draft post when only 3 features are actually needed to make suspendable functions possible (yield, FnPin, and mut).

But the brilliance of yield closures, the reason why this proposal is so appealing to me, is that all the sugar in the world is effectively irrelevant. Seize the means of suspension and the proletariat can thoroughly explore the design space as members of the ecosystem (e.g. async_stream which presently uses a capacity=1 channel and await as a replacement for yield) or privately in their own projects!

Or maybe I'm just bitter and won't let you include async yield closures when I don't get to include generalized await ;)

@CAD97
Copy link
Owner Author

CAD97 commented Jul 3, 2020

However, I think that non-trivial async closures should absolutely not be a part of this proposal.

In fact, they are so incredibly dumb that our proposal should really be able to stand on its own without any asyncyness at all.

Definitely agree here. All of this should be relegated to future extensions. It's somewhat important to show that it is extendable to support these things, but really the core (and the important part) is FnPinMut and making yield closures behave nearly the same as FnMut closures.

The first RFC really should completely ignore async and punt on await and yield in the same context.

@pitaj
Copy link

pitaj commented Jul 7, 2020

Before I get into anything

All of this should be relegated to future extensions.

Absolutely agree. We can just disallow yield within async blocks and async closures for now.


let x = async || {
    yield something;
    let s: Config = read_config();
    yield do_op_a(&s).await;
    yield do_op_b(&s).await;
    loop { panic!("exhausted") }
}

This looks less difficult than the example I gave in which there's a mutable reference. Why can't this case desugar as the following?

let x = || {
    yield async { something };
    let s: Config = read_config();
    yield async { do_op_a(&s).await };
    yield async { do_op_b(&s).await };
    loop { panic!("exhausted") }
}

I think we could guarantee you can't call x() again before resolving the Future returned by the first or second call by giving the auto-generated struct a mutable borrow on the closure, much like StreamExt::next does, requiring that any previous future be used (though I don't know what dropping the future would do with an async-yield-closure, with StreamExt it results in the stream starting back at the point before .next).

Rust actually already takes care of execution order for mutable references: playground

I do think treating async-yield-closures as a special case is probably a better way of handling it, though. Even if not purely necessary. It would as you've pointed out allow for a direct Stream impl without the intermediate "sequence of Futures" step.

calling x again, supplying new arguments, without finishing polling the previous return value is a logic error

I think this can be covered by the type system in most if not all cases by holding a mutable reference.

I guess I just don't see what we would gain from having a more sophisticated async || { ... } desugar like what @pcpthm is suggesting. Even if it is possible to fully define in every odd little case. Could you sketch out a use case where it is a significant improvement over pollable!/ordinary yield closures?

In addition to what @CAD97 pointed out, specifying the return value without impl Future... which is currently not allowed in those positions. It also just makes sense IMO to have async closures of all types if we have async functions.

@CAD97
Copy link
Owner Author

CAD97 commented Jul 7, 2020

I think this can be covered by the type system in most if not all cases by holding a mutable reference.

You could always just drop the future without polling it, as you've noted. I think the only reasonable behavior there is for it to panic, honestly.

This looks less difficult than the example I gave in which there's a mutable reference. Why can't this case desugar as the following?

Ah, right, yes, you do need to make sure it's a mutable borrow being held to enforce linearity, or to actually move the value being borrowed.

@pitaj
Copy link

pitaj commented Jul 7, 2020

You could always just drop the future without polling it, as you've noted. I think the only reasonable behavior there is for it to panic, honestly.

Good point, I agree. We could make the future drop impl place the yield closure in a poisoned state, and any later call or future would run into that.

Orrrrr... we do it the way streams do, where, since the future wasn't executed, the next call returns an identical future. Use the mutable reference to tell rust not to let us have two futures from the async-yield-closure at once, and this could work. Only issue is what to do if a future is partially executed, I haven't experimented to see how streams handle that.

@samsartor
Copy link

@CAD97 @pcpthm @pitaj The language team is currently implementing a different Pre-RFC process which begins with an MCP in the lang-team repo and then further discussion in Zulip. I think it is actually fantastic chance to get this out in the world and get people discussing it! So I've written up an MCP for us to submit.

Give me some feedback and then let's get this thing shipped!

@CAD97
Copy link
Owner Author

CAD97 commented Aug 27, 2020

|| loop {
    yield true;
    false
}

I think this needs to be yield false; a loop expression can't have a tail value. Otherwise, that MCP looks like a good overview of this direction for yielding closures and how they behave!

I'm personally in favor of yield closures poisoning after a return myself, but the MCP does note that to be determined in implementation, so that's not a big deal. And I do have to admit that the early return in the filter_map example (as opposed to a yield; continue;) is a neat motivating example for restarting after a return.

@pcpthm
Copy link

pcpthm commented Aug 27, 2020

The MCP document is pretty great! I'm impressed by how it has well-motivated examples and still concise. Thanks for writing this.

I didn't find any issues in the document. It looks good as a major change proposal. I think it's time for this proposal to reach the eyes of more people.

@CAD97
Copy link
Owner Author

CAD97 commented Aug 27, 2020

Two more small nits:

pub fn read_to_stream(read: impl AsyncRead) -> impl TrySteam<Item=u8> {

That's probably supposed to return TryStream not TrySteam.

Also, you use move mut in one place and mut move in another. You should pick one and stick with it; I think I'd recommend move mut.

@pitaj
Copy link

pitaj commented Aug 28, 2020

Summary and problem statement

One of the main benefits of this proposal is the flexibility of the behavior, especially in how well it lends itself to being built upon. For instance, withoutboats's propane could be built upon this even more simply than with the current unstable generator implementation. I'd like to see this aspect present in the summary.

I think it should be made clear that for this version of coroutines, the yield type and return type are one in the same.

Motivation, use-cases, and solution sketches

For the read_to_stream example, you may want to specify stream::poll_fn instead of just poll_fn. People may also confuse your poll! macro with the one in futures, so you may want to clarify that somewhere.

Does "Once Closures" refer to existing FnOnce or the idea of a closure which does not loop? If the second, the naming is a little confusing, but I can't think of a better name besides "Mut Closures" which I don't particularly like either.

Under "Once Closures", you may want to make it more evident that we prefer mut modifier to poisoned-by-default.

As for solution sketches, we'll probably want to throw up some desugared examples and work out the exact details a little better. (I'm not very familiar with the term, so I may be totally off here)

Links and related work

May want to mention Propane and of course the original coroutines eRFC

@samsartor
Copy link

samsartor commented Aug 28, 2020

May want to mention Propane and of course the original coroutines eRFC

Good call. I'll drop a mention of propane in next to async_stream and add a link to the eRFC.

Does "Once Closures" refer to existing FnOnce or the idea of a closure which does not loop? The naming is a little confusing.

Yah I know. But I couldn't think of a better title either, so I'm just counting on the explanation below to clarify.

Under "Once Closures", you may want to make it more evident that we prefer mut modifier to poisoned-by-default.

I'll revisit this phrasing a bit but the point of the MCP really is to create further discussion, not provide a complete solution. Especially since CAD97 is on the other side of this! Either way I'm stealing myself to battle opt-in vs default poison in Zulip later.

We'll probably want to throw up some desugared examples and work out the exact details a little better.

I'm intentionally leaving the semantics just a little ambiguous. People can always ask questions in the Zulip stream or come look at the comments here if they feel lost. The MCP template starts with "Describe the problem(s) this group is trying to solve as concisely as you can" and I'm really trying to stick to that.


Once I finish making those last changes, I'll go ahead and post this up. See you all there!

@samsartor
Copy link

@CAD97 @pcpthm @pitaj Our MCP got some good reception! Alas, the lang team currently has higher priorities. I put together some design notes to help with work on this feature in the future. The notes don't have to be super high quality but I'd appreciate it if you all sanity checked them a bit. The PR is open here.

@CAD97
Copy link
Owner Author

CAD97 commented Sep 15, 2020

It's probably worth noting in there somewhere that what you're calling coroutines is formally a "semicoroutine".

The difference is that a "semicoroutine" only returns to one place, its caller; whereas a "full coroutine" specifies another coroutine to continue from when it yields.

The distinction probably doesn't matter too much, but for people who know the difference, it'd be nice to clarify that the proposal described there only returns to its caller rather than being the "fully explicit cooperative multitasking" that full coroutines describe.

The wikipedia page is actually uncharacteristically clear in regards to this topic: https://en.wikipedia.org/wiki/Coroutine (though the list of languages with support for coroutines seems to be in somewhat of a disarray, as I'm pretty sure most of them only have semicoroutines/generator syntax...)

@CAD97
Copy link
Owner Author

CAD97 commented Jul 12, 2022

Closing this now that https://lang-team.rust-lang.org/design_notes/general_coroutines.html is available. I do hope sometime in the future a design along these lines is taken, but the design notes capture what is covered here.

@CAD97 CAD97 closed this Jul 12, 2022
@CAD97 CAD97 deleted the fn-pin-mut branch March 19, 2023 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants