interning cleanup: we no longer need to distinguish Const and ConstInner; we no longer need the ignore_interior_mut_in_const hack

Author: RalfJung

compiler/rustc_middle/src/mir/mod.rs

@@ -210,16 +210,6 @@ pub struct Body<'tcx> {
     /// We hold in this field all the constants we are not able to evaluate yet.
     pub required_consts: Vec<Constant<'tcx>>,
 
-    /// The user may be writing e.g. `&[(SOME_CELL, 42)][i].1` and this would get promoted, because
-    /// we'd statically know that no thing with interior mutability will ever be available to the
-    /// user without some serious unsafe code.  Now this means that our promoted is actually
-    /// `&[(SOME_CELL, 42)]` and the MIR using it will do the `&promoted[i].1` projection because
-    /// the index may be a runtime value. Such a promoted value is illegal because it has reachable
-    /// interior mutability. This flag just makes this situation very obvious where the previous
-    /// implementation without the flag hid this situation silently.
-    /// FIXME(oli-obk): rewrite the promoted during promotion to eliminate the cell components.
-    pub ignore_interior_mut_in_const_validation: bool,
-
     /// Does this body use generic parameters. This is used for the `ConstEvaluatable` check.
     ///
     /// Note that this does not actually mean that this body is not computable right now.
@@ -276,7 +266,6 @@ impl<'tcx> Body<'tcx> {
             var_debug_info,
             span,
             required_consts: Vec::new(),
-            ignore_interior_mut_in_const_validation: false,
             is_polymorphic: false,
             predecessor_cache: PredecessorCache::new(),
         };
@@ -306,7 +295,6 @@ impl<'tcx> Body<'tcx> {
             required_consts: Vec::new(),
             generator_kind: None,
             var_debug_info: Vec::new(),
-            ignore_interior_mut_in_const_validation: false,
             is_polymorphic: false,
             predecessor_cache: PredecessorCache::new(),
         };

compiler/rustc_mir/src/const_eval/eval_queries.rs

@@ -67,12 +67,7 @@ fn eval_body_using_ecx<'mir, 'tcx>(
             None => InternKind::Constant,
         }
     };
-    intern_const_alloc_recursive(
-        ecx,
-        intern_kind,
-        ret,
-        body.ignore_interior_mut_in_const_validation,
-    );
+    intern_const_alloc_recursive(ecx, intern_kind, ret);
 
     debug!("eval_body_using_ecx done: {:?}", *ret);
     Ok(ret)
@@ -373,7 +368,13 @@ pub fn eval_to_allocation_raw_provider<'tcx>(
             // Since evaluation had no errors, valiate the resulting constant:
             let validation = try {
                 // FIXME do not validate promoteds until a decision on
-                // https://github.com/rust-lang/rust/issues/67465 is made
+                // https://github.com/rust-lang/rust/issues/67465 and
+                // https://github.com/rust-lang/rust/issues/67534 is made.
+                // Promoteds can contain unexpected `UnsafeCell` and reference `static`s, but their
+                // otherwise restricted form ensures that this is still sound. We just lose the
+                // extra safety net of some of the dynamic checks. They can also contain invalid
+                // values, but since we do not usually check intermediate results of a computation
+                // for validity, it might be surprising to do that here.
                 if cid.promoted.is_none() {
                     let mut ref_tracking = RefTracking::new(mplace);
                     let mut inner = false;

compiler/rustc_mir/src/const_eval/mod.rs

@@ -29,7 +29,7 @@ pub(crate) fn const_caller_location(
     let mut ecx = mk_eval_cx(tcx, DUMMY_SP, ty::ParamEnv::reveal_all(), false);
 
     let loc_place = ecx.alloc_caller_location(file, line, col);
-    intern_const_alloc_recursive(&mut ecx, InternKind::Constant, loc_place, false);
+    intern_const_alloc_recursive(&mut ecx, InternKind::Constant, loc_place);
     ConstValue::Scalar(loc_place.ptr)
 }
 

compiler/rustc_mir/src/interpret/intern.rs

@@ -7,7 +7,7 @@ use super::validity::RefTracking;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_hir as hir;
 use rustc_middle::mir::interpret::InterpResult;
-use rustc_middle::ty::{self, layout::TyAndLayout, query::TyCtxtAt, Ty};
+use rustc_middle::ty::{self, layout::TyAndLayout, Ty};
 use rustc_target::abi::Size;
 
 use rustc_ast::Mutability;
@@ -40,11 +40,6 @@ struct InternVisitor<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> {
     /// This field stores whether we are *currently* inside an `UnsafeCell`. This can affect
     /// the intern mode of references we encounter.
     inside_unsafe_cell: bool,
-
-    /// This flag is to avoid triggering UnsafeCells are not allowed behind references in constants
-    /// for promoteds.
-    /// It's a copy of `mir::Body`'s ignore_interior_mut_in_const_validation field
-    ignore_interior_mut_in_const: bool,
 }
 
 #[derive(Copy, Clone, Debug, PartialEq, Hash, Eq)]
@@ -53,22 +48,14 @@ enum InternMode {
     /// this is *immutable*, and below mutable references inside an `UnsafeCell`, this
     /// is *mutable*.
     Static(hir::Mutability),
-    /// The "base value" of a const, which can have `UnsafeCell` (as in `const FOO: Cell<i32>`),
-    /// but that interior mutability is simply ignored.
-    ConstBase,
-    /// The "inner values" of a const with references, where `UnsafeCell` is an error.
-    ConstInner,
+    /// A `const`.
+    Const,
 }
 
 /// Signalling data structure to ensure we don't recurse
 /// into the memory of other constants or statics
 struct IsStaticOrFn;
 
-fn mutable_memory_in_const(tcx: TyCtxtAt<'_>, kind: &str) {
-    // FIXME: show this in validation instead so we can point at where in the value the error is?
-    tcx.sess.span_err(tcx.span, &format!("mutable memory ({}) is not allowed in constant", kind));
-}
-
 /// Intern an allocation without looking at its children.
 /// `mode` is the mode of the environment where we found this pointer.
 /// `mutablity` is the mutability of the place to be interned; even if that says
@@ -165,17 +152,13 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir
         mplace: MPlaceTy<'tcx>,
         fields: impl Iterator<Item = InterpResult<'tcx, Self::V>>,
     ) -> InterpResult<'tcx> {
+        // ZSTs cannot contain pointers, so we can skip them.
+        if mplace.layout.is_zst() {
+            return Ok(());
+        }
+
         if let Some(def) = mplace.layout.ty.ty_adt_def() {
             if Some(def.did) == self.ecx.tcx.lang_items().unsafe_cell_type() {
-                if self.mode == InternMode::ConstInner && !self.ignore_interior_mut_in_const {
-                    // We do not actually make this memory mutable.  But in case the user
-                    // *expected* it to be mutable, make sure we error.  This is just a
-                    // sanity check to prevent users from accidentally exploiting the UB
-                    // they caused.  It also helps us to find cases where const-checking
-                    // failed to prevent an `UnsafeCell` (but as `ignore_interior_mut_in_const`
-                    // shows that part is not airtight).
-                    //mutable_memory_in_const(self.ecx.tcx, "`UnsafeCell`");
-                }
                 // We are crossing over an `UnsafeCell`, we can mutate again. This means that
                 // References we encounter inside here are interned as pointing to mutable
                 // allocations.
@@ -187,11 +170,6 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir
             }
         }
 
-        // ZSTs cannot contain pointers, so we can skip them.
-        if mplace.layout.is_zst() {
-            return Ok(());
-        }
-
         self.walk_aggregate(mplace, fields)
     }
 
@@ -211,7 +189,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir
                 if let Scalar::Ptr(vtable) = mplace.meta.unwrap_meta() {
                     // Explicitly choose const mode here, since vtables are immutable, even
                     // if the reference of the fat pointer is mutable.
-                    self.intern_shallow(vtable.alloc_id, InternMode::ConstInner, None);
+                    self.intern_shallow(vtable.alloc_id, InternMode::Const, None);
                 } else {
                     // Validation will error (with a better message) on an invalid vtable pointer.
                     // Let validation show the error message, but make sure it *does* error.
@@ -223,7 +201,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir
             // Only recurse for allocation-backed pointers.
             if let Scalar::Ptr(ptr) = mplace.ptr {
                 // Compute the mode with which we intern this. Our goal here is to make as many
-                // statics as we can immutable so they can be placed in const memory by LLVM.
+                // statics as we can immutable so they can be placed in read-only memory by LLVM.
                 let ref_mode = match self.mode {
                     InternMode::Static(mutbl) => {
                         // In statics, merge outer mutability with reference mutability and
@@ -257,27 +235,11 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir
                             }
                         }
                     }
-                    InternMode::ConstBase | InternMode::ConstInner => {
-                        // Ignore `UnsafeCell`, everything is immutable.  Do some sanity checking
-                        // for mutable references that we encounter -- they must all be ZST.
-                        // This helps to prevent users from accidentally exploiting UB that they
-                        // caused (by somehow getting a mutable reference in a `const`).
-                        if ref_mutability == Mutability::Mut {
-                            /*match referenced_ty.kind() {
-                                ty::Array(_, n) if n.eval_usize(*tcx, self.ecx.param_env) == 0 => {}
-                                ty::Slice(_)
-                                    if mplace.meta.unwrap_meta().to_machine_usize(self.ecx)?
-                                        == 0 => {}
-                                _ => mutable_memory_in_const(tcx, "`&mut`"),
-                            }*/
-                        } else {
-                            // A shared reference. We cannot check `freeze` here due to references
-                            // like `&dyn Trait` that are actually immutable.  We do check for
-                            // concrete `UnsafeCell` when traversing the pointee though (if it is
-                            // a new allocation, not yet interned).
-                        }
-                        // Go on with the "inner" rules.
-                        InternMode::ConstInner
+                    InternMode::Const => {
+                        // Ignore `UnsafeCell`, everything is immutable.  Validity does some sanity
+                        // checking for mutable references that we encounter -- they must all be
+                        // ZST.
+                        InternMode::Const
                     }
                 };
                 match self.intern_shallow(ptr.alloc_id, ref_mode, Some(referenced_ty)) {
@@ -316,7 +278,6 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
     ecx: &mut InterpCx<'mir, 'tcx, M>,
     intern_kind: InternKind,
     ret: MPlaceTy<'tcx>,
-    ignore_interior_mut_in_const: bool,
 ) where
     'tcx: 'mir,
 {
@@ -325,7 +286,7 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
         InternKind::Static(mutbl) => InternMode::Static(mutbl),
         // `Constant` includes array lengths.
         // `Promoted` includes non-`Copy` array initializers and `rustc_args_required_const` arguments.
-        InternKind::Constant | InternKind::Promoted => InternMode::ConstBase,
+        InternKind::Constant | InternKind::Promoted => InternMode::Const,
     };
 
     // Type based interning.
@@ -355,7 +316,6 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
             ecx,
             mode,
             leftover_allocations,
-            ignore_interior_mut_in_const,
             inside_unsafe_cell: false,
         }
         .visit_value(mplace);

compiler/rustc_mir/src/interpret/mod.rs

@@ -24,7 +24,7 @@ pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackP
 pub use self::memory::{AllocCheck, FnVal, Memory, MemoryKind};
 pub use self::operand::{ImmTy, Immediate, OpTy, Operand};
 pub use self::place::{MPlaceTy, MemPlace, MemPlaceMeta, Place, PlaceTy};
-pub use self::validity::{RefTracking, CtfeValidationMode};
+pub use self::validity::{CtfeValidationMode, RefTracking};
 pub use self::visitor::{MutValueVisitor, ValueVisitor};
 
 crate use self::intrinsics::eval_nullary_intrinsic;

compiler/rustc_mir/src/interpret/validity.rs

@@ -119,6 +119,8 @@ pub enum CtfeValidationMode {
     Regular,
     /// Validation of a `const`. `inner` says if this is an inner, indirect allocation (as opposed
     /// to the top-level const allocation).
+    /// Being an inner allocation makes a difference because the top-level allocation of a `const`
+    /// is copied for each use, but the inner allocations are implicitly shared.
     Const { inner: bool },
 }
 
@@ -541,8 +543,10 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                 Ok(true)
             }
             ty::Ref(_, ty, mutbl) => {
-                if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { .. })) && *mutbl == hir::Mutability::Mut {
-                    // A mutable reference inside a const? That does not seem right (except of it is
+                if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { .. }))
+                    && *mutbl == hir::Mutability::Mut
+                {
+                    // A mutable reference inside a const? That does not seem right (except if it is
                     // a ZST).
                     let layout = self.ecx.layout_of(ty)?;
                     if !layout.is_zst() {

compiler/rustc_mir/src/transform/promote_consts.rs

@@ -1176,7 +1176,7 @@ pub fn promote_candidates<'tcx>(
         let mut scope = body.source_scopes[candidate.source_info(body).scope].clone();
         scope.parent_scope = None;
 
-        let mut promoted = Body::new(
+        let promoted = Body::new(
             body.source, // `promoted` gets filled in below
             IndexVec::new(),
             IndexVec::from_elem_n(scope, 1),
@@ -1187,7 +1187,6 @@ pub fn promote_candidates<'tcx>(
             body.span,
             body.generator_kind,
         );
-        promoted.ignore_interior_mut_in_const_validation = true;
 
         let promoter = Promoter {
             promoted,