impl FromIterator<()> for ()

[cuviper]

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

[rust-highfive]

r? @alexcrichton

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

[cuviper]

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 is related to #44546 too, whether or not it might take any Item.

[alexcrichton]

@rfcbot fcp merge

Seems like a nifty idea to me!

[rfcbot]

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

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @dtolnay
• @sfackler

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]

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]

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

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]

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]

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]

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

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

Nice, that's very thorough!

[scottmcm]

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]

@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 is now entering its final comment period, as per the review above. 🔔

[alexcrichton]

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]

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]

Ok!

@bors: r+

[bors]

📌 Commit 68d05b2 has been approved by alexcrichton

[bors]

⌛ Testing commit 68d05b2 with merge e177df3...

[bors]

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