Make Map::last skip closure calls until last element

[hkBst]

A continuation of #146410 which changes the direct count impl for a fold impl, to which the default count delegates. Also added tests for the new behavior.

EDIT: adds a last to Map.

[rustbot]

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[hkBst]

r? @jhpratt

[rustbot]

jhpratt is not on the review rotation at the moment.
They may take a while to respond.

[asquared31415]

Arguably one could use the infinite fold for side effects? It would be kind of weird though.

[hkBst]

@asquared31415 true, I may have to take that out then...

[LIParadise]

Adapters like std::iter::Map may override only Iterator::fold but Iterator::last, meaning assert_eq!(repeat(42).last(), repeat(42).map(std::convert::identity).last()) would fail: the former returns Some(42) while the latter delegates to <Repeat as Iterator>::fold then panics, no?

[hkBst]

@LIParadise I believe that assert should hold, but let me add a test.

[hkBst]

@LIParadise I was wrong, but I was able to fix it...

[LIParadise]

If that's the case, I'm afraid there are quite a few types that's Iterator that need to be taken into consideration, too, for instance.

[LIParadise]

Also crates like itertools might have a word or two.

Iterator is nice partly because next implies all other methods; such default method delegations are handy s.t. we definitely don't want to give up on them (and also backwards compatibility), but OTOH they imply whenever one has an implementation that departs from the default implementation would most likely lead to issues like this: iterator adapter libraries has to be aware of such possibility and delegate accordingly to preserve the inner type's semantics.

Part of me agree that <Repeat as Iterator>::last should evaluate the same as <Repeat as DoubleEndedIterator>::next_back, but another part of me thinks there's more to this issue. Iterator's default implementation means calling next i.e. moving forward should give one required results. DoubleEndedIterator is going from back. Are there mathematical equivalence between ordinals/supertasks and their reverse version (sorry but my mathematics are poor to the point I don't know how to formally define the latter)?

[hkBst]

@LIParadise I think #146410 (comment) describes it well. Alternatively you can imagine two infinite sequences with their infinite ends glued together. It amounts to the same thing.

[jhpratt]

I'm off-rotation as I don't have the time to handle nontrivial reviews at the moment.

r? libs

[Amanieu]

I don't think this is correct since even for an infinite iterator, you would still expect f to be called multiple times, which may have visible side-effects.

[Amanieu]

cc @the8472

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[hkBst]

I've removed the change from count -> fold, which, as was pointed out by several people, is wrong, and was based on too much functional thinking on my part.

[Mark-Simulacrum]

I don't think we should do this; it skips calling f on all of the elements up to the last one, which doesn't seem right to me, especially since we do iterate over those elements (i.e., we aren't specialized in some way for DoubleEndedIterator).

If we do proceed with this it'll need libs-api FCP, so nominating this PR.

[joshtriplett]

This would be an observable behavior change, and would likely cause subtle breakage. I would object to merging this as proposed.

Playground link demonstrating observable behavior: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=09b0005c5034bd8363f35180c605f1e8

[Amanieu]

This was discussed in the @rust-lang/libs-api meeting and we all agreed that this is definitely a breaking change since it prevents side-effects of Map from being evaluated.

[theemathas]

I found this code in the wild that would break due to this PR. It's in a repo containing "example" code though. https://github.com/eliovir/rust-examples/blob/d46017e846c8a5331512739d195ebc9b91a29f5e/find_max.rs#L72

[hkBst]

I've opened #147195 for the tests for the already accepted behavior, so I think this can be closed.