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

impl FromIterator<()> for () #45379

Merged
merged 1 commit into from Nov 8, 2017

Conversation

Projects
None yet
9 participants
@cuviper
Copy link
Member

cuviper commented Oct 19, 2017

This just collapses all unit items from an iterator into one. This is
more useful when combined with higher-level abstractions, like
collecting to a Result<(), E> where you only care about errors:

use std::io::*;
data = vec![1, 2, 3, 4, 5];
let res: Result<()> = data.iter()
    .map(|x| writeln!(stdout(), "{}", x))
    .collect();
assert!(res.is_ok());
impl FromIterator<()> for ()
This just collapses all unit items from an iterator into one.  This is
more useful when combined with higher-level abstractions, like
collecting to a `Result<(), E>` where you only care about errors:

```rust
use std::io::*;
data = vec![1, 2, 3, 4, 5];
let res: Result<()> = data.iter()
    .map(|x| writeln!(stdout(), "{}", x))
    .collect();
assert!(res.is_ok());
```
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 19, 2017

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Oct 19, 2017

I also thought about letting this consume any Item type, like a >/dev/null analogy. But I think that may be too surprising if someone tries to collect real items when accidentally in a context that infers () type, as this is often implicit.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Oct 19, 2017

This is related to #44546 too, whether or not it might take any Item.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 19, 2017

@rfcbot fcp merge

Seems like a nifty idea to me!

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Oct 19, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Oct 20, 2017

I consider this an antipattern. I mean we have for_each so noone has to write map(...).count() or other weird combinations. I see the use of this change for generic code, but regular code should be using for_each. Together with this change we can change for_each backwards compatibly to

    fn for_each<F, R, R2>(self, f: F) -> R where
        Self: Sized,
        F: FnMut(Self::Item) -> R2, 
        R: FromIterator<R2>,
    {
        self.map(f).collect()
    }

Both changes together I can get behind. But it feels inconsistent if we consider code idiomatic that does map + collect and throws away a unit value, but not idiomatic if it's map + count and throwing away the count.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Oct 20, 2017

@oli-obk On it's own, I would agree this is pretty useless, and I would encourage using for_each more, or to continue using map(_).collect() for non-unit collections. I only like this new impl for cases that specifically need FromIterator, like the example with Result's FromIterator.

But it feels inconsistent if we consider code idiomatic that does map + collect and throws away a unit value, but not idiomatic if it's map + count and throwing away the count.

I would not encourage map + collect to a unit, but even so throwing away a unit is a utterly trivial -- it's nothing, and costs nothing to create. A count requires actual computation, and hopefully the optimizer will prune that if you throw it away, but otherwise it's wasted work.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Oct 21, 2017

This feels like a fold to me, not a collect. Perhaps this needs a different method?

If we had try_fold (see fold_ok internal iteration thread), say, then it'd just be .try_fold((), |(),x| x) (play demo), which I find much more obvious, though of course it's somewhat longer.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Oct 21, 2017

try_fold does look more general -- it could even write the example without map:

let res: Result<()> = data.iter()
    .try_fold((), |(),x| writeln!(stdout(), "{}", x));
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 21, 2017

IntoIterator can be implemented for tuples of other IntoIterators - rust-lang/rfcs#930 (which would be a more useful extension, IMO) and there's a blanket impl of FromIterator for IntoIterators.
Any chances that impls from this PR and rust-lang/rfcs#930 will conflict or behave inconsistently?

EDIT: Disregard this, I shouldn't review in hurry.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Oct 21, 2017

there's a blanket impl of FromIterator for IntoIterators

I'm not sure what you're referring to. There's a blanket impl of IntoIterator for any Iterator, but FromIterator is on the consumption side.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Oct 25, 2017

@scottmcm Have you proposed try_fold in a PR? As a possible enticement, I've also prototyped similar methods for rayon. (it's a bit more involved... 😅)

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Oct 25, 2017

@cuviper I'm working on it 😄 (Got ahead of myself and broke something bad enough that the tests won't tell me what, though, so I need to split it up into some simpler changes. But I'm not in too much of a rush, because it'll only be full goodness once #45225 fixes #43278.)

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Oct 25, 2017

Nice, that's very thorough!

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Oct 29, 2017

Well, the try_fold PR is up: #45595

Looking at cuviper's example again, that's of course the same pattern as for_each in terms of fold. That coupled with oli-obk's mention of for_each makes me wonder if this instead ought to be

fn try_for_each<F, R>(&mut self, mut f: F)
    where Self: Sized, F: FnMut(Self::Item) -> R, R: Try<Ok=()>
{
    self.try_fold((), move |(), x| f(x))
}

(With, like for_each, no need for an r version thanks to #44682-style forwarding.)

Edit: That pattern ((), |(),x|) is also how my PR implements all and any, so having try_for_each would simplify them as well, further making me think it would be a useful addition.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Nov 2, 2017

@scottmcm you're shaving away all the motivation here... and that's OK! 😄

Are there any other higher-level abstractions that would still be useful with this FromIterator? I thought perhaps unzip, but that actually uses Extend, and I can't think of a good motivation for that anyway.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 7, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Nov 7, 2017

So at this point the libs team have all signed off on merging this, but @cuviper just to confirm, you're still ok with merging this?

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Nov 7, 2017

I'm still OK with this. I like the generality of the Try-based stuff, but I think there's still a basic niche occupied here as a building block.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Nov 7, 2017

Ok!

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 7, 2017

📌 Commit 68d05b2 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 8, 2017

⌛️ Testing commit 68d05b2 with merge e177df3...

bors added a commit that referenced this pull request Nov 8, 2017

Auto merge of #45379 - cuviper:unit_from_iter, r=alexcrichton
impl FromIterator<()> for ()

This just collapses all unit items from an iterator into one.  This is
more useful when combined with higher-level abstractions, like
collecting to a `Result<(), E>` where you only care about errors:

```rust
use std::io::*;
data = vec![1, 2, 3, 4, 5];
let res: Result<()> = data.iter()
    .map(|x| writeln!(stdout(), "{}", x))
    .collect();
assert!(res.is_ok());
```
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 8, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e177df3 to master...

@bors bors merged commit 68d05b2 into rust-lang:master Nov 8, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

bors added a commit that referenced this pull request Nov 8, 2017

Auto merge of #45595 - scottmcm:iter-try-fold, r=dtolnay
Short-circuiting internal iteration with Iterator::try_fold & try_rfold

These are the core methods in terms of which the other methods (`fold`, `all`, `any`, `find`, `position`, `nth`, ...) can be implemented, allowing Iterator implementors to get the full goodness of internal iteration by only overriding one method (per direction).

Based off the `Try` trait, so works with both `Result` and `Option` (🎉 #42526).  The `try_fold` rustdoc examples use `Option` and the `try_rfold` ones use `Result`.

AKA continuing in the vein of PRs #44682 & #44856 for more of `Iterator`.

New bench following the pattern from the latter of those:
```
test iter::bench_take_while_chain_ref_sum          ... bench:   1,130,843 ns/iter (+/- 25,110)
test iter::bench_take_while_chain_sum              ... bench:     362,530 ns/iter (+/- 391)
```

I also ran the benches without the `fold` & `rfold` overrides to test their new default impls, with basically no change.  I left them there, though, to take advantage of existing overrides and because `AlwaysOk` has some sub-optimality due to #43278 (which 45225 should fix).

If you're wondering why there are three type parameters, see issue #45462

Thanks for @bluss for the [original IRLO thread](https://internals.rust-lang.org/t/pre-rfc-fold-ok-is-composable-internal-iteration/4434) and the rfold PR and to @cuviper for adding so many folds, [encouraging me](#45379 (comment)) to make this PR, and finding a catastrophic bug in a pre-review.

@kennytm kennytm added the relnotes label Nov 15, 2017

bors added a commit that referenced this pull request Nov 17, 2017

Auto merge of #45595 - scottmcm:iter-try-fold, r=dtolnay
Short-circuiting internal iteration with Iterator::try_fold & try_rfold

These are the core methods in terms of which the other methods (`fold`, `all`, `any`, `find`, `position`, `nth`, ...) can be implemented, allowing Iterator implementors to get the full goodness of internal iteration by only overriding one method (per direction).

Based off the `Try` trait, so works with both `Result` and `Option` (🎉 #42526).  The `try_fold` rustdoc examples use `Option` and the `try_rfold` ones use `Result`.

AKA continuing in the vein of PRs #44682 & #44856 for more of `Iterator`.

New bench following the pattern from the latter of those:
```
test iter::bench_take_while_chain_ref_sum          ... bench:   1,130,843 ns/iter (+/- 25,110)
test iter::bench_take_while_chain_sum              ... bench:     362,530 ns/iter (+/- 391)
```

I also ran the benches without the `fold` & `rfold` overrides to test their new default impls, with basically no change.  I left them there, though, to take advantage of existing overrides and because `AlwaysOk` has some sub-optimality due to #43278 (which 45225 should fix).

If you're wondering why there are three type parameters, see issue #45462

Thanks for @bluss for the [original IRLO thread](https://internals.rust-lang.org/t/pre-rfc-fold-ok-is-composable-internal-iteration/4434) and the rfold PR and to @cuviper for adding so many folds, [encouraging me](#45379 (comment)) to make this PR, and finding a catastrophic bug in a pre-review.

bors bot added a commit to rayon-rs/rayon that referenced this pull request Jan 14, 2018

Merge #497 #498
497: impl FromParallelIterator<()> for () r=cuviper a=cuviper

This is more useful when combined with higher-level abstractions, like
collecting to a `Result<(), E>` where you only care about errors.

This is a parallel version of rust-lang/rust#45379.
Cc #496

498: FromParallelIterator and ParallelExtend Cow for String r=cuviper a=cuviper

Parallel version of rust-lang/rust#41449.

bors bot added a commit to rayon-rs/rayon that referenced this pull request Jan 14, 2018

Merge #497
497: impl FromParallelIterator<()> for () r=cuviper a=cuviper

This is more useful when combined with higher-level abstractions, like
collecting to a `Result<(), E>` where you only care about errors.

This is a parallel version of rust-lang/rust#45379.
Cc #496

bors bot added a commit to rayon-rs/rayon that referenced this pull request Jan 14, 2018

Merge #497
497: impl FromParallelIterator<()> for () r=cuviper a=cuviper

This is more useful when combined with higher-level abstractions, like
collecting to a `Result<(), E>` where you only care about errors.

This is a parallel version of rust-lang/rust#45379.
Cc #496

@cuviper cuviper deleted the cuviper:unit_from_iter branch Jan 26, 2018

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 19, 2018

Rollup merge of rust-lang#48157 - scottmcm:try-for-each, r=dtolnay
Add Iterator::try_for_each

The fallible version of `for_each` aka the stateless version of `try_fold`.  Inspired by @cuviper's comment in rust-lang#45379 (comment) as a more direct and obvious solution than `.map(f).collect::<Result<(), _>>()`.

Like `for_each`, no need for an `r` version thanks to overrides in `Rev`.

`iterator_try_fold` tracking issue: rust-lang#45594

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 19, 2018

Rollup merge of rust-lang#48157 - scottmcm:try-for-each, r=dtolnay
Add Iterator::try_for_each

The fallible version of `for_each` aka the stateless version of `try_fold`.  Inspired by @cuviper's comment in rust-lang#45379 (comment) as a more direct and obvious solution than `.map(f).collect::<Result<(), _>>()`.

Like `for_each`, no need for an `r` version thanks to overrides in `Rev`.

`iterator_try_fold` tracking issue: rust-lang#45594

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 23, 2018

Rollup merge of rust-lang#48157 - scottmcm:try-for-each, r=dtolnay
Add Iterator::try_for_each

The fallible version of `for_each` aka the stateless version of `try_fold`.  Inspired by @cuviper's comment in rust-lang#45379 (comment) as a more direct and obvious solution than `.map(f).collect::<Result<(), _>>()`.

Like `for_each`, no need for an `r` version thanks to overrides in `Rev`.

`iterator_try_fold` tracking issue: rust-lang#45594
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment