search

rust-lang / rust

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

Tracking issue for `ControlFlow` enum, for use with `try_fold` and in `Try` #75744

Closed NoraCodes closed 2 weeks ago

NoraCodes commented 4 years ago

(edited to turn this into a tracking issue, as it's referenced by the unstable attributes)

This is a tracking issue for the std::ops::ControlFlow type. The feature gate for the issue is #![feature(control_flow_enum)].

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

Initial PR that added LoopState as an implementation detail: https://github.com/rust-lang/rust/pull/45595 PR that exposed as unstable and renamed to ControlFlow: https://github.com/rust-lang/rust/pull/76204 Added BREAK/CONTINUE associated constants: https://github.com/rust-lang/rust/pull/76318 Changed type parameter order and defaulted C = (): https://github.com/rust-lang/rust/pull/76614 Add is_break and is_continue methods: #78200

I work with an organization that has a large amount of Rust graph traversal code as part of its core business. We used to use itertools's fold_while for short-circuiting functional-style loops over slices of our graphs, which is now deprecated in favor of try_fold.

try_fold is great, but the lack of a standard library provided Try implementation that makes the loop semantics clear is confusing. We created our own, which is fine, but I think it would make a lot of sense to expose LoopState and provide an example in the docs.

Originally related to: rust-itertools/itertools#469

ecstatic-morse commented 4 years ago

Big +1 for this. A Break/Continue enum is also the correct abstraction for the Try trait instead of Result IMO, so I think this is worthy inclusion in the standard library (although in that case a name change would be in order). FWIW, I usually call this enum ControlFlow.

scottmcm commented 4 years ago

As the one who added LoopState initially, I'm not at all tied to that name, so morse's rename sounds good to me -- in fact I've also been calling it ControlFlow in the Try discussions.

@NoraCodes Want to take on making a PR here? There's probably a few parts:

Feel free to ping me (here, in a draft PR, on community Discord, or on Zulip) if you have questions.

Its methods are probably imperfect right now, but if it's unstable that's ok. People can start using it on nightly as we'll find out what needs fixing before stabilization that way.

NoraCodes commented 4 years ago

I would be happy to do this. I may ask for some help getting a development environment set up as I have yet to contribute to the core libraries.

scottmcm commented 4 years ago

@rustbot assign @NoraCodes

scottmcm commented 4 years ago

Congrats on your first core libraries PR, @NoraCodes!

Want to take on the generic parameter reordering (that morse brought up in https://github.com/rust-lang/rust/pull/76204#discussion_r481357223) next?

NoraCodes commented 4 years ago

Thanks @scottmcm! I definitely do want to do that. I'll put in a PR either this weekend or Tuesday.

scottmcm commented 4 years ago

Another example of a type that's pretty close to this: the Transition type in https://deislabs.io/posts/a-fistful-of-states/, which is essentially ControlFlow<Result<()>, Box<dyn State>> -- either execution is done (possibly with an error), or it continues (in the specified state).

scottmcm commented 4 years ago

An MCP to use this more often in the compiler: https://github.com/rust-lang/compiler-team/issues/374

Conveniently that also suggests that ControlFlow<BreakType> would be better than the current ControlFlow<(), BreakType>.

EDIT: A great comment on the PR that implements the MCP (https://github.com/rust-lang/rust/pull/78182#issuecomment-713695594):

This PR is awesome (and exposes so many issues of different sublety)

Nice to see confirmation of the value 🙂

NoraCodes commented 4 years ago

Apologies for taking forever on this. I set up a Rust dev env on my new PC and fixed the nits in that PR so we should be good to go. I'll move forward with the other work, and documentation.

NoraCodes commented 3 years ago

Personally, I don't have much of an opinion on the CONTINUE and BREAK constants. It seems mildly unidiomatic, but I can also see how those constants are useful in writing concise code.

I do think we should default B = (); this makes the simplest cases the simplest to write, and doesn't really increase complexity for more complex cases.

scottmcm commented 3 years ago

I've posted a stabilization PR of just the type (not the methods) for consideration at https://github.com/rust-lang/rust/pull/85608

(It's not proposed here because it doesn't stabilize everything, and there can only be one FCP per issue, so having a separate PR will allow the remaining pieces to be FCPed separately later.)

mendess commented 3 years ago

I have re implemented a subset of this in a small project of mine since this enum just has really good names for many usecases and I want to use it when it becomes stable, I have to say that the constants CONTINUE/BREAK are really useful as my usecases haven't needed to use something other than ControlFlow<(), ()>. So I would be glad if these constants made it to the final release so I don't have to change much code :smile:

For the same reasons I also think B = () is a good idea.

Hope this contributes to the discussion!

dbofmmbt commented 3 years ago

Will it be stabilized in 1.55? It's in the stabilized APIs section.

scottmcm commented 3 years ago

@dbofmmbt Just the type itself was stabilized -- see https://github.com/rust-lang/rust/pull/85608.

This issue remains open to track the methods and associated constants on it which are not currently stabilized.

Robbepop commented 2 years ago

I wonder if ControlFlow could be used as a hint to the Rust compiler in order to optimize certain switch based interpreters to use the more efficient computed-goto codegen instead of branching at the top of a switch.

An example how this could be utilized can be found here: https://gist.github.com/Robbepop/756eedb75466e09e0262ef917818c553 If you paste the linked code into the Rust compiler explorer you will see that it currently does not generate the efficient computed goto syntax and it is kinda hard to make it happen deterministically in Rust.

juntyr commented 2 years ago

Would it be possible to also add boolean constructors for ControlFlow<(), ()>, e.g.

impl ControlFlow<(), ()> {
    #[must_use]
    pub fn continue_if(should_continue: bool) -> Self {
        if should_continue {
            ControlFlow::CONTINUE
        } else {
            ControlFlow::BREAK
        }
    }

    #[must_use]
    pub fn break_if(should_break: bool) -> Self {
        if should_break {
            ControlFlow::BREAK
        } else {
            ControlFlow::CONTINUE
        }
    }
}

As I am refactoring some prior bool-based code, I'm having to repeat these if-cases many times.

scottmcm commented 2 years ago

@MomoLangenstein For a simple method addition, you can absolutely send a PR. With https://doc.rust-lang.org/std/primitive.bool.html#method.then stable, there's probably space for something in this area.

What it should be, I don't know. I'm a bit skeptical of both the break_if and continue_if methods, though -- maybe you can find a nice name that would be ok with just having one method?

dbofmmbt commented 2 years ago

@MomoLangenstein AFAIK this could be done as an external lib. Actually, I think it can be implemented on stable, as the enum is stabilized. :thinking:

juntyr commented 2 years ago

AFAIK this could be done as an external lib. Actually, I think it can be implemented on stable, as the enum is stabilized. 🤔

@dbofmmbt Yes, that's true. Though I think this would really be a convenience function that serves to better document the code functionality more concisely, which would be valuable to have in core itself.

juntyr commented 2 years ago

What it should be, I don't know. I'm a bit skeptical of both the break_if and continue_if methods, though -- maybe you can find a nice name that would be ok with just having one method?

@scottmcm I think the most common pattern would be along the lines of

if some_condition {
    break;
}

meaning that break_if (or a better name) would probably be more valuable.

dbofmmbt commented 2 years ago 
trait BoolControlFlowExt {
  fn break(self) -> ControlFlow;
  fn continue(self) -> ControFlow;
}

impl BoolControlFlowExt for bool {
  fn break(self) -> ControlFlow {
    match self {
          true => ControlFlow::Break,
          false => ControlFlow::Continue,
}

  fn continue(self) -> ControlFlow {
    match self {
          true => ControlFlow::Continue,
          false => ControlFlow::Break,
}
  }
}

// Some code...
....
my_condition.break()?;
....

@MomoLangenstein what do you think? Sorry about the (probably bad) trait name.

I would define another trait extension for ControlFlow too, as AFAIK it is not easily convertible between other Try types today, and would be nice to convert between them and use?.

juntyr commented 2 years ago

@dbofmmbt Thank you very much, that looks great! Personally, when using long boolean conditions, I'd prefer the explicit ControlFlow-based version (just an example I have):

ControlFlow::break_if(steps >= max_steps || next_event_time >= max_event_time)

instead of the more implicit bool-based version:

(steps >= max_steps || next_event_time >= max_event_time).break()

But yes, both of these could be implemented with an external extension trait (so thanks again for the suggestion).

scottmcm commented 2 years ago

Note that if, as above, the common case here is if ... some cond ... { break } is the common case, then methods from bool wouldn't really help with that, since it doesn't necessarily want to return the continue immediately too.

You might consider experimenting with try blocks as an alternative to the method here. Because ControlFlow::break_if(... some cond ...) can also be spelled try { if ... some cond ... { ControlFlow::BREAK? } }, and that's more flexible to adding more code in the various different places.

juntyr commented 2 years ago

I guess I meant the if cond { break } as more of an example that breaking if a condition is met is probably more common than continuing of a condition is met. In my use case, a simulation with a user-provided early-stop condition, I need to construct ControlFlow based on a boolean condition inside a closure, the return value of which is used inside the simulation to determine whether to go on or stop early.

quark-zju commented 2 years ago

Not sure if this is the right place for this discussion but I wonder if ? with control flow can work if the function returns the B type.

fn g() -> std::ops::ControlFlow<u8> {
    std::ops::ControlFlow::Break(1)
} 

fn f() -> u8 {
    g()?; // does not compile in 1.61: E0277  the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
    0
}

// f() returns 1.

This seems useful in cases where the caller of f does not care about whether its result comes from a break or a continue.

EDIT: It seems I misunderstood the ControlFlow. It does not seem to be designed for the control flow of the current Rust program, but useful when the Rust program is interpreting another program. In this case it makes sense to only support ControlFlow<...> return type.

benluelo commented 2 years ago

Adding my 2c here - a map_continue(..) combinator (to go with the current map_break(..) one), would be very useful. Should I open a new issue and link it here, or is this suggestion OK in this issue?

scottmcm commented 2 years ago

@benluelo Since map_break exists, I think you could just open a PR to add map_continue under the same tracking issue. Feel free to r? @scottmcm on it.

ModProg commented 1 year ago

I found a usecase where maybe ControlFlow could be used as well, having an early exit from a try_fold for speed up using the same value as the aggregator.

In case this should be in a new issue, let me know.

Something like

impl<T> ControlFlow<T, T> {
    pub fn value(self) -> T {
        match self {
            ControlFlow::Continue(value) | ControlFlow::Break(value) => value,
        }
    }
}

allowing to do 

let value = smth.try_fold(Vec::new(), |aggr, curr| {
  // Do something 
  if todo!("Some condition that checks weather aggr is ready for early exit e.g. full") {
    ControlFlow::Break(aggr)
  } else {
    ControlFlow::Continue(aggr)
  }
}).value();
yoshuawuyts commented 1 year ago

I've been wondering about whether we could move some of the shared operations between Option/Result/ControlFlow/Poll to the Try trait. The most obvious one probably being Try::map, but probably methods such as Try::inspect and Try::filter could make sense too.

I realized that ControlFlow doesn't have a singular operation named map; it has both map_continue and map_break. Because ControlFlow maps Try::Output to the value contained in the ControlFlow::Continue, it might be more consistent to rename the map_continue operation to just map. This would match the Result::map / Result::map_err scheme too.

Proposed Method Naming

schuelermine commented 1 year ago

@ModProg I have also run into a case where such a method would be desirable

LeoniePhiline commented 1 year ago

The naming of ControlFlow::break_value and ControlFlow::continue_value is inconsistent with Result::err and Result::break.

Should the two methods be renamed before stabilization?

Proposed Method Naming

Current name New name Consistent with
ControlFlow::break_value ControlFlow::break Result::err
ControlFlow::continue_value ControlFlow::continue Result::ok
schuelermine commented 1 year ago

They cannot have those names because they are strict keywords.

SmnTin commented 3 months ago

Can we stabilize map_break and map_continue?

scottmcm commented 3 months ago

Hello libs-api folks! Inspired by the previous comment, I'm nominating this to get your thoughts on the methods currently tracked by this issue:

impl<B, C> ControlFlow<B, C> {
    pub fn break_value(self) -> Option<B>;
    pub fn map_break<T, F>(self, f: F) -> ControlFlow<T, C>
        where F: FnOnce(B) -> T;
    pub fn continue_value(self) -> Option<C>;
    pub fn map_continue<T, F>(self, f: F) -> ControlFlow<B, T>
        where F: FnOnce(C) -> T;
}

Methods like this were intentionally not part of the RFC that added ControlFlow (https://rust-lang.github.io/rfcs/3058-try-trait-v2.html#methods-on-controlflow) so there hasn't been any previous checkboxes about them.

These are basically like Result::ok and Result::map_err, but because break and continue are keywords, ControlFlow::break doesn't work. It'd need to be ControlFlow::r#break, which I assume we don't want. How do you feel about the break_value and continue_value as the solution to that?

The other naming question from the thread is how to name the map methods, https://github.com/rust-lang/rust/issues/75744#issuecomment-1464059870. I added them as map_break and map_continue, thinking of this type as not prioritizing one side over the other, which personally I think I still like. But map_continue is the try { f(x?) } version, so one could argue it should be named just map like Option::map and Result::map -- though of course there's no Option::map_none so you could also say that that's why the Option one is just map.

SmnTin commented 3 months ago

@scottmcm If we add these methods, unwrap_break and unwrap_continue might also be good additions. Or their utility is not as obvious as Result::unwrap and Result::unwrap_err?

m-ou-se commented 3 months ago

@scottmcm We discussed your comment in the libs-api meeting. We're happy with the current names (with _value). We agree with what you said about map_break and map_continue, to not prioritize one over the other.

dhedey commented 1 month ago

I'm sorry if this is the wrong place to mention this; but I've just been using ControlFlow for a visitor pattern (well a validator + visitor), and had a couple of notes that I think would be very beneficial to other users, and hopefully easy tweaks:

NoraCodes commented 1 month ago

I would be in favor of a #[must_use] annotation on ControlFlow.

I'm concerned that implementing Into<Result< >> would be confusing, since it wouldn't be obvious or easy to remember whether it was impl<C, B> From<ControlFlow<B, C>> for Result<C, B> or impl<C, B> From<ControlFlow<B, C>> for Result<B, C>. I think the proposed continue_result and break_result is a better idea, perhaps made even clearer if called continue_ok and break_ok.

dhedey commented 1 month ago

Thanks @NoraCodes - and just to add I much prefer the continue_ok and break_ok names you propose!

scottmcm commented 1 month ago

I've put up a stabilization PR for the remaining things tracked under this issue in #130518

@dhedey, can you open a new item for your request so it's more likely to be considered instead of lost? Maybe open an ACP to make the case to the libs-api folks, then if that's approved the attribute addition can be PRed?

scottmcm commented 1 month ago

@ModProg @schuelermine Could one of your open an ACP proposing the into_value (or whatever name) method? It sounds plausible to me, but it should have an ACP in order to add it, and then probably a different tracking issue from the existing things.

(This one's over 4 years old, so I'd like to get it closed out rather than make people expand the history and read a bunch of no-longer-relevant things.)

dhedey commented 1 month ago

Will do, thanks @scottmcm 👍

scottmcm commented 2 weeks ago

Huzzah, everything tracked here is now stable!

For anyone with ControlFlow-related requests in future, please make new ACPs/bugs/etc for them as appropriate.