properly fill a promoted's required_consts

RalfJung · RalfJung · commit 4a9ab293976f · 2024-03-21T09:27:29.000+01:00
diff --git a/compiler/rustc_const_eval/src/const_eval/machine.rs b/compiler/rustc_const_eval/src/const_eval/machine.rs
@@ -376,17 +376,8 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
     const PANIC_ON_ALLOC_FAIL: bool = false; // will be raised as a proper error
 
     #[inline]
-    fn all_required_consts_are_checked(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool {
-        // Generally we expect required_consts to be properly filled, except for promoteds where
-        // storing these consts shows up negatively in benchmarks. A promoted can only be relevant
-        // if its parent MIR is relevant, and the consts in the promoted will be in the parent's
-        // `required_consts`, so we are still sure to catch any const-eval bugs, just a bit less
-        // directly.
-        if ecx.frame_idx() == 0 && ecx.frame().body.source.promoted.is_some() {
-            false
-        } else {
-            true
-        }
+    fn all_required_consts_are_checked(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool {
+        true
     }
 
     #[inline(always)]
diff --git a/compiler/rustc_middle/src/mir/consts.rs b/compiler/rustc_middle/src/mir/consts.rs
@@ -238,6 +238,24 @@ impl<'tcx> Const<'tcx> {
         }
     }
 
+    /// Determines whether we need to add this const to `required_consts`. This is the case if and
+    /// only if evaluating it may error.
+    #[inline]
+    pub fn is_required_const(&self) -> bool {
+        match self {
+            Const::Ty(c) => match c.kind() {
+                ty::ConstKind::Value(_) => false, // already a value, cannot error
+                ty::ConstKind::Param(_) | ty::ConstKind::Error(_) => true, // these are errors or could be replaced by errors
+                _ => bug!(
+                    "only ConstKind::Param/Value/Error should be encountered here, got {:#?}",
+                    c
+                ),
+            },
+            Const::Unevaluated(..) => true,
+            Const::Val(..) => false, // already a value, cannot error
+        }
+    }
+
     #[inline]
     pub fn try_to_scalar(self) -> Option<Scalar> {
         match self {
diff --git a/compiler/rustc_mir_transform/src/promote_consts.rs b/compiler/rustc_mir_transform/src/promote_consts.rs
@@ -953,6 +953,14 @@ 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) {
+        if constant.const_.is_required_const() {
+            self.promoted.required_consts.push(*constant);
+        }
+
+        // Skipping `super_constant` as the visitor is otherwise only looking for locals.
+    }
 }
 
 fn promote_candidates<'tcx>(
@@ -997,11 +1005,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 {
@@ -1014,6 +1017,7 @@ fn promote_candidates<'tcx>(
             add_to_required: false,
         };
 
+        // `required_consts` of the promoted itself gets filled while building the MIR body.
         let mut promoted = promoter.promote_candidate(candidate, promotions.len());
         promoted.source.promoted = Some(promotions.next_index());
         promotions.push(promoted);
diff --git a/compiler/rustc_mir_transform/src/required_consts.rs b/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;
+use rustc_middle::mir::{ConstOperand, Location};
 
 pub struct RequiredConstsVisitor<'a, 'tcx> {
     required_consts: &'a mut Vec<ConstOperand<'tcx>>,
@@ -14,21 +13,7 @@ impl<'a, 'tcx> RequiredConstsVisitor<'a, 'tcx> {
 
 impl<'tcx> Visitor<'tcx> for RequiredConstsVisitor<'_, 'tcx> {
     fn visit_constant(&mut self, constant: &ConstOperand<'tcx>, _: Location) {
-        // Only unevaluated consts have to be added to `required_consts` as only those can possibly
-        // still have latent const-eval errors.
-        let is_required = match constant.const_ {
-            Const::Ty(c) => match c.kind() {
-                ty::ConstKind::Value(_) => false, // already a value, cannot error
-                ty::ConstKind::Param(_) | ty::ConstKind::Error(_) => true, // these are errors or could be replaced by errors
-                _ => bug!(
-                    "only ConstKind::Param/Value/Error should be encountered here, got {:#?}",
-                    c
-                ),
-            },
-            Const::Unevaluated(..) => true,
-            Const::Val(..) => false, // already a value, cannot error
-        };
-        if is_required {
+        if constant.const_.is_required_const() {
             self.required_consts.push(*constant);
         }
     }
diff --git a/tests/ui/consts/required-consts/collect-in-promoted-const.noopt.stderr b/tests/ui/consts/required-consts/collect-in-promoted-const.noopt.stderr
@@ -0,0 +1,37 @@
+error[E0080]: evaluation of `Fail::<i32>::C` failed
+  --> $DIR/collect-in-promoted-const.rs:9:19
+   |
+LL |     const C: () = panic!();
+   |                   ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-promoted-const.rs:9:19
+   |
+   = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+note: erroneous constant encountered
+  --> $DIR/collect-in-promoted-const.rs:20:21
+   |
+LL |         let _val = &Fail::<T>::C;
+   |                     ^^^^^^^^^^^^
+
+note: erroneous constant encountered
+  --> $DIR/collect-in-promoted-const.rs:20:20
+   |
+LL |         let _val = &Fail::<T>::C;
+   |                    ^^^^^^^^^^^^^
+
+note: erroneous constant encountered
+  --> $DIR/collect-in-promoted-const.rs:20:21
+   |
+LL |         let _val = &Fail::<T>::C;
+   |                     ^^^^^^^^^^^^
+   |
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
+note: the above error was encountered while instantiating `fn f::<i32>`
+  --> $DIR/collect-in-promoted-const.rs:25:5
+   |
+LL |     f::<i32>();
+   |     ^^^^^^^^^^
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0080`.
diff --git a/tests/ui/consts/required-consts/collect-in-promoted-const.opt.stderr b/tests/ui/consts/required-consts/collect-in-promoted-const.opt.stderr
@@ -0,0 +1,53 @@
+error[E0080]: evaluation of `Fail::<T>::C` failed
+  --> $DIR/collect-in-promoted-const.rs:9:19
+   |
+LL |     const C: () = panic!();
+   |                   ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-promoted-const.rs:9:19
+   |
+   = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+note: erroneous constant encountered
+  --> $DIR/collect-in-promoted-const.rs:20:21
+   |
+LL |         let _val = &Fail::<T>::C;
+   |                     ^^^^^^^^^^^^
+
+error[E0080]: evaluation of `Fail::<i32>::C` failed
+  --> $DIR/collect-in-promoted-const.rs:9:19
+   |
+LL |     const C: () = panic!();
+   |                   ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-promoted-const.rs:9:19
+   |
+   = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+note: erroneous constant encountered
+  --> $DIR/collect-in-promoted-const.rs:20:21
+   |
+LL |         let _val = &Fail::<T>::C;
+   |                     ^^^^^^^^^^^^
+   |
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
+note: erroneous constant encountered
+  --> $DIR/collect-in-promoted-const.rs:20:20
+   |
+LL |         let _val = &Fail::<T>::C;
+   |                    ^^^^^^^^^^^^^
+
+note: erroneous constant encountered
+  --> $DIR/collect-in-promoted-const.rs:20:21
+   |
+LL |         let _val = &Fail::<T>::C;
+   |                     ^^^^^^^^^^^^
+   |
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
+note: the above error was encountered while instantiating `fn f::<i32>`
+  --> $DIR/collect-in-promoted-const.rs:25:5
+   |
+LL |     f::<i32>();
+   |     ^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0080`.
diff --git a/tests/ui/consts/required-consts/collect-in-promoted-const.rs b/tests/ui/consts/required-consts/collect-in-promoted-const.rs
@@ -0,0 +1,26 @@
+//@revisions: noopt opt
+//@ build-fail
+//@[noopt] compile-flags: -Copt-level=0
+//@[opt] compile-flags: -O
+//! Make sure we error on erroneous consts even if they get promoted.
+
+struct Fail<T>(T);
+impl<T> Fail<T> {
+    const C: () = panic!(); //~ERROR evaluation of `Fail::<i32>::C` failed
+    //[opt]~^ ERROR evaluation of `Fail::<T>::C` failed
+    // (Not sure why optimizations lead to this being emitted twice, but as long as compilation
+    // fails either way it's fine.)
+}
+
+#[inline(never)]
+fn f<T>() {
+    if false {
+        // If promotion moved `C` from our required_consts to its own, without adding itself to
+        // our required_consts, then we'd miss the const-eval failure here.
+        let _val = &Fail::<T>::C;
+    }
+}
+
+fn main() {
+    f::<i32>();
+}
diff --git a/tests/ui/consts/required-consts/interpret-in-promoted.rs b/tests/ui/consts/required-consts/interpret-in-promoted.rs
@@ -1,6 +1,6 @@
 //@revisions: noopt opt
 //@[opt] compile-flags: -O
-//! Make sure we error on erroneous consts even if they are unused.
+//! Make sure we evaluate const fn calls even if they get promoted and their result ignored.
 
 const unsafe fn ub() {
     std::hint::unreachable_unchecked();
diff --git a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs
@@ -26,8 +26,8 @@ static SOME_STRUCT: &SomeStruct = &SomeStruct {
     foo: &Foo { bools: &[false, true] },
     bar: &Bar { bools: &[true, true] },
     f: &id,
-    //~^ ERROR mismatched types
-    //~| ERROR mismatched types
+    //~^ ERROR implementation of `FnOnce` is not general enough
+    //~| ERROR implementation of `FnOnce` is not general enough
     //~| ERROR implementation of `Fn` is not general enough
     //~| ERROR implementation of `Fn` is not general enough
 };
