Add a static analysis that prevents references to inner mutability only in the trailing expression of a constant

Author: oli-obk

compiler/rustc_feature/src/active.rs

@@ -623,6 +623,9 @@ declare_features! (
     /// Allows arbitrary expressions in key-value attributes at parse time.
     (active, extended_key_value_attributes, "1.50.0", Some(78835), None),
 
+    /// Allows references to types with interior mutability within constants
+    (active, const_refs_to_cell, "1.51.0", Some(80384), None),
+
     // -------------------------------------------------------------------------
     // feature-group-end: actual feature gates
     // -------------------------------------------------------------------------

compiler/rustc_mir/src/dataflow/framework/fmt.rs

@@ -33,7 +33,7 @@ pub trait DebugWithContext<C>: Eq + fmt::Debug {
         }
 
         write!(f, "\u{001f}-")?;
-        self.fmt_with(ctxt, f)
+        old.fmt_with(ctxt, f)
     }
 }
 
@@ -133,6 +133,30 @@ where
     }
 }
 
+impl<T, C, V> DebugWithContext<C> for rustc_index::vec::IndexVec<T, V>
+where
+    T: Idx + DebugWithContext<C>,
+    V: DebugWithContext<C> + Eq,
+{
+    fn fmt_with(&self, ctxt: &C, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_set().entries(self.iter_enumerated().map(|this| DebugWithAdapter { this, ctxt })).finish()
+    }
+
+    fn fmt_diff_with(&self, old: &Self, ctxt: &C, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        assert_eq!(self.len(), old.len());
+
+        for (new, old) in self.iter_enumerated().zip(old.iter_enumerated()) {
+            write!(f, "{:?}", DebugDiffWithAdapter {
+                new,
+                old,
+                ctxt,
+            })?;
+        }
+
+        Ok(())
+    }
+}
+
 impl<T, C> DebugWithContext<C> for &'_ T
 where
     T: DebugWithContext<C>,
@@ -146,6 +170,39 @@ where
     }
 }
 
+impl<T, C> DebugWithContext<C> for Option<T>
+where
+    T: DebugWithContext<C>,
+{
+    fn fmt_with(&self, ctxt: &C, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            None => Ok(()),
+            Some(v) => v.fmt_with(ctxt, f),
+        }
+    }
+
+    fn fmt_diff_with(&self, old: &Self, ctxt: &C, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match (self, old) {
+            (None, None) => Ok(()),
+            (Some(a), Some(b)) => a.fmt_diff_with(b, ctxt, f),
+            (Some(a), None) => {
+                write!(f, "\u{001f}+")?;
+                a.fmt_with(ctxt, f)
+            }
+            (None, Some(b)) => {
+                write!(f, "\u{001f}-")?;
+                b.fmt_with(ctxt, f)
+            }
+        }
+    }
+}
+
+impl<T, U, C> DebugWithContext<C> for (T, U)
+where
+    T: DebugWithContext<C>,
+    U: DebugWithContext<C>,
+{}
+
 impl<C> DebugWithContext<C> for rustc_middle::mir::Local {}
 impl<C> DebugWithContext<C> for crate::dataflow::move_paths::InitIndex {}
 

compiler/rustc_mir/src/transform/check_consts/ops.rs

@@ -208,9 +208,29 @@ impl NonConstOp for LiveDrop {
     }
 }
 
+#[derive(Debug)]
+pub struct CellBorrowBehindRef;
+impl NonConstOp for CellBorrowBehindRef {
+    fn status_in_item(&self, _: &ConstCx<'_, '_>) -> Status {
+        Status::Unstable(sym::const_refs_to_cell)
+    }
+    fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> DiagnosticBuilder<'tcx> {
+        feature_err(
+            &ccx.tcx.sess.parse_sess,
+            sym::const_refs_to_cell,
+            span,
+            "cannot borrow here, since the borrowed element may contain interior mutability",
+        )
+    }
+}
+
 #[derive(Debug)]
 pub struct CellBorrow;
 impl NonConstOp for CellBorrow {
+    fn importance(&self) -> DiagnosticImportance {
+        // The problematic cases will already emit a `CellBorrowBehindRef`
+        DiagnosticImportance::Secondary
+    }
     fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> DiagnosticBuilder<'tcx> {
         struct_span_err!(
             ccx.tcx.sess,

compiler/rustc_mir/src/transform/check_consts/qualifs.rs

@@ -9,7 +9,7 @@ use rustc_span::DUMMY_SP;
 use rustc_trait_selection::traits;
 use rustc_index::vec::IndexVec;
 use rustc_index::bit_set::BitSet;
-use crate::dataflow::JoinSemiLattice;
+use crate::dataflow::{JoinSemiLattice, fmt::DebugWithContext};
 
 use super::ConstCx;
 
@@ -44,7 +44,7 @@ pub(crate) trait Qualif {
     /// The dataflow result type. If it's just qualified/not qualified, then
     /// you can just use a `()` (most qualifs do that). But if you need more state, use a
     /// custom enum.
-    type Result: SetChoice = ();
+    type Result: SetChoice + std::fmt::Debug = ();
     type Set: QualifsPerLocal<Self::Result> = <Self::Result as SetChoice>::Set;
 
     /// Whether this `Qualif` is cleared when a local is moved from.
@@ -62,6 +62,12 @@ pub(crate) trait Qualif {
     /// It also determines the `Qualif`s for primitive types.
     fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> Option<Self::Result>;
 
+    /// Sometimes const fn calls cannot possibly contain the qualif, so we can treat function
+    /// calls special here.
+    fn in_any_function_call(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>, _args: Option<Self::Result>) -> Option<Self::Result> {
+        Self::in_any_value_of_ty(cx, ty)
+    }
+
     /// Returns `true` if this `Qualif` is inherent to the given struct or enum.
     ///
     /// By default, `Qualif`s propagate into ADTs in a structural way: An ADT only becomes
@@ -75,6 +81,10 @@ pub(crate) trait Qualif {
         adt: &'tcx AdtDef,
         substs: SubstsRef<'tcx>,
     ) -> Option<Self::Result>;
+
+    fn in_value_behind_ref(qualif: Option<Self::Result>) -> Option<Self::Result> {
+        qualif
+    }
 }
 
 pub(crate) trait SetChoice: Sized + Clone + JoinSemiLattice {
@@ -116,7 +126,7 @@ impl<T: Clone + Eq + JoinSemiLattice> QualifsPerLocal<T> for IndexVec<Local, Opt
         IndexVec::from_elem_n(None, n)
     }
     fn insert(&mut self, local: Local, val: T) {
-        self[local] = Some(val);
+        self[local].join(&Some(val));
     }
     fn remove(&mut self, local: Local) {
         self[local] = None;
@@ -156,6 +166,66 @@ impl Qualif for HasMutInterior {
     }
 }
 
+/// Constant containing interior mutability (`UnsafeCell<T>`) behind a reference.
+/// This must be ruled out to make sure that evaluating the constant at compile-time
+/// and at *any point* during the run-time would produce the same result. In particular,
+/// promotion of temporaries must not change program behavior; if the promoted could be
+/// written to, that would be a problem.
+pub struct HasMutInteriorBehindRef;
+
+#[derive(Clone, Eq, PartialEq, Debug)]
+pub enum HasMutInteriorBehindRefState {
+    Yes,
+    /// As long as we haven't encountered a reference yet, we use this state
+    /// which is equivalent to the `HasMutInterior` qualif.
+    OnlyHasMutInterior,
+}
+impl SetChoice for HasMutInteriorBehindRefState {}
+impl<C> DebugWithContext<C> for HasMutInteriorBehindRefState {}
+
+impl JoinSemiLattice for HasMutInteriorBehindRefState {
+    fn join(&mut self, other: &Self) -> bool {
+        match (&self, other) {
+            (Self::Yes, _) => false,
+            (Self::OnlyHasMutInterior, Self::Yes) => {
+                *self = Self::Yes;
+                true
+            },
+            (Self::OnlyHasMutInterior, Self::OnlyHasMutInterior) => false,
+        }
+    }
+}
+
+impl Qualif for HasMutInteriorBehindRef {
+    const ANALYSIS_NAME: &'static str = "flow_has_mut_interior_behind_ref";
+    type Result = HasMutInteriorBehindRefState;
+
+    fn in_qualifs(qualifs: &ConstQualifs) -> Option<HasMutInteriorBehindRefState> {
+        HasMutInterior::in_qualifs(qualifs).map(|()| HasMutInteriorBehindRefState::OnlyHasMutInterior)
+    }
+
+    fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> Option<HasMutInteriorBehindRefState> {
+        match ty.builtin_deref(false) {
+            None => HasMutInterior::in_any_value_of_ty(cx, ty).map(|()| HasMutInteriorBehindRefState::OnlyHasMutInterior),
+            Some(tam) => HasMutInterior::in_any_value_of_ty(cx, tam.ty).map(|()| HasMutInteriorBehindRefState::Yes),
+        }
+    }
+
+    #[instrument(skip(cx))]
+    fn in_any_function_call(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>, mut args: Option<Self::Result>) -> Option<Self::Result> {
+        args.join(&HasMutInterior::in_any_value_of_ty(cx, ty).map(|()| HasMutInteriorBehindRefState::OnlyHasMutInterior));
+        args
+    }
+
+    fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &'tcx AdtDef, substs: SubstsRef<'tcx>) -> Option<HasMutInteriorBehindRefState> {
+        HasMutInterior::in_adt_inherently(cx, adt, substs).map(|()| HasMutInteriorBehindRefState::OnlyHasMutInterior)
+    }
+
+    fn in_value_behind_ref(qualif: Option<HasMutInteriorBehindRefState>) -> Option<HasMutInteriorBehindRefState> {
+        qualif.map(|_| HasMutInteriorBehindRefState::Yes)
+    }
+}
+
 /// Constant containing an ADT that implements `Drop`.
 /// This must be ruled out (a) because we cannot run `Drop` during compile-time
 /// as that might not be a `const fn`, and (b) because implicit promotion would
@@ -212,6 +282,7 @@ impl Qualif for CustomEq {
 // FIXME: Use `mir::visit::Visitor` for the `in_*` functions if/when it supports early return.
 
 /// Returns `true` if this `Rvalue` contains qualif `Q`.
+#[instrument(skip(cx, in_local), fields(Q=std::any::type_name::<Q>()))]
 pub(crate) fn in_rvalue<Q, F>(cx: &ConstCx<'_, 'tcx>, in_local: &mut F, rvalue: &Rvalue<'tcx>) -> Option<Q::Result>
 where
     Q: Qualif,
@@ -250,7 +321,7 @@ where
                 }
             }
 
-            in_place::<Q, _>(cx, in_local, place.as_ref())
+            Q::in_value_behind_ref(in_place::<Q, _>(cx, in_local, place.as_ref()))
         }
 
         Rvalue::Aggregate(kind, operands) => {
@@ -270,6 +341,7 @@ where
 }
 
 /// Returns `true` if this `Place` contains qualif `Q`.
+#[instrument(skip(cx, in_local), fields(Q=std::any::type_name::<Q>()))]
 pub(crate) fn in_place<Q, F>(cx: &ConstCx<'_, 'tcx>, in_local: &mut F, place: PlaceRef<'tcx>) -> Option<Q::Result>
 where
     Q: Qualif,
@@ -305,6 +377,7 @@ where
 }
 
 /// Returns `true` if this `Operand` contains qualif `Q`.
+#[instrument(skip(cx, in_local), fields(Q=std::any::type_name::<Q>()))]
 pub(crate) fn in_operand<Q, F>(cx: &ConstCx<'_, 'tcx>, in_local: &mut F, operand: &Operand<'tcx>) -> Option<Q::Result>
 where
     Q: Qualif,

compiler/rustc_mir/src/transform/check_consts/resolver.rs

@@ -8,7 +8,7 @@ use rustc_middle::mir::{self, BasicBlock, Location};
 use std::marker::PhantomData;
 
 use super::{qualifs, ConstCx, Qualif};
-use crate::dataflow;
+use crate::dataflow::{self, JoinSemiLattice};
 use super::qualifs::QualifsPerLocal;
 
 /// A `Visitor` that propagates qualifs between locals. This defines the transfer function of
@@ -43,6 +43,7 @@ where
         }
     }
 
+    #[instrument(skip(self))]
     fn assign_qualif_direct(&mut self, place: &mir::Place<'tcx>, value: Option<Q::Result>) {
         debug_assert!(!place.is_indirect());
 
@@ -67,13 +68,25 @@ where
         &mut self,
         _block: BasicBlock,
         _func: &mir::Operand<'tcx>,
-        _args: &[mir::Operand<'tcx>],
+        args: &[mir::Operand<'tcx>],
         return_place: mir::Place<'tcx>,
     ) {
         // We cannot reason about another function's internals, so use conservative type-based
         // qualification for the result of a function call.
         let return_ty = return_place.ty(self.ccx.body, self.ccx.tcx).ty;
-        let qualif = Q::in_any_value_of_ty(self.ccx, return_ty);
+
+        // Though some qualifs merge the call arguments' qualifs into the result qualif.
+        let mut args_qualif = None;
+        for arg in args {
+            let arg = qualifs::in_operand::<Q, _>(
+                self.ccx,
+                &mut |l| self.qualifs_per_local.get(l),
+                arg,
+            );
+            args_qualif.join(&arg);
+        }
+
+        let qualif = Q::in_any_function_call(self.ccx, return_ty, args_qualif);
 
         if !return_place.is_indirect() {
             self.assign_qualif_direct(&return_place, qualif);