Add Iterator::checked_{sum, product}

[aDotInTheVoid]

Adds a checked version of sum and product to iterators,

Based on RustCrypto/formats#558 cc @tarcieri

Docs:

Tracking issue: #95484

[rust-highfive]

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

[scottmcm]

This isn't something I feel comfortable approving without extra @rust-lang/libs-api oversight.

The code is perfectly fine, but I don't know if core wants it.

• core is generally trait-averse, and this adds two more.
• It arguably adds 4 more traits, since if CheckedSum and CheckedProduct exist, I think SaturatingSum and SaturatingProduct would also be implied as desirable. (Or is it 6? Would there be WrappingSum too?)
• I think this would be the first place with a trait in core for checked_* arithmetic. (Please correct me if I'm forgetting something.)
• Just writing .try_fold(0, i32::checked_add) to do this isn't terrible. (or .try_reduce(i32::checked_add).flatten(), or ...)
• There are other potential designs here, like now that we have num::Saturating (as unstable) in addition to num::Wrapping, we could also add num::Checked and spell .checked_sum() as .map(Checked).sum(), with an impl Sum for Checked<i32>, instead of an extra trait.

So that means
@rustbot team

[tarcieri]

My 2c as my code was cited as the inspiration for this:

I think this would be the first place with a trait in core for checked_* arithmetic. (Please correct me if I'm forgetting something.)

I think the general problem here is that there aren't any abstractions for checked arithmetic whatsoever. There's not a Checked wrapper ala Wrapping, nor are there CheckedAdd/CheckedSub/CheckedMul/etc arithmetic ops traits.
So really anything in this area which can be used for building abstractions around checked arithmetic would be great.

I know there have been various proposals for those, however the API surface for those is comparatively large. This seems like something small which could provide a lot of utility for checked arithmetic users.

For anyone trying to write code which disallows clippy::integer_arithmetic, particularly code which is trying to do checked addition over something like a large number of length operands, I think something like this could be a very powerful win for succinctness.

Just writing .try_fold(0, i32::checked_add) to do this isn't terrible. (or .try_reduce(i32::checked_add).flatten(), or ...)

The reason I added RustCrypto/formats#558 was I found myself writing a slightly more verbose form of the code above often enough (across many types which are performing checked length computations) that I wanted something with less verbosity and more semantic clarity.

There are other potential designs here, like now that we have num::Saturating (as unstable) in addition to num::Wrapping, we could also add num::Checked

I would really love num::Checked! But personally if I had to pick one of num::Checked or CheckedSum, I would actually pick CheckedSum, because these sorts of checked sums for field lengths are definitely my foremost use case for checked arithmetic.

It arguably adds 4 more traits, since if CheckedSum and CheckedProduct exist, I think SaturatingSum and SaturatingProduct would also be implied as desirable. (Or is it 6? Would there be WrappingSum too?)

I think WrappingSum and WrappingProduct would be quite useful. I mentioned CheckedSum is quite useful, I'm not sure about CheckedProduct but it seems good for completeness. Personally I have a lot more use cases for WrappingProduct than there are CheckedProduct.

I just read a pretty interesting C++-focused blog post about whether should saturating be a function, a type, or both and my personal reaction to it was while Wrapping is great and I sure wish there were a corresponding Checked, and while I do use saturating arithmetic via functions, I personally haven't encountered a good "always saturating" arithmetic use case, nor one where I want to perform a saturating operation over an iterator.

Perhaps there are use cases I'm not familiar with, though.

[yaahc]

For anyone trying to write code which disallows clippy::integer_arithmetic, particularly code which is trying to do checked addition over something like a large number of length operands, I think something like this could be a very powerful win for succinctness.

Would you be interested in trialing this in your codebase and providing feedback about the experience? I don't want to start spreading FUD because I've not participated much in the threads around these wrapper types, and what I have seen I only remember poorly, but I recall there being usability issues with some of these wrappers. Hopefully someone else on the team remembers what I'm alluding to and can link the threads in question but if not it would probably be worth digging through the history for Wrapping, Saturating, and the NonZero<T>/NonZero* types to find what issues people have had in the past and make sure we're doing holistic design here.

[tarcieri]

@yaahc this PR actually implements a pattern I was already using organically: RustCrypto/formats#558

Regarding a Checked-like newtype, I already use one here: https://github.com/RustCrypto/formats/blob/master/der/src/length.rs

The main drawback is having every method be fallible and using Result almost everywhere. But that's pretty much a given if you're trying to write panic-free code.

[yaahc]

Regarding a Checked-like newtype, I already use one here: https://github.com/RustCrypto/formats/blob/master/der/src/length.rs

I don't think the issue was Checked specifically, or even something like it, I feel like I vaguely remember something about type inference and just general ergonomics issues with NonZero<T> specifically, or maybe some other non Try type related nonsense from Wrapping/Saturating. Not sure, don't consider this a blocking concern, just some due diligence / research I'd like to see done / do later myself.

[tarcieri]

Yeah, I think the reason something like Checked hasn't happened yet is because it has ergonomics concerns and a somewhat complicated API surface.

That's why I'm somewhat excited by this PR in comparison: the API surface and implementation are small, and speaking from experience I think the ergonomics are fantastic and far superior to any other approach I've tried, especially for the multi-field-checked-length-calculation use case.

I think there's merit to solving the same problem with a Checked-like type as well, since it permits a natural infix syntax, but does leave you juggling Option/Result at the end of each operation, which is more annoying for the multi-operand arithmetic operation case (but succinctly expressible via try_fold, at a slight cost to readability).

Really I'd be happy with either approach.

[scottmcm]

I think the general problem here is that there aren't any abstractions for checked arithmetic whatsoever.

That's certainly true in core, but one can always use https://docs.rs/num-traits/latest/num_traits/ops/checked/trait.CheckedAdd.html#tymethod.checked_add and friends if needed.

For example, you could toss together something like

fn checked_sum<T: CheckedAdd + Zero>(a: impl AsRef<[T]>) -> Option<T> {
    a.as_ref().iter().try_fold(T::zero(), |a, b| a.checked_add(b))
}

Those traits would also let you write the extension trait in your own code as just one thing (rather than one per type), calling the try_fold in the implemention.

Or on the far-off horizon, it could be variadic instead of taking an array/slice.

I think something like this could be a very powerful win for succinctness.

There's what I would consider a fairly substantial succinctness difference between them, because yours is on IntoIterator while here it's just on Iterator. I can totally see doing [a, b].checked_sum() even for the binary case if the general pattern is common in the code. But once it's [a, b].into_iter().checked_sum(), I start to feel much less good about it.

Indeed, for optimal succinctness it'd be something like this (which doesn't even need num-traits):

macro_rules! checked_sum {
    ( $first:expr $(, $rest:expr )* $(,)? ) => {
        [$($rest),*].into_iter().try_fold($first, |a, b| a.checked_add(b))
    }
}

Or in your code it could have a much shorter name, since checked is so much more common.

because these sorts of checked sums for field lengths are definitely my foremost use case for checked arithmetic

I think that pushes me even more in the "it feels weird that it's on Iterator then" direction. One thing it reminds me of is that core's simd now has reduce_sum.

Now, you don't want to use that specifically (it only works for power-of-two length right now, it's not checked, and simd in most chips is actually pretty bad at cross-lane reductions like this) but it makes me wonder if some of these things existing on arrays too (not just simd) might be reasonable.

I feel like I vaguely remember something about type inference and just general ergonomics issues

Literals are definitely a pain today. But someone's looking at them and got NonZero* literals working (https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/.7Binteger.7D.20inference.20location/near/277237639), so we may well have a reasonable path to Wrapping and such literals too.

[dtolnay]

I think my preference would be for checked_sum::<i32>() to instead be written as one of the following:

• try_fold(0, i32::checked_add)
• sum::<Option<Checked<i32>>>()
• map(Checked).sum::<Option<i32>>()

where the second implies adding impl Sum<i32> for Option<Checked<i32>>, and the third implies impl Sum<Checked<i32>> for Option<i32>.

Doing this by composing the existing generic sum and the wrapper types Wrapping/Saturating/Checked (which exist for other reasons anyway) avoids adding an explosion of rarely used iterator methods and traits, checked_sum, checked_product, wrapping_sum, wrapping_product, saturating_sum, saturating_product. The Iterator trait is already distastefully enormous.

The docs of sum could of course be updated with an example of how to get the checked behavior at the same time that those Sum impls are added.

[scottmcm]

This was discussed in this week's libs-api meeting. The takeaway was to not add this,

because this adds a new traits and extends the Iterator interface.

So I'm going to close this specific PR, following that.

However, there was some interest in having something that could do it through sum/product, by adding extra impls since Sum is already generic. Thus you might consider a new PR for num::Checked, with an eye to having impl Sum<Checked<i32>> for Checked<i32> or similar in the future.