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

Cell should be Copy #20813

Closed
pythonesque opened this issue Jan 9, 2015 · 14 comments
Closed

Cell should be Copy #20813

pythonesque opened this issue Jan 9, 2015 · 14 comments

Comments

@pythonesque
Copy link
Contributor

I'm not really sure why it isn't. It can only ever hold Copy data and Cell::new(some_cell.get()) has the same effect as a memcpy, so the effect of not making it Copy is mostly to just make it annoying to use types with Cells in them (since you can't #[derive(Copy)] on them, use them with the [Foo ; n] instantiation syntax in fixed-size arrays, put them in other Cells, etc.).

@huonw
Copy link
Member

huonw commented Jan 10, 2015

I think Cell and RefCell are explicitly non-Copy as a sort-of lint against accidentally not sharing them, since they're usually meant to be used for shared data. E.g. with Cell: Copy

let x = Cell::new(1);

Thread::scoped(move || {
    x.set(2);
}).join();

println!("{}", x.get());

won't do what one might be hoping.

@reem
Copy link
Contributor

reem commented Jan 10, 2015

The lack of a [Foo; n] initializer when using Cell's is kind of a pain, but I agree with @huonw that it enables a lot of unintentional non-sharing.

@pythonesque
Copy link
Contributor Author

@huonw I would rather not use Copy as a lint. Not being able to use Celllike this is just irritating ergonomically. This clearly isn't a memory safety concern and I don't think it enables unintentional sharing more than, say, [U8; 3]. It's not the absolute end of the world if it doesn't for my own data structures (since I can just make my own version of Cell) but it means I can't put many other data structures inside a Cell, which actually affects program functionality. This is why I wanted "can copy" to be a different trait from "pod", BTW.

@ranma42
Copy link
Contributor

ranma42 commented Jan 10, 2015

Does the same rationale from #20199 (comment) apply here?

@Gankra
Copy link
Contributor

Gankra commented Jan 10, 2015

Yeah, I think we've settled on a "tricky things shouldn't be Copy" convention here. It's back-compat to add later, and right now the consequences are troubling.

@Gankra Gankra closed this as completed Jan 10, 2015
@pythonesque
Copy link
Contributor Author

I don't see what's "troubling" about the consequences here, can someone clarify? The only substantive thing I've heard is the concern that someone might accidentally not share memory, which can already happen with any other Copy type. It's neither a memory safety concern nor a security concern (like it is in the linked bug). And how often do you use a bare Cell (outside of another struct)? With Copy being opt-in I don't think this is a real concern outside of toy examples.

@huonw
Copy link
Member

huonw commented Jan 11, 2015

I don't personally have a huge opinion (other than the back-compat consideration), but Cell does have a fundamental difference to most other Copy types in that its essentially only purpose is in the context of shared memory.

It seems like this, and the considerations in #20199 (same thing applying to RNGs), would be entirely resolved by a extra trait, say trait MemcpyIsSafe { fn memcpy(&self) -> Self { /* do the memcpy */ } }, where a type still moves if it implements MemcpyIsSafe, but is known to be safe to use with Cell. (Copy could then inherit from MemcpyIsSafe and would only remove the move-by-default aspect.)

(The complexity may not be worth it.)

@pythonesque
Copy link
Contributor Author

Sure, we could abbreviate MemcpyIsSafe as Pod. I don't think it would be that much more complex, since most of the time it would be the same thing, but since we haven't done it from the beginning it would be hard to add backwards-compatibly (how do you decide whether an existing potentially Copy type that doesn't #[derive(Copy)] should be Pod?).

@Gankra
Copy link
Contributor

Gankra commented Jan 11, 2015

Whoops, sorry I keep ADD'ing away every time I start writing a comment here. You know, like, re-installing my entire OS. 🐫

Basically what @huonw said. In my mind Cell is basically a container or smaht-pointer type. It's not really "plain old data" (it contains an UnsafeCell!). But mostly I think we just want to be conservative here.

@pythonesque
Copy link
Contributor Author

But it is safe, regardless of how it exists in your, my, or anyone's mind. It's only ever allowed to contain plain old data. The fact that it contains UnsafeCell is sort of immaterial, I feel. There's no point in being conservative for the sake of it--what's going to change in the future that makes it more attractive to make Cell Copy than it already is?

Your UnsafeCell point is good though, because I just checked and it's not #[derive(Copy)] either! This actually affects me, because it means I can't make a type like Cell that is Copy. Should I open a separate ticket for this? I'd rather not have to entirely abandon the standard library just to get a Copy Cell.

@Gankra
Copy link
Contributor

Gankra commented Jan 11, 2015

CC @pcwalton (you're captain OIBIT, right?)

@Gankra
Copy link
Contributor

Gankra commented Jan 11, 2015

I think this can be a general "what to do about iffy cases and Cell" meta-issue. Tracking it on multiple issues doesn't seem super helpful at this point.

@DiamondLovesYou
Copy link
Contributor

This should be reopened. There are legitimate uses of Cell for caching in Copy structures. Despite what the Rust documentation leads us to believe, Cell should be thought as a tool to allow interior mutability, not whatever "shared mutable containers" means, since neither are box types (ie basically a pointer) so can't be shared by value anyway, outside of references.

I apologize, I know this is a pretty old issue, but the current behavior is surprising. At the very least UnsafeCell needs to be copy so one can at least bypass this issue using that structure.

@Mark-Simulacrum
Copy link
Member

I think making UnsafeCell implement Copy would need an RFC at the least, though it seems surface-level potentially reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants