From d80078cf2d76e4c3f6bb5edc0a011a5eafc074a5 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 8 Jul 2025 05:57:29 +1000 Subject: [PATCH] bevy_reflect: Introduce `reflect_clone_and_take`. (#19944) # Objective There is a pattern that appears in multiple places, involving `reflect_clone`, followed by `take`, followed by `map_err` that produces a `FailedDowncast` in a particular form. ## Solution Introduces `reflect_clone_and_take`, which factors out the repeated code. ## Testing `cargo run -p ci` --------- Co-authored-by: Alice Cecile --- .../reflect_remote/invalid_definition_fail.rs | 4 ++++ .../tests/reflect_remote/nested_fail.rs | 2 +- .../tests/reflect_remote/type_mismatch_fail.rs | 1 + crates/bevy_reflect/derive/src/derive_data.rs | 13 +------------ crates/bevy_reflect/derive/src/enum_utility.rs | 15 +-------------- .../src/impls/alloc/collections/btree/map.rs | 18 ++---------------- crates/bevy_reflect/src/impls/macros/list.rs | 9 +-------- crates/bevy_reflect/src/impls/macros/map.rs | 14 ++------------ crates/bevy_reflect/src/impls/macros/set.rs | 7 +------ crates/bevy_reflect/src/impls/smallvec.rs | 17 ++++++----------- crates/bevy_reflect/src/reflect.rs | 18 ++++++++++++++++++ 11 files changed, 38 insertions(+), 80 deletions(-) diff --git a/crates/bevy_reflect/compile_fail/tests/reflect_remote/invalid_definition_fail.rs b/crates/bevy_reflect/compile_fail/tests/reflect_remote/invalid_definition_fail.rs index d691c824cc..67135669f5 100644 --- a/crates/bevy_reflect/compile_fail/tests/reflect_remote/invalid_definition_fail.rs +++ b/crates/bevy_reflect/compile_fail/tests/reflect_remote/invalid_definition_fail.rs @@ -9,6 +9,7 @@ mod structs { #[reflect_remote(external_crate::TheirStruct)] //~^ ERROR: `?` operator has incompatible types + //~| ERROR: mismatched types struct MyStruct { // Reason: Should be `u32` pub value: bool, @@ -25,6 +26,7 @@ mod tuple_structs { #[reflect_remote(external_crate::TheirStruct)] //~^ ERROR: `?` operator has incompatible types + //~| ERROR: mismatched types struct MyStruct( // Reason: Should be `u32` pub bool, @@ -48,6 +50,7 @@ mod enums { //~| ERROR: variant `enums::external_crate::TheirStruct::Unit` has no field named `0` //~| ERROR: `?` operator has incompatible types //~| ERROR: `?` operator has incompatible types + //~| ERROR: mismatched types enum MyStruct { // Reason: Should be unit variant Unit(i32), @@ -57,6 +60,7 @@ mod enums { // Reason: Should be `usize` Struct { value: String }, //~^ ERROR: mismatched types + //~| ERROR: mismatched types } } 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 457f1f75e5..391258ccc6 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 @@ -26,8 +26,8 @@ mod incorrect_inner_type { //~| ERROR: `TheirInner` does not implement `PartialReflect` so cannot be introspected //~| ERROR: `TheirInner` does not implement `PartialReflect` so cannot be introspected //~| ERROR: `TheirInner` does not implement `TypePath` so cannot provide dynamic type path information - //~| ERROR: `TheirInner` does not implement `TypePath` so cannot provide dynamic type path information //~| ERROR: `?` operator has incompatible types + //~| ERROR: mismatched types struct MyOuter { // Reason: Should not use `MyInner` directly pub inner: MyInner, 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 1983442ab7..e3c894e6d5 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 @@ -77,6 +77,7 @@ mod enums { #[reflect_remote(external_crate::TheirBar)] //~^ ERROR: `?` operator has incompatible types + //~| ERROR: mismatched types enum MyBar { // Reason: Should use `i32` Value(u32), diff --git a/crates/bevy_reflect/derive/src/derive_data.rs b/crates/bevy_reflect/derive/src/derive_data.rs index 9af7acda76..3f6532a408 100644 --- a/crates/bevy_reflect/derive/src/derive_data.rs +++ b/crates/bevy_reflect/derive/src/derive_data.rs @@ -722,18 +722,7 @@ impl<'a> ReflectStruct<'a> { } } else { quote! { - #bevy_reflect_path::PartialReflect::reflect_clone(#accessor)? - .take() - .map_err(|value| #bevy_reflect_path::ReflectCloneError::FailedDowncast { - expected: #bevy_reflect_path::__macro_exports::alloc_utils::Cow::Borrowed( - <#field_ty as #bevy_reflect_path::TypePath>::type_path() - ), - received: #bevy_reflect_path::__macro_exports::alloc_utils::Cow::Owned( - #bevy_reflect_path::__macro_exports::alloc_utils::ToString::to_string( - #bevy_reflect_path::DynamicTypePath::reflect_type_path(&*value) - ) - ), - })? + <#field_ty as #bevy_reflect_path::PartialReflect>::reflect_clone_and_take(#accessor)? } }; diff --git a/crates/bevy_reflect/derive/src/enum_utility.rs b/crates/bevy_reflect/derive/src/enum_utility.rs index ad41a1a8df..c717b7723e 100644 --- a/crates/bevy_reflect/derive/src/enum_utility.rs +++ b/crates/bevy_reflect/derive/src/enum_utility.rs @@ -316,9 +316,7 @@ impl<'a> VariantBuilder for ReflectCloneVariantBuilder<'a> { fn construct_field(&self, field: VariantField) -> TokenStream { let bevy_reflect_path = self.reflect_enum.meta().bevy_reflect_path(); - let field_ty = field.field.reflected_type(); - let alias = field.alias; let alias = match &field.field.attrs.remote { Some(wrapper_ty) => { @@ -332,18 +330,7 @@ impl<'a> VariantBuilder for ReflectCloneVariantBuilder<'a> { match &field.field.attrs.clone { CloneBehavior::Default => { quote! { - #bevy_reflect_path::PartialReflect::reflect_clone(#alias)? - .take() - .map_err(|value| #bevy_reflect_path::ReflectCloneError::FailedDowncast { - expected: #bevy_reflect_path::__macro_exports::alloc_utils::Cow::Borrowed( - <#field_ty as #bevy_reflect_path::TypePath>::type_path() - ), - received: #bevy_reflect_path::__macro_exports::alloc_utils::Cow::Owned( - #bevy_reflect_path::__macro_exports::alloc_utils::ToString::to_string( - #bevy_reflect_path::DynamicTypePath::reflect_type_path(&*value) - ) - ), - })? + <#field_ty as #bevy_reflect_path::PartialReflect>::reflect_clone_and_take(#alias)? } } CloneBehavior::Trait => { diff --git a/crates/bevy_reflect/src/impls/alloc/collections/btree/map.rs b/crates/bevy_reflect/src/impls/alloc/collections/btree/map.rs index df68b425f6..d5559a2985 100644 --- a/crates/bevy_reflect/src/impls/alloc/collections/btree/map.rs +++ b/crates/bevy_reflect/src/impls/alloc/collections/btree/map.rs @@ -9,7 +9,6 @@ use crate::{ type_registry::{FromType, GetTypeRegistration, ReflectFromPtr, TypeRegistration}, utility::GenericTypeInfoCell, }; -use alloc::borrow::Cow; use alloc::vec::Vec; use bevy_platform::prelude::*; use bevy_reflect_derive::impl_type_path; @@ -144,21 +143,8 @@ where fn reflect_clone(&self) -> Result, ReflectCloneError> { let mut map = Self::new(); for (key, value) in self.iter() { - let key = - key.reflect_clone()? - .take() - .map_err(|_| ReflectCloneError::FailedDowncast { - expected: Cow::Borrowed(::type_path()), - received: Cow::Owned(key.reflect_type_path().to_string()), - })?; - let value = - value - .reflect_clone()? - .take() - .map_err(|_| ReflectCloneError::FailedDowncast { - expected: Cow::Borrowed(::type_path()), - received: Cow::Owned(value.reflect_type_path().to_string()), - })?; + let key = key.reflect_clone_and_take()?; + let value = value.reflect_clone_and_take()?; map.insert(key, value); } diff --git a/crates/bevy_reflect/src/impls/macros/list.rs b/crates/bevy_reflect/src/impls/macros/list.rs index 72e05ba10f..81a27047cb 100644 --- a/crates/bevy_reflect/src/impls/macros/list.rs +++ b/crates/bevy_reflect/src/impls/macros/list.rs @@ -113,14 +113,7 @@ macro_rules! impl_reflect_for_veclike { fn reflect_clone(&self) -> Result, $crate::error::ReflectCloneError> { Ok(bevy_platform::prelude::Box::new( self.iter() - .map(|value| { - value.reflect_clone()?.take().map_err(|_| { - $crate::error::ReflectCloneError::FailedDowncast { - expected: alloc::borrow::Cow::Borrowed(::type_path()), - received: alloc::borrow::Cow::Owned(alloc::string::ToString::to_string(value.reflect_type_path())), - } - }) - }) + .map(|value| value.reflect_clone_and_take()) .collect::>()?, )) } diff --git a/crates/bevy_reflect/src/impls/macros/map.rs b/crates/bevy_reflect/src/impls/macros/map.rs index d356ddba58..e87bb314b5 100644 --- a/crates/bevy_reflect/src/impls/macros/map.rs +++ b/crates/bevy_reflect/src/impls/macros/map.rs @@ -146,18 +146,8 @@ macro_rules! impl_reflect_for_hashmap { fn reflect_clone(&self) -> Result, $crate::error::ReflectCloneError> { let mut map = Self::with_capacity_and_hasher(self.len(), S::default()); for (key, value) in self.iter() { - let key = key.reflect_clone()?.take().map_err(|_| { - $crate::error::ReflectCloneError::FailedDowncast { - expected: alloc::borrow::Cow::Borrowed(::type_path()), - received: alloc::borrow::Cow::Owned(alloc::string::ToString::to_string(key.reflect_type_path())), - } - })?; - let value = value.reflect_clone()?.take().map_err(|_| { - $crate::error::ReflectCloneError::FailedDowncast { - expected: alloc::borrow::Cow::Borrowed(::type_path()), - received: alloc::borrow::Cow::Owned(alloc::string::ToString::to_string(value.reflect_type_path())), - } - })?; + let key = key.reflect_clone_and_take()?; + let value = value.reflect_clone_and_take()?; map.insert(key, value); } diff --git a/crates/bevy_reflect/src/impls/macros/set.rs b/crates/bevy_reflect/src/impls/macros/set.rs index 599ec1c0c8..844b904cde 100644 --- a/crates/bevy_reflect/src/impls/macros/set.rs +++ b/crates/bevy_reflect/src/impls/macros/set.rs @@ -129,12 +129,7 @@ macro_rules! impl_reflect_for_hashset { fn reflect_clone(&self) -> Result, $crate::error::ReflectCloneError> { let mut set = Self::with_capacity_and_hasher(self.len(), S::default()); for value in self.iter() { - let value = value.reflect_clone()?.take().map_err(|_| { - $crate::error::ReflectCloneError::FailedDowncast { - expected: alloc::borrow::Cow::Borrowed(::type_path()), - received: alloc::borrow::Cow::Owned(alloc::string::ToString::to_string(value.reflect_type_path())), - } - })?; + let value = value.reflect_clone_and_take()?; set.insert(value); } diff --git a/crates/bevy_reflect/src/impls/smallvec.rs b/crates/bevy_reflect/src/impls/smallvec.rs index 561111a901..86b7284381 100644 --- a/crates/bevy_reflect/src/impls/smallvec.rs +++ b/crates/bevy_reflect/src/impls/smallvec.rs @@ -4,7 +4,7 @@ use crate::{ ReflectMut, ReflectOwned, ReflectRef, TypeInfo, TypeParamInfo, TypePath, TypeRegistration, Typed, }; -use alloc::{borrow::Cow, boxed::Box, string::ToString, vec::Vec}; +use alloc::{boxed::Box, vec::Vec}; use bevy_reflect::ReflectCloneError; use bevy_reflect_derive::impl_type_path; use core::any::Any; @@ -137,16 +137,11 @@ where fn reflect_clone(&self) -> Result, ReflectCloneError> { Ok(Box::new( - self.iter() - .map(|value| { - value - .reflect_clone()? - .take() - .map_err(|_| ReflectCloneError::FailedDowncast { - expected: Cow::Borrowed(::type_path()), - received: Cow::Owned(value.reflect_type_path().to_string()), - }) - }) + // `(**self)` avoids getting `SmallVec as List::iter`, which + // would give us the wrong item type. + (**self) + .iter() + .map(PartialReflect::reflect_clone_and_take) .collect::>()?, )) } diff --git a/crates/bevy_reflect/src/reflect.rs b/crates/bevy_reflect/src/reflect.rs index 1bd1795066..ffe9be54fe 100644 --- a/crates/bevy_reflect/src/reflect.rs +++ b/crates/bevy_reflect/src/reflect.rs @@ -313,6 +313,24 @@ where }) } + /// For a type implementing [`PartialReflect`], combines `reflect_clone` and + /// `take` in a useful fashion, automatically constructing an appropriate + /// [`ReflectCloneError`] if the downcast fails. + /// + /// This is an associated function, rather than a method, because methods + /// with generic types prevent dyn-compatibility. + fn reflect_clone_and_take(&self) -> Result + where + Self: TypePath + Sized, + { + self.reflect_clone()? + .take() + .map_err(|_| ReflectCloneError::FailedDowncast { + expected: Cow::Borrowed(::type_path()), + received: Cow::Owned(self.reflect_type_path().to_string()), + }) + } + /// Returns a hash of the value (which includes the type). /// /// If the underlying type does not support hashing, returns `None`.