From fe852fd0adbce6856f5886d66d20d62cfc936287 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 18 Apr 2023 22:36:44 -0400 Subject: [PATCH] Fix boxed labels (#8436) # Objective Label traits such as `ScheduleLabel` currently have a major footgun: the trait is implemented for `Box`, but the implementation does not function as one would expect since `Box` is considered to be a distinct type from `T`. This is because the behavior of the `ScheduleLabel` trait is specified mainly through blanket implementations, which prevents `Box` from being properly special-cased. ## Solution Replace the blanket-implemented behavior with a series of methods defined on `ScheduleLabel`. This allows us to fully special-case `Box` . --- ## Changelog Fixed a bug where boxed label types (such as `Box`) behaved incorrectly when compared with concretely-typed labels. ## Migration Guide The `ScheduleLabel` trait has been refactored to no longer depend on the traits `std::any::Any`, `bevy_utils::DynEq`, and `bevy_utils::DynHash`. Any manual implementations will need to implement new trait methods in their stead. ```rust impl ScheduleLabel for MyType { // Before: fn dyn_clone(&self) -> Box { ... } // After: fn dyn_clone(&self) -> Box { ... } fn as_dyn_eq(&self) -> &dyn DynEq { self } // No, `mut state: &mut` is not a typo. fn dyn_hash(&self, mut state: &mut dyn Hasher) { self.hash(&mut state); // Hashing the TypeId isn't strictly necessary, but it prevents collisions. TypeId::of::().hash(&mut state); } } ``` --- crates/bevy_ecs/src/schedule/set.rs | 37 +++++++++++++++++++++++++ crates/bevy_macro_utils/src/lib.rs | 12 +++++++++ crates/bevy_utils/src/label.rs | 42 ++++++++++++++++++++++++----- 3 files changed, 85 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/set.rs b/crates/bevy_ecs/src/schedule/set.rs index 5e2797a969..1e00a039a5 100644 --- a/crates/bevy_ecs/src/schedule/set.rs +++ b/crates/bevy_ecs/src/schedule/set.rs @@ -172,3 +172,40 @@ where SystemTypeSet::new() } } + +#[cfg(test)] +mod tests { + use crate::{ + schedule::{tests::ResMut, Schedule}, + system::Resource, + }; + + use super::*; + + #[test] + fn test_boxed_label() { + use crate::{self as bevy_ecs, world::World}; + + #[derive(Resource)] + struct Flag(bool); + + #[derive(ScheduleLabel, Debug, Default, Clone, Copy, PartialEq, Eq, Hash)] + struct A; + + let mut world = World::new(); + + let mut schedule = Schedule::new(); + schedule.add_systems(|mut flag: ResMut| flag.0 = true); + world.add_schedule(schedule, A); + + let boxed: Box = Box::new(A); + + world.insert_resource(Flag(false)); + world.run_schedule_ref(&boxed); + assert!(world.resource::().0); + + world.insert_resource(Flag(false)); + world.run_schedule(boxed); + assert!(world.resource::().0); + } +} diff --git a/crates/bevy_macro_utils/src/lib.rs b/crates/bevy_macro_utils/src/lib.rs index ee1c1670d6..212a78ce3d 100644 --- a/crates/bevy_macro_utils/src/lib.rs +++ b/crates/bevy_macro_utils/src/lib.rs @@ -160,6 +160,8 @@ pub fn ensure_no_collision(value: Ident, haystack: TokenStream) -> Ident { /// - `input`: The [`syn::DeriveInput`] for struct that is deriving the label trait /// - `trait_path`: The path [`syn::Path`] to the label trait pub fn derive_boxed_label(input: syn::DeriveInput, trait_path: &syn::Path) -> TokenStream { + let bevy_utils_path = BevyManifest::default().get_path("bevy_utils"); + let ident = input.ident; let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); let mut where_clause = where_clause.cloned().unwrap_or_else(|| syn::WhereClause { @@ -178,6 +180,16 @@ pub fn derive_boxed_label(input: syn::DeriveInput, trait_path: &syn::Path) -> To fn dyn_clone(&self) -> std::boxed::Box { std::boxed::Box::new(std::clone::Clone::clone(self)) } + + fn as_dyn_eq(&self) -> &dyn #bevy_utils_path::label::DynEq { + self + } + + fn dyn_hash(&self, mut state: &mut dyn ::std::hash::Hasher) { + let ty_id = #trait_path::inner_type_id(self); + ::std::hash::Hash::hash(&ty_id, &mut state); + ::std::hash::Hash::hash(self, &mut state); + } } }) .into() diff --git a/crates/bevy_utils/src/label.rs b/crates/bevy_utils/src/label.rs index 631ef7a426..3c0fb762d9 100644 --- a/crates/bevy_utils/src/label.rs +++ b/crates/bevy_utils/src/label.rs @@ -42,7 +42,7 @@ pub trait DynHash: DynEq { /// Feeds this value into the given [`Hasher`]. /// - /// [`Hash`]: std::hash::Hasher + /// [`Hasher`]: std::hash::Hasher fn dyn_hash(&self, state: &mut dyn Hasher); } @@ -72,16 +72,34 @@ where macro_rules! define_boxed_label { ($label_trait_name:ident) => { /// A strongly-typed label. - pub trait $label_trait_name: - 'static + Send + Sync + ::std::fmt::Debug + ::bevy_utils::label::DynHash - { - #[doc(hidden)] + pub trait $label_trait_name: 'static + Send + Sync + ::std::fmt::Debug { + /// Return's the [TypeId] of this label, or the the ID of the + /// wrappped label type for `Box` + /// + /// [TypeId]: std::any::TypeId + fn inner_type_id(&self) -> ::std::any::TypeId { + std::any::TypeId::of::() + } + + /// Clones this ` + #[doc = stringify!($label_trait_name)] + /// ` fn dyn_clone(&self) -> Box; + + /// Casts this value to a form where it can be compared with other type-erased values. + fn as_dyn_eq(&self) -> &dyn ::bevy_utils::label::DynEq; + + /// Feeds this value into the given [`Hasher`]. + /// + /// [`Hasher`]: std::hash::Hasher + fn dyn_hash(&self, state: &mut dyn ::std::hash::Hasher); } impl PartialEq for dyn $label_trait_name { fn eq(&self, other: &Self) -> bool { - self.dyn_eq(other.as_dyn_eq()) + self.as_dyn_eq().dyn_eq(other.as_dyn_eq()) } } @@ -100,11 +118,23 @@ macro_rules! define_boxed_label { } impl $label_trait_name for Box { + fn inner_type_id(&self) -> ::std::any::TypeId { + (**self).inner_type_id() + } + fn dyn_clone(&self) -> Box { // Be explicit that we want to use the inner value // to avoid infinite recursion. (**self).dyn_clone() } + + fn as_dyn_eq(&self) -> &dyn ::bevy_utils::label::DynEq { + (**self).as_dyn_eq() + } + + fn dyn_hash(&self, state: &mut dyn ::std::hash::Hasher) { + (**self).dyn_hash(state); + } } }; }