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

impl TrustedLen for Cycle #68352

Closed

Conversation

KamilaBorowska
Copy link
Contributor

@KamilaBorowska KamilaBorowska commented Jan 18, 2020

That was something I noticed in #66531 (comment), so sure, why not.

size_hint method for Cycle<I> can return either (0, Some(0)) or (usize::MAX, None) for TrustedLen implementations ((0, None) is impossible as (0, x) if x != Some(0) returned by inner iterator is impossible by TrustedLen invariant).

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2020
@Centril Centril added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 18, 2020
@Centril Centril added this to the 1.42 milestone Jan 18, 2020
@dtolnay dtolnay added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jan 18, 2020
@@ -504,6 +504,9 @@ where
#[stable(feature = "fused", since = "1.26.0")]
impl<I> FusedIterator for Cycle<I> where I: Clone + Iterator {}

#[unstable(feature = "trusted_len", issue = "37572")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no such thing as an unstable trait impl so this would need to be changed to #[stable].

Copy link
Contributor Author

@KamilaBorowska KamilaBorowska Jan 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TrustedLen isn't a stable trait. Unstable is correct here, I think.

@dtolnay
Copy link
Member

dtolnay commented Jan 18, 2020

This PR is adding:

unsafe impl<I> TrustedLen for Cycle<I> where I: Clone + TrustedLen

where Cycle is the iterator returned by Iterator::cycle.

I don't think this impl is correct because I: TrustedLen does not imply any knowledge about I's Clone impl. Here is a Cycle whose length is neither 0 nor ∞. Returning the wrong length for this Cycle could result in undefined behavior.

use std::iter::TrustedLen;

struct S(usize);

impl Clone for S {
    fn clone(&self) -> Self {
        S(0)
    }
}

impl Iterator for S {
    type Item = ();

    fn next(&mut self) -> Option<Self::Item> {
        if self.0 > 0 {
            self.0 -= 1;
            Some(())
        } else {
            None
        }
    }

    fn size_hint(&self) -> (usize, Option<usize>) {
        (self.0, Some(self.0))
    }
}

unsafe impl TrustedLen for S {}

fn main() {
    for x in S(4).cycle() {
        println!("{:?}", x);
    }
}

FYI @rust-lang/libs

@KamilaBorowska
Copy link
Contributor Author

Yeah, that's a fair point. While it's strictly speaking fine with built-in implementations, there is nothing to guarantee Clone is sane, which may not be the case with external implantations.

@Centril Centril removed the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 31, 2020
@Centril Centril removed this from the 1.42 milestone Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants