-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
split the Copy
trait in two traits: Pod
and Copy
#936
Conversation
What is the reasoning behind "iterators shouldn't be |
@glaebhoerl Weird cases where you think you changed an iterator but didn't because you implicitly copied it somewhere. It's an inherently stateful thing, which suggests it shouldn't be Copy'd implicitly. +1 for Pod distinction. +100 for derive-inheritance. |
Without having read this, I'm in favor =) |
@gankro Could you link me the relevant discussion? |
@glaebhoerl The problem seen in practice is explained in the last part of this rust-lang/rust#21809 (comment) . Also see rust-lang/rust#18045, and rust-lang/rust#21846 for previous discussion. |
@japaric Delighted to see this! I like the design, and am definitely in favor of introducing this distinction. I do agree that the derive situation here is getting a bit hairy, and of course would, like you, prefer to provide blanket implementations. But since that's not really on the table, the "more magic" derive alternative sounds pretty good to me. We've talked about doing similar things with 👍 |
@japaric Thanks! |
I am in favor of the general idea. However, there is a missing alternative. We could add a In fact, it need not be builtin, we can just use OIBIT (something like this): unsafe trait Pod { }
unsafe impl Pod for .. { }
unsafe<'a,T:?Sized> impl Pod for &'a T { }
unsize<'a,T:?Sized> impl !Pod for &'a mut T { }
unsafe<T:?Sized> impl Pod for *const T { }
unsize<T:?Sized> impl !Pod for *mut T { }
unsafe impl<T:Drop> !Pod for T { } The caveat here is that I don't know that the semantics of UPDATE: Edited the set of rules above slightly. |
@nikomatsakis I think there shouldn't be a difference between the |
I really like this idea for How would (I agree with @tbu- that |
@nikomatsakis Taking the OIBIT approach for (I guess this is no different than the situation with other OIBIT traits like Just to clarify: The issue I am concerned about with hidden implicit properties of public interfaces is that conditional cfg and/or changes between versions of code can cause one of the implicit properties to go away without the developer realizing it. (But again, as I said above, this is the current situation for |
Yes. I should have mentioned that as a downside. I am not sure whether pod-ness should be an implicit part of the public interface or not. Probably not, since I tend to think that adding a -------- Original message -------- @nikomatsakis Taking the OIBIT approach for Podintroduces the problem that the Pod-ness of a type is an implicit part of its public interface, which is part of what we were trying to correct when we changed things to make Copy opt-in, no? (I guess this is no different than the situation with other OIBIT traits like Send and Sync...) — |
Something I'd like to have spelled out in the RFC -- in what cases is this backwards incompatible if we do it after 1.0? |
On Thu, Mar 05, 2015 at 05:53:21AM -0800, tbu- wrote:
I agree. I didn't mean to suggest there would be one. |
@eddyb seems to dislike this, and even opt-in From reddit:
From IRC:
Here is the start of that IRC conversation. |
AFAICT, both the proposed change and the A backward incompatible change would be changing a type from being [1] Breaking changes and upgrade path: The proposed change does break today's code. Manual The I consider both of these breaking changes to be tolerable post 1.0 and in line with the "stability caveats" outlined in this blog post. I also think it'd be possible to ship a "rustup" script with the next point release that fixes most/all of these breaking changes. |
Some more thoughts on OIBIT I think OIBIT
The biggest issue I see with OIBIT (in general) is that you can "implicitly" remove the // OIBIT
struct IsPod {
a: char,
+ b: String, // `Pod`ness is lost without a warning
} vs struct IsPod {
a: char,
+ b: String,
}
impl Pod for IsPod {}
+ //~^ error: can't implement `Pod` because the `b` field is not `Pod` Removing |
I am a fan of opt in Pod as well. |
Why would we be keeping |
AFAIK, |
@japaric Thanks for breaking down the current situation. Even if it's "acceptable" breakage, I'd like to see this land pre-1.0 so libraries can properly audit if their Copy's should really be Copy. |
Here's an instance where an object that is "pod" is explicitly not made This RFC would propose that an |
@seanmonstar it would at least require the use of an |
If we remove
Yes, that's an option that has been mentioned before. See the last part of rust-lang/rust#21846 (comment), and the following comment.
I agree. It's already marked as unsafe, but I'd be in favor of a lint, denied by default.
No. Even the OIBIT alternative would not mark |
@RalfJung Do you talk about |
You are right, I meant Well, if I followed this discussion correctly then any solution would make ergonomics worse. Certainly, having another trait harms ergonomics. Also, I don't see why type inference couldn't be improved to handle the case you gave at rust-lang/rust#21846 (comment). And finally, writing something like
until, later, type inference is improved, doesn't strike me as too bad either. (Sorry if this is the wrong place to discuss that. It seems to me the issues are entangled enough that it ends up being the same discussion.) |
I would like to disagree, having another trait doesn't make me write |
True. Though probably, many things currently taking an
I would argue that the additional complexity this introduces harms ergonomics as well, as it makes it harder to learn the language. (Maybe you used a more narrow definition of ergonomics than I did.) Explaining the difference between |
Mainly overlapping impls, see rust-lang/rust#17884 (comment).
Yes, that's another option. I don't know if
Yes, that was the main idea - there is only one way to clone a |
Ah ok. Perhaps in the short term lints can be used to prevent anything that would break if the blanket impls were added later (once negative bounds exist). |
OIBIT Pod can silently change the semantics of some structs, which may be considered a backcompat hazard. For example: // from rust-lang/rand - iOS specific code
struct OsRng {
_dummy: (),
}
It should be noted that these types that are not Copy and don't have destructors are fairly rare (at least in my experience). |
That being said, I'd prefer to do this before 1.0, because crate versions are short lived, so a buggy combination of crate version + (nightly) compiler version will be phased out quickly. |
Yes, we have been relaxing some bounds from
The main pain point comes from things like |
Well, it would be more of an |
Well, it would be more of an `IntoIteratorExt`, right ;-) ? If I see it
correctly, both `map` and `filter` would work fine. But I see this has a
significant tail of changes required to get the old ergonomics back. But
then, I could just as well argue that `some_vec.map()` is something I
should be able to write. I still think it's the "right thing" to do (and
I have yet to see anybody reasoning against this), but it may not be
feasibly anymore at this point in time. Fair enough, though it's a pity.
|
I disagree that vec.map should work; IteratorExt has premium method names. map, filter, chain, cloned, zip, enumerate, skip, take, scan, inspect, by_ref, max, min... Making those methods inaccessible (or worse, traps) on anything that implements IntoIterator would be a nightmare. |
@gankro is right. Persistent collections will need those method names. Looking at the original problem, Ranges could just implement Interator though. |
@gankro is right. Persistent collections will need those method names.
Okay. I can see this point.
Looking at the original problem, Ranges could just implement interator though.
They do...? And that's the problem here, right? Both `Iterator` and
`Copy` are sensible traits to implement for a range. Currently, some
ranges implement `Iterator`, while others implement `Copy`. That's not
really satisfying.
|
Oh sorry, I thought the proposed change of making the methods work on all IntoIterators was in response to Range already implementing IntoIterator and not Iterator, when in relativity none of that is true today. |
@glaebhoerl Apologies for the late response, I've been meaning to get to this. The problem I see there is that With that scheme, Given Instead, we can either introduce an internal expression kind (just like Alternatively, we could either make One other case I see If, instead, we used lints for this, here's what we could do:
|
I'll admit that having for loops take by value or by mutable reference depending on the context feels odd and can make code harder to read, though we may end up doing something similiar if we decide to do autoref on binary operations.
This sounds like a reasonable alternative, but how would it be implemented?
I think this would be too noisy, it would throw a warning on all the
I think that having a
or if they want to use the move semantics. Your proposed lints plus profiling can help users choose between implicitly copyable or not. |
@japaric why deny copies through the type-system when you can just turn the lint on? "Move semantics" do not prevent calling Marking a value as moved even though its type implements And no, the lint would not warn |
@japaric I disagree that Copy/Pod should be used to lint against moving large values. This seems like a completely orthogonal issue. @eddyb Iterator is only one of the problems with the binary Copy/Clone situation. RNGs are another example of things that can often be bitwise copied safely, but will likely lead to incorrect behaviour if it is done implicitly. I also disagree that this proposal complicates the Clone hierarchy. This completes and unifies it. Right now we have two unrelated traits: Copy and Clone. Although everything that is Copy is logically Clone with a particular impl, this is not enforced. This creates a proper hierarchy:
99% of generic bounds should take Clone, but a few will want Pod. Copy is simply an ergonomic helper in concrete code. |
After having given this more thought (and some discussion) I am pretty strongly in favor of adding a lint. The idea would be that you can annotate a type to say it should not be implicitly copied. The lint would detect when a value is implicitly copied and then used again afterwards, and warn on the second use. Advantages I see:
This seems to be pretty similar to the |
It seems to me that there's some edge-cases that make a lint hard, e.g. what happens when storing the Pod type inside a Copy type? (Especially a generic struct Foo {
bad_copy_field: SomeTypeThatShouldntBeCopied
some_other_field: i32
}
{
// ok, not the same field
let foo = make();
let a = foo.bad_copy_field;
let b = foo.some_other_field;
}
{
// ok, reinitialising
let foo = make();
let a = foo.bad_copy_field;
foo = make(); // or foo.bad_copy_field = make2()
let b = foo.bad_copy_field;
}
{
// bad, using the same field twice
let foo = new();
let a = foo.bad_copy_field;
let b = foo.bad_copy_field;
}
{
// bad, some_method might use bad_copy_field
let foo = new();
let a = foo.bad_copy_field;
foo.some_method();
} I guess the lint is effectively a move checker that emits warnings, rather than errors. (To be clear, despite that, I don't think the lint option is bad.) |
Oh, that's indeed surprising. I would have expected a blanket "Clone" |
Is there really any actual distinction here? Even with a simple |
@glaebhoerl I full-heartedly agree, which is why I'd rather error/warn on accidental copies in OTOH, implementing |
Consensus seems to be that the lint option is better (I've also spoken with the author, @japaric, about this). There I am going to close this RFC. |
@nikomatsakis even Copy: Clone is out of the question? |
Rendered
Implementation
cc @aturon @nikomatsakis @tbu-
cc rust-lang/rust#21846