Skip to content

Commit

Permalink
Add a debug assertion in codegen that unsize casts of the same princi…
Browse files Browse the repository at this point in the history
…pal trait def id are truly NOPs
  • Loading branch information
compiler-errors committed Sep 23, 2024
1 parent 481886e commit 6bdfd13
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 16 deletions.
17 changes: 16 additions & 1 deletion compiler/rustc_codegen_cranelift/src/unsize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
17 changes: 17 additions & 0 deletions compiler/rustc_codegen_ssa/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,23 @@ 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() {
// 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, should be allowed even with
// invalid vtables.
return old_info;
Expand Down
15 changes: 0 additions & 15 deletions compiler/rustc_mir_transform/src/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1236,21 +1236,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<A>: Unsize<dyn Tr<B>>`
// 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<Struct<.., dyn Tr<A>, ..>>` 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::IntToInt | CastKind::IntToFloat => {
let input_valid = op_ty.is_integral() || op_ty.is_char() || op_ty.is_bool();
Expand Down

0 comments on commit 6bdfd13

Please sign in to comment.