Introduce the `IntoIterator` trait and reimplement for-loops to use it by japaric · Pull Request #20790 · rust-lang/rust

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

As per RFC #235, you can now do:

letmut v = vec![1];// iterate over immutable referencesfor x in&v {assert_eq!(x,&1);}// iterate over mutable referencesfor x in&mut v {assert_eq!(x,&mut1);}// 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:

fnfoo(mutit:&mutIter){// `Iter` implements `Iterator`for x in it { .. }//~ error: `&mut Iter` doesn't implement Iterator}fnbar(){for x in it { .. }//~ note: `it` moved herefor 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:

fnfoo(mutit:&mutIter){for x in it.by_ref(){ .. }}fnbar(){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]externcrate core;fnmain(){for x in0..10{}}#[doc(hidden)]mod std {// these imports are needed to use for-loopspubuse core::iter;pubuse 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

Could some loops like this be altered to:
for ch inself.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

structMyStruct{x:isize,y:isize,}implMyStruct{fnnext(&mutself) -> Option<isize>{Option::Some(self.x)}}fnmain(){letmut 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

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

The message is more obscure now.

for-loop-type-error.rs

fnmain(){let x = () + ();//~ ERROR binary operation// this shouldn't have a flow-on error:// japaric: and it doesn't with the either version of for loopsfor _ in x {}}

Behavior remains the same.

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

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:

fnmain(){for&1i8//~^ old-ERROR refutable pattern in `for` loop binding//~^^ new-ERROR non-exhaustive patterns: `Some(&_)` not coveredin[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:

fnconcat<I:IntoIterator>(it:I) -> Stringwhere <I::IterasIterator>::Item:Str{unimplemented!();}

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

ugh..
/me snipes file

alexcrichton · 2015-01-30T19:58:00Z

@bors: retry

@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#18424closesrust-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.

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 6f91e46Compare January 10, 2015 15:36

japaric force-pushed the for-loops branch from 6f91e46 to 49fae39Compare 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 2678d5aCompare 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 itJan 23, 2015

japaric force-pushed the for-loops branch from 2678d5a to f4bd701Compare 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 522e902Compare 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 d2d9270Compare January 26, 2015 16:25

alexcrichton reviewed Jan 27, 2015
View reviewed changes

alexcrichton merged commit b9a9030 into rust-lang:masterJan 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 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

Uh oh!

Uh oh!

Conversation

japaric commented Jan 9, 2015

Uh oh!

rust-highfive commented Jan 9, 2015

Uh oh!

huonw commented Jan 9, 2015

Uh oh!

japaric commented Jan 9, 2015

Uh oh!

huonw commented Jan 9, 2015

Uh oh!

japaric commented Jan 9, 2015

Uh oh!

alexcrichtonJan 9, 2015

Choose a reason for hiding this comment

Uh oh!

alexcrichtonJan 9, 2015

Choose a reason for hiding this comment

Uh oh!

japaricJan 10, 2015

Choose a reason for hiding this comment

Uh oh!

japaric commented Jan 10, 2015

Uh oh!

nikomatsakis commented Jan 10, 2015

Uh oh!

jroesch commented Jan 10, 2015

Uh oh!

japaric commented Jan 12, 2015

Uh oh!

alexcrichtonJan 12, 2015

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jan 13, 2015

Uh oh!

huonw commented Jan 13, 2015

Uh oh!

japaric commented Jan 23, 2015

Uh oh!

japaric commented Jan 26, 2015

Uh oh!

japaric commented Jan 26, 2015

Uh oh!

alexcrichtonJan 27, 2015

Choose a reason for hiding this comment

Uh oh!

japaricJan 27, 2015

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Jan 30, 2015

Uh oh!

bors commented Jan 30, 2015

Uh oh!

alexcrichton commented Jan 30, 2015

Uh oh!

bors commented Jan 31, 2015

Uh oh!

bors commented Jan 31, 2015

Uh oh!

alexcrichton commented Jan 31, 2015

Uh oh!

bors commented Jan 31, 2015

Uh oh!

bors commented Jan 31, 2015

Uh oh!

alexcrichton commented Jan 31, 2015

Uh oh!

bors commented Jan 31, 2015

Uh oh!

bors commented Jan 31, 2015

Uh oh!

alexcrichton commented Jan 31, 2015

Uh oh!

bors commented Jan 31, 2015

Uh oh!

taralx commented Feb 3, 2015

Uh oh!

japaric commented Feb 3, 2015