Clarify UnsafeCell docs; fix #34496

[Manishearth]

No description provided.

[Manishearth]

r? @gankro or @steveklabnik

[euclio]

s/us/is

[Manishearth]

updated

[diwic]

👍

[aidanhs]

While these docs are being improved, could you mention that an UnsafeCell itself should never be mutable or you break its key functionality - #16022

[Manishearth]

could you mention that an UnsafeCell itself should never be mutable or you break its key functionality

I'm not quite sure what this means. It is possible to, for example, nest RefCells; e.g. RefCell<Vec<RefCell>>, giving you an &mut to a RefCell.

[Manishearth]

I don't see that issue being a problem, really. &mut is unique, even if there is an unsafecell behind it. UnsafeCell is not meant to be used to make &mut shareable, it is to be used to make & mutable. There is no key functionality being broken, that's &mut working as intended.

[aidanhs]

&mut is unique, even if there is an unsafecell behind it

Yes, but this may initially not be expected given the documentation has just talked about how UnsafeCell is special and lets you use mutably aliased data - reinforcing that it is not special when the UnsafeCell itself is mutably aliased doesn't hurt.

Concrete example:

fn main() {
    process::exit(foo() as i32);
}
fn foo() -> u8 {
    let mut x = UnsafeCell::new(1);
    let px = x.get();
    bar(&mut x, px);
    unsafe { x.into_inner() }
}
fn bar(v1: &mut UnsafeCell<u8>, v2: *mut u8) {
    unsafe {
        let a = *v1.get();
        *v2 = 3;
        *v1.get() = a;
    }
}

1.7.0 (the last version before &mut alias annotations were removed as an LLVM bug workaround) compiles this to return 3 (validly). My argument is that this UB caused by the two &mut isn't obvious to a first-time UnsafeCell user without a reinforcement that you should basically never have a &mut UnsafeCell because it 'overrides' the whole aliased mutable data thing (because, as you say, it's obeying the standard rules of &mut).

[aidanhs]

If you're not convinced then don't worry about it and I'll make a separate PR at some point.

[Manishearth]

Fixed

[bugaevc]

Typo: should be "other"

[steveklabnik]

@Manishearth just a few nits, and this PR is good to go

[Manishearth]

@bors r=steveklabnik

fixed nits

[bors]

📌 Commit 3873402 has been approved by steveklabnik

[bors]

⌛ Testing commit 3873402 with merge 1d588dc...

[bors]

💔 Test failed - auto-win-msvc-64-opt-rustbuild

[Manishearth]

@bors retry

[bors]

⌛ Testing commit 3873402 with merge ccd5daf...

[bors]

💔 Test failed - auto-win-msvc-64-opt

[steveklabnik]

@bors: retry

[bors]

⌛ Testing commit 3873402 with merge 007f721...

[bors]

⛄ The build was interrupted to prioritize another pull request.

[bors]

⌛ Testing commit 3873402 with merge 40f3ee2...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-no-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-no-mir, auto-win-msvc-64-opt-rustbuild
Approved by: steveklabnik
Pushing 40f3ee2 to master...