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

How to use try_fold in place of fold_while? #469

Closed
NoraCodes opened this issue Aug 17, 2020 · 12 comments
Closed

How to use try_fold in place of fold_while? #469

NoraCodes opened this issue Aug 17, 2020 · 12 comments

Comments

@NoraCodes
Copy link

fold_while has been deprecated in favor of try_fold, and will eventually be removed ( #223 ). However, there is little documentation on how to actually do this in an idiomatic way.

For example, this playground comparison is the best I could do and it looks awful. The iterator finishing early isn't an error - it's a totally acceptable condition.

I'm sure there's a better way to do this, but it would be nice to document that in fold_while or, perhaps, in try_fold (I'll be happy to open a MR for that if someone can give me a good example.)

@scottmcm
Copy link
Contributor

It's true that a different impl Try makes this much nicer -- I added one to libcore that's used internally to make things like this more readable:

https://github.com/rust-lang/rust/blob/515c9fa505e18a65d7f61bc3e9eb833b79a68618/library/core/src/iter/mod.rs#L373-L378

@NoraCodes
Copy link
Author

Oh yeah, that's much nicer! Would we be amenable to including such a construct in itertools since it's such a common case?

@NoraCodes
Copy link
Author

Actually, never mind, that's silly.

@jswrenn
Copy link
Member

jswrenn commented Aug 20, 2020

@NoraCodes I'll hold off on removing fold_while for the foreseeable future, at least until a more ergonomic alternative is available.

@NoraCodes
Copy link
Author

Thank you 🙏

@jswrenn
Copy link
Member

jswrenn commented Aug 21, 2020

This feels like an abuse of import renaming, but...

fn main() {
    use Result::{
        self as LoopState,
        Ok as Continue,
        Err as Break,
    };

    println!("{}", (1..10).try_fold(0, |mut acc, v| {
        acc += v;
        if (acc >= 6) {
            Break(acc)
        } else {
            Continue(acc)
        }
    }).unwrap_or_else(|v| v));
}

I think this makes for a good demonstration of how to translate from fold_while to try_fold.

@NoraCodes
Copy link
Author

I actually just committed almost that exact change to our codebase. It works great, except that .unwrap_or_else(|x| x) is a little janky. But it's totally workable until something else comes along.

@jswrenn
Copy link
Member

jswrenn commented Aug 21, 2020

except that .unwrap_or_else(|x| x) is a little janky.

Agreed. There's this old PR introducing Result::into_inner for situations like this, but it was unfortunately rejected: rust-lang/rust#54219

jswrenn added a commit to jswrenn/rust-itertools that referenced this issue Sep 4, 2020
@jswrenn
Copy link
Member

jswrenn commented Sep 5, 2020

fold_while will be un-deprecated (and slightly optimized!) our next release (0.10.0) with #476.

@NoraCodes
Copy link
Author

NoraCodes commented Sep 5, 2020 via email

bors bot added a commit that referenced this issue Sep 22, 2020
475: Trait impls for tuples of arity <= 12 r=jswrenn a=jswrenn

I stopped at 12, because that's where libcore stops.

fixes #428
fixes #398

476: Undeprecate and optimize `fold_while` r=jswrenn a=jswrenn

Prompted by #469.

479: fix compiler warning on array.into_iter() r=jswrenn a=dmitris

Fix the compile warnings listed below by changing `into_iter()` invocation to `iter()` 
in the `impl_zip_iter` macro as recommended in rust-lang/rust#66145.
For additional background, see also rust-lang/rust#65819 and rust-lang/rust#66017 (the latter is the linter change producing the warning).

```
$ cargo build
   Compiling itertools v0.9.0 (/Users/dsavints/dev/hack/github.com/rust-itertools/itertools)
warning: this method call currently resolves to `<&[T; N] as IntoIterator>::into_iter` (due to autoref coercions), but that might change in the future when `IntoIterator` impls for arrays are added.
   --> src/ziptuple.rs:111:47
    |
111 |                 let size = *[$( $B.len(), )*].into_iter().min().unwrap();
    |                                               ^^^^^^^^^ help: use `.iter()` instead of `.into_iter()` to avoid ambiguity: `iter`
...
128 | impl_zip_iter!(A);
    | ------------------ in this macro invocation
    |
    = note: `#[warn(array_into_iter)]` on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #66145 <rust-lang/rust#66145>
    = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: this method call currently resolves to `<&[T; N] as IntoIterator>::into_iter` (due to autoref coercions), but that might change in the future when `IntoIterator` impls for arrays are added.
   --> src/ziptuple.rs:111:47
    |
111 |                 let size = *[$( $B.len(), )*].into_iter().min().unwrap();
    |                                               ^^^^^^^^^ help: use `.iter()` instead of `.into_iter()` to avoid ambiguity: `iter`
...
129 | impl_zip_iter!(A, B);
    | --------------------- in this macro invocation
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #66145 <rust-lang/rust#66145>
    = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
```

Co-authored-by: Jack Wrenn <me@jswrenn.com>
Co-authored-by: Dmitry Savintsev <dsavints@verizonmedia.com>
@scottmcm
Copy link
Contributor

scottmcm commented Feb 7, 2021

...and I now have rust-lang/rfcs#3058 open to start moving ControlFlow towards stabilization.

@scottmcm
Copy link
Contributor

Stabilization is now approved, but it'll take until September 9th to reach stable, so there's still a few months before any more work can happen here.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 15, 2021
…m-basics, r=m-ou-se

Stabilize `ops::ControlFlow` (just the type)

Tracking issue: rust-lang#75744 (which also tracks items *not* closed by this PR).

With the new `?` desugar implemented, [it's no longer possible to mix `Result` and `ControlFlow`](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=13feec97f5c96a9d791d97f7de2d49a6).  (At the time of making this PR, godbolt was still on the 2021-05-01 nightly, where you can see that [the mixing example compiled](https://rust.godbolt.org/z/13Ke54j16).)  That resolves the only blocker I know of, so I'd like to propose that `ControlFlow` be considered for stabilization.

Its basic existence was part of rust-lang/rfcs#3058, where it got a bunch of positive comments (examples [1](rust-lang/rfcs#3058 (comment)) [2](rust-lang/rfcs#3058 (review)) [3](rust-lang/rfcs#3058 (comment)) [4](rust-lang/rfcs#3058 (comment))).  Its use in the compiler has been well received (rust-lang#78182 (comment)), and there are ecosystem updates interested in using it (rust-itertools/itertools#469 (comment), jonhoo/rust-imap#194).

As this will need an FCP, picking a libs member manually:
r? `@m-ou-se`

## Stabilized APIs

```rust
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum ControlFlow<B, C = ()> {
    /// Exit the operation without running subsequent phases.
    Break(B),
    /// Move on to the next phase of the operation as normal.
    Continue(C),
}
```

As well as using `?` on a `ControlFlow<B, _>` in a function returning `ControlFlow<B, _>`.  (Note, in particular, that there's no `From::from`-conversion on the `Break` value, the way there is for `Err`s.)

## Existing APIs *not* stabilized here

All the associated methods and constants: `break_value`, `is_continue`, `map_break`, [`CONTINUE`](https://doc.rust-lang.org/nightly/std/ops/enum.ControlFlow.html#associatedconstant.CONTINUE), etc.

Some of the existing methods in nightly seem reasonable, some seem like they should be removed, and some need more discussion to decide.  But none of them are *essential*, so [as in the RFC](https://rust-lang.github.io/rfcs/3058-try-trait-v2.html#methods-on-controlflow), they're all omitted from this PR.

They can be considered separately later, as further usage demonstrates which are important.
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

No branches or pull requests

3 participants