Skip to content
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

UnsafeCell should implement the Copy trait #25053

Closed
diwic opened this issue May 2, 2015 · 60 comments
Closed

UnsafeCell should implement the Copy trait #25053

diwic opened this issue May 2, 2015 · 60 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@diwic
Copy link
Contributor

diwic commented May 2, 2015

Currently UnsafeCell does not implement Copy. It would be beneficial if it could, for at least two reasons:

  • Easier initialization of fixed arrays, e g: [UnsafeCell::new(0i32); 75]
  • It enables people to make cell-like types which are Copy.

AFAIK, there are no disadvantages for UnsafeCell to implement Copy.

Note: the reason people can't just copy-and-paste the code for UnsafeCell to make their own variant with copy semantics, is that UnsafeCell is a #[lang="unsafe_cell"]: "The UnsafeCell<T> type is the only legal way to obtain aliasable data that is considered mutable. In general, transmuting an &T type into an &mut T is considered undefined behavior."

@mattico
Copy link
Contributor

mattico commented May 2, 2015

This seems like it would require an RFC:
Any semantic or syntactic change to the language that is not a bugfix.
https://github.com/rust-lang/rfcs

@ftxqxd
Copy link
Contributor

ftxqxd commented May 2, 2015

@mattico Making UnsafeCell implement Copy isn’t a change to the language, it’s a change to the standard library. The rules for what requires an RFC aren’t very clear, anyway.

@reem
Copy link
Contributor

reem commented May 3, 2015

If adding a trait impl to a type required an RFC we would never get anything done.

@alexcrichton
Copy link
Member

There is currently no stable and safe method to read the value in an UnsafeCell, and this is intentional. There is no knowledge about concurrent reads/writes/etc with an UnsafeCell, and introducing an unsynchronized read with an implicit Copy may produce undefined behavior in some situations.

@diwic
Copy link
Contributor Author

diwic commented May 3, 2015

@alexcrichton UnsafeCell is a building block for higher level abstractions, e g Cell, RefCell and AtomicUsize. This higher level abstraction determines whether unsynchronized reads are possible or not, by optionally deriving Copy (and/or Send/Sync) themselves. (Or by adding NoCopy markers.)
If UnsafeCell implements Copy, then the higher level abstractions are free to make this choice. The current situation makes this impossible.

Could you be more specific about the "undefined behavior in some situations"? And is this a problem with UnsafeCell in itself, or something that can be easily fixed in the type containing the UnsafeCell?

@alexcrichton
Copy link
Member

Yes each type which contains an UnsafeCell could also take on deciding these kinds of decisions, there's no absolutely fundamental reason why it does not implement Copy.

@diwic
Copy link
Contributor Author

diwic commented Jun 2, 2015

I was thinking about this again today and I'm wondering if it would be better to remove the unsafe_cell lang_item and instead introduce a new marker, e g AliasedMutRef or so, that would mean essentially the same (disable the assumption/optimisation that two mutable references can never refer to the same object)?

Then people could implement their own cells with what ever degree of unsafety they want.

@nox
Copy link
Contributor

nox commented May 5, 2016

We need this for FFI with C and C++ too, where it would be nice to be able to tell that some struct fields may change through unknown means.

@arielb1
Copy link
Contributor

arielb1 commented May 5, 2016

+1

@aturon aturon added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 5, 2016
@aturon
Copy link
Member

aturon commented May 5, 2016

Nominating for libs team discussion.

@alexcrichton
Copy link
Member

This libs team discussed this issue a few days ago and we unfortunately didn't reach a firm consensus. The case I mentioned above means that guaranteeing the safety of an unsafe block with an UnsafeCell would not also entail auditing all copies in addition to accesses. This does, however, somewhat align with the desire for safe methods like get_mut (to get a &mut T reference).

One idea brought up by @sfackler was perhaps something like unsafe impl Copy for T {} where the type T is then copyable regardless of contents (but is clearly unsafe, hence the requirement of unsafe). This idea hasn't been played out too much, though, and would certainly require an RFC.

One other concern here is that if UnsafeCell implements Copy that it would also necessitate an implementation of Clone, which may be more dubious.

@comex
Copy link
Contributor

comex commented Nov 27, 2016

Just ran into this.

@alexcrichton Can you think of any other cases in which it would make sense to unsafe impl Copy for something with a non-Copy field? Maybe something involving unions? I can't think of anything concrete.

If there aren't any, then special syntax seems unwarranted; if UnsafeCell shouldn't impl Copy, seems easiest to just create a second type that acts the same but does.

@comex
Copy link
Contributor

comex commented Dec 11, 2016

Another possible use case for unsafe impl Copy: bluss/arrayvec#32

Though it probably wouldn't be the best approach in that case. In short, ArrayVec is a Vec-like type backed by an inline [NoDrop<T>; N]. In general it needs a Drop impl to manually drop only elements up to len(), but ideally an ArrayVec with Copy elements would be Copy. One potential way to do that would be a hypothetical unsafe impl Copy that worked for types with Drop. Probably a superior approach would involve some way to bound the Drop impl on !Copy, though.

@diwic
Copy link
Contributor Author

diwic commented Dec 11, 2016

If it makes things easier, we could make an additional type CopyUnsafeCell (name to be bikeshedded), which is the lang item and implements copy (and a clone, which just does bit-copy), and then the UnsafeCell which wraps CopyUnsafeCell would just change its implementation to call CopyUnsafeCell, and it would automatically disable copy (by not deriving it).

As additional bonus, our regular Cell could then wrap CopyUnsafeCell instead of the current non-copy UnsafeCell and we'll be able to do [Cell::new(6u8); 1000].

@aturon
Copy link
Member

aturon commented Jan 4, 2017

cc @rust-lang/lang, anybody on the lang team have an opinion on this?

@nikomatsakis
Copy link
Contributor

I am in favor. UnsafeCell is a building block. It should be possible for people to build Copy types that use it if they so choose.

@nikomatsakis
Copy link
Contributor

Discussed this a bit with @aturon -- I agree with the consensus that it would be easy to write broken code if UnsafeCell: Copy. Something like unsafe impl Copy is also appealing as an alternative.

This reminds me of the discussion around a Pod trait, which was supposed to be for "things that could be copy, but where copying is likely a bug". We would up at that time deciding against it -- having a trait made for a lot of sharp edges iirc -- but thinking that bugs might be well served by something analogous to the #[must_use] lint, which could warn about implicit copies of things you don't want to copy.

Still, I feel like while having implicit copies might be dangerous from a race condition POV, it's also easy to imagine types that wouldn't have to worry about that. If my type is not Sync, for example, then it is fine to copy the data in the UnsafeCell. FFI might be another example.

@aturon
Copy link
Member

aturon commented Jan 21, 2017

This issue seems to be at a bit of a standstill.

  • On the one hand, there's a legit need for a Copy version of UnsafeCell, for things like FFI.
  • On the other hand, making UnsafeCell itself Copy is a potential footgun.

If we want to deal with both of these perspectives, we can rule out changing UnsafeCell itself. That leaves us with at least two options mentioned so far:

  • Providing a separate version of UnsafeCell that is copy (perhaps VeryUnsafeCell 😉)
  • Adding a mechanism like unsafe impl Copy

The former is clearly the easier approach, and personally I'm hesitant to add special casing to the handling of Copy impls just for this particular case.

Does anyone strongly object to adding some additional version of UnsafeCell that is Copy?

@nox
Copy link
Contributor

nox commented Jan 21, 2017

Doesn't VeryUnsafeCell mean yet another ad-hoc language item?

@eddyb
Copy link
Member

eddyb commented Jan 21, 2017

I thought @nikomatsakis had some more usecases for unsafe impl Copy in mind.

@diwic
Copy link
Contributor Author

diwic commented Jan 22, 2017

@nox, if VeryUnsafeCell will be the new lang item and UnsafeCell is just a non-copy wrapper around that, then the total amount of lang items remains unchanged.

@diwic
Copy link
Contributor Author

diwic commented Jan 22, 2017

For name bikeshedding, how about RawUnsafeCell?

@alexcrichton
Copy link
Member

@aturon I feel that adding a new type wouldn't actually solve the problem the problem I raised earlier. That'd still be a problem for anyone using VeryUnsafeCell, and presumably a bunch of types would want to move over to using that.

In that sense then if we must solve this specific issue then I'd be in favor of just adding the impl to UnsafeCell itself. Ideally we could find a totally separate route for solving this issue, but I'm not sure that's possible.

@comex
Copy link
Contributor

comex commented Jan 23, 2017

@alexcrichton Which types do you think would want to move over? As I see it, the only reason to use VeryUnsafeCell would be if you want your type to be Copy.

In any case, UnsafeCell is already !Sync; assuming VeryUnsafeCell is too, a naive user of UnsafeCell will be blocked from accessing it from multiple threads to start with. To get a bug, you need to either unsafe impl Sync or do something unsafe with FFI, e.g. calling a C function that writes to the pointer from another thread. The former is asking for trouble, and the latter is not really an UnsafeCell-specific problem (there are all sorts of ways to create footguns out of C functions with weird semantics).

@alexcrichton
Copy link
Member

Well presumably there's some motivation for some type that wants to move over, right? Otherwise this issue wouldn't exist? We may not have cases in the standard library itself but I would hate to have a convention of "use UnsafeCell but if you're serious use VeryUnsafeCell"

@comex
Copy link
Contributor

comex commented Jan 23, 2017

IMO, VeryUnsafeCell is probably the wrong name. If there is a separate type, it should be specifically for Copy and named as such, e.g. CopyUnsafeCell. It's not just a generic 'more low-level' version.

When it comes to motivations, I'm not totally sure it's wrong to just mark both UnsafeCell and Cell as Copy. If I have Vec<Cell<u8>>, it really should be eligible for copy_from_slice. If I have a Cell in my struct, it should still be usable in copy_arena::Arena and other APIs that require Copy for bookkeeping purposes (though arguably a better solution in this case could be to have a "trivial drop" auto trait separate from Copy). But... if this is considered inadvisable, the only option is for me to make my own version of Cell, for which I cannot avoid UnsafeCell for the reasons specified in the original post.

(sidenote: if UnsafeCell because a wrapper, people might be tempted to avoid it to improve the experience in a debugger, where at least IME you often have to manually traverse wrapper structs. Maybe #[repr(transparent)] could help with this? Or just use two lang items instead of a wrapper. Ultimately the 'right' solution is to build rustc and Rust syntax into the debugger so you can just use the normal accessor methods, but that's probably a long way off.)

@pythonesque
Copy link
Contributor

I think Cell is not Copy because it currently can't be thanks to UnsafeCell.

@RalfJung
Copy link
Member

I think ergonomics also played a rule, see my question from 2 years ago: https://users.rust-lang.org/t/why-is-cell-not-copy/2208

@pythonesque
Copy link
Contributor

I find those examples very unconvincing, but why not make sure people are super explicit, then, and make the CopyMove Copy implementation activated in some super weird way (like creating a new array off of the base element)?

@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 22, 2017
@strega-nil
Copy link
Contributor

strega-nil commented Jul 22, 2017

CopyOwn seems fine, but also... I'm not sure why we can't do both? It seems unreasonable, at least to me, to keep UnsafeCell from being Copy, given all the other arguments in this thread, and given that CopyOwn requires a lot of work (at least, I think).

@glaebhoerl
Copy link
Contributor

glaebhoerl commented Aug 21, 2017

@RalfJung

(and indeed, proving a type Copy in our formalism requires both of these to be established separately, neither of them sees to imply the other)

Could you explain how/why the second does not imply the first? Intuitively it seems to me that if you are given the ability to dereference an &T to make a new T (2.), then if you own a T (1.), you should be able to just make a reference to it (&T) and then invoke (2.).

@RalfJung
Copy link
Member

RalfJung commented Aug 21, 2017

Maybe that is an artifact of the framework we use for formalization. The reason is that (1) is an implication, saying that every possible way to justify the assertion "we own these bytes at type T" cannot possibly be making any exclusive "ownership"-style assumptions beyond owning these bytes. It has to all be duplicable. "ownership" here, as is common in correctness proofs of concurrent (and even sequential) programs, includes not only ownership of bytes in memory but also ownership of "ghost state", which is additional state that is tracked for the purpose of the proof but not actually present in the machine (e.g., this could be tracking which thread currently has "acquired" a RefCell even though no such tracking happens in the actual code). Property (2), on the other hand, is not a mere implication; we allow the proof of this to perform changes to the ghost state before producing a proof that the copy of the bytes is a new T. Since (1) must not make such changes, it cannot use (2).

Off the top of my head, I am not sure if we ever rely on (1) not making such changes. It certainly makes working with this much simpler though...

@glaebhoerl
Copy link
Contributor

Cc #60715, an actual use case which would break if UnsafeCell were made Copy (unless if Freeze were also exposed as a better alternative).

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label May 19, 2019
@RalfJung
Copy link
Member

Ouch, that's quite bad indeed. The alternative proposal of being able to unsafe impl<T> Copy for Cell<T> ("overriding" the compiler checks) would also suffer from this.

Basically, currently we have no interior-mutable-Copy type, but that is an accident. And we also have legit needs for Copy interior mutable types, which is irreconcilable with using Copy to exclude interior mutability.

@diwic
Copy link
Contributor Author

diwic commented May 19, 2019

Making changes such as making UnsafeCell copy would have been less risky in 2015 (when this bug was opened) than it is now, when people are experimenting with transactional memory and other interesting things.

Anyhow one basic argument still holds: It's trivial to make a !Copy wrapper around a Copy enabled UnsafeCell, but impossible to make a Copy wrapper around a !Copy UnsafeCell. Hence UnsafeCell should implement Copy.

@pythonesque
Copy link
Contributor

I'm guessing we need an RFC to add the ability to unsafe impl Copy for types that fulfill all the requirements? Or add CopyOwn? Given the potential to abuse Copy's guarantee not to include an UnsafeCell, and the fact that people seem very reluctant to make UnsafeCell itself Copy where T is (for understandable reasons, given that it is conflated with "also make the copy implicit" in current Rust), it seems like it might be better to do this sooner rather than later, so if we need one I'm happy to do the RFC and (if it's approved) whatever compiler work is needed.

Was there any particular objection to unsafe impl Copy for ...? Or CopyOwn for that matter (if it doesn't implicitly copy like Copy does), but I understand that the latter involves adding a brand new OIBIT trait (or whatever we're calling them these days) and that is always a hard sell.

@RalfJung
Copy link
Member

Was there any particular objection to unsafe impl Copy for ...?

I do not recall any, but there also was not a ton of discussion.

@tbu-
Copy link
Contributor

tbu- commented Aug 5, 2019

Not being able to assert that a struct is Copy also affects all structs that contain a Range<usize> or similar.

@m-ou-se
Copy link
Member

m-ou-se commented Aug 11, 2021

This doesn't seem to be waiting on a team decision, so removing I-needs-decision. Looks like this is waiting for a concrete proposal to be discussed by the team(s).

@m-ou-se m-ou-se removed the I-needs-decision Issue: In need of a decision. label Aug 11, 2021
@Kile-Asmussen
Copy link

I think the argument that a Copy interior mutability primitive is a 'potential footgun' can be alleviated with a deny(implicit_copy) lint.

@Dylan-DPC
Copy link
Member

Closing this as it would be better suited for an RFC

@Dylan-DPC Dylan-DPC closed this as not planned Won't fix, can't repro, duplicate, stale Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests