Revert "remove Copy impls from remaining iterators"

[tbu-]

This reverts commit c3841b9.

The commit made it impossible to copy the pretty fundamental data type Range and RangeFrom, which e.g. means that they can't be used in otherwise copyable structures anymore (or Cells for that matter), where the reason for removal was that it can also be used as an iterator.

CC @japaric
#21809

[rust-highfive]

r? @alexcrichton

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

[Diggsey]

IMO ranges should implement "IntoIterator" rather than being iterators themselves, it doesn't make too much sense and it's caused a few other conflicts as well as this.

For example, currently "RangeTo" is not iterable at all, because there's currently no such thing as a "ReverseIterator" trait, and it only makes sense to iterate a "RangeTo" in reverse. If ranges are not themselves iterators, then "RangeTo" can just have a method to return a reverse iterator over that range, without breaking the symmetry with "RangeFrom" which has a similar method as part of its implementation of "IntoIterator", but for a forward iterator.

[Gankra]

CC @aturon

[aturon]

I quite like @Diggsey's solution, personally. @tbu- would you be interested in going in that direction?

[tbu-]

@aturon I can do this, but it will break much code like (0..len).map(|_| 'a').collect(), making the need for Vec::from_elem even bigger. Doing it together with re-adding the respective from_elem functions might be viable.

[tbu-]

@aturon Any news on this? Range still can't be embedded into structures...

[japaric]

Range still can't be embedded into structures...

Is that an insurmountable problem? Range fields are public, you can repack them into a copyable tuple (or equivalent) at the constructor/function boundary.

#[derive(Copy)] struct Struct { .. }
fn constructor(r: Range<i32>) -> Struct {
    Struct { start: r.start, end: r.end, .. }
}

Then you can reconstruct the iterable range from the copies: struct.start..struct.end (yes, verbose but not impossible)

@tbu- Could you elaborate on what functionality can't be achieved because Range is not Copy? I do reckon it can be a little unergonomic (I'd still take range.clone().map(..) over (a..b).into_iter().map(..) any time of the day though).

[tbu-]

@japaric I can't use them as public fields in my structs, constructors means you cna't construct a struct at compile-time, setters mean that I can't have per-field mutable borrows anymore.

Things that are "a little unergonomic" have really bad impact.

[tbu-]

@japaric There's no reason Range must be non-Copy and by making it so you make it impossible to re-use language-features in user code, because unlike other traits you can't even override Copy although it is obvious that Range is trivially copyable. You're changing Range to be no-Copy because it's an iterator, but there's more things to Range than just that it's an iterator, it's also a struct that has language-level sugar for construction.

[japaric]

I can't use them as public fields in my structs

Of course, you don't have to, you can keep the fields private.

constructors means you cna't construct a struct at compile-time

Yes, but you just said above that you can't make the range public, so you need a constructor. (?)

setters mean that I can't have per-field mutable borrows anymore

Sorry, I don't quite understand this statement, you mean a getter that freezes the struct?

There's no reason Range must be non-Copy

The original problem was #18045, which perhaps could have been prevented using a lint that triggers when an iterator is implicitly copied, but the core-devs opted for making iterators not implement Copy.

you make it impossible to re-use language-features in user code

(I think impossible is an overstatement)

that Range is trivially copyable

Range is a trivial memcpy (which is what clone does), yes, but that doesn't mean that it should be implicitly copyable. The current Copy trait conflates both concepts (memcpyable and implictly copyable). There was some idea floating around about splitting the trait in two: Pod (plain old data/memcpyable data), and Copy (implicitly copyable/cloneable data), but no one has written an RFC on the subject (perhaps I should before it's too late?). Splitting the traits may help in this case, but I don't know what exactly @tbu- is intending to do.

@tbu- I'd still like to see what use case you have problems with.

[tbu-]

@japaric It may indeed be the problem that the two things are conflated – not implementing Copy locks out compound data types from implementing Copy themselves.

The core-devs might have missed that they were making Range non-Copy-able with their decision.

I'm not sure if you're mocking me with your line-by-line reply, but what I wanted to say with the different options is that none of them are suitable: If I make the fields public, the sugar isn't there, if I use a constructor it can't be constructed statically, if I use setters and getters I always need complete ownership of the struct before I can set a single member.

[tbu-]

@japaric To see why sugar is important, let me suggest to remove the Iterator implementation of Range: You can still do the same things, surely it's a little more verbose, but what kind of functionality cannot be achieved with that?

[aturon]

@tbu-

@aturon Any news on this? Range still can't be embedded into structures...

Sorry for the delay. Let me respond to your earlier comment.

@aturon I can do this, but it will break much code like (0..len).map(|_| 'a').collect(), making the need for Vec::from_elem even bigger. Doing it together with re-adding the respective from_elem functions might be viable.

There have been a lot of requests to reintroduce from_elem, which was dropped only to slim down the API surface. So I'd be happy to review a PR re-introducing it, along with the other changes we've talked about here. Feel free to r? me.

[aturon]

@japaric

There was some idea floating around about splitting the trait in two: Pod (plain old data/memcpyable data), and Copy (implicitly copyable/cloneable data), but no one has written an RFC on the subject (perhaps I should before it's too late?).

I've talked about this with @nikomatsakis and I think we definitely want to do it at some point, but I believe it can be done backwards-compatibly.

[Gankra]

Being able to treat (a..b) as an iterator is a really nice pattern outside of from_elem. (a..b).iter() isn't exactly a tragedy, though.

[Gankra]

A nice benefit of making ranges only IntoIter is that it resolves the need for a hypothetical RangeInclusive to include a has_yielded_inclusive boolean.

[aturon]

@gankro

Being able to treat (a..b) as an iterator is a really nice pattern outside of from_elem. (a..b).iter() isn't exactly a tragedy, though.

Can you give some examples to clarify this? Any code that currently takes an Iterator should be changed to take an IntoIterator instead.

[aturon]

@gankro

Being able to treat (a..b) as an iterator is a really nice pattern outside of from_elem. (a..b).iter() isn't exactly a tragedy, though.

Can you give some examples to clarify this? Any code that currently takes an Iterator should be changed to take an IntoIterator instead.

Ah, hm, I bet you mean things like (0..10).map(f).collect()?

[aturon]

To be clear, the non-Copy for iterators is essentially a guideline to help avoid confusing bugs -- not a hard rule. If it turns out that it's needed for ranges for the best ergonomics, that may be a trade worth making.

cc @alexcrichton @huonw

[Gankra]

Yeah (a..b).mess().of().adaptors() is pretty pleasant in my opinion.

[japaric]

@tbu-

I'm not sure if you're mocking me with your line-by-line reply

Certainly no. I apologize if it seem that way. I was trying to understand your use case but failed to do so.

To see why sugar is important, let me suggest to remove the Iterator implementation of Range: You can still do the same things, surely it's a little more verbose, but what kind of functionality cannot be achieved with that?

Sugar is important, but there is trade off between ergonomics and a potential source of bugs. Removing Iterator from Range will generate way more fallout (not just the from_elem replacement, but also stuff like for _ in (a..b).map(..)) than removing Copy from Range.

@aturon

I've talked about this with @nikomatsakis and I think we definitely want to do it at some point, but I believe it can be done backwards-compatibly.

I was mocking a design, and it was backward incompatible, but if you have explored the space and think it can implement in bakcward compatible way, then I trust your decision of postponing it for after 1.0.

If it turns out that it's needed for ranges for the best ergonomics, that may be a trade worth making.

I'd be in favor of implementing Copy for Range, and adding some lint that either:

• warns when a struct derives both Iterator, and Copy (I don't hink this is enough, the lint should warn at "call site" rather than at "impl site")
• warns when an iterator is implictly copied (this feels too noisy for Range)
• warns when the copy of an iterator is advanced, and then the original is used, basically catch this for _ in it { if cond { break } } do_something_with(it.next())); (this feels hard to implement, but I don't have experience writing lints).

[Diggsey]

warns when the copy of an iterator is advanced, and then the original is used, basically catch this for _ in it { if cond { break } } do_something_with(it.next()));

This example makes me feel especially strongly that ranges should be IntoIterator rather than Iterator. If I iterate through a range using a for loop, it's extremely counterintuitive to me that the range is modified in the process.

While it's slightly more work to type ".iter()" to use it as an iterator:

1. It's not that big a deal
2. Commonly used methods of the type "range.iter().method()" can always be implemented directly for "IntoIterator" allowing "range.method()" to continue to work. This comes with the added benefit that those methods will work for all iterable types, not just ranges.

[pnkfelix]

The conflict between "leaving out Copy to try to catch bugs like #18045" and "doing so (leaving out Copy) makes such types unusable with Cell" was actually brought up during the discussion on #18045.

One possible way to resolve that particular conflict could be the route suggested by @huonw here:
#20813 (comment) which was to decouple the notion of "safe for use with cell" (aka "memcpy-is-safe", aka Pod) from "remove move-by-default" (aka Copy). Then we would add in trait Copy : Pod { }, so that there is still a relationship between the two, (but merely a unidirectional implication).

Though as @huonw said at the time, the complexity of that solution may not pay for itself.

[aturon]

@pnkfelix I think in the long run we will definitely want to go in this direction, but @nikomatsakis and I believe that we can do so backwards-compatibly.

[pnkfelix]

@aturon oh sorry, I see you wrote the same thing up above; sorry to make you repeat yourself!

[tbu-]

@aturon So I made an IntoIterator implementation for Range – but the problem is that type inference falls down for that. For example this code snippet

fn a(x: u32) {}

for i in (0..10) { a(i) }

fails with the error message "expected u32, found i32", so I think that this isn't a viable option. I think the best option (and the only one I can think of), is just to implement Copy for Range again, and continue to implement Iterator, like before.

[aturon]

cc rust-lang/rfcs#936

[tbu-]

@aturon Now the decision in the RFC was to add a lint some day – so could we merge this Copy implementation?

[bors]

☔ The latest upstream changes (presumably #25262) made this pull request unmergeable. Please resolve the merge conflicts.

[aturon]

@tbu- Sorry, this one got buried and I missed it.

In the Pod/Copy decision we determined that a lint was better than adding a further split to the marker traits, but I don't think there was an explicit decision about cases like this prior to the lint landing. I personally don't have a strong opinion here, though. I will ask the library subteam!

[aturon]

As per discussion within the libs team on IRC (https://botbot.me/mozilla/rust-libs/2015-05-19/?msg=39609682&page=1), we're not going to take this at the moment. If in the future we develop a lint to make it more feasible to make this Copy without being a footgun, we can reconsider.

[upsuper]

The link to IRC log above seems to have been broken now. If anyone still has the log, could you paste the whole log here (maybe hidden inside <details>) so that we don't need to rely on external service to keep the information forever?

[steveklabnik]

Here is a copy of the log on archive.org: https://web.archive.org/web/20181012140432/https://botbot.me/mozilla/rust-libs/2015-05-19/

I hope archive.org doesn't go the way of botbot.me, but just in case, since this was such a big decision that people still ask about:

11:37 aturon
lib subteam: any thoughts on this Copy implementation? https://web.archive.org/web/20181012140432/https://github.com/rust-lang/rust/pull/21846#issuecomment-103609165 -- was left unresolved in the discussion on Pod/Copy
11:39 sfackler
Seems to me like "chainable" APIs like Iterator or builders that take self by value shouldn't be Copy
11:39 
seems like it'd be too easy to accidentally not apply changes where the should be
11:41 aturon
sfackler: yeah, so that's been our general policy,
11:41 
which is why Copy was removed here in the first place
11:41 
i guess the question here is, we may eventually get a lint to help with this,
11:41 
but until that time, should we apply Copy?
11:41 cmr
Having them Clone but not Copy seems like a good balance to me for now.
11:41 sfackler
yeah
11:41 aturon
i guess i lean toward "no"
11:42 sfackler
not clear to me that the benefits of not having to write .clone() are worth it
11:42 cmr
I've flip-flopped on this issue a few times though.
11:42 aturon
sfackler: i think the concern is more that this prevents Copy applying when you embed in another struct
11:42 pm
but still, seems like not a big thing
11:42 cmr
another concern that was raised is losing Cell.
11:43 
Seems fine though, just an optimization.
11:43 aturon
yeah, and in the long run we *can* add Copy with the lint
11:48 sfackler
acrichto: what do you want to do about debug builder stability now? should we let it bake another release or so after the changes or stabilize now?
11:53 acrichto
sfackler: I don't have too many thoughts, I think that we may want to stabilize in this cycle, slating them to be stable for 0.2, aturon do you feel any differently here?
11:55 aturon
acrichto: that's fine by me
0:04 sfackler
acrichto: ok cool, will update the stabilization pr