From 1cc12e44e1d3c0e8a49b501d4c5515962570a065 Mon Sep 17 00:00:00 2001 From: MrGVSV Date: Wed, 18 May 2022 12:26:11 +0000 Subject: [PATCH] bevy_reflect: Small refactor and default `Reflect` methods (#4739) # Objective Quick followup to #4712. While updating some [other PRs](https://github.com/bevyengine/bevy/pull/4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function. This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file. It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method. ## Solution Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively. As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`. --- ## Changelog * Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive` * Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations --- .../src/container_attributes.rs | 55 ++++++++----- .../bevy_reflect_derive/src/impls.rs | 82 ++++++------------- crates/bevy_reflect/src/impls/smallvec.rs | 12 +-- crates/bevy_reflect/src/impls/std.rs | 8 -- crates/bevy_reflect/src/map.rs | 10 +-- crates/bevy_reflect/src/reflect.rs | 12 ++- crates/bevy_reflect/src/struct_trait.rs | 10 +-- crates/bevy_reflect/src/tuple.rs | 20 +---- crates/bevy_reflect/src/tuple_struct.rs | 10 +-- 9 files changed, 75 insertions(+), 144 deletions(-) diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs index 8e16e77626c5af..39cc946b570e9e 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs @@ -171,55 +171,70 @@ impl ReflectTraits { &self.idents } - /// Returns the logic for `Reflect::reflect_hash` as a `TokenStream`. + /// Returns the implementation of `Reflect::reflect_hash` as a `TokenStream`. /// /// If `Hash` was not registered, returns `None`. - pub fn get_hash_impl(&self, path: &Path) -> Option { + pub fn get_hash_impl(&self, bevy_reflect_path: &Path) -> Option { match &self.hash { TraitImpl::Implemented => Some(quote! { - use std::hash::{Hash, Hasher}; - let mut hasher = #path::ReflectHasher::default(); - Hash::hash(&std::any::Any::type_id(self), &mut hasher); - Hash::hash(self, &mut hasher); - Some(hasher.finish()) + fn reflect_hash(&self) -> Option { + use std::hash::{Hash, Hasher}; + let mut hasher = #bevy_reflect_path::ReflectHasher::default(); + Hash::hash(&std::any::Any::type_id(self), &mut hasher); + Hash::hash(self, &mut hasher); + Some(hasher.finish()) + } }), TraitImpl::Custom(impl_fn) => Some(quote! { - Some(#impl_fn(self)) + fn reflect_hash(&self) -> Option { + Some(#impl_fn(self)) + } }), TraitImpl::NotImplemented => None, } } - /// Returns the logic for `Reflect::reflect_partial_eq` as a `TokenStream`. + /// Returns the implementation of `Reflect::reflect_partial_eq` as a `TokenStream`. /// /// If `PartialEq` was not registered, returns `None`. - pub fn get_partial_eq_impl(&self) -> Option { + pub fn get_partial_eq_impl( + &self, + bevy_reflect_path: &Path, + ) -> Option { match &self.partial_eq { TraitImpl::Implemented => Some(quote! { - let value = value.any(); - if let Some(value) = value.downcast_ref::() { - Some(std::cmp::PartialEq::eq(self, value)) - } else { - Some(false) + fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option { + let value = value.any(); + if let Some(value) = value.downcast_ref::() { + Some(std::cmp::PartialEq::eq(self, value)) + } else { + Some(false) + } } }), TraitImpl::Custom(impl_fn) => Some(quote! { - Some(#impl_fn(self, value)) + fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option { + Some(#impl_fn(self, value)) + } }), TraitImpl::NotImplemented => None, } } - /// Returns the logic for `Reflect::serializable` as a `TokenStream`. + /// Returns the implementation of `Reflect::serializable` as a `TokenStream`. /// /// If `Serialize` was not registered, returns `None`. - pub fn get_serialize_impl(&self, path: &Path) -> Option { + pub fn get_serialize_impl(&self, bevy_reflect_path: &Path) -> Option { match &self.serialize { TraitImpl::Implemented => Some(quote! { - Some(#path::serde::Serializable::Borrowed(self)) + fn serializable(&self) -> Option<#bevy_reflect_path::serde::Serializable> { + Some(#bevy_reflect_path::serde::Serializable::Borrowed(self)) + } }), TraitImpl::Custom(impl_fn) => Some(quote! { - Some(#impl_fn(self)) + fn serializable(&self) -> Option<#bevy_reflect_path::serde::Serializable> { + Some(#impl_fn(self)) + } }), TraitImpl::NotImplemented => None, } diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs b/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs index 18b4c0ab96e4b3..ecea8a62655eb7 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs @@ -35,20 +35,16 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream { let field_count = field_idents.len(); let field_indices = (0..field_count).collect::>(); - let hash_fn = derive_data - .traits() - .get_hash_impl(bevy_reflect_path) - .unwrap_or_else(|| quote!(None)); - let serialize_fn = derive_data - .traits() - .get_serialize_impl(bevy_reflect_path) - .unwrap_or_else(|| quote!(None)); + let hash_fn = derive_data.traits().get_hash_impl(bevy_reflect_path); + let serialize_fn = derive_data.traits().get_serialize_impl(bevy_reflect_path); let partial_eq_fn = derive_data .traits() - .get_partial_eq_impl() + .get_partial_eq_impl(bevy_reflect_path) .unwrap_or_else(|| { quote! { - #bevy_reflect_path::struct_partial_eq(self, value) + fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option { + #bevy_reflect_path::struct_partial_eq(self, value) + } } }); @@ -166,17 +162,11 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream { #bevy_reflect_path::ReflectMut::Struct(self) } - fn serializable(&self) -> Option<#bevy_reflect_path::serde::Serializable> { - #serialize_fn - } + #hash_fn - fn reflect_hash(&self) -> Option { - #hash_fn - } + #partial_eq_fn - fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option { - #partial_eq_fn - } + #serialize_fn } }) } @@ -194,20 +184,16 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream let field_count = field_idents.len(); let field_indices = (0..field_count).collect::>(); - let hash_fn = derive_data - .traits() - .get_hash_impl(bevy_reflect_path) - .unwrap_or_else(|| quote!(None)); - let serialize_fn = derive_data - .traits() - .get_serialize_impl(bevy_reflect_path) - .unwrap_or_else(|| quote!(None)); + let hash_fn = derive_data.traits().get_hash_impl(bevy_reflect_path); + let serialize_fn = derive_data.traits().get_serialize_impl(bevy_reflect_path); let partial_eq_fn = derive_data .traits() - .get_partial_eq_impl() + .get_partial_eq_impl(bevy_reflect_path) .unwrap_or_else(|| { quote! { - #bevy_reflect_path::tuple_struct_partial_eq(self, value) + fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option { + #bevy_reflect_path::tuple_struct_partial_eq(self, value) + } } }); @@ -301,17 +287,11 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream #bevy_reflect_path::ReflectMut::TupleStruct(self) } - fn serializable(&self) -> Option<#bevy_reflect_path::serde::Serializable> { - #serialize_fn - } + #hash_fn - fn reflect_hash(&self) -> Option { - #hash_fn - } + #partial_eq_fn - fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option { - #partial_eq_fn - } + #serialize_fn } }) } @@ -322,17 +302,11 @@ pub(crate) fn impl_value( generics: &Generics, get_type_registration_impl: proc_macro2::TokenStream, bevy_reflect_path: &Path, - reflect_attrs: &ReflectTraits, + reflect_traits: &ReflectTraits, ) -> TokenStream { - let hash_fn = reflect_attrs - .get_hash_impl(bevy_reflect_path) - .unwrap_or_else(|| quote!(None)); - let partial_eq_fn = reflect_attrs - .get_partial_eq_impl() - .unwrap_or_else(|| quote!(None)); - let serialize_fn = reflect_attrs - .get_serialize_impl(bevy_reflect_path) - .unwrap_or_else(|| quote!(None)); + let hash_fn = reflect_traits.get_hash_impl(bevy_reflect_path); + let serialize_fn = reflect_traits.get_serialize_impl(bevy_reflect_path); + let partial_eq_fn = reflect_traits.get_partial_eq_impl(bevy_reflect_path); let (impl_generics, ty_generics, where_clause) = generics.split_for_impl(); TokenStream::from(quote! { @@ -394,17 +368,11 @@ pub(crate) fn impl_value( #bevy_reflect_path::ReflectMut::Value(self) } - fn reflect_hash(&self) -> Option { - #hash_fn - } + #hash_fn - fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option { - #partial_eq_fn - } + #partial_eq_fn - fn serializable(&self) -> Option<#bevy_reflect_path::serde::Serializable> { - #serialize_fn - } + #serialize_fn } }) } diff --git a/crates/bevy_reflect/src/impls/smallvec.rs b/crates/bevy_reflect/src/impls/smallvec.rs index 6bd32dc0b25c99..adec1ca92a8f0a 100644 --- a/crates/bevy_reflect/src/impls/smallvec.rs +++ b/crates/bevy_reflect/src/impls/smallvec.rs @@ -1,9 +1,7 @@ use smallvec::SmallVec; use std::any::Any; -use crate::{ - serde::Serializable, Array, ArrayIter, FromReflect, List, Reflect, ReflectMut, ReflectRef, -}; +use crate::{Array, ArrayIter, FromReflect, List, Reflect, ReflectMut, ReflectRef}; impl Array for SmallVec where @@ -100,17 +98,9 @@ where Box::new(List::clone_dynamic(self)) } - fn reflect_hash(&self) -> Option { - None - } - fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { crate::list_partial_eq(self, value) } - - fn serializable(&self) -> Option { - None - } } impl FromReflect for SmallVec diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs index 2bd11c035a10ba..7c0b987ef47dd8 100644 --- a/crates/bevy_reflect/src/impls/std.rs +++ b/crates/bevy_reflect/src/impls/std.rs @@ -273,17 +273,9 @@ unsafe impl Reflect for HashMap { Box::new(self.clone_dynamic()) } - fn reflect_hash(&self) -> Option { - None - } - fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { map_partial_eq(self, value) } - - fn serializable(&self) -> Option { - None - } } impl GetTypeRegistration for HashMap diff --git a/crates/bevy_reflect/src/map.rs b/crates/bevy_reflect/src/map.rs index f67683c1b661a1..ec6a748839a72f 100644 --- a/crates/bevy_reflect/src/map.rs +++ b/crates/bevy_reflect/src/map.rs @@ -2,7 +2,7 @@ use std::any::Any; use bevy_utils::{Entry, HashMap}; -use crate::{serde::Serializable, Reflect, ReflectMut, ReflectRef}; +use crate::{Reflect, ReflectMut, ReflectRef}; /// An ordered mapping between [`Reflect`] values. /// @@ -186,17 +186,9 @@ unsafe impl Reflect for DynamicMap { Box::new(self.clone_dynamic()) } - fn reflect_hash(&self) -> Option { - None - } - fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { map_partial_eq(self, value) } - - fn serializable(&self) -> Option { - None - } } /// An iterator over the key-value pairs of a [`Map`]. diff --git a/crates/bevy_reflect/src/reflect.rs b/crates/bevy_reflect/src/reflect.rs index aeb72669faab90..4dd2797e630a9f 100644 --- a/crates/bevy_reflect/src/reflect.rs +++ b/crates/bevy_reflect/src/reflect.rs @@ -130,17 +130,23 @@ pub unsafe trait Reflect: Any + Send + Sync { /// Returns a hash of the value (which includes the type). /// /// If the underlying type does not support hashing, returns `None`. - fn reflect_hash(&self) -> Option; + fn reflect_hash(&self) -> Option { + None + } /// Returns a "partial equality" comparison result. /// /// If the underlying type does not support equality testing, returns `None`. - fn reflect_partial_eq(&self, _value: &dyn Reflect) -> Option; + fn reflect_partial_eq(&self, _value: &dyn Reflect) -> Option { + None + } /// Returns a serializable version of the value. /// /// If the underlying type does not support serialization, returns `None`. - fn serializable(&self) -> Option; + fn serializable(&self) -> Option { + None + } } /// A trait for types which can be constructed from a reflected type. diff --git a/crates/bevy_reflect/src/struct_trait.rs b/crates/bevy_reflect/src/struct_trait.rs index f1941ce1bbac26..b68a6ab7d4e1ff 100644 --- a/crates/bevy_reflect/src/struct_trait.rs +++ b/crates/bevy_reflect/src/struct_trait.rs @@ -1,4 +1,4 @@ -use crate::{serde::Serializable, Reflect, ReflectMut, ReflectRef}; +use crate::{Reflect, ReflectMut, ReflectRef}; use bevy_utils::{Entry, HashMap}; use std::{any::Any, borrow::Cow}; @@ -312,17 +312,9 @@ unsafe impl Reflect for DynamicStruct { Ok(()) } - fn reflect_hash(&self) -> Option { - None - } - fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { struct_partial_eq(self, value) } - - fn serializable(&self) -> Option { - None - } } /// Compares a [`Struct`] with a [`Reflect`] value. diff --git a/crates/bevy_reflect/src/tuple.rs b/crates/bevy_reflect/src/tuple.rs index e62fc2bc8c8ac7..9803e88dd064df 100644 --- a/crates/bevy_reflect/src/tuple.rs +++ b/crates/bevy_reflect/src/tuple.rs @@ -1,6 +1,6 @@ use crate::{ - serde::Serializable, FromReflect, FromType, GetTypeRegistration, Reflect, ReflectDeserialize, - ReflectMut, ReflectRef, TypeRegistration, + FromReflect, FromType, GetTypeRegistration, Reflect, ReflectDeserialize, ReflectMut, + ReflectRef, TypeRegistration, }; use serde::Deserialize; use std::any::Any; @@ -259,17 +259,9 @@ unsafe impl Reflect for DynamicTuple { Ok(()) } - fn reflect_hash(&self) -> Option { - None - } - fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { tuple_partial_eq(self, value) } - - fn serializable(&self) -> Option { - None - } } /// Applies the elements of `b` to the corresponding elements of `a`. @@ -408,17 +400,9 @@ macro_rules! impl_reflect_tuple { Box::new(self.clone_dynamic()) } - fn reflect_hash(&self) -> Option { - None - } - fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { crate::tuple_partial_eq(self, value) } - - fn serializable(&self) -> Option { - None - } } impl<$($name: Reflect + for<'de> Deserialize<'de>),*> GetTypeRegistration for ($($name,)*) { diff --git a/crates/bevy_reflect/src/tuple_struct.rs b/crates/bevy_reflect/src/tuple_struct.rs index 09f484ca156445..92a9d7bb641626 100644 --- a/crates/bevy_reflect/src/tuple_struct.rs +++ b/crates/bevy_reflect/src/tuple_struct.rs @@ -1,4 +1,4 @@ -use crate::{serde::Serializable, Reflect, ReflectMut, ReflectRef}; +use crate::{Reflect, ReflectMut, ReflectRef}; use std::any::Any; /// A reflected Rust tuple struct. @@ -251,17 +251,9 @@ unsafe impl Reflect for DynamicTupleStruct { Ok(()) } - fn reflect_hash(&self) -> Option { - None - } - fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { tuple_struct_partial_eq(self, value) } - - fn serializable(&self) -> Option { - None - } } /// Compares a [`TupleStruct`] with a [`Reflect`] value.