Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flatten and FlatMap have incorrect bounds for their FusedIterator impls #81248

Closed
SkiFire13 opened this issue Jan 21, 2021 · 1 comment · Fixed by #81306
Closed

Flatten and FlatMap have incorrect bounds for their FusedIterator impls #81248

SkiFire13 opened this issue Jan 21, 2021 · 1 comment · Fixed by #81306
Labels
A-iterators Area: Iterators C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@SkiFire13
Copy link
Contributor

SkiFire13 commented Jan 21, 2021

I would expect the following code to either not compile (because iter shouldn't implement FusedIterator) or to not panic (because it does, so its contract should guarantee that calling next after the first None will always return None), however it compiles and panics:

use core::iter::FusedIterator;

fn check_is_fused<T, I: Iterator<Item = T> + FusedIterator>(mut i: I) {
    while let Some(_) = i.next() {}
    assert!(i.next().is_none());
}

struct NonFusedIter(bool);

impl Iterator for NonFusedIter {
    type Item = ();
    fn next(&mut self) -> Option<Self::Item> {
        self.0 = !self.0;
        Some(()).filter(|_| !self.0)
    }
}

impl DoubleEndedIterator for NonFusedIter {
    fn next_back(&mut self) -> Option<Self::Item> {
        self.0 = !self.0;
        Some(()).filter(|_| !self.0)
    }
}

fn main() {
    let mut iter = vec![NonFusedIter(true)].into_iter().flatten();
    iter.next_back();
    check_is_fused(iter);
}

Playground example

This is caused by the fact that the impl of FusedIterator for Flatten only requires the outer iterator to implement FusedIterator, not the inner one, however the implementation doesn't actually fuse backiter, which is what causes the assert to fail. The same problem is present for FlatMap.

Possible solutions:

  • Change how FlattenCompat is implemented to guarantee it to be fused even if the inner iterators are not fused. This would silently change its behavior, however it wouldn't be a breaking change.
  • Change the bounds for those impls. This would be a (minor?) breaking change.

Additionally I don't think there's anything that requires the outer iterator to always be manually fused in FlattenCompat. Should we also change it in the process?

See also https://users.rust-lang.org/t/why-doesn-t-std-flatten-implement-std-fusediterator-regardless-if-the-outer-or-inner-iterators-do/54477

@SkiFire13 SkiFire13 added the C-bug Category: This is a bug. label Jan 21, 2021
@cuviper
Copy link
Member

cuviper commented Jan 21, 2021

Change how FlattenCompat is implemented to guarantee it to be fused even if the inner iterators are not fused. This would silently change its behavior, however it wouldn't be a breaking change.

I think this is the best choice. They're already semi-fused in an unspecified manner, so I'm not concerned about the behavior change of going all the way.

That's not to say the front/back should be Fuse<U> though, because that has some potential performance impacts for the specialized U: FusedIterator -- we'd end up calling those no-ops repeatedly before moving to the other parts. That's already true for the iter: Fuse<I>, but I think a more limited impact.

Additionally I don't think there's anything that requires the outer iterator to always be manually fused in FlattenCompat. Should we also change it in the process?

That's a relatively recent feature of #68915. We could decide to commit unconditional FusedIterator after fixing this bug.

@cuviper cuviper added A-iterators Area: Iterators T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 21, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 27, 2021
Fuse inner iterator in FlattenCompat and improve related tests

Fixes rust-lang#81248
@bors bors closed this as completed in 94e093a Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants