1
Fork 0

Auto merge of #128543 - RalfJung:const-interior-mut, r=fee1-dead

const-eval interning: accept interior mutable pointers in final value

…but keep rejecting mutable references

This fixes https://github.com/rust-lang/rust/issues/121610 by no longer firing the lint when there is a pointer with interior mutability in the final value of the constant. On stable, such pointers can be created with code like:
```rust
pub enum JsValue {
    Undefined,
    Object(Cell<bool>),
}
impl Drop for JsValue {
    fn drop(&mut self) {}
}
// This does *not* get promoted since `JsValue` has a destructor.
// However, the outer scope rule applies, still giving this 'static lifetime.
const UNDEFINED: &JsValue = &JsValue::Undefined;
```
It's not great to accept such values since people *might* think that it is legal to mutate them with unsafe code. (This is related to how "infectious" `UnsafeCell` is, which is a [wide open question](https://github.com/rust-lang/unsafe-code-guidelines/issues/236).) However, we [explicitly document](https://doc.rust-lang.org/reference/behavior-considered-undefined.html) that things created by `const` are immutable. Furthermore, we also accept the following even more questionable code without any lint today:
```rust
let x: &'static Option<Cell<i32>> = &None;
```
This is even more questionable since it does *not* involve a `const`, and yet still puts the data into immutable memory. We could view this as promotion [potentially introducing UB](https://github.com/rust-lang/unsafe-code-guidelines/issues/493). However, we've accepted this since ~forever and it's [too late to reject this now](https://github.com/rust-lang/rust/pull/122789); the pattern is just too useful.

So basically, if you think that `UnsafeCell` should be tracked fully precisely, then you should want the lint we currently emit to be removed, which this PR does. If you think `UnsafeCell` should "infect" surrounding `enum`s, the big problem is really https://github.com/rust-lang/unsafe-code-guidelines/issues/493 which does not trigger the lint -- the cases the lint triggers on are actually the "harmless" ones as there is an explicit surrounding `const` explaining why things end up being immutable.

What all this goes to show is that the hard error added in https://github.com/rust-lang/rust/pull/118324 (later turned into the future-compat lint that I am now suggesting we remove) was based on some wrong assumptions, at least insofar as it concerns shared references. Furthermore, that lint does not help at all for the most problematic case here where the potential UB is completely implicit. (In fact, the lint is actively in the way of [my preferred long-term strategy](https://github.com/rust-lang/unsafe-code-guidelines/issues/493#issuecomment-2028674105) for dealing with this UB.) So I think we should go back to square one and remove that error/lint for shared references. For mutable references, it does seem to work as intended, so we can keep it. Here it serves as a safety net in case the static checks that try to contain mutable references to the inside of a const initializer are not working as intended; I therefore made the check ICE to encourage users to tell us if that safety net is triggered.

Closes https://github.com/rust-lang/rust/issues/122153 by removing the lint.

Cc `@rust-lang/opsem` `@rust-lang/lang`
This commit is contained in:
bors 2024-09-14 21:11:04 +00:00
commit 9b72238eb8
23 changed files with 225 additions and 578 deletions

View file

@ -80,14 +80,23 @@ pub trait Provenance: Copy + fmt::Debug + 'static {
}
/// The type of provenance in the compile-time interpreter.
/// This is a packed representation of an `AllocId` and an `immutable: bool`.
/// This is a packed representation of:
/// - an `AllocId` (non-zero)
/// - an `immutable: bool`
/// - a `shared_ref: bool`
///
/// with the extra invariant that if `immutable` is `true`, then so
/// is `shared_ref`.
#[derive(Copy, Clone, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub struct CtfeProvenance(NonZero<u64>);
impl From<AllocId> for CtfeProvenance {
fn from(value: AllocId) -> Self {
let prov = CtfeProvenance(value.0);
assert!(!prov.immutable(), "`AllocId` with the highest bit set cannot be used in CTFE");
assert!(
prov.alloc_id() == value,
"`AllocId` with the highest bits set cannot be used in CTFE"
);
prov
}
}
@ -103,12 +112,14 @@ impl fmt::Debug for CtfeProvenance {
}
const IMMUTABLE_MASK: u64 = 1 << 63; // the highest bit
const SHARED_REF_MASK: u64 = 1 << 62;
const ALLOC_ID_MASK: u64 = u64::MAX & !IMMUTABLE_MASK & !SHARED_REF_MASK;
impl CtfeProvenance {
/// Returns the `AllocId` of this provenance.
#[inline(always)]
pub fn alloc_id(self) -> AllocId {
AllocId(NonZero::new(self.0.get() & !IMMUTABLE_MASK).unwrap())
AllocId(NonZero::new(self.0.get() & ALLOC_ID_MASK).unwrap())
}
/// Returns whether this provenance is immutable.
@ -117,10 +128,38 @@ impl CtfeProvenance {
self.0.get() & IMMUTABLE_MASK != 0
}
/// Returns whether this provenance is derived from a shared reference.
#[inline]
pub fn shared_ref(self) -> bool {
self.0.get() & SHARED_REF_MASK != 0
}
pub fn into_parts(self) -> (AllocId, bool, bool) {
(self.alloc_id(), self.immutable(), self.shared_ref())
}
pub fn from_parts((alloc_id, immutable, shared_ref): (AllocId, bool, bool)) -> Self {
let prov = CtfeProvenance::from(alloc_id);
if immutable {
// This sets both flags, so we don't even have to check `shared_ref`.
prov.as_immutable()
} else if shared_ref {
prov.as_shared_ref()
} else {
prov
}
}
/// Returns an immutable version of this provenance.
#[inline]
pub fn as_immutable(self) -> Self {
CtfeProvenance(self.0 | IMMUTABLE_MASK)
CtfeProvenance(self.0 | IMMUTABLE_MASK | SHARED_REF_MASK)
}
/// Returns a "shared reference" (but not necessarily immutable!) version of this provenance.
#[inline]
pub fn as_shared_ref(self) -> Self {
CtfeProvenance(self.0 | SHARED_REF_MASK)
}
}

View file

@ -165,8 +165,7 @@ impl<'tcx, E: TyEncoder<I = TyCtxt<'tcx>>> Encodable<E> for AllocId {
impl<'tcx, E: TyEncoder<I = TyCtxt<'tcx>>> Encodable<E> for CtfeProvenance {
fn encode(&self, e: &mut E) {
self.alloc_id().encode(e);
self.immutable().encode(e);
self.into_parts().encode(e);
}
}
@ -295,10 +294,8 @@ impl<'tcx, D: TyDecoder<I = TyCtxt<'tcx>>> Decodable<D> for AllocId {
impl<'tcx, D: TyDecoder<I = TyCtxt<'tcx>>> Decodable<D> for CtfeProvenance {
fn decode(decoder: &mut D) -> Self {
let alloc_id: AllocId = Decodable::decode(decoder);
let prov = CtfeProvenance::from(alloc_id);
let immutable: bool = Decodable::decode(decoder);
if immutable { prov.as_immutable() } else { prov }
let parts = Decodable::decode(decoder);
CtfeProvenance::from_parts(parts)
}
}

View file

@ -75,11 +75,9 @@ impl<'a> HashStable<StableHashingContext<'a>> for mir::interpret::AllocId {
}
}
// CtfeProvenance is an AllocId and a bool.
impl<'a> HashStable<StableHashingContext<'a>> for mir::interpret::CtfeProvenance {
fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
self.alloc_id().hash_stable(hcx, hasher);
self.immutable().hash_stable(hcx, hasher);
self.into_parts().hash_stable(hcx, hasher);
}
}