Add core::ops::GeneratorState::{is_yielded,is_complete} methods

[yoshuawuyts]

Similar to Option::is_*, Result::is_* and ControlFlow::is_* methods, this adds is_* methods for GeneratorState. Thanks!

[rust-highfive]

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

[m-ou-se]

Thanks! Can you give an example of a concrete use case?

[yoshuawuyts]

Thanks! Can you give an example of a concrete use case?

Sure thing! I only recently started looking at generators more closely, so I don't have loads to back it up in terms of experience. But one thing where these methods are particularly useful for is when validating generator state. For example, we could rewrite the main generator example using is_ + assert!:

#![feature(generators, generator_trait)]

use std::ops::{Generator, GeneratorState};
use std::pin::Pin;

fn main() {
    let mut generator = || {
        yield 1;
        return "foo"
    };

-    match Pin::new(&mut generator).resume(()) {
-        GeneratorState::Yielded(1) => {}
-        _ => panic!("unexpected return from resume"),
-    }
+    assert!(Pin::new(&mut generator).resume(()).is_yielded());
-    match Pin::new(&mut generator).resume(()) {
-        GeneratorState::Complete("foo") => {}
-        _ => panic!("unexpected return from resume"),
-    }
+    assert!(Pin::new(&mut generator).resume(()).is_complete());
}

I don't know how much manual generator impls, generator state introspection, and is_ methods specifically will be used in practice. But that's part of the stabilization process to figure out. Given many other enums in the stdlib which serve similar functions (Result, Option, Poll, ControlFlow, etc) implement is_ accessors, it seemed to me like adding these methods would be fairly uncontroversial.

[m-ou-se]

Sounds like it should also have things like unwrap_yielded, unwrap_complete, as_ref, as_mut, complete, yielded, into_yielded, into_yielded_or_complete and more methods that Result also has, maybe. (ControlFlow has just break_value, is_break, is_continue, and map_break and nothing else, which also seems incomplete and a bit inconsistent.)

I'm not necessarily opposed to adding is_{yielded, complete} here, but I'd much rather see a more complete proposal for what the interface (or even the name) of GeneratorState should be. (And its relationship to ControlFlow and Poll etc.) I don't think GeneratorState has seen much design work. Looks like it was just the minimal type required to express the possible results of Generator::resume(), leaving most of the design for later.

[yoshuawuyts]

Hmm, yeah that seems like it would be useful indeed. Thinking about this a bit: if we consider ControlFlow and GeneratorState as aliases for Result, so too could we consider Poll to be an alias for Option. I think it could make sense to ensure that methods implemented on the base type are also guaranteed to exist on their aliases. For example on ControlFlow that would mean:

• keep map_break and add map_continue
• rename break_value -> ok
• keep is_break and is_continue
• add remainder of Result methods

This could equally be applied to GeneratorState. The only struct where I don't think we can do this (and in hindsight this is likely a mistake) is Poll. Methods such as map_err and map_ok don't have counterparts in Option. And the way Try for Poll has been implemented has lead to the need to add task::ready!.

It definitely seems like it would've made sense to keep these types consistent from the start. But because late is better than never, we might as well do it now. If this reasoning all makes sense; what would the right way be to proceed with this? I believe libs has both RFCs and policies; but I'm not sure which one this would fall under?

[SkiFire13]

For example, we could rewrite the main generator example using is_ + assert!:

That will lose informations about the values that were yielded though.

[yoshuawuyts]

@SkiFire13 I'm not proposing we change the example; keeping as much information as possible makes sense here. It merely served to illustrate that when folks author their own generators by hand, being able to assert intermediate states can sometimes be simplified by providing is_ methods.

[m-ou-se]

It definitely seems like it would've made sense to keep these types consistent from the start. But because late is better than never, we might as well do it now. If this reasoning all makes sense; what would the right way be to proceed with this?

An RFC would work well for this kind of thing.

[Dylan-DPC]

@yoshuawuyts any updates on the rfc for this change?

[Dylan-DPC]

Closing this as its inactive and waiting on a rfc that will take time to merge.