Fix boxed labels (#8436)

# Objective

Label traits such as `ScheduleLabel` currently have a major footgun: the
trait is implemented for `Box<dyn ScheduleLabel>`, but the
implementation does not function as one would expect since `Box<T>` 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<dyn ScheduleLabel>` 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<dyn ScheduleLabel>` .

---

## Changelog

Fixed a bug where boxed label types (such as `Box<dyn ScheduleLabel>`)
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<dyn ScheduleLabel> { ... }

    // After:
    fn dyn_clone(&self) -> Box<dyn ScheduleLabel> { ... }

    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::<Self>().hash(&mut state);
    }
}
```
This commit is contained in:
JoJoJet 2023-04-18 22:36:44 -04:00 committed by GitHub
parent 5ed6b628eb
commit fe852fd0ad
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 85 additions and 6 deletions

View file

@ -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>| flag.0 = true);
world.add_schedule(schedule, A);
let boxed: Box<dyn ScheduleLabel> = Box::new(A);
world.insert_resource(Flag(false));
world.run_schedule_ref(&boxed);
assert!(world.resource::<Flag>().0);
world.insert_resource(Flag(false));
world.run_schedule(boxed);
assert!(world.resource::<Flag>().0);
}
}

View file

@ -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<dyn #trait_path> {
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()

View file

@ -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<dyn
#[doc = stringify!($label_trait_name)]
/// >`
///
/// [TypeId]: std::any::TypeId
fn inner_type_id(&self) -> ::std::any::TypeId {
std::any::TypeId::of::<Self>()
}
/// Clones this `
#[doc = stringify!($label_trait_name)]
/// `
fn dyn_clone(&self) -> Box<dyn $label_trait_name>;
/// 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<dyn $label_trait_name> {
fn inner_type_id(&self) -> ::std::any::TypeId {
(**self).inner_type_id()
}
fn dyn_clone(&self) -> Box<dyn $label_trait_name> {
// 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);
}
}
};
}