From 3209943604a9b3565a3cef4c43b567f65cfdf192 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 23 Sep 2024 13:52:02 -0400 Subject: [PATCH] Add a debug assertion in codegen that unsize casts of the same principal trait def id are truly NOPs --- .../rustc_codegen_cranelift/src/unsize.rs | 17 ++++++++++++- compiler/rustc_codegen_ssa/src/base.rs | 24 +++++++++++++++++-- compiler/rustc_mir_transform/src/validate.rs | 15 ------------ 3 files changed, 38 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/unsize.rs b/compiler/rustc_codegen_cranelift/src/unsize.rs index 8cfe93b4d9c..339628053a9 100644 --- a/compiler/rustc_codegen_cranelift/src/unsize.rs +++ b/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: Unsize>` + // 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; } diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index fcf48d3e4a3..5c67600e4ee 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/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: Unsize>` + // 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`) + // are also equal, which is ensured by the fact that normalization is + // a function and we do not allow overlapping impls. return old_info; } diff --git a/compiler/rustc_mir_transform/src/validate.rs b/compiler/rustc_mir_transform/src/validate.rs index c9a844e4734..e353be6a105 100644 --- a/compiler/rustc_mir_transform/src/validate.rs +++ b/compiler/rustc_mir_transform/src/validate.rs @@ -1233,21 +1233,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { )) { self.fail(location, format!("Unsize coercion, but `{op_ty}` isn't coercible to `{target_type}`")); } - - // FIXME: Codegen has an 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: Unsize>` - // requires that `A = B`; we don't allow *upcasting* objects - // between the same trait with different args. Nothing actually - // validates this, though. While it's true right now, if we for - // some reason were to relax the `Unsize` trait, it could become - // unsound. We should eventually validate that, but it would - // require peeling `&Box, ..>>` down to - // the trait object that's being unsized, and that's rather - // annoying, and also it would need to be opportunistic since - // this MIR is not yet fully monomorphized, so we may bottom - // out in an alias or a projection or something. } CastKind::PointerCoercion(PointerCoercion::DynStar, _) => { // FIXME(dyn-star): make sure nothing needs to be done here.