Niches of `Cell` and others still not hidden

[QuineDot]

Original conversation can be found in this IRLO thread, including a request from @RalfJung to file a bug report.

PR 68491 was accepted to hide niches of types that are in an UnsafeCell. However, experiments show that niches are still exposed within Cell, Mutex, and others.

Consider this code, adapted from a playground by @SkiFire13. It currently runs on the playground without panicking and optimizes down to a ret in the Godbolt explorer.

    assert_eq!( 8, mem::size_of::<       UnsafeCell<&()> >()); //  8
    assert_eq!(16, mem::size_of::<Option<UnsafeCell<&()>>>()); // 16 (✗ niche opt)
    assert_eq!( 8, mem::size_of::<             Cell<&()> >()); //  8
    assert_eq!( 8, mem::size_of::<Option<      Cell<&()>>>()); //  8 (✓ niche opt)
    assert_eq!(16, mem::size_of::<          RefCell<&()> >()); // 16
    assert_eq!(24, mem::size_of::<Option<   RefCell<&()>>>()); // 24 (✗ niche opt)
    assert_eq!(24, mem::size_of::<           RwLock<&()> >()); // 24
    assert_eq!(24, mem::size_of::<Option<    RwLock<&()>>>()); // 24 (✓ niche opt)
    assert_eq!(24, mem::size_of::<            Mutex<&()> >()); // 24
    assert_eq!(24, mem::size_of::<Option<     Mutex<&()>>>()); // 24 (✓ niche opt)

I would have expected Option<Cell<&()>> to not exhibit the niche-exploiting size optimization. The RwLock and Mutex cases may or may not be problematic, depending on their platform-specific implementations. But Cell in particular is a #[repr(transparent)] wrapper around UnsafeCell.

Here is another playground by @SkiFire13 from which they observed:

Even more interesting, looks like the current behaviour of a type that contains an UnsafeCell is to allow niche optimizations only if it doesn't contains something else before the UnsafeCell, and that something is not a ZST.

This indicates that the behavior is not Cell-specific. And although there has been some discussion about revealing the niches for Cell in particular, the discussion in the PR and in this hackmd seem to indicate that the niches being exploited is not intentional, even for Cell. The behavior can be observed back until Rust 1.43 when the PR landed.

Meta

Current playground (all versions) and Godbolt on all versions I tried from 1.43 forward.

[SkiFire13]

(Tiny correction to give proper credits: the first playground link you posted (the one with Mutex ecc ecc) is pretty much the one from @CAD97 from here. My playground (the one with custom types) is also adapted from that one.)

[RalfJung]

Cc @rust-lang/lang @eddyb

[RustyYato]

Note: Mutex and RwLock are probably using the niche in Box on some platforms. (Raw os locking primitives need to be boxed on some platforms). Otherwise we'd run into this bug

#68206

[eddyb]

I guess this logic introduced by #68491 isn't useful unless Cell also had #[repr(no_niche)]:

rust/compiler/rustc_middle/src/ty/layout.rs

Lines 956 to 960 in 18840b0

	let niche = if def.repr.hide_niche() {
	None
	} else {
	Niche::from_scalar(dl, Size::ZERO, scalar.clone())
	};

The issue is that UnsafeCell's niche was removed, but it was only done without touching the abi field, so it still looks like there's an available niche, it's just that the "cached computation" is missing it. And Cell being a newtype means the niche is rediscovered from abi being Scalar with its validity range having a hole in it (at 0).

What needs to happen for #[repr(no_niche)] (tho tbh I would kind of prefer doing it for the UnsafeCell lang item, I don't remember if there was a reason to add a new #[repr] instead) is what we do for unions, i.e. resetting validity ranges to the full size for the bitwidth of the scalar:

rust/compiler/rustc_middle/src/ty/layout.rs

Lines 836 to 848 in 18840b0

	// Normalize scalar_unit to the maximal valid range
	let field_abi = match &field.abi {
	Abi::Scalar(x) => Abi::Scalar(scalar_unit(x.value)),
	Abi::ScalarPair(x, y) => {
	Abi::ScalarPair(scalar_unit(x.value), scalar_unit(y.value))
	}
	Abi::Vector { element: x, count } => {
	Abi::Vector { element: scalar_unit(x.value), count: *count }
	}
	Abi::Uninhabited | Abi::Aggregate { .. } => {
	Abi::Aggregate { sized: true }
	}
	};

However, that might impact e.g. miri negatively, as it may make it seem like Cell<&T> could be null. Then again, IIRC miri's such validation has to traverse the fields, so it would see the &T field inside the UnsafeCell anyway.

[RalfJung]

However, that might impact e.g. miri negatively, as it may make it seem like Cell<&T> could be null. Then again, IIRC miri's such validation has to traverse the fields, so it would see the &T field inside the UnsafeCell anyway.

Miri should be fine -- it both checks the "validity range" in abi::Scalar, and recursively traverses the fields to look at their types.

[joshtriplett]

Can we get some clarification on what the question being asked of @rust-lang/lang is here? Is the desire here to fix the semantics (making all of the cell types not have niches)? Or is there a question about the correct semantics? I see in the internals thread several questions about what transmutes are valid (e.g. between Option<NonZeroU32> and u32), but this issue doesn't seem to be referencing those bits of the internals thread.

Could we get a specific ask to go with this nomination?

[pnkfelix]

It sounds like I should have explicitly included testing of Cell when I wrote PR #68491. At least, based on all the dialogue I can find, we were all operating under the assumption that removing the niche from UnsafeCell would imply it would also be removed from Cell (unless some action were taken to remove it from Cell as well), while the tests given on this issue demonstrate that the assumption did not hold.

However, these experimental results do concern me a little bit. Do they imply that under the current implementation, any wrapper type around UnsafeCell is also going to re-expose the niche? (Or just transparent wrapper types around UnsafeCell?)

Anyway, I'm pretty sure we are justified to make changes to hide any niche under Cell et al. There may be questions of how we deploy that change; its not something we can easily issue a future-incompatibility warning for. And I'm not sure it belongs under an edition boundary. Maybe best to go review how the original deployment for UnsafeCell was handled...

[SkiFire13]

Do they imply that under the current implementation, any wrapper type around UnsafeCell is also going to re-expose the niche? (Or just transparent wrapper types around UnsafeCell?)

Any wrapper that has a non-zero-sized field before the UnsafeCell. #[repr(transparent)] is not required. See this playground (also linked in the first post)

[RalfJung]

Can we get some clarification on what the question being asked of @rust-lang/lang is here?

Basically, the question is: should Cell hide its niches? And what about other user-defined UnsafeCell wrappers?

I think the intention was that yes the niche should be hidden for all UnsafeCell wrappers. But I wanted to confirm with t-lang to be sure.

[pnkfelix]

Do they imply that under the current implementation, any wrapper type around UnsafeCell is also going to re-expose the niche? (Or just transparent wrapper types around UnsafeCell?)

Any wrapper that has a non-zero-sized field before the UnsafeCell. #[repr(transparent)] is not required. See this playground (also linked in the first post)

I was pretty confused by this response, because I took it to mean that the niche is only reexposed when there is another field before the UnsafeCell.

After reading the linked playground, I think that interpretation was backwards. I.e., I think the intended statement of the current behavior is that: Any struct holding an UnsafeCell that has a non-zero-sized field before the UnsafeCell will not re-expose any niche within the UnsafeCell.

(Which sounds pretty weird, in terms of why that ends up being the case where the rule is enforced...)

Anyway, I'm personally in agreement with @RalfJung that the intention was to hide the niche for all occurrences of UnsafeCell in another data type. All of the dialogue I can find supports that interpretation. It looks like I just wildly missed the mark in my implementation.

[SkiFire13]

I was pretty confused by this response, because I took it to mean that the niche is only reexposed when there is another field before the UnsafeCell.

After reading the linked playground, I think that interpretation was backwards. I.e., I think the intended statement of the current behavior is that: Any struct holding an UnsafeCell that has a non-zero-sized field before the UnsafeCell will not re-expose any niche within the UnsafeCell.

You're right, I wrote it backwards. Your final interpretation is the right one.

[eddyb]

Anyway, I'm personally in agreement with @RalfJung that the intention was to hide the niche for all occurrences of UnsafeCell in another data type.

My assumption (in #87341 (comment)) was that the initial implementation "just" has a bug, where it's hiding niches but not invalid values (which give rise to niches in the first place).

There may be questions of how we deploy that change; its not something we can easily issue a future-incompatibility warning for. And I'm not sure it belongs under an edition boundary. Maybe best to go review how the original deployment for UnsafeCell was handled...

Did we guarantee anything around Cell? Feels like a crater run might be enough.