Introduce the `IntoIterator` trait and reimplement for-loops to use it #20790

japaric · 2015-01-09T03:29:52Z

As per RFC #235, you can now do:

let mut v = vec![1];

// iterate over immutable references
for x in &v {
    assert_eq!(x, &1);
}

// iterate over mutable references
for x in &mut v {
    assert_eq!(x, &mut 1);
}

// iterate over values, this consumes `v`
for x in v {
    assert_eq!(x, 1);
}

[breaking-change]s

For loops now "consume" (move) the iterator, this breaks iterating over mutable references to iterators, and also breaks multiple iterations over the same iterator:

fn foo(mut it: &mut Iter) {  // `Iter` implements `Iterator`
    for x in it { .. }  //~ error: `&mut Iter` doesn't implement Iterator
}

fn bar() {
    for x in it { .. }  //~ note: `it` moved here
    for x in it { .. }  //~ error: `it` has been moved
}

Both cases can be fixed using the by_ref() adapter to create an iterator from the mutable reference:

fn foo(mut it: &mut Iter) {
    for x in it.by_ref() { .. }
}

fn bar() {
    for x in it.by_ref() { .. }
    for x in it { .. }
}

This PR also makes iterator non-implicitly copyable, as this was source of subtle bugs in the libraries. You can still use clone() to explictly copy the iterator.

Finally, since the for loops are implemented in the frontend and use global paths to IntoIterator, Iterator and Option variants, users of the core crate will have to use add an std module to the root of their crate to be able to use for loops:

#![no_std]

extern crate core;

fn main() {
    for x in 0..10 {}
}

#[doc(hidden)]
mod std {
    // these imports are needed to use for-loops
    pub use core::iter;
    pub use core::option;
}

r? @nikomatsakis @aturon
cc #18424
closes #18045

rust-highfive · 2015-01-09T03:29:57Z

r? @alexcrichton

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

huonw · 2015-01-09T03:32:36Z

Can we do this without expanding in the front end?

japaric · 2015-01-09T03:33:35Z

@huonw @nikomatsakis was interested in an implementation that used the frontend (hence this prototype)

huonw · 2015-01-09T03:33:54Z

Also, expanding to include _ => break will mean that refutable patterns like for 1 in iter will "work", which seems like something that we don't want to allow.

japaric · 2015-01-09T03:48:21Z

(I think @aturon and @nikomatsakis mainly want to measure the size of the fallout and the change in ergonomics of these new for-loop semantics before commiting to it. This implementation is minimal work to measure that. We can come up with a better implementation if we decide to actually do this.)

alexcrichton · 2015-01-09T22:19:34Z

src/libcollections/str.rs

@@ -273,7 +273,7 @@ impl<'a> Iterator for Recompositions<'a> {
        loop {
            match self.state {
                Composing => {
-                    for ch in self.iter {
+                    while let Some(ch) = self.iter.next() {


Could some loops like this be altered to:

for ch in self.iter.by_ref() { /* ... */ }

That may avoid consuming the entire iterator.

Actually, if it helps in basically all cases, I wonder if we could spec it like this...

I had forgotten about by_ref()! I think we can use that instead of most (all?) the while lets.

Actually, if it helps in basically all cases, I wonder if we could spec it like this...

You mean using by_ref() in the for loop expansion?

japaric · 2015-01-10T16:07:22Z

Updated to use the by_ref() adapter instead of while let.

nikomatsakis · 2015-01-10T16:18:09Z

Some thoughts:

As I mentioned to @japaric on IRC, I think we can expand to match <iterator> { mut iter => ... } instead of { let mut iter = <iterator>; .. } to resolve the matter of temporary lifetimes.
Personally, I prefer to do this expansion than to have for loops in the AST that typeck/borrowck/everybody-else has to deal with. Basically desugaring makes the rest of the compiler simpler. However, the catch would be if it negatively affects error messages and so forth. So I'm curious @japaric as to the impact there. There is also the problem of our inability to really name things in std reliably, though this is ameliorated by the fact that #![no_std] is not stable yet.

jroesch · 2015-01-10T19:23:50Z

@japaric and @nikomatsakis As a fan of desugaring as much as possible I think this seems like a good idea. If error messages become a problem you could use a similar scheme to what they do with Pyret where you provide a tag (either in some table or directly on the AST) for which rewrite rule you used, and then utilize that information when reporting errors by re-sugaring it then pretty printing. I would be happy to help explore such a scheme if the error reporting suffers.

japaric · 2015-01-12T22:21:55Z

Updated PR with @nikomatsakis suggestions (match IntoIterator + UFCS with absolute paths, see expansion in the top comment), which got rid of the temporary lifetime issue, reduced the size of the import fallout and removed the need to have IntoIterator in the prelude. I'm now blocked on a type inference bug, but @nikomatsakis is aware of it.

I've collected some error messages from 3 cfail tests related to for-loops:

for-loop-bogosity.rs

struct MyStruct {
    x: isize,
    y: isize,
}

impl MyStruct {
    fn next(&mut self) -> Option<isize> {
        Option::Some(self.x)
    }
}

fn main() {
    let mut bogus = MyStruct {
        x: 1,
        y: 2,
    };
    for x in bogus {
//~^  old-ERROR has type `MyStruct` which does not implement the `Iterator` trait
//~^^ new-ERROR the trait `iter::Iterator` is not implemented for the type `MyStruct`
    }
}

The error message changed its wording but the meaning is the same.

for-loop-refutable-pattern-error-message.rs

fn main() {
    for
        &1i8
//~^  old-ERROR refutable pattern in `for` loop binding
//~^^ new-ERROR non-exhaustive patterns: `Some(&_)` not covered
        in [1i8].iter() {}
}

The message is more obscure now.

for-loop-type-error.rs

fn main() {
    let x = () + (); //~ ERROR binary operation

    // this shouldn't have a flow-on error:
    // japaric: and it doesn't with the either version of for loops
    for _ in x {}
}

Behavior remains the same.

alexcrichton · 2015-01-12T22:26:51Z

src/libcore/lib.rs

@@ -65,6 +65,8 @@
 #![allow(unknown_features)] #![feature(int_uint)]
 #![feature(on_unimplemented)]
 #![deny(missing_docs)]
+// SNAP 9e4e524 remove `allow(unused_mut)` after a snapshot
+#![allow(unused_mut)]


Now that we have cfg_attr, could this be #![cfg_attr(stage0, allow(unused_mut))]? Anything with the string "stage0" in it is a pretty easy flag for something to clean out when dealing with snapshots.

nikomatsakis · 2015-01-13T16:10:31Z

@japaric and I were talking on IRC. My feeling is that the error in this example:

fn main() {
    for
        &1i8
//~^  old-ERROR refutable pattern in `for` loop binding
//~^^ new-ERROR non-exhaustive patterns: `Some(&_)` not covered
        in [1i8].iter() {}
}

is actually better. I doubt most "lay people" know what a "refutable pattern" is. (I personally have to work out from first principles which is "irrefutable" and which is "refutable".) But having a concrete counter example seems immediately understandable. It might be better still to combine the two messages:

"refutable pattern in `for` loop binding: `Some(&_)` not covered"

huonw · 2015-01-13T23:08:56Z

@nikomatsakis I think introducing the Some out of nowhere is confusing (and is not very useful since it happens for all invalid iterators, and is not related to how one might fix it, in the common case).

japaric · 2015-01-23T19:03:56Z

This PR is ready! (passed check-stage1, running full make check now)

re: error message on refutable patterns, I decide to reuse the old message/diagnostic code, we can customize it later if desired.

japaric · 2015-01-26T15:17:09Z

rebased and added tests for issues that new for loops fix. Also fixed a recursive call that the new unconditional_recursion lint pointed out. (last 3 commits are new)

japaric · 2015-01-26T16:36:59Z

Just a heads up, until #21637 gets fixed, we won't be able to use the IntoIterator trait to build iterator-centric APIs. Code like this one ICEs right now:

fn concat<I: IntoIterator>(it: I) -> String where <I::Iter as Iterator>::Item: Str {
    unimplemented!();
}

alexcrichton · 2015-01-27T02:09:47Z

src/failures

@@ -0,0 +1,7 @@
+std


ugh..
/me snipes file

@nikomatsakis

As per [RFC rust-lang#235][rfc], you can now do: [rfc]: https://github.com/rust-lang/rfcs/blob/master/text/0235-collections-conventions.md#intoiterator-and-iterable ``` rust let mut v = vec![1]; // iterate over immutable references for x in &v { assert_eq!(x, &1); } // iterate over mutable references for x in &mut v { assert_eq!(x, &mut 1); } // iterate over values, this consumes `v` for x in v { assert_eq!(x, 1); } ``` [breaking-change]s For loops now "consume" (move) the iterator, this breaks iterating over mutable references to iterators, and also breaks multiple iterations over the same iterator: ``` rust fn foo(mut it: &mut Iter) { // `Iter` implements `Iterator` for x in it { .. } //~ error: `&mut Iter` doesn't implement Iterator } fn bar() { for x in it { .. } //~ note: `it` moved here for x in it { .. } //~ error: `it` has been moved } ``` Both cases can be fixed using the `by_ref()` adapter to create an iterator from the mutable reference: ``` rust fn foo(mut it: &mut Iter) { for x in it.by_ref() { .. } } fn bar() { for x in it.by_ref() { .. } for x in it { .. } } ``` This PR also makes iterator non-implicitly copyable, as this was source of subtle bugs in the libraries. You can still use `clone()` to explictly copy the iterator. Finally, since the for loops are implemented in the frontend and use global paths to `IntoIterator`, `Iterator` and `Option` variants, users of the `core` crate will have to use add an `std` module to the root of their crate to be able to use for loops: ``` rust #![no_std] extern crate core; fn main() { for x in 0..10 {} } #[doc(hidden)] mod std { // these imports are needed to use for-loops pub use core::iter; pub use core::option; } ``` --- r? @nikomatsakis @aturon cc rust-lang#18424 closes rust-lang#18045

bors · 2015-01-30T23:08:02Z

⌛ Testing commit b9a9030 with merge e47284b...

alexcrichton · 2015-01-30T23:10:32Z

@bors: retry

bors · 2015-01-31T00:34:24Z

⌛ Testing commit b9a9030 with merge e513946...

bors · 2015-01-31T00:46:05Z

💔 Test failed - auto-linux-64-opt

alexcrichton · 2015-01-31T00:48:04Z

@bors: retry

bors · 2015-01-31T02:19:36Z

⌛ Testing commit b9a9030 with merge 581a5c8...

bors · 2015-01-31T02:21:03Z

💔 Test failed - auto-linux-64-x-android-t

alexcrichton · 2015-01-31T02:21:39Z

@bors: retry

bors · 2015-01-31T03:54:38Z

⌛ Testing commit b9a9030 with merge 692c620...

bors · 2015-01-31T03:57:00Z

💔 Test failed - auto-linux-32-opt

alexcrichton · 2015-01-31T03:57:31Z

@bors: retry

bors · 2015-01-31T07:17:37Z

⌛ Testing commit b9a9030 with merge 72eab3e...

@nikomatsakis

Removes `Copy` from `ops::Range` (`a..b`) and `ops::RangeFrom` (`a..`) [breaking-change] --- I forgot about these two in #20790, this PR also adds `Clone` to the `Peekable` adapter which used to be `Copy`able. r? @nikomatsakis or anyone

taralx · 2015-02-03T17:50:28Z

The RFC didn't mention this "mod std" business, and I'm a bit uncomfortable with it. Why not directly access the core crate?

japaric · 2015-02-03T18:07:43Z

Why not directly access the core crate?

If you mean expanding to ::core::iter::IntoIterator, that won't work in most cases, since most crates are linked to std but not to core (they have access to core stuff because of re-exports).

The RFC didn't mention this "mod std" business, and I'm a bit uncomfortable with it.

You already have to use the mod std to use the #[derive] syntax extension and/or slicing syntax in #![no_std] crates. At the moment this required for stuff that's implemented in the frontend, but we'd definitely like to improve this situation in the future.

See rust-lang/rust#20790 for details.

japaric · 2015-02-03T21:27:42Z

@taralx see #21912 ;-)

alexchandel · 2015-02-04T04:52:18Z

The relevant section of the Rust Reference ought to be updated to reflect this, cc @steveklabnik

rust-highfive assigned alexcrichton Jan 9, 2015

alexcrichton reviewed Jan 9, 2015
View reviewed changes

japaric force-pushed the for-loops branch from b7f577d to 6f91e46 Compare January 10, 2015 15:36

japaric force-pushed the for-loops branch from 6f91e46 to 49fae39 Compare January 12, 2015 21:59

alexcrichton reviewed Jan 12, 2015
View reviewed changes

japaric mentioned this pull request Jan 16, 2015

Type inference works with method calls, but not with UFCS #21245

Closed

pnkfelix mentioned this pull request Jan 23, 2015

new scoping rules for safe dtors can yield spurious semi-colon or trailing unit expr #21114

Closed

japaric force-pushed the for-loops branch from 49fae39 to 2678d5a Compare January 23, 2015 18:23

japaric changed the title ~~[proto][WIP] Introduce the IntoIterator trait and reimplement for-loops to use it~~ Introduce the IntoIterator trait and reimplement for-loops to use it Jan 23, 2015

japaric force-pushed the for-loops branch from 2678d5a to f4bd701 Compare January 23, 2015 18:56

This was referenced Jan 26, 2015

ICE - trying to take the sizing type of <core::iter::Iterator + 'static as core::iter::Iterator>::Item, an unsized type #20605

Closed

Trying to iterate over &mut Iterator using a for loop, results in lifetime error #21655

Closed

japaric force-pushed the for-loops branch from f4bd701 to 522e902 Compare January 26, 2015 15:13

nikomatsakis mentioned this pull request Jan 26, 2015

New destructor semantics #8861

Closed

7 tasks

japaric force-pushed the for-loops branch from 522e902 to d2d9270 Compare January 26, 2015 16:25

alexcrichton reviewed Jan 27, 2015
View reviewed changes

alexcrichton merged commit b9a9030 into rust-lang:master Jan 31, 2015

japaric deleted the for-loops branch January 31, 2015 13:29

This was referenced Jan 31, 2015

Have for pattern in iter not borrow the iterator #8372

Closed

remove Copy impls from remaining iterators #21809

Merged

milibopp added a commit to milibopp/rust-itertools that referenced this pull request Feb 3, 2015

Adapt code to the new semantics of the for-loop

e304f87

See rust-lang/rust#20790 for details.

milibopp mentioned this pull request Feb 3, 2015

Adapt code to the new semantics of the for-loop rust-itertools/itertools#13

Merged

nagisa mentioned this pull request Jan 9, 2016

Iterator is needlessly a language item #30803

Closed

Introduce the IntoIterator trait and reimplement for-loops to use it #20790

Introduce the IntoIterator trait and reimplement for-loops to use it #20790

Conversation

japaric commented Jan 9, 2015

rust-highfive commented Jan 9, 2015

huonw commented Jan 9, 2015

japaric commented Jan 9, 2015

huonw commented Jan 9, 2015

japaric commented Jan 9, 2015

alexcrichton Jan 9, 2015

Choose a reason for hiding this comment

alexcrichton Jan 9, 2015

Choose a reason for hiding this comment

japaric Jan 10, 2015

Choose a reason for hiding this comment

japaric commented Jan 10, 2015

nikomatsakis commented Jan 10, 2015

jroesch commented Jan 10, 2015

japaric commented Jan 12, 2015

alexcrichton Jan 12, 2015

Choose a reason for hiding this comment

nikomatsakis commented Jan 13, 2015

huonw commented Jan 13, 2015

japaric commented Jan 23, 2015

japaric commented Jan 26, 2015

japaric commented Jan 26, 2015

alexcrichton Jan 27, 2015

Choose a reason for hiding this comment

japaric Jan 27, 2015

Choose a reason for hiding this comment

bors commented Jan 30, 2015

alexcrichton commented Jan 30, 2015

bors commented Jan 31, 2015

bors commented Jan 31, 2015

alexcrichton commented Jan 31, 2015

bors commented Jan 31, 2015

bors commented Jan 31, 2015

alexcrichton commented Jan 31, 2015

bors commented Jan 31, 2015

bors commented Jan 31, 2015

alexcrichton commented Jan 31, 2015

bors commented Jan 31, 2015

taralx commented Feb 3, 2015

japaric commented Feb 3, 2015

japaric commented Feb 3, 2015

alexchandel commented Feb 4, 2015

Introduce the `IntoIterator` trait and reimplement for-loops to use it #20790

Introduce the `IntoIterator` trait and reimplement for-loops to use it #20790