Rollup merge of rust-lang#130735 - compiler-errors:validate-unsize, r=spastorino,RalfJung

Simple validation for unsize coercion in MIR validation

This adds the most basic validity check to unsize coercions in MIR. The src and target of an unsize cast must *at least* implement `Src: CoerceUnsized<Target>` for this to be valid.

This doesn't the second, more subtle validity check that is taken of advantage in codegen [here](https://github.com/rust-lang/rust/blob/914193c8f40528fe82696e1054828de8c399882e/compiler/rustc_codegen_ssa/src/base.rs#L126), but I did leave a beefy FIXME for that explaining what it is.

As a consequence, this also fixes an ICE with GVN and invalid unsize coercions. This is somewhat coincidental, since MIR inlining will check that a body is valid before inlining it; so now that we determine it to be invalid, we don't inline it, and we don't encounter the GVN ICE. I'm not certain if the same GVN ICE is triggerable without the inliner, and perhaps instead with trivial where clauses or something.

cc `@RalfJung`

Author: matthiaskrgr

compiler/rustc_codegen_cranelift/src/unsize.rs

@@ -34,7 +34,22 @@ pub(crate) fn unsized_info<'tcx>(
             let old_info =
                 old_info.expect("unsized_info: missing old info for trait upcasting coercion");
             if data_a.principal_def_id() == data_b.principal_def_id() {
-                // A NOP cast that doesn't actually change anything, should be allowed even with invalid vtables.
+                // Codegen takes advantage of the additional assumption, where if the
+                // principal trait def id of what's being casted doesn't change,
+                // then we don't need to adjust the vtable at all. This
+                // corresponds to the fact that `dyn Tr<A>: Unsize<dyn Tr<B>>`
+                // requires that `A = B`; we don't allow *upcasting* objects
+                // between the same trait with different args. If we, for
+                // some reason, were to relax the `Unsize` trait, it could become
+                // unsound, so let's assert here that the trait refs are *equal*.
+                //
+                // We can use `assert_eq` because the binders should have been anonymized,
+                // and because higher-ranked equality now requires the binders are equal.
+                debug_assert_eq!(
+                    data_a.principal(),
+                    data_b.principal(),
+                    "NOP unsize vtable changed principal trait ref: {data_a} -> {data_b}"
+                );
                 return old_info;
             }
 

compiler/rustc_codegen_ssa/src/base.rs

@@ -125,8 +125,28 @@ fn unsized_info<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
             let old_info =
                 old_info.expect("unsized_info: missing old info for trait upcasting coercion");
             if data_a.principal_def_id() == data_b.principal_def_id() {
-                // A NOP cast that doesn't actually change anything, should be allowed even with
-                // invalid vtables.
+                // Codegen takes advantage of the additional assumption, where if the
+                // principal trait def id of what's being casted doesn't change,
+                // then we don't need to adjust the vtable at all. This
+                // corresponds to the fact that `dyn Tr<A>: Unsize<dyn Tr<B>>`
+                // requires that `A = B`; we don't allow *upcasting* objects
+                // between the same trait with different args. If we, for
+                // some reason, were to relax the `Unsize` trait, it could become
+                // unsound, so let's assert here that the trait refs are *equal*.
+                //
+                // We can use `assert_eq` because the binders should have been anonymized,
+                // and because higher-ranked equality now requires the binders are equal.
+                debug_assert_eq!(
+                    data_a.principal(),
+                    data_b.principal(),
+                    "NOP unsize vtable changed principal trait ref: {data_a} -> {data_b}"
+                );
+
+                // A NOP cast that doesn't actually change anything, let's avoid any
+                // unnecessary work. This relies on the assumption that if the principal
+                // traits are equal, then the associated type bounds (`dyn Trait<Assoc=T>`)
+                // are also equal, which is ensured by the fact that normalization is
+                // a function and we do not allow overlapping impls.
                 return old_info;
             }
 

compiler/rustc_mir_transform/src/validate.rs

@@ -4,7 +4,8 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_hir::LangItem;
 use rustc_index::IndexVec;
 use rustc_index::bit_set::BitSet;
-use rustc_infer::traits::Reveal;
+use rustc_infer::infer::TyCtxtInferExt;
+use rustc_infer::traits::{Obligation, ObligationCause, Reveal};
 use rustc_middle::mir::coverage::CoverageKind;
 use rustc_middle::mir::visit::{NonUseContext, PlaceContext, Visitor};
 use rustc_middle::mir::*;
@@ -16,6 +17,8 @@ use rustc_middle::ty::{
 use rustc_middle::{bug, span_bug};
 use rustc_target::abi::{FIRST_VARIANT, Size};
 use rustc_target::spec::abi::Abi;
+use rustc_trait_selection::traits::ObligationCtxt;
+use rustc_type_ir::Upcast;
 
 use crate::util::{is_within_packed, relate_types};
 
@@ -586,6 +589,22 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
 
         crate::util::relate_types(self.tcx, self.param_env, variance, src, dest)
     }
+
+    /// Check that the given predicate definitely holds in the param-env of this MIR body.
+    fn predicate_must_hold_modulo_regions(
+        &self,
+        pred: impl Upcast<TyCtxt<'tcx>, ty::Predicate<'tcx>>,
+    ) -> bool {
+        let infcx = self.tcx.infer_ctxt().build();
+        let ocx = ObligationCtxt::new(&infcx);
+        ocx.register_obligation(Obligation::new(
+            self.tcx,
+            ObligationCause::dummy(),
+            self.param_env,
+            pred,
+        ));
+        ocx.select_all_or_error().is_empty()
+    }
 }
 
 impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
@@ -1202,8 +1221,18 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                         }
                     }
                     CastKind::PointerCoercion(PointerCoercion::Unsize, _) => {
-                        // This is used for all `CoerceUnsized` types,
-                        // not just pointers/references, so is hard to check.
+                        // Pointers being unsize coerced should at least implement
+                        // `CoerceUnsized`.
+                        if !self.predicate_must_hold_modulo_regions(ty::TraitRef::new(
+                            self.tcx,
+                            self.tcx.require_lang_item(
+                                LangItem::CoerceUnsized,
+                                Some(self.body.source_info(location).span),
+                            ),
+                            [op_ty, *target_type],
+                        )) {
+                            self.fail(location, format!("Unsize coercion, but `{op_ty}` isn't coercible to `{target_type}`"));
+                        }
                     }
                     CastKind::PointerCoercion(PointerCoercion::DynStar, _) => {
                         // FIXME(dyn-star): make sure nothing needs to be done here.

tests/crashes/129219.rs

@@ -0,0 +1,33 @@
+//@ compile-flags: -Zmir-opt-level=0 -Zmir-enable-passes=+Inline,+GVN -Zvalidate-mir
+
+#![feature(unsize)]
+
+use std::marker::Unsize;
+
+pub trait CastTo<U: ?Sized>: Unsize<U> {}
+
+// Not well-formed!
+impl<T: ?Sized, U: ?Sized> CastTo<U> for T {}
+//~^ ERROR the trait bound `T: Unsize<U>` is not satisfied
+
+pub trait Cast {
+    fn cast<U: ?Sized>(&self)
+    where
+        Self: CastTo<U>;
+}
+impl<T: ?Sized> Cast for T {
+    #[inline(always)]
+    fn cast<U: ?Sized>(&self)
+    where
+        Self: CastTo<U>,
+    {
+        let x: &U = self;
+    }
+}
+
+fn main() {
+    // When we inline this call, then we run GVN, then
+    // GVN tries to evaluate the `() -> [i32]` unsize.
+    // That's invalid!
+    ().cast::<[i32]>();
+}

tests/ui/mir/validate/validate-unsize-cast.rs

@@ -0,0 +1,20 @@
+error[E0277]: the trait bound `T: Unsize<U>` is not satisfied
+  --> $DIR/validate-unsize-cast.rs:10:42
+   |
+LL | impl<T: ?Sized, U: ?Sized> CastTo<U> for T {}
+   |                                          ^ the trait `Unsize<U>` is not implemented for `T`
+   |
+   = note: all implementations of `Unsize` are provided automatically by the compiler, see <https://doc.rust-lang.org/stable/std/marker/trait.Unsize.html> for more information
+note: required by a bound in `CastTo`
+  --> $DIR/validate-unsize-cast.rs:7:30
+   |
+LL | pub trait CastTo<U: ?Sized>: Unsize<U> {}
+   |                              ^^^^^^^^^ required by this bound in `CastTo`
+help: consider further restricting this bound
+   |
+LL | impl<T: ?Sized + std::marker::Unsize<U>, U: ?Sized> CastTo<U> for T {}
+   |                ++++++++++++++++++++++++
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0277`.