"impl Copy" can bypass field privacy

[RalfJung]

@idanarye makes an excellent point:

Say I have a MyBox struct - an implementation of Box that uses a raw pointer directly. The fields are private - the module where MyBox is defined is that the only place where you can touch them - and thus the rule is that this module is responsible for maintaining the safety variants of the pointer within (and if it exposes any API that may violate it - it should mark it as unsafe)

Naturally, I should not impl Copy for MyBox because that would break the uniqueness invariant, and more specifically - when I drop one copy and the memory is released I'll still have the other copy with a dangling pointer.

But I can impl Copy for MyBox in a different module of the same crate.

I had never realized this, and I think it can be viewed as a violation of our privacy rules: outside modules in the same crate are not able to access the private field, and yet they are able to make the type copyable!

@rust-lang/types Is there any chance we can fix impl Copy (over an edition, presumably) so that it is only allowed inside modules where all fields of the type are accessible, i.e., where you could have written the obvious Clone impl that copies all fields?

[CodesInChaos]

An alternative would be to remove the special rules around implementing Copy, and make implementing it unsafe with a safe derive. This should be possible to do in an edition.

This rule would bring it in line with other unsafe marker traits, which can also implemented elsewhere in the same crate.

Though it would unnecessarily require unsafe for generic code where derive infers incorrect bounds. So probably not a great solution.

[RalfJung]

I think we definitely want to keep the ability to write a safe impl Copy and have the compiler do all the required checks -- that's quite useful to have.

Being able to write unsafe impl Copy as a way to bypass these checks makes a lot of sense and I'd like to see it happen, but that's a separate discussion from this issue.

[lcnr]

Pretty sure we can! We already have checks that all fields are actually copy-able, so also checking that they're visible at the location of the impl doesn't feel too challenging. It also feels very reasonable to do so. I think we should future compat lint this everywhere. Don't have strong opinions whether to increase the severity of the lint in editions

@idanarye this is an excellent observation and thanks @RalfJung for moving it into a new issue and pinging t-types/me :3

[the8472]

If we had unsafe fields (e.g. because the pointer is unsafe to access/expose) then could the unsafe in unsafe impl Copy be conditional on that?

[lcnr]

form an impl perspective, sure

[traviscross]

Unpin can also be safely implemented in the same crate but outside of the module that some type is defined, and doing so could create unsoundness. How do we think about that in comparison to the point raised about Copy here?

[RalfJung]

I think making Unpin a safe trait was a mistake for a number of reasons... (see e.g. rust-lang/rfcs#3467)

[idanarye]

What if instead of the rule being "Copy can only be implemented on a type from a module that can see all its fields" it'd be "_Copy can only be implemented on a type from the same module that declared the type"?

This will mean, of course, that even if a struct only has pub fields you still won't be able to impl Copy for it from a different module. This is a little more strict that what the safety-by-encapsulation principles dictate, but we won't lose much from that because moving the impl Copy to the same module as the type is almost always a straightforward fix (and definitely better than making the fields pub just so that you won't have to move it)

The advantage of this approach is that it does not rely on the fact that Copy already looks at the fields, and thus it could be done with an attribute. The same attribute could also be placed on Unpin - and also other marker traits like Send and Sync.

If we go that route, it'd be important to remember (not that it can be forgotten - the standard library will break if this neglected) that the attribute should not prevent implementing owned traits on foreign types from. This does raise a question though - should this attribute restrict that kind of impl to the module that defined the trait, or should it allow it from any module in the crate that defined the trait?

[RalfJung]

Send and Sync are unsafe traits, I see no reason to subject them to constraints like this.

[traviscross]

I'm a bit torn on whether the trait being safe or unsafe to implement should distinguish these cases or not. Even with Copy, you have to be using unsafe somewhere for this impl to produce UB, and we accept that in writing unsafe { .. } you are also making guarantees about what you do and do not do within in the crate outside of the unsafe block.

As another angle, as the concern here is about seeming to violate field privacy, it's not obvious to me that one of the powers of an unsafe impl should be to violate field privacy. I.e., it's not obvious that's part of what unsafe should mean.

[RalfJung]

The way I view Copy, it is an unsafe trait (it fundamentally has safety-critical promises attached to it) with some compiler magic that can identify cases where the trait is actually safe to implement. However, that magic has a hole since it does not respect field privacy. That's why IMO it makes sense to consider Copy as quite different from Send/Sync.

However, there's probably other ways to view the situation that can lead to different results.

[ia0]

We don't have negative impls yet, but wouldn't a possible alternative (not necessarily better, but for completeness of design space exploration) to instead require of users to impl !Copy for TypeWithNonCopyInvariants {} when needed?

In this particular example, the module defining MyBox would also impl !Copy for MyBox {} which is required to be present in their proof of soundness. Other modules in the same crate won't be able to impl Copy for MyBox {} anymore, and the safe language remains sound.

Again this is for completeness only, because it adds some additional burden on unsafe Rust users, now having to explicitly state that their types are not Copy, instead of it being implicit. So there's some design trade-off between corner-casing Copy and unsafe user convenience (I'm assuming negative impls to be a general enough idea, that its cost is factorized with other uses, and thus negligible for this particular case).

[Sky9x]

Say I have a MyBox struct - an implementation of Box that uses a raw pointer directly. The fields are private - the module where MyBox is defined is that the only place where you can touch them - and thus the rule is that this module is responsible for maintaining the safety variants of the pointer within (and if it exposes any API that may violate it - it should mark it as unsafe)

To implement Copy for a type, all of its fields must be Copy, and it must not implement Drop. No type can implement both Copy and Drop (ie. they are mutually exclusive: needs_drop<impl Copy> is always false, and needs_drop<impl Drop> is always true).

Naturally, I should not impl Copy for MyBox because that would break the uniqueness invariant, and more specifically - when I drop one copy and the memory is released I'll still have the other copy with a dangling pointer.

This specific situation has always been impossible. If your type "manages" some resources (RAII etc), it or one of its fields must impl Drop, and therefore the compiler will reject any attempt to impl Copy for MyBox.

But I can impl Copy for MyBox in a different module of the same crate.

This is technically an issue for all traits, but Copy is special in that adding a safe impl might cause UB (depending on your API guarantees). I'm not sure how severe it is, as such an impl could only be added within the crate (ie. code that you control, so as long as you don't write such an impl your code is fine).

That being said, there are workarounds to ensure that the compiler will reject such an impl:

impl Copy for Foo can be prevented with an empty Drop impl:

impl Drop for Foo {
    fn drop(&mut self) {}
}

or by adding a non copy ZST field:

struct NotCopyZst;

struct Foo {
    something: *const u8,
    _marker: NotCopyZst,
}

An empty Drop impl works almost like an impl !Copy for Foo {} (however, it does prohibit destructuring and makes the type always !needs_drop). The !Copy ZST field avoids these restrictions, but is perhaps less obvious. Maybe we want to add some kind of impl !Copy for Foo {}.

[RalfJung]

impl Copy for Foo can be prevented with an empty Drop impl:

That's a cute hack but does not solve the fundamental problem. I should be able to attach invariants to my fields without having a Drop impl.

[Sky9x]

I think the solution here is negative impls.

For example, if I have some type where I've opted out of Unpin via a PhantomPinned marker field, there is once again nothing stopping an impl Unpin for Foo {} from being written outside of the defining file (and no cute hacks like for Copy). An impl !Unpin for Foo {} would solve that.

[ia0]

There are many "solutions" to this "problem". From the zulip thread:

• MyBox implements Drop
• MyBox implements !Copy
• MyBox moves to its own crate
• MyBox reminds unsafe reviewers that there is only one crate author (not one author per module) and so the scope of unsafe is the crate
• Copy can't be derived without visibility on the fields
• Copy becomes an unsafe trait

I'm using quotes because it's not clear whether it's a problem, due to the lack of real examples. There are at least 2 ways to describe the problem from a theoretical point of view:

• Violation of privacy rules, as described in the issue description.
• Non-modular semantics, as advertised by some unsafe authors and mostly reviewers. When authoring or reviewing unsafe code, one needs to understand the semantics of the code. The more modular (in terms of granularity) the semantics is, the easier it is to understand. Concepts like privacy (see above) is a way to provide some form of modularity. The orphan rule is another form (although at crate granularity rather than module granularity). The problem here is that some unsafe authors and reviewers are not happy with the orphan rule for Copy, because it's a trait that affects the semantics of code, making it less modular to understand the semantics of code.