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

Iterator::fuse is not guaranteed to fuse a generic iterator. #83969

Closed
lcnr opened this issue Apr 7, 2021 · 6 comments · Fixed by #84489
Closed

Iterator::fuse is not guaranteed to fuse a generic iterator. #83969

lcnr opened this issue Apr 7, 2021 · 6 comments · Fixed by #84489
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Apr 7, 2021

struct S(bool);

impl Iterator for S {
    type Item = ();
    fn next(&mut self) -> Option<()> {
        self.0 = !self.0;
        if self.0 {
            Some(())
        } else {
            None
        }
    }
}

impl std::iter::FusedIterator for S {}

fn main() {
    let mut x = S(false).fuse();
    for _i in &mut x {}
    // x is fused, so it must be empty.
    assert_eq!(x.next(), None);
}

playground

This means unsafe code may not rely on iter.fuse() to actually fuse the iterator if iter is generic, which is unexpected to me.
I personally would like us to either add a note to Iterator::fuse mentioning this or to remove the specialization for std::iter::Fused.

@lcnr lcnr added the C-bug Category: This is a bug. label Apr 7, 2021
@lcnr lcnr changed the title Iterator::fuse is not guaranteed to actually fuse the given iterator. Iterator::fuse is not guaranteed to fuse a generic iterator. Apr 7, 2021
@lcnr lcnr added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 7, 2021
@amorison
Copy link
Contributor

amorison commented Apr 7, 2021

Well, by implementing std::iter::FusedIterator, you declare that S is already fused even though it isn't. You should remove the line impl std::iter::FusedIterator for S {} to get the expected behaviour. The only thing implementing FusedIterator on T does is tell Iterator::fuse that there is nothing to do because the type T is already fused. It doesn't fuse the iterator.

(As a side note, unsafe code should not blindly rely on safe code, especially traits implementation, precisely because it is impossible to automatically check that implementations are logically correct.)


EDIT : maybe I misunderstood your request, are you suggesting FusedIterator be deprecated and ignored by fuse because it can lead to fuse not working properly when wrongly implemented? That doesn't seem like a good idea to me, correct implementation of any trait is necessarily the burden of the user of that trait. Maybe the documentation could be more clear but it seems good to me as is to be honest.

@ExpHP
Copy link
Contributor

ExpHP commented Apr 9, 2021

Of course, the option of making the trait unsafe was discussed as part of the RFC, but it seems that the use case for the guarantees of Fuse in unsafe code were unclear at the time.

That said, I do kind of see how somebody might look of the signature of fuse:

pub fn fuse(self) -> Fuse<Self>

and think, "ah, this returns a standard library type, so I should be able to trust this on an arbitrary iterator," without considering that Fuse<I> uses specialization on a safe trait. Perhaps this merits a warning in the documentation. (Edit: Ah, this is precisely as you suggested!)

@workingjubilee
Copy link
Member

workingjubilee commented Apr 22, 2021

I honestly imagine there is also a lot of unsafe code that relies on Eq implementations being correct for generic types, no? Is this different?

@ExpHP
Copy link
Contributor

ExpHP commented Apr 22, 2021

I honestly imagine there is also a lot of unsafe code that relies on Eq implementations being correct for generic types, no? Is this different?

I feel that there is a difference, due to way that specialization impacts this. The danger of reliance on safe traits in unsafe code is so omnipresent that I would hope that most people who audit unsafe code already look for things like bad Eq impls. I know that when I see T: Eq in an unsafe function, things that immediately come to mind are:

  • equality comparisons may violate the properties of Eq
  • equality comparisons may panic

On the other hand, if I were auditing unsafe code and saw a function take T: Iterator and call Iterator::fuse, I might think:

  • somebody could override the fuse method and use a different/modified instance of Self that returns different items.
  • somebody could override the fuse method and make it panic.
  • ...but since it returns std::iter::Fuse, I can see myself easily mistakenly thinking that, at the very least, the result is properly fused. I might not think of false FusedIterator implementations since that trait is not mentioned anywhere in the code or even in the docs of Iterator::fuse

@amorison
Copy link
Contributor

I guess adding something along the line of

This method is a no-op on iterators that implement the FusedIterator trait. It may therefore behave incorrectly if this trait is improperly implemented.

in the doc of Iterator::fuse would be enough of a warning? Maybe a similar sentence in the doc of FusedIterator itself, but I feel like the current documentation is clear on the expected contract. If this sounds like a good idea, I could make a PR.

@workingjubilee
Copy link
Member

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. 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.

4 participants