diff --git a/crates/bevy_reflect/compile_fail/tests/reflect_remote/nested_fail.rs b/crates/bevy_reflect/compile_fail/tests/reflect_remote/nested_fail.rs index 391258ccc6507..4454d232128c3 100644 --- a/crates/bevy_reflect/compile_fail/tests/reflect_remote/nested_fail.rs +++ b/crates/bevy_reflect/compile_fail/tests/reflect_remote/nested_fail.rs @@ -44,6 +44,7 @@ mod mismatched_remote_type { #[reflect_remote(super::external_crate::TheirOuter)] //~^ ERROR: mismatched types //~| ERROR: mismatched types + //~| ERROR: mismatched types struct MyOuter { // Reason: Should be `MyInner` #[reflect(remote = MyOuter)] diff --git a/crates/bevy_reflect/compile_fail/tests/reflect_remote/type_mismatch_fail.rs b/crates/bevy_reflect/compile_fail/tests/reflect_remote/type_mismatch_fail.rs index e3c894e6d5fa3..6871434939053 100644 --- a/crates/bevy_reflect/compile_fail/tests/reflect_remote/type_mismatch_fail.rs +++ b/crates/bevy_reflect/compile_fail/tests/reflect_remote/type_mismatch_fail.rs @@ -24,6 +24,7 @@ mod structs { #[derive(Reflect)] //~^ ERROR: mismatched types //~| ERROR: mismatched types + //~| ERROR: mismatched types struct MyStruct { // Reason: Should use `MyFoo` #[reflect(remote = MyBar)] @@ -50,6 +51,7 @@ mod tuple_structs { #[derive(Reflect)] //~^ ERROR: mismatched types //~| ERROR: mismatched types + //~| ERROR: mismatched types struct MyStruct( // Reason: Should use `MyFoo` #[reflect(remote = MyBar)] external_crate::TheirFoo, diff --git a/crates/bevy_reflect/derive/src/field_attributes.rs b/crates/bevy_reflect/derive/src/field_attributes.rs index 2f7369f9545c9..715d6d8758b06 100644 --- a/crates/bevy_reflect/derive/src/field_attributes.rs +++ b/crates/bevy_reflect/derive/src/field_attributes.rs @@ -252,21 +252,4 @@ impl FieldAttributes { Ok(()) } - - /// Returns `Some(true)` if the field has a generic remote type. - /// - /// If the remote type is not generic, returns `Some(false)`. - /// - /// If the field does not have a remote type, returns `None`. - pub fn is_remote_generic(&self) -> Option { - if let Type::Path(type_path) = self.remote.as_ref()? { - type_path - .path - .segments - .last() - .map(|segment| !segment.arguments.is_empty()) - } else { - Some(false) - } - } } diff --git a/crates/bevy_reflect/derive/src/from_reflect.rs b/crates/bevy_reflect/derive/src/from_reflect.rs index 7ba645b28a11a..efe08ce114d15 100644 --- a/crates/bevy_reflect/derive/src/from_reflect.rs +++ b/crates/bevy_reflect/derive/src/from_reflect.rs @@ -265,23 +265,14 @@ fn get_active_fields( }; let into_remote = |value: proc_macro2::TokenStream| { - if field.attrs.is_remote_generic().unwrap_or_default() { + if field.attrs().remote.is_some() { quote! { #FQOption::Some( - // SAFETY: The remote type should always be a `#[repr(transparent)]` for the actual field type - unsafe { - ::core::mem::transmute_copy::<#ty, #real_ty>( - &::core::mem::ManuallyDrop::new(#value?) - ) - } - ) - } - } else if field.attrs().remote.is_some() { - quote! { - #FQOption::Some( - // SAFETY: The remote type should always be a `#[repr(transparent)]` for the actual field type - unsafe { - ::core::mem::transmute::<#ty, #real_ty>(#value?) + { + let wrapper: #ty = #value?; + let remote: #real_ty = + <#ty as #bevy_reflect_path::ReflectRemote>::into_remote(wrapper); + remote } ) } diff --git a/crates/bevy_reflect/derive/src/impls/enums.rs b/crates/bevy_reflect/derive/src/impls/enums.rs index 05e04260a9d33..6eb08996625eb 100644 --- a/crates/bevy_reflect/derive/src/impls/enums.rs +++ b/crates/bevy_reflect/derive/src/impls/enums.rs @@ -334,7 +334,7 @@ fn generate_impls(reflect_enum: &ReflectEnum, ref_index: &Ident, ref_name: &Iden /// Process the field value to account for remote types. /// - /// If the field is a remote type, then the value will be transmuted accordingly. + /// If the field is a remote type, then the value will be converted accordingly. fn process_field_value( ident: &Ident, field: &StructField, diff --git a/crates/bevy_reflect/derive/src/lib.rs b/crates/bevy_reflect/derive/src/lib.rs index c05936d1a16f9..4ef6c2d80b919 100644 --- a/crates/bevy_reflect/derive/src/lib.rs +++ b/crates/bevy_reflect/derive/src/lib.rs @@ -570,7 +570,8 @@ pub fn reflect_trait(args: TokenStream, input: TokenStream) -> TokenStream { /// Generates a wrapper type that can be used to "derive `Reflect`" for remote types. /// /// This works by wrapping the remote type in a generated wrapper that has the `#[repr(transparent)]` attribute. -/// This allows the two types to be safely [transmuted] back-and-forth. +/// The generated `ReflectRemote` implementation uses this representation to convert between the +/// wrapper and remote type. /// /// # Defining the Wrapper /// @@ -640,13 +641,12 @@ pub fn reflect_trait(args: TokenStream, input: TokenStream) -> TokenStream { /// } /// ``` /// -/// ## Safety +/// ## Conversion /// -/// When using the `#[reflect(remote = path::to::MyType)]` field attribute, be sure you are defining the correct wrapper type. -/// Internally, this field will be unsafely [transmuted], and is only sound if using a wrapper generated for the remote type. -/// This also means keeping your wrapper definitions up-to-date with the remote types. +/// The wrapper type must implement `ReflectRemote` with `Remote` equal to the field's actual +/// type. Generated reflection code uses the wrapper's conversion methods; in particular, +/// `FromReflect` calls `ReflectRemote::into_remote` to construct the field value. /// -/// [transmuted]: std::mem::transmute #[proc_macro_attribute] pub fn reflect_remote(args: TokenStream, input: TokenStream) -> TokenStream { remote::reflect_remote(args, input) diff --git a/crates/bevy_reflect/derive/src/struct_utility.rs b/crates/bevy_reflect/derive/src/struct_utility.rs index 9bfd72de60596..18736a24fbd64 100644 --- a/crates/bevy_reflect/derive/src/struct_utility.rs +++ b/crates/bevy_reflect/derive/src/struct_utility.rs @@ -2,10 +2,8 @@ use crate::ReflectStruct; /// A helper struct for creating remote-aware field accessors. /// -/// These are "remote-aware" because when a field is a remote field, it uses a [`transmute`] internally -/// to access the field. -/// -/// [`transmute`]: std::mem::transmute +/// These are "remote-aware" because when a field is a remote field, they use its +/// `ReflectRemote` implementation to access the field. pub(crate) struct FieldAccessors { /// The referenced field accessors, such as `&self.foo`. pub fields_ref: Vec, diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index 44a7bae1c7fb3..9f771f23746b3 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -3625,6 +3625,52 @@ bevy_reflect::tests::Test { assert_eq!("Goodbye", data.0.value); } + #[test] + fn from_reflect_uses_remote_conversion() { + #[derive(Reflect)] + struct BadWrapper(u8); + + impl ReflectRemote for BadWrapper { + type Remote = bool; + + fn as_remote(&self) -> &Self::Remote { + panic!("not used by this test") + } + + fn as_remote_mut(&mut self) -> &mut Self::Remote { + panic!("not used by this test") + } + + fn into_remote(self) -> Self::Remote { + false + } + + fn as_wrapper(_: &Self::Remote) -> &Self { + panic!("not used by this test") + } + + fn as_wrapper_mut(_: &mut Self::Remote) -> &mut Self { + panic!("not used by this test") + } + + fn into_wrapper(_: Self::Remote) -> Self { + panic!("not used by this test") + } + } + + #[derive(Reflect)] + struct Container { + #[reflect(remote = BadWrapper)] + value: bool, + } + + let mut reflected = DynamicStruct::default(); + reflected.insert("value", BadWrapper(2)); + + let container = Container::from_reflect(&reflected).unwrap(); + assert!(!container.value); + } + #[test] fn should_reflect_remote_value_type() { mod external_crate { diff --git a/crates/bevy_reflect/src/remote.rs b/crates/bevy_reflect/src/remote.rs index a19d3bdb148d6..ae28ca6f3dc60 100644 --- a/crates/bevy_reflect/src/remote.rs +++ b/crates/bevy_reflect/src/remote.rs @@ -4,18 +4,15 @@ use crate::Reflect; /// /// This allows types from external libraries (remote types) to be included in reflection. /// -/// # Safety +/// The [`#[reflect_remote]`](crate::reflect_remote) attribute macro generates a +/// `#[repr(transparent)]` wrapper and an implementation of this trait. Its conversion methods +/// use the wrapper's transparent representation. /// -/// It is highly recommended to avoid implementing this trait manually and instead use the -/// [`#[reflect_remote]`](crate::reflect_remote) attribute macro. -/// This is because the trait tends to rely on [`transmute`], which is [very unsafe]. -/// -/// The macro will ensure that the following safety requirements are met: -/// - `Self` is a single-field tuple struct (i.e. a newtype) containing the remote type. -/// - `Self` is `#[repr(transparent)]` over the remote type. -/// -/// Additionally, the macro will automatically generate [`Reflect`] and [`FromReflect`] implementations, -/// along with compile-time assertions to validate that the safety requirements have been met. +/// Manual implementations may use a different representation and conversion behavior. The +/// associated `Remote` type identifies the remote type represented by this wrapper. +/// When implementing this trait manually, you need to design carefully about the conversion +/// between `Self` and `Remote` type. For example, if you need to resolve the conversion of +/// `u8` and `bool`, you can set the rule that all the values that more than 1 return false. /// /// # Example /// @@ -41,9 +38,6 @@ use crate::Reflect; /// ``` /// /// [reflectable]: Reflect -/// [`transmute`]: core::mem::transmute -/// [very unsafe]: https://doc.rust-lang.org/1.71.0/nomicon/transmutes.html -/// [`FromReflect`]: crate::FromReflect pub trait ReflectRemote: Reflect { /// The remote type this type represents via reflection. type Remote;