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

Implicitly copyable iterators are mega confusing #18045

Closed
kmcallister opened this issue Oct 14, 2014 · 10 comments · Fixed by #20790
Closed

Implicitly copyable iterators are mega confusing #18045

kmcallister opened this issue Oct 14, 2014 · 10 comments · Fixed by #20790

Comments

@kmcallister
Copy link
Contributor

For example, this code:

fn main() {
    let stream = "Hello, world!".chars().cycle();
    for _ in range(0u, 10) {
        let chunk: String = stream.take(3).collect();
        println!("{}", chunk);
    }
}

compiles and prints Hel ten times, rather than the expected Hel, lo,, wo, etc.

The fact that stream isn't mut is a clue that this code doesn't work as expected.

I guess the solution is stream.as_ref().take(3) but the "default" behavior here is really confusing.

@thestinger
Copy link
Contributor

In a garbage collected language, adaptors would always take the wrapped iterator by-reference. It makes more sense that way, but it doesn't work well in Rust. Rust's iterator adaptors need to be able to take the wrapped iterator by-value, and the natural way to deal with it is only having by-value adaptors and then taking advantage of references being values (by_ref).

@thestinger
Copy link
Contributor

I think it would be wrong to avoid Copy on types like this, because it's necessary for features like Cell. I don't really think there's a problem to be fixed here.

@kmcallister
Copy link
Contributor Author

Sure, I agree with all that. I just think there's a usability / confusion issue here that we need to resolve. Opt-in Copy would do it, or maybe a lint of some kind?

@kmcallister
Copy link
Contributor Author

Hm, I hadn't thought about putting these iterators in Cell. How about a #[warn_on_copy] attribute for types, and Cell would allow it?

@brson
Copy link
Contributor

brson commented Oct 14, 2014

cc @aturon

@thestinger
Copy link
Contributor

@kmcallister: It would end up causing warnings for typical iterator chains. I never liked that I had to use by-value for the parameters but I don't really see an alternative.

@aturon
Copy link
Member

aturon commented Oct 15, 2014

@kmcallister Opt-in Copy is on the way: see @pcwalton's PR, which is part of opt-in built-in traits.

I think that PR probably makes everything in std opt-in to Copy if it would've been Copy before, but we can audit iterators to not opt-in. Seems like it would address this problem?

@thestinger
Copy link
Contributor

@aturon: It doesn't address the problem. It would be incorrect to leave out of a Copy implementation for style / performance reasons because it's used by types like Cell.

@thestinger
Copy link
Contributor

It's hardly an issue specific to iterator types anyway...

@steveklabnik
Copy link
Member

This code still acts the same today, even after opt-in Copy.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 30, 2015
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
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 a pull request may close this issue.

5 participants