Safe Transmute: Check mutability before creating dst -> src obligation

bryangarza · bryangarza · commit 62663582375d · 2023-05-24T15:00:06.000-07:00
- Only create dst -&gt; src obligation if Dst is mutable
- Add some long comments to explain parts of the transmutability code that were
  unclear to me when reading
- Update/add tests
diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
@@ -2784,9 +2784,10 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
                     rustc_transmute::Reason::DstIsTooBig => {
                         format!("The size of `{src}` is smaller than the size of `{dst}`")
                     }
+                    // FIXME(bryangarza): Say exactly what the minimum alignments of src and dst are
                     rustc_transmute::Reason::DstHasStricterAlignment => {
                         format!(
-                            "The alignment of `{src}` should be stricter than that of `{dst}`, but it is not"
+                            "The minimum alignment of `{src}` should be greater than that of `{dst}`, but it is not"
                         )
                     }
                     rustc_transmute::Reason::DstIsMoreUnique => {
diff --git a/compiler/rustc_trait_selection/src/traits/select/confirmation.rs b/compiler/rustc_trait_selection/src/traits/select/confirmation.rs
@@ -6,6 +6,7 @@
 //!
 //! [rustc dev guide]:
 //! https://rustc-dev-guide.rust-lang.org/traits/resolution.html#confirmation
+use rustc_ast::Mutability;
 use rustc_data_structures::stack::ensure_sufficient_stack;
 use rustc_hir::lang_items::LangItem;
 use rustc_infer::infer::LateBoundRegionConversionTime::HigherRankedType;
@@ -295,6 +296,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                 Ok(None) => Ok(vec![]),
                 Err(_) => Err(Unimplemented),
                 // FIXME(bryangarza): Add separate `IfAny` case, instead of treating as `IfAll`
+                // Not possible until the trait solver supports disjunctions of obligations
                 Ok(Some(rustc_transmute::Condition::IfAll(answers)))
                 | Ok(Some(rustc_transmute::Condition::IfAny(answers))) => {
                     let mut nested = vec![];
@@ -311,7 +313,8 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                     let scope = predicate.trait_ref.substs.type_at(2);
                     let assume_const = predicate.trait_ref.substs.const_at(3);
                     let make_obl = |from_ty, to_ty| {
-                        let trait_ref1 = tcx.mk_trait_ref(
+                        let trait_ref1 = ty::TraitRef::new(
+                            tcx,
                             trait_def_id,
                             [
                                 ty::GenericArg::from(to_ty),
@@ -329,8 +332,14 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                         )
                     };
 
-                    // FIXME(bryangarza): Check src.mutability or dst.mutability to know whether dst -> src obligation is needed
-                    Ok(vec![make_obl(src.ty, dst.ty), make_obl(dst.ty, src.ty)])
+                    // If Dst is mutable, check bidirectionally.
+                    // For example, transmuting bool -> u8 is OK as long as you can't update that u8
+                    // to be > 1, because you could later transmute the u8 back to a bool and get UB.
+                    let mut obligations = vec![make_obl(src.ty, dst.ty)];
+                    if dst.mutability == Mutability::Mut {
+                        obligations.push(make_obl(dst.ty, src.ty));
+                    }
+                    Ok(obligations)
                 }
             }
         }
@@ -353,6 +362,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
 
         let dst = predicate.trait_ref.substs.type_at(0);
         let src = predicate.trait_ref.substs.type_at(1);
+        debug!(?src, ?dst);
         let mut transmute_env = rustc_transmute::TransmuteTypeEnv::new(self.infcx);
         let maybe_transmutable = transmute_env.is_transmutable(
             obligation.cause.clone(),
@@ -361,7 +371,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
             assume,
         );
 
-        debug!(?src, ?dst);
         let fully_flattened =
             flatten_answer_tree(self.tcx(), obligation, predicate, maybe_transmutable)?;
         debug!(?fully_flattened);
diff --git a/compiler/rustc_transmute/src/layout/mod.rs b/compiler/rustc_transmute/src/layout/mod.rs
@@ -31,18 +31,21 @@ impl fmt::Debug for Byte {
 
 pub(crate) trait Def: Debug + Hash + Eq + PartialEq + Copy + Clone {}
 pub trait Ref: Debug + Hash + Eq + PartialEq + Copy + Clone {
+    fn min_align(&self) -> usize;
+
+    fn is_mutable(&self) -> bool;
+}
+
+impl Def for ! {}
+impl Ref for ! {
     fn min_align(&self) -> usize {
-        1
+        unreachable!()
     }
-
     fn is_mutable(&self) -> bool {
-        false
+        unreachable!()
     }
 }
 
-impl Def for ! {}
-impl Ref for ! {}
-
 #[cfg(feature = "rustc")]
 pub mod rustc {
     use rustc_middle::mir::Mutability;
diff --git a/compiler/rustc_transmute/src/lib.rs b/compiler/rustc_transmute/src/lib.rs
@@ -1,4 +1,4 @@
-#![feature(alloc_layout_extra, decl_macro, iterator_try_reduce, never_type, let_chains)]
+#![feature(alloc_layout_extra, decl_macro, iterator_try_reduce, never_type)]
 #![allow(dead_code, unused_variables)]
 #![deny(rustc::untranslatable_diagnostic)]
 #![deny(rustc::diagnostic_outside_of_impl)]
diff --git a/compiler/rustc_transmute/src/maybe_transmutable/mod.rs b/compiler/rustc_transmute/src/maybe_transmutable/mod.rs
@@ -203,8 +203,29 @@ where
         if let Some(answer) = cache.get(&(src_state, dst_state)) {
             answer.clone()
         } else {
+            debug!(?src_state, ?dst_state);
+            debug!(src = ?self.src);
+            debug!(dst = ?self.dst);
+            debug!(
+                src_transitions_len = self.src.transitions.len(),
+                dst_transitions_len = self.dst.transitions.len()
+            );
             let answer = if dst_state == self.dst.accepting {
                 // truncation: `size_of(Src) >= size_of(Dst)`
+                //
+                // Why is truncation OK to do? Because even though the Src is bigger, all we care about
+                // is whether we have enough data for the Dst to be valid in accordance with what its
+                // type dictates.
+                // For example, in a u8 to `()` transmutation, we have enough data available from the u8
+                // to transmute it to a `()` (though in this case does `()` really need any data to
+                // begin with? It doesn't). Same thing with u8 to fieldless struct.
+                // Now then, why is something like u8 to bool not allowed? That is not because the bool
+                // is smaller in size, but rather because those 2 bits that we are re-interpreting from
+                // the u8 could introduce invalid states for the bool type.
+                //
+                // So, if it's possible to transmute to a smaller Dst by truncating, and we can guarantee
+                // that none of the actually-used data can introduce an invalid state for Dst's type, we
+                // are able to safely transmute, even with truncation.
                 Ok(None)
             } else if src_state == self.src.accepting {
                 // extension: `size_of(Src) >= size_of(Dst)`
@@ -259,6 +280,7 @@ where
                 // ...if `refs_answer` was computed lazily. The below early
                 // returns can be deleted without impacting the correctness of
                 // the algoritm; only its performance.
+                debug!(?bytes_answer);
                 match bytes_answer {
                     Err(_) if !self.assume.validity => return bytes_answer,
                     Ok(None) if self.assume.validity => return bytes_answer,
diff --git a/tests/ui/transmutability/alignment/align-fail.stderr b/tests/ui/transmutability/alignment/align-fail.stderr
@@ -1,8 +1,8 @@
 error[E0277]: `&[u8; 0]` cannot be safely transmuted into `&[u16; 0]` in the defining scope of `assert::Context`
   --> $DIR/align-fail.rs:22:55
    |
-LL | ...tatic [u8; 0], &'static [u16; 0]>();
-   |                   ^^^^^^^^^^^^^^^^^ The alignment of `&[u8; 0]` should be stricter than that of `&[u16; 0]`, but it is not
+LL | ...c [u8; 0], &'static [u16; 0]>();
+   |               ^^^^^^^^^^^^^^^^^ The minimum alignment of `&[u8; 0]` should be greater than that of `&[u16; 0]`, but it is not
    |
 note: required by a bound in `is_maybe_transmutable`
   --> $DIR/align-fail.rs:10:14
diff --git a/tests/ui/transmutability/primitives/bool-mut.rs b/tests/ui/transmutability/primitives/bool-mut.rs
@@ -0,0 +1,17 @@
+// check-fail
+//[next] compile-flags: -Ztrait-solver=next
+
+#![feature(transmutability)]
+mod assert {
+    use std::mem::{Assume, BikeshedIntrinsicFrom};
+    pub struct Context;
+
+    pub fn is_transmutable<Src, Dst>()
+    where
+        Dst: BikeshedIntrinsicFrom<Src, Context, { Assume::SAFETY }>
+    {}
+}
+
+fn main() {
+    assert::is_transmutable::<&'static mut bool, &'static mut u8>() //~ ERROR cannot be safely transmuted
+}
diff --git a/tests/ui/transmutability/primitives/bool-mut.stderr b/tests/ui/transmutability/primitives/bool-mut.stderr
@@ -0,0 +1,18 @@
+error[E0277]: `u8` cannot be safely transmuted into `bool` in the defining scope of `assert::Context`
+  --> $DIR/bool-mut.rs:16:50
+   |
+LL |     assert::is_transmutable::<&'static mut bool, &'static mut u8>()
+   |                                                  ^^^^^^^^^^^^^^^ At least one value of `u8` isn't a bit-valid value of `bool`
+   |
+note: required by a bound in `is_transmutable`
+  --> $DIR/bool-mut.rs:11:14
+   |
+LL |     pub fn is_transmutable<Src, Dst>()
+   |            --------------- required by a bound in this function
+LL |     where
+LL |         Dst: BikeshedIntrinsicFrom<Src, Context, { Assume::SAFETY }>
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `is_transmutable`
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0277`.
diff --git a/tests/ui/transmutability/primitives/bool.current.stderr b/tests/ui/transmutability/primitives/bool.current.stderr
@@ -1,11 +1,11 @@
 error[E0277]: `u8` cannot be safely transmuted into `bool` in the defining scope of `assert::Context`
-  --> $DIR/bool.rs:24:35
+  --> $DIR/bool.rs:21:35
    |
 LL |     assert::is_transmutable::<u8, bool>();
    |                                   ^^^^ At least one value of `u8` isn't a bit-valid value of `bool`
    |
 note: required by a bound in `is_transmutable`
-  --> $DIR/bool.rs:14:14
+  --> $DIR/bool.rs:11:14
    |
 LL |     pub fn is_transmutable<Src, Dst>()
    |            --------------- required by a bound in this function
diff --git a/tests/ui/transmutability/primitives/bool.next.stderr b/tests/ui/transmutability/primitives/bool.next.stderr
@@ -1,11 +1,11 @@
 error[E0277]: `u8` cannot be safely transmuted into `bool` in the defining scope of `assert::Context`
-  --> $DIR/bool.rs:24:35
+  --> $DIR/bool.rs:21:35
    |
 LL |     assert::is_transmutable::<u8, bool>();
    |                                   ^^^^ At least one value of `u8` isn't a bit-valid value of `bool`
    |
 note: required by a bound in `is_transmutable`
-  --> $DIR/bool.rs:14:14
+  --> $DIR/bool.rs:11:14
    |
 LL |     pub fn is_transmutable<Src, Dst>()
    |            --------------- required by a bound in this function
diff --git a/tests/ui/transmutability/primitives/bool.rs b/tests/ui/transmutability/primitives/bool.rs
@@ -1,10 +1,7 @@
 // revisions: current next
 //[next] compile-flags: -Ztrait-solver=next
 
-#![crate_type = "lib"]
 #![feature(transmutability)]
-#![allow(dead_code)]
-#![allow(incomplete_features)]
 mod assert {
     use std::mem::{Assume, BikeshedIntrinsicFrom};
     pub struct Context;
@@ -20,7 +17,7 @@ mod assert {
     {}
 }
 
-fn contrast_with_u8() {
+fn main() {
     assert::is_transmutable::<u8, bool>(); //~ ERROR cannot be safely transmuted
     assert::is_maybe_transmutable::<u8, bool>();
     assert::is_transmutable::<bool, u8>();
diff --git a/tests/ui/transmutability/references/recursive-wrapper-types-bit-compatible-mut.rs b/tests/ui/transmutability/references/recursive-wrapper-types-bit-compatible-mut.rs
@@ -0,0 +1,25 @@
+// check-fail
+#![feature(transmutability)]
+
+mod assert {
+    use std::mem::{Assume, BikeshedIntrinsicFrom};
+    pub struct Context;
+
+    pub fn is_maybe_transmutable<Src, Dst>()
+    where
+        Dst: BikeshedIntrinsicFrom<Src, Context, {
+            Assume {
+                alignment: true,
+                lifetimes: false,
+                safety: true,
+                validity: false,
+            }
+        }>
+    {}
+}
+
+fn main() {
+    #[repr(C)] struct A(bool, &'static A);
+    #[repr(C)] struct B(u8, &'static B);
+    assert::is_maybe_transmutable::<&'static A, &'static mut B>(); //~ ERROR cannot be safely transmuted
+}
diff --git a/tests/ui/transmutability/references/recursive-wrapper-types-bit-compatible-mut.stderr b/tests/ui/transmutability/references/recursive-wrapper-types-bit-compatible-mut.stderr
@@ -1,11 +1,11 @@
-error[E0277]: `B` cannot be safely transmuted into `A` in the defining scope of `assert::Context`
-  --> $DIR/recursive-wrapper-types-bit-compatible.rs:26:49
+error[E0277]: `&A` cannot be safely transmuted into `&mut B` in the defining scope of `assert::Context`
+  --> $DIR/recursive-wrapper-types-bit-compatible-mut.rs:24:49
    |
-LL |     assert::is_maybe_transmutable::<&'static A, &'static B>();
-   |                                                 ^^^^^^^^^^ At least one value of `B` isn't a bit-valid value of `A`
+LL |     assert::is_maybe_transmutable::<&'static A, &'static mut B>();
+   |                                                 ^^^^^^^^^^^^^^ `&A` is a shared reference, but `&mut B` is a unique reference
    |
 note: required by a bound in `is_maybe_transmutable`
-  --> $DIR/recursive-wrapper-types-bit-compatible.rs:10:14
+  --> $DIR/recursive-wrapper-types-bit-compatible-mut.rs:10:14
    |
 LL |       pub fn is_maybe_transmutable<Src, Dst>()
    |              --------------------- required by a bound in this function
diff --git a/tests/ui/transmutability/references/recursive-wrapper-types-bit-compatible.rs b/tests/ui/transmutability/references/recursive-wrapper-types-bit-compatible.rs
@@ -1,4 +1,4 @@
-// check-fail
+// check-pass
 #![feature(transmutability)]
 
 mod assert {
@@ -21,7 +21,5 @@ mod assert {
 fn main() {
     #[repr(C)] struct A(bool, &'static A);
     #[repr(C)] struct B(u8, &'static B);
-    // FIXME(bryangarza): Make 2 variants of this test, depending on mutability.
-    // Right now, we are being strict by default and checking A->B and B->A both.
-    assert::is_maybe_transmutable::<&'static A, &'static B>(); //~ ERROR `B` cannot be safely transmuted into `A`
+    assert::is_maybe_transmutable::<&'static A, &'static B>();
 }
diff --git a/tests/ui/transmutability/references/recursive-wrapper-types-bit-incompatible.rs b/tests/ui/transmutability/references/recursive-wrapper-types-bit-incompatible.rs
@@ -21,5 +21,5 @@ mod assert {
 fn main() {
     #[repr(C)] struct A(bool, &'static A);
     #[repr(C)] struct B(u8, &'static B);
-    assert::is_maybe_transmutable::<&'static B, &'static A>(); //~ ERROR `B` cannot be safely transmuted into `A`
+    assert::is_maybe_transmutable::<&'static B, &'static A>(); //~ ERROR cannot be safely transmuted
 }
diff --git a/tests/ui/transmutability/references/u8-to-unit.rs b/tests/ui/transmutability/references/u8-to-unit.rs
@@ -1,4 +1,4 @@
-// check-fail
+// check-pass
 #![feature(transmutability)]
 
 mod assert {
@@ -9,16 +9,16 @@ mod assert {
     where
         Dst: BikeshedIntrinsicFrom<Src, Context, {
             Assume {
-                alignment: true,
+                alignment: false,
                 lifetimes: true,
                 safety: true,
-                validity: true,
+                validity: false,
             }
         }>
     {}
 }
 
 fn main() {
     #[repr(C)] struct Unit;
-    assert::is_maybe_transmutable::<&'static u8, &'static Unit>(); //~ ERROR `Unit` cannot be safely transmuted into `u8`
+    assert::is_maybe_transmutable::<&'static u8, &'static Unit>();
 }
diff --git a/tests/ui/transmutability/references/u8-to-unit.stderr b/tests/ui/transmutability/references/u8-to-unit.stderr
diff --git a/tests/ui/transmutability/references/unit-to-u8.rs b/tests/ui/transmutability/references/unit-to-u8.rs
@@ -12,13 +12,13 @@ mod assert {
                 alignment: true,
                 lifetimes: true,
                 safety: true,
-                validity: true,
+                validity: false,
             }
         }>
     {}
 }
 
 fn main() {
     #[repr(C)] struct Unit;
-    assert::is_maybe_transmutable::<&'static Unit, &'static u8>(); //~ ERROR `Unit` cannot be safely transmuted into `u8`
+    assert::is_maybe_transmutable::<&'static Unit, &'static u8>(); //~ ERROR cannot be safely transmuted
 }

Original file line number	Diff line number	Diff line change
`@@ -2784,9 +2784,10 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {`
`2784`	`2784`	`rustc_transmute::Reason::DstIsTooBig => {`
`2785`	`2785`	format!("The size of `{src}` is smaller than the size of `{dst}`")
`2786`	`2786`	`}`
	`2787`	`+ // FIXME(bryangarza): Say exactly what the minimum alignments of src and dst are`
`2787`	`2788`	`rustc_transmute::Reason::DstHasStricterAlignment => {`
`2788`	`2789`	`format!(`
`2789`		- "The alignment of `{src}` should be stricter than that of `{dst}`, but it is not"
	`2790`	+ "The minimum alignment of `{src}` should be greater than that of `{dst}`, but it is not"
`2790`	`2791`	`)`
`2791`	`2792`	`}`
`2792`	`2793`	`rustc_transmute::Reason::DstIsMoreUnique => {`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-#![feature(alloc_layout_extra, decl_macro, iterator_try_reduce, never_type, let_chains)]`
	`1`	`+#![feature(alloc_layout_extra, decl_macro, iterator_try_reduce, never_type)]`
`2`	`2`	`#![allow(dead_code, unused_variables)]`
`3`	`3`	`#![deny(rustc::untranslatable_diagnostic)]`
`4`	`4`	`#![deny(rustc::diagnostic_outside_of_impl)]`
Original file line number	Diff line number	Diff line change
`@@ -1,8 +1,8 @@`
`1`	`1`	error[E0277]: `&[u8; 0]` cannot be safely transmuted into `&[u16; 0]` in the defining scope of `assert::Context`
`2`	`2`	`--> $DIR/align-fail.rs:22:55`
`3`	`3`	`\|`
`4`		`-LL \| ...tatic [u8; 0], &'static [u16; 0]>();`
`5`		- \| ^^^^^^^^^^^^^^^^^ The alignment of `&[u8; 0]` should be stricter than that of `&[u16; 0]`, but it is not
	`4`	`+LL \| ...c [u8; 0], &'static [u16; 0]>();`
	`5`	+ \| ^^^^^^^^^^^^^^^^^ The minimum alignment of `&[u8; 0]` should be greater than that of `&[u16; 0]`, but it is not
`6`	`6`	`\|`
`7`	`7`	note: required by a bound in `is_maybe_transmutable`
`8`	`8`	`--> $DIR/align-fail.rs:10:14`
Original file line number	Diff line number	Diff line change
`@@ -1,11 +1,11 @@`
`1`	`1`	error[E0277]: `u8` cannot be safely transmuted into `bool` in the defining scope of `assert::Context`
`2`		`- --> $DIR/bool.rs:24:35`
	`2`	`+ --> $DIR/bool.rs:21:35`
`3`	`3`	`\|`
`4`	`4`	`LL \| assert::is_transmutable::<u8, bool>();`
`5`	`5`	\| ^^^^ At least one value of `u8` isn't a bit-valid value of `bool`
`6`	`6`	`\|`
`7`	`7`	note: required by a bound in `is_transmutable`
`8`		`- --> $DIR/bool.rs:14:14`
	`8`	`+ --> $DIR/bool.rs:11:14`
`9`	`9`	`\|`
`10`	`10`	`LL \| pub fn is_transmutable<Src, Dst>()`
`11`	`11`	`\| --------------- required by a bound in this function`