const checking: do not do value-based reasoning for interior mutability

Author: RalfJung

Diff for: compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -412,10 +412,10 @@ pub fn const_validate_mplace<'mir, 'tcx>(
             _ if cid.promoted.is_some() => CtfeValidationMode::Promoted,
             Some(mutbl) => CtfeValidationMode::Static { mutbl }, // a `static`
             None => {
-                // In normal `const` (not promoted), the outermost allocation is always only copied,
-                // so having `UnsafeCell` in there is okay despite them being in immutable memory.
-                let allow_immutable_unsafe_cell = cid.promoted.is_none() && !inner;
-                CtfeValidationMode::Const { allow_immutable_unsafe_cell }
+                // This is a normal `const` (not promoted).
+                // The outermost allocation is always only copied, so having `UnsafeCell` in there
+                // is okay despite them being in immutable memory.
+                CtfeValidationMode::Const { allow_immutable_unsafe_cell: !inner }
             }
         };
         ecx.const_validate_operand(&mplace.into(), path, &mut ref_tracking, mode)?;

Diff for: compiler/rustc_const_eval/src/transform/check_consts/check.rs

@@ -30,7 +30,7 @@ type QualifResults<'mir, 'tcx, Q> =
     rustc_mir_dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'mir, 'mir, 'tcx, Q>>;
 
 #[derive(Default)]
-pub struct Qualifs<'mir, 'tcx> {
+pub(crate) struct Qualifs<'mir, 'tcx> {
     has_mut_interior: Option<QualifResults<'mir, 'tcx, HasMutInterior>>,
     needs_drop: Option<QualifResults<'mir, 'tcx, NeedsDrop>>,
     needs_non_const_drop: Option<QualifResults<'mir, 'tcx, NeedsNonConstDrop>>,
@@ -484,11 +484,10 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
 
             Rvalue::Ref(_, BorrowKind::Shared | BorrowKind::Fake, place)
             | Rvalue::AddressOf(Mutability::Not, place) => {
-                let borrowed_place_has_mut_interior = qualifs::in_place::<HasMutInterior, _>(
-                    self.ccx,
-                    &mut |local| self.qualifs.has_mut_interior(self.ccx, local, location),
-                    place.as_ref(),
-                );
+                // We don't do value-based reasoning here, since the rules for interior mutability
+                // are not finalized yet and they seem likely to not be full value-based in the end.
+                let borrowed_place_has_mut_interior =
+                    !place.ty(self.body, self.tcx).ty.is_freeze(self.tcx, self.param_env);
 
                 // If the place is indirect, this is basically a reborrow. We have a reborrow
                 // special case above, but for raw pointers and pointers/references to `static` and
@@ -498,6 +497,17 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
                 // `TransientMutBorrow`), but such reborrows got accidentally stabilized already and
                 // it is too much of a breaking change to take back.
                 if borrowed_place_has_mut_interior && !place.is_indirect() {
+                    // We used to do a value-based check here, so when we changed to a purely
+                    // type-based check we started rejecting code that used to work on stable. So
+                    // for that reason we already stabilize "transient borrows of interior mutable
+                    // borrows where value-based reasoning says that there actually is no interior
+                    // mutability". We don't do anything like that for non-transient borrows since
+                    // those are and will remain hard errors.
+                    let allow_transient_on_stable = !qualifs::in_place::<HasMutInterior, _>(
+                        self.ccx,
+                        &mut |local| self.qualifs.has_mut_interior(self.ccx, local, location),
+                        place.as_ref(),
+                    );
                     match self.const_kind() {
                         // In a const fn all borrows are transient or point to the places given via
                         // references in the arguments (so we already checked them with
@@ -506,7 +516,11 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
                         // NOTE: Once we have heap allocations during CTFE we need to figure out
                         // how to prevent `const fn` to create long-lived allocations that point
                         // to (interior) mutable memory.
-                        hir::ConstContext::ConstFn => self.check_op(ops::TransientCellBorrow),
+                        hir::ConstContext::ConstFn => {
+                            if !allow_transient_on_stable {
+                                self.check_op(ops::TransientCellBorrow)
+                            }
+                        }
                         _ => {
                             // Locals with StorageDead are definitely not part of the final constant value, and
                             // it is thus inherently safe to permit such locals to have their
@@ -517,7 +531,9 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
                             // The good news is that interning will detect if any unexpected mutable
                             // pointer slips through.
                             if self.local_has_storage_dead(place.local) {
-                                self.check_op(ops::TransientCellBorrow);
+                                if !allow_transient_on_stable {
+                                    self.check_op(ops::TransientCellBorrow);
+                                }
                             } else {
                                 self.check_op(ops::CellBorrow);
                             }

Diff for: tests/ui/consts/refs-to-cell-in-final.rs

@@ -15,4 +15,19 @@ static RAW_SYNC_S: SyncPtr<Cell<i32>> = SyncPtr { x: &Cell::new(42) };
 const RAW_SYNC_C: SyncPtr<Cell<i32>> = SyncPtr { x: &Cell::new(42) };
 //~^ ERROR: cannot refer to interior mutable data
 
+// This one does not get promoted because of `Drop`, and then enters interesting codepaths because
+// as a value it has no interior mutability, but as a type it does. See
+// <https://github.com/rust-lang/rust/issues/121610>. Value-based reasoning for interior mutability
+// is questionable (https://github.com/rust-lang/unsafe-code-guidelines/issues/493) so for hnow we
+// reject this, i.e., this tests that const-qualif does *not* do value-based reasoning.
+pub enum JsValue {
+    Undefined,
+    Object(Cell<bool>),
+}
+impl Drop for JsValue {
+    fn drop(&mut self) {}
+}
+const UNDEFINED: &JsValue = &JsValue::Undefined;
+//~^ERROR: cannot refer to interior mutable data
+
 fn main() {}

Diff for: tests/ui/consts/refs-to-cell-in-final.stderr

@@ -12,6 +12,12 @@ error[E0492]: constants cannot refer to interior mutable data
 LL | const RAW_SYNC_C: SyncPtr<Cell<i32>> = SyncPtr { x: &Cell::new(42) };
    |                                                     ^^^^^^^^^^^^^^ this borrow of an interior mutable value may end up in the final value
 
-error: aborting due to 2 previous errors
+error[E0492]: constants cannot refer to interior mutable data
+  --> $DIR/refs-to-cell-in-final.rs:30:29
+   |
+LL | const UNDEFINED: &JsValue = &JsValue::Undefined;
+   |                             ^^^^^^^^^^^^^^^^^^^ this borrow of an interior mutable value may end up in the final value
+
+error: aborting due to 3 previous errors
 
 For more information about this error, try `rustc --explain E0492`.