ExactlyOneError should not iterate itself

[phimuemue]

Follow-up to #1046

I'm pondering whether

fn exactly_one(mut self) -> Result<Self::Item, ExactlyOneError<Self>>

should rather be

fn exactly_one(&mut self) -> Result<Self::Item, ExactlyOneError<Self::Item>>

and ExactlyOneError should contain an Option<[Self::Item; 2]>, so that the user can inspect the first two elements, and simply re-use the iterator afterwards, if they want to. (Similar to our all_equal_value.)

I think this is better, because at the moment we must specialize each and every function/trait (e.g. ExactSizeIterator, DoubleEndedIterator) to exploit the underlying iterator's specializations.

With my proposal, the user could still do this:

match iter.exactly_one() {
 Ok(value) => do_stuff_with(value),
 Err(None) => iterator_was_empty(),
 Err(Some(values)) => {
  // process *all* elements of the original iterator:
  values.chain(iter).for_each(...);
 }
}

or - terse -:

match iter.exactly_one() {
 Ok(value) => do_stuff_with(value),
 Err(optional_2_values) => {
  // process *all* elements of the original iterator:
  optional_2_values.into_iter().flatten().chain(iter).for_each(...);
 }
}

That is, we do not lose functionality, but the callers profit all specializations given by std.

Note: From my experience, re-visiting all elements of the original iterator is very rare. In most cases, it only matters if exactly_one worked or not, and if it did not work, the elements themselves are not relevant.

Following similar reasoning, I advocate for fn at_most_one(&mut self) -> Result<Option<Item>, AtMostOneError<Self::Item>> where AtMostOneError wraps a [Self::Item; 2].

@jswrenn I'd appreciate your opinion on this.

[stepancheg]

let one = iter.exactly_one().map_err(|e| anyhow!("expecting one hostname, got {}", e.count()));
// or
let one = iter.exactly_one().map_err(|e| anyhow!("expecting one hostname, got {:?}", e.collect::<Vec<_>>()));

With proposed API this one-liner won't be possible to the level that itertools won't give any advantage in some cases. One could just write the same or less code using only std API:

let Some(one) = iter.next() else {
  return Err(anyhow!("expecting one hostname, got zero"))
};
if iter.next().is_some() {
  return Err(anyhow!("expecting one hostname, got {}", iter.count() + 2))
}

[phimuemue]

I get it, but this case seems exceedingly rare. Do you have evidence for the opposite?

If not, we should return what the algorithm uses anyway (i.e. an optional 2-array) instead of wrapping it in some struct. And in the rare case, the caller can do whatever they want, including chaining it.

[stepancheg]

This is what I have found on the first five pages of search on github by .exactly_one().

https://github.com/smol-dot/smoldot/blob/c0498f95355a843af73fce833b4f208cc58024a6/light-base/src/lib.rs#L545

https://github.com/bm-w/advent22-rs/blob/4ae5a20dccfa78c4464ed3cf7468f91f05297f2b/src/day09.rs#L84

https://github.com/qryxip/bikecase/blob/94ea7f1caec3e99735e7a4012dab369d8d2e0f61/src/gist.rs#L36

https://github.com/lvyitian/RustPython_x86_32win_build/blob/133e1b36e64e3ef53668855f04b7e02c8479d6a5/vm/src/vm.rs#L109

https://github.com/JosieElliston/Hyperspeedcube/blob/0ad2aa12a601a4d3a6e0fe4ff9f9b5bf051816a2/src/app.rs#L691

Not all of these are relevant (I did not do thorough check), and search output is not representative of course (can be biased one way or another), but it does not seem like using error as iterator is very rare.

But even if it is rare, what is the harm of making error iterator?

• We are not gaining anything from not making it iterator (except for not needing to specialize a few function)
• In the worst case we can just declare that we don't care about performance of such iterator and call it a day
• if we need to recover iterator, we can provide .into_inner function

[jswrenn]

I'm not convinced that we should go out of our way to avoid designs that benefit from ample specialization. Specialization is an implementation detail; the API design should be instead guided by the practical needs of users. As a rule, iterator adaptors consume the underlying iterator by-value, not by reference. It would be a little surprising (but not totally unprecedented) to have a method that consumed &mut self, and chainability is an affordance that users expect from iterator adaptors.

[jswrenn]

It's rare that we would need to specialize every method of an iterator. For #1046, it probably would have been almost as good to just specialize fold, since that would have not only optimized count (which just calls fold), but many other methods too.

[phimuemue]

I may be wrong, but I think the 90% case is to check the result for zero/one/many elements, and I imagine this is better served by not returning an iterator.