interpret: double-check that required_consts captures all consts that can fail

Author: RalfJung

compiler/rustc_const_eval/src/interpret/eval_context.rs

@@ -792,15 +792,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         self.stack_mut().push(frame);
 
         // Make sure all the constants required by this frame evaluate successfully (post-monomorphization check).
-        if M::POST_MONO_CHECKS {
-            for &const_ in &body.required_consts {
-                let c = self
-                    .instantiate_from_current_frame_and_normalize_erasing_regions(const_.const_)?;
-                c.eval(*self.tcx, self.param_env, Some(const_.span)).map_err(|err| {
-                    err.emit_note(*self.tcx);
-                    err
-                })?;
-            }
+        for &const_ in &body.required_consts {
+            let c =
+                self.instantiate_from_current_frame_and_normalize_erasing_regions(const_.const_)?;
+            c.eval(*self.tcx, self.param_env, Some(const_.span)).map_err(|err| {
+                err.emit_note(*self.tcx);
+                err
+            })?;
         }
 
         // done
@@ -1141,18 +1139,20 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         self.raw_const_to_mplace(val)
     }
 
-    pub fn eval_mir_constant(
+    /// Evaluate a constant from the current frame.
+    /// This function relies on the fact that when the frame got pushed, all `required_consts` have been checked.
+    pub(super) fn eval_frame_mir_constant(
         &self,
-        val: &mir::Const<'tcx>,
+        const_: &mir::Const<'tcx>,
         span: Option<Span>,
         layout: Option<TyAndLayout<'tcx>>,
     ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> {
-        M::eval_mir_constant(self, *val, span, layout, |ecx, val, span, layout| {
-            let const_val = val.eval(*ecx.tcx, ecx.param_env, span).map_err(|err| {
-                // FIXME: somehow this is reachable even when POST_MONO_CHECKS is on.
-                // Are we not always populating `required_consts`?
-                err.emit_note(*ecx.tcx);
-                err
+        M::eval_mir_constant(self, *const_, span, layout, |ecx, val, span, layout| {
+            let const_val = val.eval(*ecx.tcx, ecx.param_env, span).map_err(|err| match err {
+                ErrorHandled::Reported(..) => {
+                    bug!("interpret const eval failure of {val:?} which is not in required_consts")
+                }
+                ErrorHandled::TooGeneric(_) => err,
             })?;
             ecx.const_val_to_op(const_val, val.ty(), layout)
         })

compiler/rustc_const_eval/src/interpret/machine.rs

@@ -140,9 +140,6 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
     /// Should the machine panic on allocation failures?
     const PANIC_ON_ALLOC_FAIL: bool;
 
-    /// Should post-monomorphization checks be run when a stack frame is pushed?
-    const POST_MONO_CHECKS: bool = true;
-
     /// Whether memory accesses should be alignment-checked.
     fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool;
 

compiler/rustc_const_eval/src/interpret/operand.rs

@@ -740,14 +740,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 // * During ConstProp, with `TooGeneric` or since the `required_consts` were not all
                 //   checked yet.
                 // * During CTFE, since promoteds in `const`/`static` initializer bodies can fail.
-                self.eval_mir_constant(&c, Some(constant.span), layout)?
+                self.eval_frame_mir_constant(&c, Some(constant.span), layout)?
             }
         };
         trace!("{:?}: {:?}", mir_op, op);
         Ok(op)
     }
 
-    pub(crate) fn const_val_to_op(
+    pub fn const_val_to_op(
         &self,
         val_val: mir::ConstValue<'tcx>,
         ty: Ty<'tcx>,

compiler/rustc_middle/src/mir/consts.rs

@@ -230,6 +230,22 @@ impl<'tcx> Const<'tcx> {
         ))
     }
 
+    #[inline]
+    pub fn is_required_const(&self) -> bool {
+        // Only unevalated consts must be added to `required_consts` as only those can possibly
+        // still have latent const-eval errors.
+        match self {
+            Const::Ty(c) => match c.kind() {
+                ty::ConstKind::Param(_) | ty::ConstKind::Error(_) | ty::ConstKind::Value(_) => {
+                    false
+                }
+                _ => bug!("only ConstKind::Param/Value should be encountered here, got {:#?}", c),
+            },
+            Const::Unevaluated(..) => true,
+            Const::Val(..) => false,
+        }
+    }
+
     #[inline(always)]
     pub fn ty(&self) -> Ty<'tcx> {
         match self {

compiler/rustc_mir_transform/src/dataflow_const_prop.rs

@@ -9,16 +9,16 @@ use rustc_data_structures::fx::FxHashMap;
 use rustc_hir::def::DefKind;
 use rustc_middle::mir::interpret::{AllocId, ConstAllocation, InterpResult, Scalar};
 use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
-use rustc_middle::mir::*;
+use rustc_middle::mir::{self, *};
 use rustc_middle::query::TyCtxtAt;
-use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
+use rustc_middle::ty::layout::{HasParamEnv, LayoutOf, TyAndLayout};
 use rustc_middle::ty::{self, Ty, TyCtxt};
 use rustc_mir_dataflow::value_analysis::{
     Map, PlaceIndex, State, TrackElem, ValueAnalysis, ValueAnalysisWrapper, ValueOrPlace,
 };
 use rustc_mir_dataflow::{lattice::FlatSet, Analysis, Results, ResultsVisitor};
 use rustc_span::def_id::DefId;
-use rustc_span::DUMMY_SP;
+use rustc_span::{Span, DUMMY_SP};
 use rustc_target::abi::{Abi, FieldIdx, Size, VariantIdx, FIRST_VARIANT};
 
 /// Macro for machine-specific `InterpError` without allocation.
@@ -393,7 +393,9 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
                 }
             }
             Operand::Constant(box constant) => {
-                if let Ok(constant) = self.ecx.eval_mir_constant(&constant.const_, None, None) {
+                if let Ok(constant) =
+                    DummyMachine::eval_mir_constant(&self.ecx, &constant.const_, None)
+                {
                     self.assign_constant(state, place, constant, &[]);
                 }
             }
@@ -889,6 +891,23 @@ impl<'tcx> Visitor<'tcx> for OperandCollector<'tcx, '_, '_, '_> {
 
 pub(crate) struct DummyMachine;
 
+impl DummyMachine {
+    /// Evaluate a MIR constant that doesn't have to be already pre-checked via `required_consts`.
+    pub fn eval_mir_constant<'tcx>(
+        ecx: &InterpCx<'_, 'tcx, Self>,
+        const_: &mir::Const<'tcx>,
+        span: Option<Span>,
+    ) -> InterpResult<'tcx, OpTy<'tcx>> {
+        let const_val = const_.eval(*ecx.tcx, ecx.param_env(), span).map_err(|err| {
+            // This *may* be the only time we're actually evaluating this const, so show a note at
+            // the place it is used.
+            err.emit_note(*ecx.tcx);
+            err
+        })?;
+        ecx.const_val_to_op(const_val, const_.ty(), None)
+    }
+}
+
 impl HasStaticRootDefId for DummyMachine {
     fn static_def_id(&self) -> Option<rustc_hir::def_id::LocalDefId> {
         None

compiler/rustc_mir_transform/src/gvn.rs

@@ -367,7 +367,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
             Repeat(..) => return None,
 
             Constant { ref value, disambiguator: _ } => {
-                self.ecx.eval_mir_constant(value, None, None).ok()?
+                DummyMachine::eval_mir_constant(&self.ecx, value, None).ok()?
             }
             Aggregate(kind, variant, ref fields) => {
                 let fields = fields

compiler/rustc_mir_transform/src/jump_threading.rs

@@ -416,7 +416,8 @@ impl<'tcx, 'a> TOFinder<'tcx, 'a> {
         match rhs {
             // If we expect `lhs ?= A`, we have an opportunity if we assume `constant == A`.
             Operand::Constant(constant) => {
-                let constant = self.ecx.eval_mir_constant(&constant.const_, None, None).ok()?;
+                let constant =
+                    DummyMachine::eval_mir_constant(&self.ecx, &constant.const_, None).ok()?;
                 self.process_constant(bb, lhs, constant, state);
             }
             // Transfer the conditions on the copied rhs.

compiler/rustc_mir_transform/src/known_panics_lint.rs

@@ -259,9 +259,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         // that the `RevealAll` pass has happened and that the body's consts
         // are normalized, so any call to resolve before that needs to be
         // manually normalized.
-        let val = self.tcx.try_normalize_erasing_regions(self.param_env, c.const_).ok()?;
+        let const_ = self.tcx.try_normalize_erasing_regions(self.param_env, c.const_).ok()?;
 
-        self.use_ecx(|this| this.ecx.eval_mir_constant(&val, Some(c.span), None))?
+        self.use_ecx(|this| DummyMachine::eval_mir_constant(&this.ecx, &const_, Some(c.span)))?
             .as_mplace_or_imm()
             .right()
     }

compiler/rustc_mir_transform/src/promote_consts.rs

@@ -934,7 +934,7 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> {
         self.assign(RETURN_PLACE, rvalue, span);
 
         // Now that we did promotion, we know whether we'll want to add this to `required_consts`.
-        if self.add_to_required {
+        if self.add_to_required && promoted_op.const_.is_required_const() {
             self.source.required_consts.push(promoted_op);
         }
 
@@ -953,6 +953,12 @@ impl<'a, 'tcx> MutVisitor<'tcx> for Promoter<'a, 'tcx> {
             *local = self.promote_temp(*local);
         }
     }
+
+    fn visit_constant(&mut self, constant: &mut ConstOperand<'tcx>, _location: Location) {
+        self.promoted.required_consts.push(*constant);
+
+        // Skipping `super_constant` as the visitor is otherwise only looking for locals.
+    }
 }
 
 fn promote_candidates<'tcx>(
@@ -997,11 +1003,6 @@ fn promote_candidates<'tcx>(
             None,
             body.tainted_by_errors,
         );
-        // We keep `required_consts` of the new MIR body empty. All consts mentioned here have
-        // already been added to the parent MIR's `required_consts` (that is computed before
-        // promotion), and no matter where this promoted const ends up, our parent MIR must be
-        // somewhere in the reachable dependency chain so we can rely on its required consts being
-        // evaluated.
         promoted.phase = MirPhase::Analysis(AnalysisPhase::Initial);
 
         let promoter = Promoter {

compiler/rustc_mir_transform/src/required_consts.rs

@@ -1,6 +1,5 @@
 use rustc_middle::mir::visit::Visitor;
-use rustc_middle::mir::{Const, ConstOperand, Location};
-use rustc_middle::ty::ConstKind;
+use rustc_middle::mir::{ConstOperand, Location};
 
 pub struct RequiredConstsVisitor<'a, 'tcx> {
     required_consts: &'a mut Vec<ConstOperand<'tcx>>,
@@ -14,14 +13,8 @@ impl<'a, 'tcx> RequiredConstsVisitor<'a, 'tcx> {
 
 impl<'tcx> Visitor<'tcx> for RequiredConstsVisitor<'_, 'tcx> {
     fn visit_constant(&mut self, constant: &ConstOperand<'tcx>, _: Location) {
-        let const_ = constant.const_;
-        match const_ {
-            Const::Ty(c) => match c.kind() {
-                ConstKind::Param(_) | ConstKind::Error(_) | ConstKind::Value(_) => {}
-                _ => bug!("only ConstKind::Param/Value should be encountered here, got {:#?}", c),
-            },
-            Const::Unevaluated(..) => self.required_consts.push(*constant),
-            Const::Val(..) => {}
+        if constant.const_.is_required_const() {
+            self.required_consts.push(*constant);
         }
     }
 }