remove Copy impls from remaining iterators

[japaric]

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 Copyable.

r? @nikomatsakis or anyone

[alexcrichton]

@bors: r+ c3841b9

[bors]

⌛ Testing commit c3841b9 with merge f1f9cb7...

[tbu-]

@japaric Range and RangeFrom aren't only iterators, they're also used in indexing etc. The better solution for this is probably for Range to implement into_iterator.

[bluss]

This hurts the use of range in other APIs, as planned by std::rand (where it ends up) and collections.

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

[ranma42]

Right now RangeTo and RangeFull are Copy, but Range and RangeFrom are not. Is this intentional?

[japaric]

@tbu-

Range and RangeFrom aren't only iterators, they're also used in indexing etc.

Yes, they are heavily used for slicing in the whole rust repo, and as evidenced by this PR, it wasn't necessary to explictly clone any range at all, which means that the implicitly copyable property of ranges wasn't used in the whole repository. (Of course, we can't extrapolate this to all the other existing libraries, but it's worth noting)

The better solution for this is probably for Range to implement into_iterator.

That would hurt the ergonomics of the Vec::from_elem replacement:

// from
let v = (a..b).map(f).collect::<Vec<_>>();
// to
use std::iter::IntoIterator;
let v = (a..b).into_iter().map(f).collect::<Vec<_>>();

@bluss

This hurts the use of range in other APIs, as planned by std::rand (where it ends up) and collections.

Could you show me an example on how non implictly copyable ranges would hurt the ergonomics of these planned APIs? I'd like to better understand the situation.

I don't know the exact API that will be added, but IIRC one of them was using range with Vec::remove. I can see this happen:

// with implicit copy
let range = a..b;
v1.remove(range);
v2.remove(range);
// without implict copy (today)
let range = a..b;
v1.remove(range.clone());
v2.remove(range);
// however, I think it would be clearer to do
v1.remove(a..b);
v2.remove(a..b);
// which doesn't require an implicit copy

@ranma42

Right now RangeTo and RangeFull are Copy, but Range and RangeFrom are not. Is this intentional?

Yes, structs that implement Iterator should not be implictly copyable.

---

To give some background. When I was implementing the new for-loops which take the iterator by value. The implictly copyability of iterators was causing several collections tests to SIGILL and broke some demangle function. I figured out what the problem was:

The new for-loops changed the semantics of the following pattern:

// original code
for x in it {  // a *copy* of the iterator is used here
    // ..
}

match it.next() {  // the original iterator is used here
    // ..
}

When the intended behaviour was:

for x in it.by_ref() {  // advance the iterator
    // ..
}

match it.next() {  // reuse the mutated iterator
    // ..
}

But I didn't know where the problems were. The fastest way to find the bugs was to make the iterators non-implictly copyable, that way the original code would error at compile time with it.next() error: it has been moved. After doing that, I managed to fix all the bugs.

My original plan was to fix the bugs, then make the iterators implicitly copyable again and defer the decision of whether iterators should be implictly copyable to a later date. But after discussing with the core devs, they told me that they wanted to make iterators non-implictly copyable anyways, so I went ahead with that.