Implement iter::Sum and iter::Product for Result

[shepmaster]

This introduces a private iterator adapter ResultShunt, which allows
treating an iterator of Result<T, E> as an iterator of T.

[rust-highfive]

r? @aturon

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

[shepmaster]

/cc @bluss

[shepmaster]

I'm opening this PR to see if we think it's small enough to merge, wants an RFC, or doesn't belong at all. If we think it should be merged, I can create a stability tracking issue and add the appropriate tests added.

This allows code like

use std::io;
use std::io::prelude::*;

fn main() {
    let stdin = io::stdin();
    let sum: Result<usize, _> = stdin.lock().lines().map(|l| {
        l.map(|l| l.len())
    }).sum();
    println!("{:?}", sum);
}

Likewise, I can add an implementation for Product added.

[aturon]

Ooh, neat!

Trait impls like this are instantly stable, regardless of the stability attribute, unfortunately. So we'll need libs team consensus to move forward. This doesn't require an RFC, though.

Libs team: please signal agreement/disagreement to this idea in principle. I'll work with @shepmaster to get the PR itself in shape if we want to do this.

@rfcbot fcp merge

[rfcbot]

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

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @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.

[Mark-Simulacrum]

This feels somewhat similar to the trait @eddyb and I added to rustc::ty::context, which allows transparently collecting iterators of Result<T, E> or T into a R or Result<R, E> (i.e., it allows passing a function similar to map to convert the value). I'm not sure if that's really relevant, but thought I'd mention it at least in passing since it felt similar.

I feel like it would be nice to make the reconstruction step obvious. That is, make let sum = shunt.by_ref().sum(); happen within a closure, for example, the sum impl would be rewritten like below. I guess the ergonomics of using it are slightly worse, but I feel like the API is somewhat safer, since you cannot lose the error accidentally.

impl<T, U, E> Sum<Result<U, E>> for Result<T, E>
    where T: Sum<U>,
{
    fn sum<I>(iter: I) -> Result<T, E>
        where I: Iterator<Item = Result<U, E>>,
    {
        ResultShunt::process(iter, |iter| iter.sum())
    }
}

[sfackler]

ResultShunt is a private type, so there shouldn't ever be any uses of it other than this one method I think.

[shepmaster]

ResultShunt is private, but it's a pattern that I think I'll make strong use of. I CCed @bluss in the hopes that it can make its way into itertools (and maybe eventually be public in the standard library?). Making it less likely to misuse in general seems like a good thing to me.

[sfackler]

(See also https://crates.io/crates/fallible-iterator for all of your fallible iteration needs)

[shepmaster]

I've added the suggested process method which also had the benefit of DRYing up the call sites. 👍

[shepmaster]

This feels somewhat similar to the trait @eddyb and I added to rustc::ty::context

I've now found and read InternAs / InternIteratorElement, and I agree that while in the same area, it seems distinct and solves a different problem. My brain can't fully brain if one is more powerful / generic than the other.

[eddyb]

They're all monads, basically. Except Haskell oversimplifies and Iterator is... a monadic type class? And I have no real suggestions on how to actually generically express such things, it all gives me headaches.

[eddyb]

On a more serious note, rust-lang/rfcs#1672 would go a long way making at least ad-hoc abstractions over T and Result<T, E> simpler (e.g. InternIteratorElement wouldn't be needed).

[shepmaster]

2 week gentle reminder for @brson

[brson]

Why are these tagged unstable? Isn't this an insta-stable implementation?

[brson]

I'm not sure how impls are usually tagged.

[shepmaster]

Isn't this an insta-stable implementation?

Yep! It was a misunderstanding of mine. As @aturon said:

Trait impls like this are instantly stable, regardless of the stability attribute, unfortunately. So we'll need libs team consensus to move forward. This doesn't require an RFC, though.

Libs team: please signal agreement/disagreement to this idea in principle. I'll work with @shepmaster to get the PR itself in shape if we want to do this.

So I was waiting for those checkboxes before removing that. 😸

[brson]

I guess this is fine by me, but it seems to be solving a specific case of a more general problem, that of doing something useful with collections of results. That is a problem I have a lot, but specifically summing results is not.

[shepmaster]

solving a specific case of a more general problem, that of doing something useful with collections of results

Absolutely! Although I feel like I'm usually dealing with iterators of Results. My secret hope is that ResultShunt (or some improved version) can help in these cases by allowing the "inner" iterator to not even know about the Result while still preserving the rich iterator-ness.

(And after typing all of that, the monad name-dropping from @eddyb seems painfully obvious in retrospect)

[shepmaster]

I'll work with @shepmaster to get the PR itself in shape if we want to do this.

@aturon what changes would you like to see done? The stability attributes need to be updated, certainly; what's appropriate to do there?

[rfcbot]

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

psst @aturon, I wasn't able to add the final-comment-period label, please do so.

[alexcrichton]

@shepmaster from my look I think the only thing that needs to be done here is update the stability attributes. Want to file an issue and cc me? I'll be sure to tag it and we can r+ this then

[aturon]

Yeah, I didn't have any specific concerns; land when ready!

[alexcrichton]

Oh actually sorry I forgot that impls are insta-stable, so we don't need the rigamarole with unstable issues at all! Want to just switch these tags to stable and we can land?

[shepmaster]

I forgot that impls are insta-stable

I was a bit confused why we were making a tracking issue, but figured I'd just go along and learn as I went. ^_^

[shepmaster]

these tags to stable and we can land?

What feature and version should I use for the stability attributes?

[alexcrichton]

I believe the master branch is 1.16.0

[shepmaster]

I believe the master branch is 1.16.0

Sounds good; I reused the iter_arith_traits feature; hope that's OK!

[alexcrichton]

Oh I think reusing an existing feature causes problems with make tidy, but it's ok to just manufacture a new feature out of thin air.

[shepmaster]

reusing an existing feature causes problems with make tidy, but it's ok to just manufacture a new feature out of thin air.

yup and yup!

[alexcrichton]

@bors: r+

[bors]

📌 Commit 23715d3 has been approved by alexcrichton

[bors]

⌛ Testing commit 23715d3 with merge a180dd7...

[bors]

💔 Test failed - status-travis

[alexcrichton]

@bors: retry * osx linker segfaulted #38878

…

On Tue, Jan 10, 2017 at 1:15 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/190731132> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38580 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95HQcEzJ-EyBOa0x6ymOLtLSVwRbVks5rQ_T5gaJpZM4LVCuE> .

[bors]

⌛ Testing commit 23715d3 with merge 0500fbf...

[bors]

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