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

interpret: project_downcast: do not ICE for uninhabited variants

Fixes rust-lang#120337

This assertion was already under discussion for a bit; I think the [example](rust-lang#120337 (comment)) `@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`

Author: bors

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,

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)

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

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 =

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 };
+}

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;
+      }
+  }
+  

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;
+      }
+  }
+  

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() {}

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() {}