Add Iterator::{sum_nonempty, product_nonempty} #50884

Emerentius · 2018-05-19T02:43:41Z

This implements {Sum, Product} for Option<T> such that it returns None if the iter is empty.
That allows one to distinguish between an empty iter and iters that sum/multiply to their neutral element.

There's an alternative way of summing into Option<T>, which doesn't require a neutral starting element and therefore no T: Sum / Product bound. It works by taking the first element and then adding / multiplying the rest onto that, but it can't accomodate the case of different types such as Iterator<Item = U> -> Option<T>. This PR keeps in line with the rest of the trait impls and therefore does require T: Sum.

This impl is for a stable trait and would therefore be insta-stable if merged.

clarfonthey · 2018-05-19T05:49:21Z

Invoking once(first).chain(iter) isn't a very good idea, IMHO. Also, I'm not sure if Option is the right type for this.

The way this should be done is through a separate iterator adaptor, like:

pub struct IteratedCheck<I> {
    iter: I,
    iterated: bool,
}
impl<I: Iterator> Iterator for I {
    type Item = <I as Iterator>::Item;
    #[inline]
    fn next(&mut self) -> Option<Self::Item> {
        self.iterated = true;
        self.iter.next()
    }
    // forward other methods too
}
impl<T, U> Sum<U> for Option<T>
where
    T: Sum<U>,
{
    #[inline]
    fn sum<I: Iterator<Item = U>>(iter: I) -> Option<T> {
        let mut adapter = IteratedCheck { iter, iterated: false };
        Some(adapter.by_ref().sum()).filter(adapter.iterated)
    }
}
// and product too

This allows you to avoid any performance burdens from chain.

Emerentius · 2018-05-19T15:27:52Z

src/libcore/iter/traits.rs

@@ -836,6 +837,52 @@ macro_rules! float_sum_product {
 integer_sum_product! { i8 i16 i32 i64 i128 isize u8 u16 u32 u64 u128 usize }
 float_sum_product! { f32 f64 }

+#[stable(feature = "iter_arith_traits_option", since="1.29.0")]


Heh, just planning ahead. I'm sure the decision process will delay this at least 4 weeks given that it's insta-stable.

kennytm · 2018-05-19T14:17:34Z

highfive failed to assign anybody again cc @rust-lang/infra.

r? @sfackler

pietroalbini · 2018-05-28T08:52:57Z

Ping from triage @sfackler! This PR needs your review.

sfackler · 2018-06-02T23:00:59Z

This seemed a bit weird to me at first, but I think seems reasonable.

@rfcbot fcp merge

rfcbot · 2018-06-02T23:00:59Z

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

clarfonthey · 2018-06-03T00:19:55Z

As I stated in my comment above, this should use an iterator adapter instead of using chain to avoid performance issues.

Emerentius · 2018-06-03T01:30:14Z

For completeness' sake I should mention that there is also an implementation of Sum<Result<U, E>> for Result<T, E> where T: Sum which short-circuits on errors. A corresponding impl that short-circuits on None would conflict with this one.

To get the same behaviour with Options, users could convert to Result:
.map(|opt| opt.ok_or(())).sum::<Result<_, ()>().ok()

or we impl the Sum on Option to mimic Result and users write what this PR implements and hope the Sum impl uses fold so the chain doesn't cost extra.
In some cases it would suffice to do:

let mut iter = some_iter();
iter.next().map(|first| first + iter.sum()); // requires U: Add<T>

@clarcharr
I'm waiting to see if the libs team accepts the semantics. I can make the necessary changes afterwards. I was thinking of sum like fold which optimizes that cost away but there's of course no guarantee users impl Sum through fold.

sfackler · 2018-06-03T03:06:42Z

Yeah, the implementation is separate from the impl itself.

bors · 2018-06-13T02:34:49Z

☔ The latest upstream changes (presumably #50941) made this pull request unmergeable. Please resolve the merge conflicts.

scottmcm · 2018-06-13T04:39:26Z

Similarly to the analogy to Result, would this conflict with a hypothetical future impl Add<Option> for Option<T>? Like fold vs fold1, I personally would have expected the behaviour difference here to be denoted by a different function name, not just a different impl.

Edit: Not that the impl itself would directly conflict, but that it would imply a Sum that I don't think could coexist with this one.

~~Also, with None for empty, is Sum even needed to do this? .sum1() is just .fold1(Add::add), right? (No need to an additional trait for getting the additive identity element...)~~

Emerentius · 2018-06-13T06:46:05Z

(clarification from irc: scottmcm is asking about a potential impl such that one could sum or product for types implementing Add or Mul)
I've mentioned this in the OP, it is possible to impl {Sum, Product}<T> for Option<T> where T: {Add, Mul}. That implementation conflicts with this one.

It doesn't allow different types U and T because there are no traits other than {Sum, Product} that offer us a neutral starting element. An iterator of length 1 has nothing to add | multiply to the first element of type U to get T.
For the same reason, the lack of a starter element means that an Option<T>-like type is the only viable target for {Sum, Product}<T> where T: {Add, Mul}

rfcbot · 2018-06-22T17:50:31Z

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

scottmcm · 2018-06-22T18:28:05Z

I want to reiterate what @Emerentius said above:

For completeness' sake I should mention that there is also an implementation of Sum<Result<U, E>> for Result<T, E> where T: Sum which short-circuits on errors. A corresponding impl that short-circuits on None would conflict with this one.

I think that, with both Option and Result being usable with ?, it's surprising to have them behave so differently here.

Concretely, I propose taking the impls from here and putting them in a new sum1 method on Iterator instead. (Edit: And product1, of course.)

clarfonthey · 2018-06-22T19:18:51Z

I agree with choosing another method on Iterator. Bikeshedding on the name can happen after it's implemented, although I'd like to point out that 1-suffixed convention that Haskell uses probably would not be the best way to go about things, as a 1 is not easily noticeable at first glance. nonempty_sum might be better.

shepmaster · 2018-06-22T19:37:52Z

1-suffixed convention

I'm not in love with it, but Itertools does make use of it:

pietroalbini · 2018-07-09T10:53:39Z

Ping from triage @sfackler @Emerentius! How should we move forward with this PR?

Emerentius · 2018-07-19T00:53:59Z

Sorry, I forgot about this PR for a while.

I'll add the {sum1, product1} methods based on the {Sum, Product} traits.

@clarcharr The advantage of IteratedCheck is pretty small if there is one at all, because it can't just set the boolean to true if iteration has been attempted. It needs to check if at least 1 one of the items is Some.
That means, IteratedCheck::next will contain a check just like Chain::next does. The design of the iter arithmetic traits seems to make it impossible to avoid conditional execution when the implementing type does not use fold.

Edit: Below is the original, wrong text of this post I forgot that Sum it's supposed to be generic over a different type than the output type.

Thinking about it again, I don't see why `Sum for Option` and `Sum> for Option` couldn't co-exist. It would simply mean that an `Iterator>` could sum into either `Option` or `Option>`. Awesome for type inference (not), but otherwise [perfectly legal](http://play.rust-lang.org/?gist=233188ff5da98fc4bc360b29a953d4f6&version=stable&mode=debug&edition=2015).

Summing into Option<Option<T>> has the esoteric semantics where the outer Option shows nonemptyness of the iter, the inner is the result of a short-circuiting sum. I could see someone confused about their types trial-and-erroring themselves into this by mistake, but I don't think it's a big issue.

I believe having those two trait impls is much preferable to having a special method that still couldn't sum T: Add because of having to maintain consistency with the Sum trait.

rust-highfive · 2018-07-20T02:50:20Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:04:01] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:01] tidy error: /checkout/src/libcore/iter/iterator.rs:2288: line longer than 100 chars
[00:04:01] tidy error: /checkout/src/libcore/iter/iterator.rs:2343: line longer than 100 chars
[00:04:03] some tidy checks failed
[00:04:03] 
[00:04:03] 
[00:04:03] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:03] 
[00:04:03] 
[00:04:03] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:03] Build completed unsuccessfully in 0:00:53
[00:04:03] Build completed unsuccessfully in 0:00:53
[00:04:03] Makefile:79: recipe for target 'tidy' failed
[00:04:03] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1de96387
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:05fe7433:start=1532054533138858180,finish=1532054533144611462,duration=5753282
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0381d878
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2f931ef4
travis_time:start:2f931ef4
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:04610180
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rust-highfive · 2018-07-20T04:19:12Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:03:39] travis_fold:start:test_stage1-core
travis_time:start:test_stage1-core
Testing core stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:03:40]    Compiling core v0.0.0 (file:///checkout/src/libcore)
[01:03:47] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:03:47]      |
[01:03:47]      |
[01:03:47] 1041 |     assert_eq!(v[..0].iter().cloned().sum_nonempty::<i32>(), None);
[01:03:47]      |
[01:03:47]      |
[01:03:47]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:03:47] 
[01:03:47] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:03:47]      |
[01:03:47]      |
[01:03:47] 1042 |     assert_eq!(v[1..2].iter().cloned().sum_nonempty::<i32>(), Some(1));
[01:03:47]      |
[01:03:47]      |
[01:03:47]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:03:47] 
[01:03:47] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:03:47]      |
[01:03:47]      |
[01:03:47] 1043 |     assert_eq!(v[1..3].iter().cloned().sum_nonempty::<i32>(), Some(3));
[01:03:47]      |
[01:03:47]      |
[01:03:47]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:03:47] 
[01:03:47] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:03:47]      |
[01:03:47]      |
[01:03:47] 1044 |     assert_eq!(v.iter().cloned().sum_nonempty::<i32>(), Some(55));
[01:03:47]      |
[01:03:47]      |
[01:03:47]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:03:47] 
[01:03:47] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:03:47]      |
[01:03:47]      |
[01:03:47] 1066 |     assert_eq!(v[..0].iter().cloned().product_nonempty::<i32>(), None);
[01:03:47]      |
[01:03:47]      |
[01:03:47]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:03:47] 
[01:03:47] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:03:47]      |
[01:03:47]      |
[01:03:47] 1067 |     assert_eq!(v[..1].iter().cloned().product_nonempty::<i32>(), Some(0));
[01:03:47]      |
[01:03:47]      |
[01:03:47]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:03:47] 
[01:03:47] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:03:47]      |
[01:03:47]      |
[01:03:47] 1068 |     assert_eq!(v[1..3].iter().cloned().product_nonempty::<i32>(), Some(2));
[01:03:47]      |
[01:03:47]      |
[01:03:47]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:03:47] 
[01:03:47] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:03:47]      |
[01:03:47]      |
[01:03:47] 1069 |     assert_eq!(v[1..5].iter().cloned().product_nonempty::<i32>(), Some(24));
[01:03:47]      |
[01:03:47]      |
[01:03:47]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:03:54] error: aborting due to 8 previous errors
[01:03:54] 
[01:03:54] For more information about this error, try `rustc --explain E0658`.
[01:03:54] error: Could not compile `core`.
[01:03:54] error: Could not compile `core`.
[01:03:54] 
[01:03:54] To learn more, run the command again with --verbose.
[01:03:54] 
[01:03:54] 
[01:03:54] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--" "--quiet"
[01:03:54] 
[01:03:54] 
[01:03:54] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:03:54] Build completed unsuccessfully in 0:18:47
[01:03:54] Build completed unsuccessfully in 0:18:47
[01:03:54] Makefile:58: recipe for target 'check' failed
[01:03:54] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2b8506d5
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rust-highfive · 2018-07-20T14:24:34Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:06:51] travis_fold:start:test_stage1-core
travis_time:start:test_stage1-core
Testing core stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:06:52]    Compiling core v0.0.0 (file:///checkout/src/libcore)
[01:06:59] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:06:59]      |
[01:06:59]      |
[01:06:59] 1042 |     assert_eq!(v[..0].iter().cloned().sum_nonempty::<i32>(), None);
[01:06:59]      |
[01:06:59]      |
[01:06:59]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:06:59] 
[01:06:59] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:06:59]      |
[01:06:59]      |
[01:06:59] 1043 |     assert_eq!(v[1..2].iter().cloned().sum_nonempty::<i32>(), Some(1));
[01:06:59]      |
[01:06:59]      |
[01:06:59]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:06:59] 
[01:06:59] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:06:59]      |
[01:06:59]      |
[01:06:59] 1044 |     assert_eq!(v[1..3].iter().cloned().sum_nonempty::<i32>(), Some(3));
[01:06:59]      |
[01:06:59]      |
[01:06:59]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:06:59] 
[01:06:59] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:06:59]      |
[01:06:59]      |
[01:06:59] 1045 |     assert_eq!(v.iter().cloned().sum_nonempty::<i32>(), Some(55));
[01:06:59]      |
[01:06:59]      |
[01:06:59]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:06:59] 
[01:06:59] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:06:59]      |
[01:06:59]      |
[01:06:59] 1067 |     assert_eq!(v[..0].iter().cloned().product_nonempty::<i32>(), None);
[01:06:59]      |
[01:06:59]      |
[01:06:59]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:06:59] 
[01:06:59] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:06:59]      |
[01:06:59]      |
[01:06:59] 1068 |     assert_eq!(v[..1].iter().cloned().product_nonempty::<i32>(), Some(0));
[01:06:59]      |
[01:06:59]      |
[01:06:59]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:06:59] 
[01:06:59] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:06:59]      |
[01:06:59]      |
[01:06:59] 1069 |     assert_eq!(v[1..3].iter().cloned().product_nonempty::<i32>(), Some(2));
[01:06:59]      |
[01:06:59]      |
[01:06:59]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:06:59] 
[01:06:59] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[01:06:59]      |
[01:06:59]      |
[01:06:59] 1070 |     assert_eq!(v[1..5].iter().cloned().product_nonempty::<i32>(), Some(24));
[01:06:59]      |
[01:06:59]      |
[01:06:59]      = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[01:07:07] error: aborting due to 8 previous errors
[01:07:07] 
[01:07:07] For more information about this error, try `rustc --explain E0658`.
[01:07:07] error: Could not compile `core`.
[01:07:07] error: Could not compile `core`.
[01:07:07] 
[01:07:07] To learn more, run the command again with --verbose.
[01:07:07] 
[01:07:07] 
[01:07:07] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--" "--quiet"
[01:07:07] 
[01:07:07] 
[01:07:07] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:07:07] Build completed unsuccessfully in 0:19:35
[01:07:07] Build completed unsuccessfully in 0:19:35
[01:07:07] Makefile:58: recipe for target 'check' failed
[01:07:07] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:09651798
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
145348 ./obj/build/bootstrap/debug/incremental
133184 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
133180 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
130536 ./obj/build/bootstrap/debug/incremental/bootstrap-2fbxwhl9tnp02
130532 ./obj/build/bootstrap/debug/incremental/bootstrap-2fbxwhl9tnp02/s-f32xp7cfib-1qry09o-2iqfvo5raelnm
128732 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstroc
65412 ./src/llvm-emscripten/test/CodeGen
60840 ./src/llvm-emscripten/lib
58528 ./obj/build/x86_64-unknown-linux-gnu/stage2-tools
55236 ./src/llvm/test/MC

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Emerentius · 2018-07-20T15:52:20Z

hmm.. I thought I just did add that

rust-highfive · 2018-07-20T20:23:08Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:56:45] running 2098 tests
[00:56:52] ....................................................................................................
[00:56:59] ....................................................................................................
[00:57:07] ....................................................................................................
[00:57:15] ....F..............F...............i................................................................
[00:57:28] ....................................................................................................
[00:57:34] ....................................................................................................
[00:57:41] ....................................................................................................
[00:57:48] ....................................................................................................
---
[00:59:03] ....................................................................................................
<i32>();
[00:59:12]   |                        ^^^^^^^^^^^^
[00:59:12]   |
[00:59:12]   = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[00:59:12] 
[00:59:12] error[E0658]: use of unstable library feature 'nonempty_iter_arith': recently added unstable API
[00:59:12]  --> iter/iterator.rs:2285:29
[00:59:12]   |
[00:59:12] 7 | let nonempty_sum = (1..=10).sum_nonempty::<i32>();
[00:59:12]   |
[00:59:12]   |
[00:59:12]   = help: add #![feature(nonempty_iter_arith)] to the crate attributes to enable
[00:59:12] thread 'iter/iterator.rs - iter::iterator::Iterator::sum_nonempty (line 2281)' panicked at 'couldn't compile the test', librustdoc/test.rs:327:17
[00:59:12] 
[00:59:12] 
[00:59:12] failures:
---
[00:59:12] 
[00:59:12] error: test failed, to rerun pass '--doc'
[00:59:12] 
[00:59:12] 
[00:59:12] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--" "--quiet"
[00:59:12] 
[00:59:12] 
[00:59:12] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:59:12] Build completed unsuccessfully in 0:19:38
[00:59:12] Build completed unsuccessfully in 0:19:38
[00:59:12] make: *** [check] Error 1
[00:59:12] Makefile:58: recipe for target 'check' failed

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

These return Option<T>. Some(result) if the iter is nonempty, None otherwise. Allows differentiation between empty iterators and iterators that reduce to neutral elements.

alexcrichton · 2018-07-24T16:01:54Z

Thanks @Emerentius for the updates! I wonder if we can perhaps think of a different name other than *_nonempty though? That name implies to me sort of that it's somehow asserting it's nonempty, in that I'd expect the sum and sum_nonempty type signatures to be switched. Something like sum_or_none seems like a bit wordy though...

Emerentius · 2018-07-25T04:23:59Z

@alexcrichton I can see that if you read the nonempty as belonging to the output, but it's a requirement on the iterator. And because its nonemptyness can't be guaranteed, of course, sum_nonempty returns an option much like other methods in the same boat which don't have to add anything to their name to disambiguate (next, max, find, etc.).

I've looked at how other languages distinguish these fold- and fold1-like operations and came upon this handy list from wikipedia: folds in various languages

Almost all of them either don't have a fold1, avoid the naming issue entirely by overloading or use a synonym for fold (namely, reduce).

The *1 suffixing has weak precedence (Haskell) and the rest aren't applicable. I'm afraid there is no concise, immediately obvious name for this kind of operation.

Edit: nom also uses the fold1 terminology

shepmaster · 2018-07-25T13:10:00Z

The *1 suffixing has weak precedence (Haskell)
Edit: nom also uses the fold1 terminology

As does itertools.

alexcrichton · 2018-07-26T01:11:08Z

To make sure I understand, you're thinking we could add sum1 as a method which returns None if the iterator is empty?

Emerentius · 2018-07-26T01:18:03Z

Either sum1 or sum_nonempty is fine by me. I was trying to evade the bikeshed.

alexcrichton · 2018-07-26T23:45:49Z

Do others from @rust-lang/libs have thoughts on this? I personally sort of feel like this functionality belongs in itertools rather than the standard library, but if others feel it should stay we may need some help with naming!

scottmcm · 2018-07-27T00:59:50Z

Hmm, definitely feels hard to argue that core should have sum1 xor fold1.

TimNN · 2018-08-07T17:19:25Z

Ping from triage, @sfackler, @alexcrichton & @Emerentius. What is the status of this PR? It looks like there's been an FCP but there are still some open questions.

TimNN · 2018-08-14T16:20:27Z

Ping from triage again, @sfackler, @alexcrichton & @Emerentius!

Emerentius · 2018-08-14T17:58:20Z

Given that the reception is just lukewarm, it probably is best to put it in itertools for now.
There's also still the downside with the current design that T: {Add | Mul} don't work with it. Maybe we'll get the necessary features for overlapping trait impls in the future.

alexcrichton · 2018-08-14T22:49:08Z

That sounds reasonable to me! @Emerentius shall we close this PR in that case?

Emerentius · 2018-08-17T08:25:34Z

Opened rust-itertools/itertools#300.

This comment has been minimized.

Sign in to view

Emerentius force-pushed the iter_arith_traits_option branch from 1ab27ac to c1cb1ff Compare May 19, 2018 03:08

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

kennytm reviewed May 19, 2018

View reviewed changes

rust-highfive assigned sfackler May 19, 2018

kennytm added the relnotes Marks issues that should be documented in the release notes of the next release. label May 30, 2018

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 2, 2018

pietroalbini added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2018

kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 18, 2018

rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 22, 2018

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 22, 2018

Emerentius force-pushed the iter_arith_traits_option branch from c1cb1ff to 178f0dc Compare July 20, 2018 02:35

Emerentius changed the title ~~Implement iter::{Sum, Product} for Option<T>~~ Implement Iterator::{sum_nonempty, product_nonempty} Jul 20, 2018

Emerentius changed the title ~~Implement Iterator::{sum_nonempty, product_nonempty}~~ Add Iterator::{sum_nonempty, product_nonempty} Jul 20, 2018

Emerentius force-pushed the iter_arith_traits_option branch from 178f0dc to 7f3d0ca Compare July 20, 2018 03:12

Emerentius force-pushed the iter_arith_traits_option branch from 7f3d0ca to 77a91a6 Compare July 20, 2018 13:14

Emerentius force-pushed the iter_arith_traits_option branch from 77a91a6 to fba4e1c Compare July 20, 2018 19:20

Add Iterator::{sum_nonempty, product_nonempty}

87d5107

These return Option<T>. Some(result) if the iter is nonempty, None otherwise. Allows differentiation between empty iterators and iterators that reduce to neutral elements.

Emerentius force-pushed the iter_arith_traits_option branch from fba4e1c to 87d5107 Compare July 21, 2018 11:28

kennytm removed the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 24, 2018

Emerentius closed this Aug 17, 2018

scottmcm mentioned this pull request Mar 7, 2019

Implement iter::Sum and iter::Product for Option #58975

Merged

Add Iterator::{sum_nonempty, product_nonempty} #50884

Add Iterator::{sum_nonempty, product_nonempty} #50884

Conversation

Emerentius commented May 19, 2018 • edited Loading

This comment has been minimized.

clarfonthey commented May 19, 2018 • edited Loading

This comment was marked as outdated.

Emerentius May 19, 2018

Choose a reason for hiding this comment

kennytm commented May 19, 2018

pietroalbini commented May 28, 2018

sfackler commented Jun 2, 2018

rfcbot commented Jun 2, 2018 • edited by aturon Loading

clarfonthey commented Jun 3, 2018

Emerentius commented Jun 3, 2018

sfackler commented Jun 3, 2018

bors commented Jun 13, 2018

scottmcm commented Jun 13, 2018 • edited Loading

Emerentius commented Jun 13, 2018 • edited Loading

rfcbot commented Jun 22, 2018

scottmcm commented Jun 22, 2018 • edited Loading

clarfonthey commented Jun 22, 2018

shepmaster commented Jun 22, 2018

pietroalbini commented Jul 9, 2018

Emerentius commented Jul 19, 2018 • edited Loading

rust-highfive commented Jul 20, 2018

rust-highfive commented Jul 20, 2018

rust-highfive commented Jul 20, 2018

Emerentius commented Jul 20, 2018

rust-highfive commented Jul 20, 2018

alexcrichton commented Jul 24, 2018

Emerentius commented Jul 25, 2018 • edited Loading

shepmaster commented Jul 25, 2018

alexcrichton commented Jul 26, 2018

Emerentius commented Jul 26, 2018

alexcrichton commented Jul 26, 2018

scottmcm commented Jul 27, 2018

TimNN commented Aug 7, 2018

TimNN commented Aug 14, 2018

Emerentius commented Aug 14, 2018

alexcrichton commented Aug 14, 2018

Emerentius commented Aug 17, 2018

Emerentius commented May 19, 2018 •

edited

Loading

clarfonthey commented May 19, 2018 •

edited

Loading

rfcbot commented Jun 2, 2018 •

edited by aturon

Loading

scottmcm commented Jun 13, 2018 •

edited

Loading

Emerentius commented Jun 13, 2018 •

edited

Loading

scottmcm commented Jun 22, 2018 •

edited

Loading

Emerentius commented Jul 19, 2018 •

edited

Loading

Emerentius commented Jul 25, 2018 •

edited

Loading