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

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

Open
4 of 9 tasks
NoraCodes opened this issue Aug 20, 2020 · 32 comments
Open
4 of 9 tasks

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

NoraCodes opened this issue Aug 20, 2020 · 32 comments
Assignees
Labels
A-control-flow Area: Relating to control flow A-iterators Area: Iterators C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@NoraCodes
Copy link
Contributor

NoraCodes commented Aug 20, 2020

(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: #45595
PR that exposed as unstable and renamed to ControlFlow: #76204
Added BREAK/CONTINUE associated constants: #76318
Changed type parameter order and defaulted C = (): #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
Copy link
Contributor

ecstatic-morse commented Aug 22, 2020

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

scottmcm commented Aug 24, 2020

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:

  • Renaming from LoopState to ControlFlow
  • Making it pub and having appropriate #[unstable] markers on it
  • Probably moving it out of core::iter; maybe to core::ops instead?
  • Reexporting from std
  • Derive a few more things on it now that it's intended to be a real type and not just something internal -- probably at least Clone, Copy, Debug (like morse's example has) in addition to its current PartialEq. (I would personally avoid PartialOrd and Ord for now -- if we stay order-agnostic for now we can change the discriminants later to make interconversion with result as cheap as possible without breaking people.)

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.

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. C-enhancement Category: An issue proposing an enhancement or a PR with one. C-feature-request Category: A feature request, i.e: not implemented / a PR. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Aug 24, 2020
@NoraCodes
Copy link
Contributor Author

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

This comment has been minimized.

@scottmcm scottmcm added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Sep 3, 2020
@scottmcm scottmcm changed the title Expose LoopState to make try_fold closures easier to write and more idiomatic Tracking issue for ControlFlow enum, for use with try_fold and in Try Sep 3, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 3, 2020
…=scottmcm

Rename and expose LoopState as ControlFlow

Basic PR for rust-lang#75744. Addresses everything there except for documentation; lots of examples are probably a good idea.
@scottmcm
Copy link
Member

scottmcm commented Sep 3, 2020

Congrats on your first core libraries PR, @NoraCodes!

Want to take on the generic parameter reordering (that morse brought up in #76204 (comment)) next?

@NoraCodes
Copy link
Contributor Author

NoraCodes commented Sep 3, 2020 via email

@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Sep 10, 2020
@KodrAus KodrAus added A-control-flow Area: Relating to control flow A-iterators Area: Iterators labels Sep 22, 2020
@scottmcm
Copy link
Member

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

scottmcm commented Oct 21, 2020

An MCP to use this more often in the compiler: rust-lang/compiler-team#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 (#78182 (comment)):

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

Nice to see confirmation of the value 🙂

@NoraCodes
Copy link
Contributor Author

NoraCodes commented Oct 21, 2020

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.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 22, 2020
…, r=scottmcm

Add `ControlFlow::is_{break,continue}` methods

r? @scottmcm cc rust-lang#75744
@dbofmmbt
Copy link

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

@scottmcm
Copy link
Member

@dbofmmbt Just the type itself was stabilized -- see #85608.

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

@Robbepop
Copy link
Contributor

Robbepop commented Jan 7, 2022

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

juntyr commented Jan 22, 2022

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

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

dbofmmbt commented Jan 22, 2022

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

@juntyr
Copy link
Contributor

juntyr commented Jan 22, 2022

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

juntyr commented Jan 22, 2022

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

dbofmmbt commented Jan 22, 2022

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

juntyr commented Jan 22, 2022

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

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

juntyr commented Jan 23, 2022

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

quark-zju commented Jun 29, 2022

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

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

@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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 20, 2022
…inators, r=scottmcm

Add map_continue and continue_value combinators to ControlFlow

As suggested in this comment: rust-lang#75744 (comment)

Related tracking issue: rust-lang#75744

r? `@scottmcm`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 21, 2022
…inators, r=scottmcm

Add map_continue and continue_value combinators to ControlFlow

As suggested in this comment: rust-lang#75744 (comment)

Related tracking issue: rust-lang#75744

r? ``@scottmcm``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 21, 2022
…inators, r=scottmcm

Add map_continue and continue_value combinators to ControlFlow

As suggested in this comment: rust-lang#75744 (comment)

Related tracking issue: rust-lang#75744

r? ```@scottmcm```
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 21, 2022
…inators, r=scottmcm

Add map_continue and continue_value combinators to ControlFlow

As suggested in this comment: rust-lang#75744 (comment)

Related tracking issue: rust-lang#75744

r? ````@scottmcm````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 21, 2022
…inators, r=scottmcm

Add map_continue and continue_value combinators to ControlFlow

As suggested in this comment: rust-lang#75744 (comment)

Related tracking issue: rust-lang#75744

r? `````@scottmcm`````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 21, 2022
…inators, r=scottmcm

Add map_continue and continue_value combinators to ControlFlow

As suggested in this comment: rust-lang#75744 (comment)

Related tracking issue: rust-lang#75744

r? ``````@scottmcm``````
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
…r=scottmcm

Add map_continue and continue_value combinators to ControlFlow

As suggested in this comment: rust-lang/rust#75744 (comment)

Related tracking issue: rust-lang/rust#75744

r? ``````@scottmcm``````
@ModProg
Copy link
Contributor

ModProg commented Dec 10, 2022

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

yoshuawuyts commented Mar 10, 2023

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

  • ControlFlow::map_continue -> ControlFlow::map
  • ControlFlow::map_break -> ControlFlow::map_break (no change)

@schuelermine
Copy link
Contributor

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

@LeoniePhiline
Copy link

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

schuelermine commented Sep 27, 2023

They cannot have those names because they are strict keywords.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-control-flow Area: Relating to control flow A-iterators Area: Iterators C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests