"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).