Auto merge of rust-lang#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 rust-lang#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](rust-lang/unsafe-code-guidelines#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](rust-lang/unsafe-code-guidelines#493). However, we've accepted this since ~forever and it's [too late to reject this now](rust-lang#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 rust-lang/unsafe-code-guidelines#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 rust-lang#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](rust-lang/unsafe-code-guidelines#493 (comment)) 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 rust-lang#122153 by removing the lint.

Cc `@rust-lang/opsem` `@rust-lang/lang`

Author: bors

compiler/rustc_const_eval/src/check_consts/check.rs

@@ -538,8 +538,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_is_transient(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))]
@@ -100,18 +98,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());
         }
     }
 
@@ -443,7 +438,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

@@ -718,16 +718,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

@@ -20,6 +20,7 @@ use rustc_hir as hir;
 use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs;
 use rustc_middle::mir::interpret::{ConstAllocation, CtfeProvenance, InterpResult};
 use rustc_middle::query::TyCtxtAt;
+use rustc_middle::span_bug;
 use rustc_middle::ty::layout::TyAndLayout;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::sym;
@@ -223,45 +224,59 @@ 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 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 allowing interior-mutable shared
+        // references: they can actually be created in safe code while pointing to apparently
+        // "immutable" values, via promotion or tail expression lifetime extension of
+        // `&None::<Cell<T>>`.
+        // We also exclude promoteds from this as `&mut []` can be promoted, which is a mutable
+        // reference pointing to an immutable (zero-sized) allocation. We rely on the promotion
+        // analysis not screwing up to ensure that it is sound to intern promoteds as immutable.
         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)
-            {
+            let is_already_global = ecx.tcx.try_get_global_alloc(alloc_id).is_some();
+            if is_already_global && !just_interned.contains(&alloc_id) {
                 // This is a pointer to some memory from another constant. We encounter mutable
                 // pointers to such memory since we do not always track immutability through
                 // these "global" pointers. Allowing them is harmless; the point of these checks
                 // during interning is to justify why we intern the *new* allocations immutably,
-                // so we can completely ignore existing allocations. We also don't need to add
-                // this to the todo list, since after all it is already interned.
+                // so we can completely ignore existing allocations.
+                // We can also skip the rest of this loop iteration, since after all it is already
+                // interned.
                 continue;
             }
-            // Found a mutable pointer 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
-            // promoteds as immutable.
-            trace!("found bad mutable pointer");
-            // Prefer dangling pointer errors over mutable pointer errors
-            if result.is_ok() {
-                result = Err(InternResult::FoundBadMutablePointer);
+            // If this is a dangling pointer, that's actually fine -- the problematic case is
+            // when there is memory there that someone might expect to be mutable, but we make it immutable.
+            let dangling = !is_already_global && !ecx.memory.alloc_map.contains_key(&alloc_id);
+            if !dangling {
+                // Found a mutable reference inside a const where inner allocations should be
+                // immutable.
+                if !ecx.tcx.sess.opts.unstable_opts.unleash_the_miri_inside_of_you {
+                    span_bug!(
+                        ecx.tcx.span,
+                        "the static const safety checks accepted mutable references they should not have accepted"
+                    );
+                }
+                // Prefer dangling pointer errors over mutable pointer errors
+                if result.is_ok() {
+                    result = Err(InternResult::FoundBadMutablePointer);
+                }
             }
         }
         if ecx.tcx.try_get_global_alloc(alloc_id).is_some() {
             // Already interned.
             debug_assert!(!ecx.memory.alloc_map.contains_key(&alloc_id));
             continue;
         }
-        just_interned.insert(alloc_id);
         // We always intern with `inner_mutability`, and furthermore we ensured above that if
         // that is "immutable", then there are *no* mutable pointers anywhere in the newly
         // interned memory -- justifying that we can indeed intern immutably. However this also
@@ -272,6 +287,7 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval
         // pointers before deciding which allocations can be made immutable; but for now we are
         // okay with losing some potential for immutability here. This can anyway only affect
         // `static mut`.
+        just_interned.insert(alloc_id);
         match intern_shallow(ecx, alloc_id, inner_mutability) {
             Ok(nested) => todo.extend(nested),
             Err(()) => {

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -14,14 +14,14 @@ use hir::def::DefKind;
 use rustc_ast::Mutability;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir as hir;
-use rustc_middle::bug;
 use rustc_middle::mir::interpret::ValidationErrorKind::{self, *};
 use rustc_middle::mir::interpret::{
     alloc_range, ExpectedKind, InterpError, InvalidMetaKind, Misalignment, PointerKind, Provenance,
     UnsupportedOpInfo, ValidationErrorInfo,
 };
 use rustc_middle::ty::layout::{LayoutCx, LayoutOf, TyAndLayout};
 use rustc_middle::ty::{self, Ty, TyCtxt};
+use rustc_middle::{bug, span_bug};
 use rustc_span::symbol::{sym, Symbol};
 use rustc_target::abi::{
     Abi, FieldIdx, FieldsShape, Scalar as ScalarAbi, Size, VariantIdx, Variants, WrappingRange,
@@ -617,6 +617,13 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
                         if ptr_expected_mutbl == Mutability::Mut
                             && alloc_actual_mutbl == Mutability::Not
                         {
+                            if !self.ecx.tcx.sess.opts.unstable_opts.unleash_the_miri_inside_of_you
+                            {
+                                span_bug!(
+                                    self.ecx.tcx.span,
+                                    "the static const safety checks accepted mutable references they should not have accepted"
+                                );
+                            }
                             throw_validation_failure!(self.path, MutableRefToImmutable);
                         }
                         // In a const, everything must be completely immutable.

compiler/rustc_lint/src/lib.rs

@@ -579,6 +579,10 @@ fn register_builtins(store: &mut LintStore) {
          <https://github.com/rust-lang/rust/issues/107457> for more information",
     );
     store.register_removed("writes_through_immutable_pointer", "converted into hard error");
+    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,
@@ -2804,51 +2803,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.