Iterator repeat: no infinite loop for last and count
#146410
Conversation
rustbot
added
S-waiting-on-review
|
@hkBst I think the |
|
@workingjubilee I think they are both debatable. Happy to split if it should become clear that that would be beneficial! |
|
imo there is no last element given that the iterator is infinite, even if all elements are identical. Is there any precedent for saying otherwise? The docs for |
|
@jhpratt that is certainly a valid viewpoint, but I would argue that in that case a panic is more appropriate than a foreseeable infinite loop. From last's docs:
There is currently no allowance for panicking or looping, so imagine that you exhaust the infinite iterator (supertask), is there a possible element that could be returned? Since all elements are the same, there is only one choice. The other option is None, but that would seem to be reserved for empty sequences...
The previous analysis seems consistent with this elaboration, although it does not readily transfer to other infinite iterators. Those should probably panic instead. |
|
Aside from the question of what’s “correct” per API contract, the more important question to me is what’s helpful. For For |
|
Personally I agree, but it's not my prerogative to add the possibility of new panics or different behavior to documentation. |
|
I would be happy with a panic for |
|
@rust-lang/libs-api, what are your thoughts on this? As currently written, I don't see room for anything other than an infinite loop for |
|
Given that Repeat already implements DoubleEndedIterator, I think that the ship has sailed. last() should not infinite loop, in order to be consistent with next_back(). |
I agree, the |
That's an interesting way of thinking of it. Given this and the 👍 from the8472, I'm taking that as approval from two team members to move forward with that. @hkBst Can you add |
rust-cloud-vms
bot
force-pushed
the
repeat-1
branch
from
7293765 to
c89b6a9
Compare
|
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. |
|
@jhpratt added track_caller to count, and fixed a small typo. |
rustbot
added
the
relnotes
bors
added
S-waiting-on-bors
Iterator repeat: no infinite loop for `last` and `count` This removes two cases of infinite looping from [`Repeat`](https://doc.rust-lang.org/nightly/std/iter/struct.Repeat.html): - [`last`](https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.last): By viewing the iterator as returning None after [omega](https://en.wikipedia.org/wiki/Ordinal_number) calls to `next`, this method can simply return the repeated element. - [`count`](https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.count): From its docs: """The method does no guarding against overflows, so counting elements of an iterator with more than [usize::MAX](https://doc.rust-lang.org/nightly/std/primitive.usize.html#associatedconstant.MAX) elements either produces the wrong result or panics. If overflow checks are enabled, a panic is guaranteed.""", so a panic'ing impl is allowed by the docs, and is more honest than an infinite loop.
Rollup of 9 pull requests Successful merges: - #144871 (Stabilize `btree_entry_insert` feature) - #145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables) - #145838 (don't apply temporary lifetime extension rules to non-extended `super let`) - #146259 (Suggest removing Box::new instead of unboxing it) - #146410 (Iterator repeat: no infinite loop for `last` and `count`) - #146460 (Add tidy readme) - #146581 (Detect attempt to use var-args in closure) - #146588 (tests/run-make: Update list of statically linked musl targets) - #146647 (Move `#[rustc_coherence_is_core]` to the `crate_level` file) r? `@ghost` `@rustbot` modify labels: rollup
|
The new behaviors of these two methods should be documented with a doc-comment, at the very least; they contradict the docs on the |
|
I also think it’s quite strange and unexpected that with this PR, |
Iterator repeat: no infinite loop for `last` and `count` This removes two cases of infinite looping from [`Repeat`](https://doc.rust-lang.org/nightly/std/iter/struct.Repeat.html): - [`last`](https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.last): By viewing the iterator as returning None after [omega](https://en.wikipedia.org/wiki/Ordinal_number) calls to `next`, this method can simply return the repeated element. - [`count`](https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.count): From its docs: """The method does no guarding against overflows, so counting elements of an iterator with more than [usize::MAX](https://doc.rust-lang.org/nightly/std/primitive.usize.html#associatedconstant.MAX) elements either produces the wrong result or panics. If overflow checks are enabled, a panic is guaranteed.""", so a panic'ing impl is allowed by the docs, and is more honest than an infinite loop.
|
@Jules-Bertholet I suppose you're correct, since &mut Iterator uses this impl which does not override count and last... I'll see about adding those and some tests. |
Rollup of 14 pull requests Successful merges: - #142807 (libtest: expose --fail-fast as an unstable command-line option) - #144871 (Stabilize `btree_entry_insert` feature) - #145071 (Update the minimum external LLVM to 20) - #145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables) - #145660 (initial implementation of the darwin_objc unstable feature) - #145838 (don't apply temporary lifetime extension rules to non-extended `super let`) - #146259 (Suggest removing Box::new instead of unboxing it) - #146410 (Iterator repeat: no infinite loop for `last` and `count`) - #146460 (Add tidy readme) - #146552 (StateTransform: Do not renumber resume local.) - #146564 (Remove Rvalue::Len again.) - #146581 (Detect attempt to use var-args in closure) - #146588 (tests/run-make: Update list of statically linked musl targets) - #146631 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3)) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #146410 - hkBst:repeat-1, r=jhpratt Iterator repeat: no infinite loop for `last` and `count` This removes two cases of infinite looping from [`Repeat`](https://doc.rust-lang.org/nightly/std/iter/struct.Repeat.html): - [`last`](https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.last): By viewing the iterator as returning None after [omega](https://en.wikipedia.org/wiki/Ordinal_number) calls to `next`, this method can simply return the repeated element. - [`count`](https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.count): From its docs: """The method does no guarding against overflows, so counting elements of an iterator with more than [usize::MAX](https://doc.rust-lang.org/nightly/std/primitive.usize.html#associatedconstant.MAX) elements either produces the wrong result or panics. If overflow checks are enabled, a panic is guaranteed.""", so a panic'ing impl is allowed by the docs, and is more honest than an infinite loop.
|
@Jules-Bertholet Actually it does not seem possible to delegate any methods that take |
This removes two cases of infinite looping from
Repeat:last: By viewing the iterator as returning None after omega calls tonext, this method can simply return the repeated element.count: From its docs: """The method does no guarding against overflows, so counting elements of an iterator with more than usize::MAX elements either produces the wrong result or panics. If overflow checks are enabled, a panic is guaranteed.""", so a panic'ing impl is allowed by the docs, and is more honest than an infinite loop.