Auto merge of #120367 - RalfJung:project_downcast_uninhabited, r=oli-obk

interpret: project_downcast: do not ICE for uninhabited variants

Fixes https://github.com/rust-lang/rust/issues/120337

This assertion was already under discussion for a bit; I think the [example](https://github.com/rust-lang/rust/issues/120337#issuecomment-1911076292) `@tmiasko` found is the final nail in the coffin. One could argue maybe MIR building should read the discriminant before projecting, but even then MIR optimizations should be allowed to remove that read, so the downcast should still not ICE. Maybe the downcast should be UB, but in this example UB already arises earlier when a value of type `E` is constructed.

r? `@oli-obk`

9 files changed, 128 insertions, 26 deletions

diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs
index 80d4bda4827..4653c9016c6 100644
--- a/compiler/rustc_const_eval/src/interpret/operand.rs
+++ b/compiler/rustc_const_eval/src/interpret/operand.rs
@@ -260,8 +260,12 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> {
         // This makes several assumptions about what layouts we will encounter; we match what
         // codegen does as good as we can (see `extract_field` in `rustc_codegen_ssa/src/mir/operand.rs`).
         let inner_val: Immediate<_> = match (**self, self.layout.abi) {
-            // if the entire value is uninit, then so is the field (can happen in ConstProp)
+            // If the entire value is uninit, then so is the field (can happen in ConstProp).
             (Immediate::Uninit, _) => Immediate::Uninit,
+            // If the field is uninhabited, we can forget the data (can happen in ConstProp).
+            // `enum S { A(!), B, C }` is an example of an enum with Scalar layout that
+            // has an `Uninhabited` variant, which means this case is possible.
+            _ if layout.abi.is_uninhabited() => Immediate::Uninit,
             // the field contains no information, can be left uninit
             // (Scalar/ScalarPair can contain even aligned ZST, not just 1-ZST)
             _ if layout.is_zst() => Immediate::Uninit,
diff --git a/compiler/rustc_const_eval/src/interpret/projection.rs b/compiler/rustc_const_eval/src/interpret/projection.rs
index bd60e066f72..3a441c1d649 100644
--- a/compiler/rustc_const_eval/src/interpret/projection.rs
+++ b/compiler/rustc_const_eval/src/interpret/projection.rs
@@ -201,25 +201,8 @@ where
         // see https://github.com/rust-lang/rust/issues/93688#issuecomment-1032929496.)
         // So we just "offset" by 0.
         let layout = base.layout().for_variant(self, variant);
-        // In the future we might want to allow this to permit code like this:
-        // (this is a Rust/MIR pseudocode mix)
-        // ```
-        // enum Option2 {
-        //   Some(i32, !),
-        //   None,
-        // }
-        //
-        // fn panic() -> ! { panic!() }
-        //
-        // let x: Option2;
-        // x.Some.0 = 42;
-        // x.Some.1 = panic();
-        // SetDiscriminant(x, Some);
-        // ```
-        // However, for now we don't generate such MIR, and this check here *has* found real
-        // bugs (see https://github.com/rust-lang/rust/issues/115145), so we will keep rejecting
-        // it.
-        assert!(!layout.abi.is_uninhabited());
+        // This variant may in fact be uninhabited.
+        // See <https://github.com/rust-lang/rust/issues/120337>.
 
         // This cannot be `transmute` as variants *can* have a smaller size than the entire enum.
         base.offset(Size::ZERO, layout, self)
diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs
index a5c229879a7..a4b6c4f9c3f 100644
--- a/compiler/rustc_middle/src/mir/syntax.rs
+++ b/compiler/rustc_middle/src/mir/syntax.rs
@@ -1074,6 +1074,8 @@ pub enum ProjectionElem<V, T> {
     /// "Downcast" to a variant of an enum or a coroutine.
     ///
     /// The included Symbol is the name of the variant, used for printing MIR.
+    ///
+    /// This operation itself is never UB, all it does is change the type of the place.
     Downcast(Option<Symbol>, VariantIdx),
 
     /// Like an explicit cast from an opaque type to a concrete type, but without
diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs
index d5f22b2cdbc..ad12bce9b02 100644
--- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs
+++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs
@@ -403,12 +403,7 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
             operand,
             &mut |elem, op| match elem {
                 TrackElem::Field(idx) => self.ecx.project_field(op, idx.as_usize()).ok(),
-                TrackElem::Variant(idx) => {
-                    if op.layout.for_variant(&self.ecx, idx).abi.is_uninhabited() {
-                        return None;
-                    }
-                    self.ecx.project_downcast(op, idx).ok()
-                }
+                TrackElem::Variant(idx) => self.ecx.project_downcast(op, idx).ok(),
                 TrackElem::Discriminant => {
                     let variant = self.ecx.read_discriminant(op).ok()?;
                     let discr_value =
diff --git a/src/tools/miri/tests/pass/issues/issue-120337-irrefutable-let-ice.rs b/src/tools/miri/tests/pass/issues/issue-120337-irrefutable-let-ice.rs
new file mode 100644
index 00000000000..5af0d0e4bbd
--- /dev/null
+++ b/src/tools/miri/tests/pass/issues/issue-120337-irrefutable-let-ice.rs
@@ -0,0 +1,16 @@
+// Validation stops the test before the ICE we used to hit
+//@compile-flags: -Zmiri-disable-validation
+
+#![feature(never_type)]
+#[derive(Copy, Clone)]
+pub enum E {
+    A(!),
+}
+pub union U {
+    u: (),
+    e: E,
+}
+
+fn main() {
+    let E::A(ref _a) = unsafe { &(&U { u: () }).e };
+}
diff --git a/tests/mir-opt/gvn_uninhabited.f.GVN.panic-abort.diff b/tests/mir-opt/gvn_uninhabited.f.GVN.panic-abort.diff
new file mode 100644
index 00000000000..0b6819ad483
--- /dev/null
+++ b/tests/mir-opt/gvn_uninhabited.f.GVN.panic-abort.diff
@@ -0,0 +1,34 @@
+- // MIR for `f` before GVN
++ // MIR for `f` after GVN
+  
+  fn f() -> u32 {
+      let mut _0: u32;
+      let _1: u32;
+      let mut _2: E;
+      let mut _3: &U;
+      let _4: U;
+      scope 1 {
+          debug i => _1;
+      }
+      scope 2 {
+          let mut _5: &U;
+      }
+  
+      bb0: {
+          StorageLive(_2);
+          StorageLive(_3);
+          _5 = const _;
+          _3 = &(*_5);
+          _2 = ((*_3).1: E);
+          StorageLive(_1);
+-         _1 = ((_2 as A).1: u32);
++         _1 = const 0_u32;
+          StorageDead(_3);
+          StorageDead(_2);
+-         _0 = _1;
++         _0 = const 0_u32;
+          StorageDead(_1);
+          return;
+      }
+  }
+  
diff --git a/tests/mir-opt/gvn_uninhabited.f.GVN.panic-unwind.diff b/tests/mir-opt/gvn_uninhabited.f.GVN.panic-unwind.diff
new file mode 100644
index 00000000000..0b6819ad483
--- /dev/null
+++ b/tests/mir-opt/gvn_uninhabited.f.GVN.panic-unwind.diff
@@ -0,0 +1,34 @@
+- // MIR for `f` before GVN
++ // MIR for `f` after GVN
+  
+  fn f() -> u32 {
+      let mut _0: u32;
+      let _1: u32;
+      let mut _2: E;
+      let mut _3: &U;
+      let _4: U;
+      scope 1 {
+          debug i => _1;
+      }
+      scope 2 {
+          let mut _5: &U;
+      }
+  
+      bb0: {
+          StorageLive(_2);
+          StorageLive(_3);
+          _5 = const _;
+          _3 = &(*_5);
+          _2 = ((*_3).1: E);
+          StorageLive(_1);
+-         _1 = ((_2 as A).1: u32);
++         _1 = const 0_u32;
+          StorageDead(_3);
+          StorageDead(_2);
+-         _0 = _1;
++         _0 = const 0_u32;
+          StorageDead(_1);
+          return;
+      }
+  }
+  
diff --git a/tests/mir-opt/gvn_uninhabited.rs b/tests/mir-opt/gvn_uninhabited.rs
new file mode 100644
index 00000000000..a55b2dd763a
--- /dev/null
+++ b/tests/mir-opt/gvn_uninhabited.rs
@@ -0,0 +1,24 @@
+// unit-test: GVN
+// compile-flags: -O
+// EMIT_MIR_FOR_EACH_PANIC_STRATEGY
+// skip-filecheck
+
+#![feature(never_type)]
+
+#[derive(Copy, Clone)]
+pub enum E {
+    A(!, u32),
+}
+
+pub union U {
+    i: u32,
+    e: E,
+}
+
+// EMIT_MIR gvn_uninhabited.f.GVN.diff
+pub const fn f() -> u32 {
+    let E::A(_, i) = unsafe { (&U { i: 0 }).e };
+    i
+}
+
+fn main() {}
diff --git a/tests/ui/consts/let-irrefutable-pattern-ice-120337.rs b/tests/ui/consts/let-irrefutable-pattern-ice-120337.rs
new file mode 100644
index 00000000000..7da6b7ca285
--- /dev/null
+++ b/tests/ui/consts/let-irrefutable-pattern-ice-120337.rs
@@ -0,0 +1,10 @@
+// check-pass
+#![feature(never_type)]
+#[derive(Copy, Clone)]
+pub enum E { A(!), }
+pub union U { u: (), e: E, }
+pub const C: () = {
+    let E::A(ref a) = unsafe { &(&U { u: () }).e};
+};
+
+fn main() {}