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

impl FromIterator<()> for () #45379

Merged
merged 1 commit into from
Nov 8, 2017
Merged

impl FromIterator<()> for () #45379

merged 1 commit into from
Nov 8, 2017

Conversation

cuviper
Copy link
Member

@cuviper 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());

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

r? @alexcrichton

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

@cuviper
Copy link
Member Author

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

cuviper commented Oct 19, 2017

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

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 19, 2017
@alexcrichton
Copy link
Member

@rfcbot fcp merge

Seems like a nifty idea to me!

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

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 19, 2017
@oli-obk
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
Copy link
Member Author

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

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

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

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

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

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

cuviper commented Oct 25, 2017

Nice, that's very thorough!

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

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

rfcbot commented Nov 7, 2017

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 7, 2017
@alexcrichton
Copy link
Member

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

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

Ok!

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 7, 2017

📌 Commit 68d05b2 has been approved by alexcrichton

@bors
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
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
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
@kennytm kennytm added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 15, 2017
bors added a commit that referenced this pull request Nov 17, 2017
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` (:tada: #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
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
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
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 unit_from_iter branch January 26, 2018 21:28
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 19, 2018
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
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
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants