Skip to content

Only implement FusedIterator for TakeWhileInclusive when the inner iterator is fused#1110

Open
patrickwehbe wants to merge 1 commit into
rust-itertools:masterfrom
patrickwehbe:fix/take-while-inclusive-fused
Open

Only implement FusedIterator for TakeWhileInclusive when the inner iterator is fused#1110
patrickwehbe wants to merge 1 commit into
rust-itertools:masterfrom
patrickwehbe:fix/take-while-inclusive-fused

Conversation

@patrickwehbe

Copy link
Copy Markdown

Fixes #1088.

TakeWhileInclusive sets its done flag only when the predicate rejects an element. It does not set it when the inner iterator returns None, so with a non-fused inner iterator the adaptor can return Some again after a None. The FusedIterator impl is currently unconditional (where I: Iterator), so that contract is violated.

This bounds the impl on I: FusedIterator, which is the same approach std::iter::TakeWhile takes (option 3 in the issue) and matches how the other conditionally-fused adaptors here are written (for example InterleaveShortest).

Note this is technically breaking: TakeWhileInclusive no longer implements FusedIterator when the inner iterator isn't fused. The previous impl was unsound in that case, so I left the changelog placement for release prep.

Added a fused_take_while_inclusive quickcheck test mirroring fused_interleave_shortest: not fused over the deliberately non-fused Iter, fused once the inner iterator is .fuse()d. cargo test, cargo fmt --check, and cargo clippy --all-features --all-targets all pass.

…erator is fused

TakeWhileInclusive sets its `done` flag only when the predicate rejects an
element, not when the inner iterator returns None. With a non-fused inner
iterator it can therefore yield Some after returning None, which breaks the
FusedIterator contract it currently claims for every Iterator.

Bound the impl on `I: FusedIterator`, matching std::iter::TakeWhile.

Fixes rust-itertools#1088
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect FusedIterator implementation on TakeWhileInclusive

1 participant