const-eval interning: accpt interior mutable pointers in final value (but keep rejecting mutable references)

Author: RalfJung

compiler/rustc_const_eval/src/check_consts/check.rs

@@ -524,8 +524,9 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
                             // final value.
                             // Note: This is only sound if every local that has a `StorageDead` has a
                             // `StorageDead` in every control flow path leading to a `return` terminator.
-                            // The good news is that interning will detect if any unexpected mutable
-                            // pointer slips through.
+                            // If anything slips through, there's no safety net -- safe code can create
+                            // references to variants of `!Freeze` enums as long as that variant is `Freeze`,
+                            // so interning can't protect us here.
                             if self.local_has_storage_dead(place.local) {
                                 self.check_op(ops::TransientCellBorrow);
                             } else {

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -10,21 +10,19 @@ use rustc_middle::traits::Reveal;
 use rustc_middle::ty::layout::LayoutOf;
 use rustc_middle::ty::print::with_no_trimmed_paths;
 use rustc_middle::ty::{self, Ty, TyCtxt};
-use rustc_session::lint;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::{Span, DUMMY_SP};
 use rustc_target::abi::{self, Abi};
 use tracing::{debug, instrument, trace};
 
 use super::{CanAccessMutGlobal, CompileTimeInterpCx, CompileTimeMachine};
 use crate::const_eval::CheckAlignment;
-use crate::errors::{self, ConstEvalError, DanglingPtrInFinal};
 use crate::interpret::{
     create_static_alloc, eval_nullary_intrinsic, intern_const_alloc_recursive, throw_exhaust,
     CtfeValidationMode, GlobalId, Immediate, InternKind, InternResult, InterpCx, InterpError,
     InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking, StackPopCleanup,
 };
-use crate::CTRL_C_RECEIVED;
+use crate::{errors, CTRL_C_RECEIVED};
 
 // Returns a pointer to where the result lives
 #[instrument(level = "trace", skip(ecx, body))]
@@ -105,18 +103,15 @@ fn eval_body_using_ecx<'tcx, R: InterpretationResult<'tcx>>(
             return Err(ecx
                 .tcx
                 .dcx()
-                .emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
+                .emit_err(errors::DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
                 .into());
         }
         Err(InternResult::FoundBadMutablePointer) => {
-            // only report mutable pointers if there were no dangling pointers
-            let err_diag = errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind };
-            ecx.tcx.emit_node_span_lint(
-                lint::builtin::CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
-                ecx.machine.best_lint_scope(*ecx.tcx),
-                err_diag.span,
-                err_diag,
-            )
+            return Err(ecx
+                .tcx
+                .dcx()
+                .emit_err(errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind })
+                .into());
         }
     }
 
@@ -448,7 +443,12 @@ fn report_eval_error<'tcx>(
         error,
         DUMMY_SP,
         || super::get_span_and_frames(ecx.tcx, ecx.stack()),
-        |span, frames| ConstEvalError { span, error_kind: kind, instance, frame_notes: frames },
+        |span, frames| errors::ConstEvalError {
+            span,
+            error_kind: kind,
+            instance,
+            frame_notes: frames,
+        },
     )
 }
 

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -712,16 +712,29 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
         _kind: mir::RetagKind,
         val: &ImmTy<'tcx, CtfeProvenance>,
     ) -> InterpResult<'tcx, ImmTy<'tcx, CtfeProvenance>> {
-        // If it's a frozen shared reference that's not already immutable, make it immutable.
+        // If it's a frozen shared reference that's not already immutable, potentially make it immutable.
         // (Do nothing on `None` provenance, that cannot store immutability anyway.)
         if let ty::Ref(_, ty, mutbl) = val.layout.ty.kind()
             && *mutbl == Mutability::Not
-            && val.to_scalar_and_meta().0.to_pointer(ecx)?.provenance.is_some_and(|p| !p.immutable())
-            // That next check is expensive, that's why we have all the guards above.
-            && ty.is_freeze(*ecx.tcx, ecx.param_env)
+            && val
+                .to_scalar_and_meta()
+                .0
+                .to_pointer(ecx)?
+                .provenance
+                .is_some_and(|p| !p.immutable())
         {
+            // That next check is expensive, that's why we have all the guards above.
+            let is_immutable = ty.is_freeze(*ecx.tcx, ecx.param_env);
             let place = ecx.ref_to_mplace(val)?;
-            let new_place = place.map_provenance(CtfeProvenance::as_immutable);
+            let new_place = if is_immutable {
+                place.map_provenance(CtfeProvenance::as_immutable)
+            } else {
+                // Even if it is not immutable, remember that it is a shared reference.
+                // This allows it to become part of the final value of the constant.
+                // (See <https://github.com/rust-lang/rust/pull/128543> for why we allow this
+                // even when there is interior mutability.)
+                place.map_provenance(CtfeProvenance::as_shared_ref)
+            };
             Ok(ImmTy::from_immediate(new_place.to_ref(ecx), val.layout))
         } else {
             Ok(val.clone())

compiler/rustc_const_eval/src/errors.rs

@@ -35,13 +35,10 @@ pub(crate) struct NestedStaticInThreadLocal {
     pub span: Span,
 }
 
-#[derive(LintDiagnostic)]
+#[derive(Diagnostic)]
 #[diag(const_eval_mutable_ptr_in_final)]
 pub(crate) struct MutablePtrInFinal {
-    // rust-lang/rust#122153: This was marked as `#[primary_span]` under
-    // `derive(Diagnostic)`. Since we expect we may hard-error in future, we are
-    // keeping the field (and skipping it under `derive(LintDiagnostic)`).
-    #[skip_arg]
+    #[primary_span]
     pub span: Span,
     pub kind: InternKind,
 }

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -223,16 +223,20 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval
             continue;
         }
 
-        // Crucially, we check this *before* checking whether the `alloc_id`
-        // has already been interned. The point of this check is to ensure that when
-        // there are multiple pointers to the same allocation, they are *all* immutable.
-        // Therefore it would be bad if we only checked the first pointer to any given
-        // allocation.
+        // Ensure that this is is derived from a shared reference. Crucially, we check this *before*
+        // checking whether the `alloc_id` has already been interned. The point of this check is to
+        // ensure that when there are multiple pointers to the same allocation, they are *all*
+        // derived from a shared reference. Therefore it would be bad if we only checked the first
+        // pointer to any given allocation.
         // (It is likely not possible to actually have multiple pointers to the same allocation,
         // so alternatively we could also check that and ICE if there are multiple such pointers.)
+        // See <https://github.com/rust-lang/rust/pull/128543> for why we are checking for
+        // "shared reference" and not "immutable", i.e., for why we are allowed interior-mutable
+        // shared references: they can actually be created in safe code while pointing to apparently
+        // "immutable" values, via promotion of `&None::<Cell<T>>`.
         if intern_kind != InternKind::Promoted
             && inner_mutability == Mutability::Not
-            && !prov.immutable()
+            && !prov.shared_ref()
         {
             if ecx.tcx.try_get_global_alloc(alloc_id).is_some()
                 && !just_interned.contains(&alloc_id)
@@ -245,7 +249,7 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval
                 // this to the todo list, since after all it is already interned.
                 continue;
             }
-            // Found a mutable pointer inside a const where inner allocations should be
+            // Found a mutable reference inside a const where inner allocations should be
             // immutable. We exclude promoteds from this, since things like `&mut []` and
             // `&None::<Cell<i32>>` lead to promotion that can produce mutable pointers. We rely
             // on the promotion analysis not screwing up to ensure that it is sound to intern

compiler/rustc_lint/src/lib.rs

@@ -569,7 +569,11 @@ fn register_builtins(store: &mut LintStore) {
         "byte_slice_in_packed_struct_with_derive",
         "converted into hard error, see issue #107457 \
          <https://github.com/rust-lang/rust/issues/107457> for more information",
-    )
+    );
+    store.register_removed(
+        "const_eval_mutable_ptr_in_final_value",
+        "partially allowed now, otherwise turned into a hard error",
+    );
 }
 
 fn register_internals(store: &mut LintStore) {

compiler/rustc_lint_defs/src/builtin.rs

@@ -29,7 +29,6 @@ declare_lint_pass! {
         CENUM_IMPL_DROP_CAST,
         COHERENCE_LEAK_CHECK,
         CONFLICTING_REPR_HINTS,
-        CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
         CONST_EVALUATABLE_UNCHECKED,
         CONST_ITEM_MUTATION,
         DEAD_CODE,
@@ -2772,51 +2771,6 @@ declare_lint! {
     @feature_gate = strict_provenance;
 }
 
-declare_lint! {
-    /// The `const_eval_mutable_ptr_in_final_value` lint detects if a mutable pointer
-    /// has leaked into the final value of a const expression.
-    ///
-    /// ### Example
-    ///
-    /// ```rust
-    /// pub enum JsValue {
-    ///     Undefined,
-    ///     Object(std::cell::Cell<bool>),
-    /// }
-    ///
-    /// impl ::std::ops::Drop for JsValue {
-    ///     fn drop(&mut self) {}
-    /// }
-    ///
-    /// const UNDEFINED: &JsValue = &JsValue::Undefined;
-    ///
-    /// fn main() {
-    /// }
-    /// ```
-    ///
-    /// {{produces}}
-    ///
-    /// ### Explanation
-    ///
-    /// In the 1.77 release, the const evaluation machinery adopted some
-    /// stricter rules to reject expressions with values that could
-    /// end up holding mutable references to state stored in static memory
-    /// (which is inherently immutable).
-    ///
-    /// This is a [future-incompatible] lint to ease the transition to an error.
-    /// See [issue #122153] for more details.
-    ///
-    /// [issue #122153]: https://github.com/rust-lang/rust/issues/122153
-    /// [future-incompatible]: ../index.md#future-incompatible-lints
-    pub CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
-    Warn,
-    "detects a mutable pointer that has leaked into final value of a const expression",
-    @future_incompatible = FutureIncompatibleInfo {
-        reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
-        reference: "issue #122153 <https://github.com/rust-lang/rust/issues/122153>",
-    };
-}
-
 declare_lint! {
     /// The `const_evaluatable_unchecked` lint detects a generic constant used
     /// in a type.

compiler/rustc_middle/src/mir/interpret/pointer.rs

@@ -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 flas, 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)
     }
 }
 

compiler/rustc_middle/src/ty/codec.rs

@@ -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)
     }
 }
 

compiler/rustc_middle/src/ty/impls_ty.rs

@@ -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);
     }
 }