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

explicitly show how iterating over .. fails #35701

Merged
merged 3 commits into from
Aug 20, 2016

Conversation

matthew-piziak
Copy link
Contributor

I've also removed the main() wrapper, which I believe is extraneous.
LMK if that's incorrect.

I've also removed the `main()` wrapper, which I believe is extraneous.
LMK if that's incorrect.
@matthew-piziak
Copy link
Contributor Author

r? @steveklabnik

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@strega-nil
Copy link
Contributor

It's technically an IntoIterator implementation that's the problem.

@matthew-piziak
Copy link
Contributor Author

@ubsan I thought that too, but the error threw me off.

main.rs:15:5: 17:6 error: the trait bound `std::ops::RangeFull: std::iter::Iterator` is not satisfied [E0277]
main.rs:15     for i in .. {
               ^
main.rs:15:5: 17:6 help: run `rustc --explain E0277` to see a detailed explanation
main.rs:15:5: 17:6 note: `std::ops::RangeFull` is not an iterator; maybe try calling `.iter()` or a similar method
main.rs:15:5: 17:6 note: required by `std::iter::IntoIterator::into_iter`

@strega-nil
Copy link
Contributor

@matthew-piziak weird implementation thing. It's looking for IntoIter through Iterator

@matthew-piziak
Copy link
Contributor Author

@ubsan Could you elaborate on that? I think I'm missing a piece of the puzzle. I know that IntoIterator is supposed to be the trait that governs for loops, but the difference between Range and RangeFull in the source is that only Range has an Iterator implementation. Do you think saying "This errors because .. has no Iterator impl" is disingenuous? What would be more accurate?

@strega-nil
Copy link
Contributor

@matthew-piziak Okay, so.

for x in y {
    ...
}

becomes

let __tmp = IntoIterator::into_iter(y);
while let Some(x) = __tmp.next() {
    ...
}

the only thing that is necessarily an Iterator is __tmp, but that's guaranteed by the IntoIterator impl; that's not what the error's about.

However, the question is, how does rustc find an IntoIterator implementation? It first checks typeof(y) for an implementation of IntoIterator. It then looks for any impl<T: ...> IntoIterator for T { ... }. It then, for each of these blanket implementations, checks typeof(y) for an implementation of the given traits. Since there's an impl<T: Iterator> IntoIterator for T { ... }, it checks typeof(y) for an Iterator impl after checking for an IntoIterator impl, but there is none. Since Iterator was the last trait checked for, rustc errors, saying it was unable to find the Iterator implementation, even though it was looking for an IntoIterator implementation.

Sorry if this is confusing, I'm pretty tired.

@matthew-piziak
Copy link
Contributor Author

Sorry if this is confusing, I'm pretty tired.

Not at all. Thank you so much for the explanation. I get how it works now. :)

Saying that "[for-loop iteration] fails because .. has no IntoIterator
impl" is more direct than saying "...no Iterator impl" because for loops
sugar into IntoIterator invocations. It just happens that the other
Range* operators implement Iterator and rely on the fact that
`IntoIterator` is implemented for `T: Iterator`.
/// assert_eq!(arr[1..3], [ 1,2 ]);
/// }
/// // for i in .. {
/// // println!("This errors because .. has no IntoIterator impl");
Copy link
Member

Choose a reason for hiding this comment

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

So, usually rather than commenting the code out, we put things in its own example marked with ignore. And in general, these feel like three separate examples to me. Could you maybe split them up, and put a little sentence between them explaining it?

So for example:

/// The `..` syntax is a `RangeFull`:
///
/// ```
/// assert_eq!((..), std::ops::RangeFull);
/// ```
///
/// It does not have an `IntoIterator` implementation, so you can't use it in a
/// `for` loop directly. This won't compile:
///
/// ```ignore
/// for i in .. {
///    // ...
/// }
/// ```

Something like that? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I'm thinking that's much better. Will impl. :)

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Aug 19, 2016

📌 Commit 469b7fd has been approved by steveklabnik

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 19, 2016
…ror, r=steveklabnik

explicitly show how iterating over `..` fails

I've also removed the `main()` wrapper, which I believe is extraneous.
LMK if that's incorrect.
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 20, 2016
…ror, r=steveklabnik

explicitly show how iterating over `..` fails

I've also removed the `main()` wrapper, which I believe is extraneous.
LMK if that's incorrect.
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 20, 2016
…ror, r=steveklabnik

explicitly show how iterating over `..` fails

I've also removed the `main()` wrapper, which I believe is extraneous.
LMK if that's incorrect.
bors added a commit that referenced this pull request Aug 20, 2016
matthew-piziak added a commit to matthew-piziak/rust that referenced this pull request Aug 20, 2016
Feedback on PR rust-lang#35701 seems to be positive, so this does the same thing for `RangeTo` and `RangeToInclusive`.
@bors bors merged commit 469b7fd into rust-lang:master Aug 20, 2016
@matthew-piziak matthew-piziak deleted the rangefull-example-error branch August 23, 2016 15:12
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 30, 2016
…ple-error, r=steveklabnik

show how iterating over `RangeTo` and `RangeToInclusive` fails

Feedback on PR rust-lang#35701 seems to be positive, so this does the same thing for `RangeTo` and `RangeToInclusive`.
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.

6 participants