-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bevy_reflect: Consistent reflect_hash
and reflect_partial_eq
#8695
base: main
Are you sure you want to change the base?
Conversation
06e1fb8
to
a84dfc9
Compare
a84dfc9
to
6c1c0ff
Compare
Warning: failing tests
Account for new Hash/PartialEq behavior
6c1c0ff
to
643c990
Compare
One thing I'm getting hung up on here is that we'll no longer use I'm curious why we can't fall back to the concrete type implementation for proxies. There's one note in #6601 about using the concrete implementations:
Looking at the PR (ex: Why did the PR move away from falling back on the concrete implementations? How difficult would it be to use them if implemented? |
This is a great callout! I should have mentioned this in the PR description, but I'll mention it here instead. The reason we can't rely on concrete implementations, even though I at one time thought we could, is that proxies are not those concrete types. And this becomes even clearer when we look at an example. This will fail to compile: #[derive(Reflect)]
struct Foo {
id: usize,
data: String
}
impl Hash for Foo {
fn hash<H: Hasher>(&self, state: &mut H) {
self.id.hash(state);
}
}
let foo = Foo { id: 123, data: String::new() };
let proxy: DynamicStruct = foo.clone_dynamic();
let mut hasher = AHasher::default();
Foo::hash(&proxy, &mut hasher); // ERROR We can't pass
So that's why I abandoned that original approach in the issue. Hopefully that helps. Please let me know if you need more clarification or if you have any other questions! |
So I've dug pretty deeply into this. There is nightly support for creating a fat pointer for something like From that RFC, there is a unsafe (probably cursed) method to do it in stable (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2015&gist=bbeecccc025f5a7a0ad06086678e13f3). Using this method it's possible for us to store the vtable for Admittedly this is unsafe and cursed and there's real support coming in nightly (whenever it gets into stable). I just want to put this here so it's discussed. I'll understand if this is a scary direction to go in. use std::fmt::Debug;
use std::mem;
#[derive(Debug)]
#[repr(C)]
struct FatPtr {
data: *mut (),
meta: *mut (),
}
fn build_fat_ptr<T: ?Sized>(ptr: *mut (), meta: *mut ()) -> &'static T {
let fat_ptr = FatPtr { data: ptr, meta };
unsafe { mem::transmute_copy::<FatPtr, &T>(&fat_ptr) }
}
#[derive(Debug)]
struct DerivedDebug;
struct ImplDebug;
impl Debug for ImplDebug {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "ImplDebug (manual impl)")
}
}
fn main() {
assert_eq!(mem::size_of::<&dyn Debug>(), mem::size_of::<FatPtr>());
// example using derived debug
let m = DerivedDebug;
let dyn_debug: &dyn Debug = &m;
let repr = unsafe { mem::transmute_copy::<&dyn Debug, FatPtr>(&dyn_debug) };
// rebuild the &dyn Debug from `mut *` and the vtable
let dyn_debug: &dyn Debug = build_fat_ptr(repr.data, repr.meta);
println!("m {:?}, dyn_debug {:?}", m, dyn_debug);
// Do the same thing with a manually implemented debug
let m = ImplDebug;
let dyn_debug: &dyn Debug = &m;
let repr = unsafe { mem::transmute_copy::<&dyn Debug, FatPtr>(&dyn_debug) };
// rebuild the &dyn Debug from `mut *` and the vtable
let dyn_debug: &dyn Debug = build_fat_ptr(repr.data, repr.meta);
println!("m {:?}, dyn_debug {:?}", m, dyn_debug);
} |
Yeah the unsafe stuff is a little out of my comfort zone haha. However, this still would be an issue because I'm pretty sure the vtable means nothing to a type that isn't the type that vtable expects, right? And that's the bigger issue with trying to store a function pointer to the conrete impl: dynamic types are not guaranteed to have the same layout as their concrete counterparts. |
I’m also a little uncomfortable with unsafe in rust, which is kind of silly with the number of years I’ve been writing C & assembly.
Understood. My initial thought was for those dynamic types that are proxies, we reflect to the concrete type There are performance & caching things to sort out, but it’s a pattern for ensuring reflection leverages user impls. That said, I still don’t know how comfortable the bevy project is with adding unsafes in cases like this. (edit) And probably more importantly, is this level of complexity really even worth it. Perhaps this can wait until someone encounters a real case where they need the type impl to be executed before doing the complex, unsafe thing. I can take some time to flesh out an example to make it easier to see how scary things get. Something that shows how we’d go from |
Oh are you suggesting we construct the concrete types using
Yeah, that makes sense. Though, I'm wondering how much users really need to rely on these custom impls. The
We should probably avoid as much unsafe as we reasonably can. That being said, I don't think Bevy is uncomfortable adding unsafe code (the entire ECS relies on it). Reflection doesn't have much, but we could consider it if it solves a real problem (#6042, for example, uses unsafe code).
Sounds good! I think it will also be more helpful (for me anyways) to see it using Also, keep in mind that if we did decide to go the unsafe route for |
For now my approach of storing the vtable for Until https://doc.rust-lang.org/std/ptr/trait.Pointee.html#associatedtype.Metadata becomes stable, we'll have to stick with dynamic-only implementations for As MrGVSV already explained, it's unlikely the dynamic-only implementations for these traits will cause any problem for users. |
Update, it is possible for us to save off By doing this we are able to use the implementations on the concrete types. use std::collections::hash_map::DefaultHasher;
use std::hash::{Hash, Hasher};
#[derive(Eq, PartialEq)]
struct Location {
x: u32,
y: u32,
}
impl Hash for Location {
fn hash<H>(&self, _state: &mut H)
where H: Hasher
{
println!("ran hash function")
}
}
// fake "registration" function that really just returns the function pointer for <T as Hash>::hash
pub fn register<T: std::hash::Hash + 'static, H: std::hash::Hasher + 'static>() -> impl Fn(&T, &mut H) {
T::hash::<H>
}
pub fn main() {
let loc = Location { x: 1_u32, y: 1_32 };
let hash_func = register::<Location, DefaultHasher>();
let mut hasher = DefaultHasher::new();
hash_func(&loc, &mut hasher);
println!("Hash result {:x}", hasher.finish())
} Playground: https://godbolt.org/z/7bYPn9b9a |
Objective
Fixes #6601
The issue already describes the problem in detail but here's a quick description of what we want.
A reflected (
dyn Reflect
) type should share the samereflect_hash
andreflect_partial_eq
behavior as a proxy of that type.Additionally,
reflect_partial_eq
should be symmetric. In other words,a.reflect_partial_eq(b)
produces the same result asb.reflect_partial_eq(a)
.This means that the following should test should succeed:
Solution
Both
reflect_hash
andreflect_partial_eq
now make use of stored type information in order to perform their computations. Furthermore, they are now consistent and symmetric.This replaces the old system of registering
Hash
andPartialEq
like#[reflect(Hash, PartialEq)]
. Although, this still exists for value types via#[reflect_value(Hash, PartialEq)]
, since value types base their implementation on the actual traits.Any field that cannot be hashed or compared, may opt-out of the computation using
#[reflect(skip_hash)]
and#[reflect(skip_partial_eq)]
, respectively.If any field returns
None
during one of these operations, the entire container will returnNone
.reflect_partial_eq
BehaviorThe behavior of
reflect_partial_eq
is a bit more intuitive now that it is consistent between concrete and proxy types. We essentially only consider types equal if they all share the same non-skipped fields and those fields are equivalent between them.Example Behavior
Here's an example comparing a type,
Foo
, to a dynamic type.Explanation for the code above:
false
:Foo
requires fieldsa
andc
, butdynamic
is missing both of them.false
:Foo
requires fieldsa
andc
, butdynamic
is missingc
.false
:Foo
anddynamic
share the same required fields, but have different values forc
.true
:Foo
anddynamic
share the same required fields and are all equivalent.true
: Despite theb
fields not being equivalent,Foo
skips it so it is not considered for comparison.false
:Foo
does not contain ad
field butdynamic
does.true
:dynamic
has become a proxy ofBar
which skipsd
, making only fieldsa
andc
relevant for comparison.false
:dynamic
has become a proxy ofBaz
which requiresb
even thoughFoo
skipsb
***Meta
TypesThis PR also adds the concept of "metadata" on info types. This is meant to capture various attribute ("meta") data on containers and their fields. For example, the
docs
attribute has been moved to these types rather than on the info type directly.These meta types are also where we keep track of
skip_hash
andskip_partial_eq
so they can be utilized within dynamic contexts.Rationale
There were a few ways to accomplish this: add getters and setters to the meta fields, create separate builders for each meta type, or just make all members
pub
. I went with that last one as it was the simplest and the others felt like possible overkill with the data we have now. Also, there's no concern over accidental mutations since types always share their type information as a static immutable reference.I also decided to make the constructor for this type
const
. This might be controversial, but my thought was that this helps force us to:TypeId
andtype_name
are not const-stable. Stabilization is probably a ways out for them, but it's probably best we don't establish any patterns that would prevent us from easily making the switch to being const.pub
— it indicates that these fields are standalone and no additional care is needed in using them.With all that being said, I'm definitely open to changing how meta types are constructed and what they're allowed to contain. This was all a very opinionated implementation on my part and I understand it might enforce restrictions prematurely when we could just wait until it's absolutely needed. So feel free to leave a comment telling me I'm wrong so that we can find a pattern the community can agree on.
Notes
Concrete vs Reflected
This change now means that the corresponding concrete impls may give different results than the reflected ones:
Reflect::reflect_hash
= / ≠Hash::hash
Reflect::reflect_partial_eq
= / ≠PartialEq::eq
This could be an issue when comparing results between the concrete realm and the reflected one. However, this is likely not a major issue for the following reasons:
Hash
vsreflect_hash
The general usage for hashing is to interact with maps and sets. Most users are not manually creating a hasher and calculating the
u64
of a value. Because of this, they are unlikely to run into the scenario where they would be comparing the results ofHash
andreflect_hash
.PartialEq
vsreflect_partial_eq
This could be a potential issue for users who rely on
PartialEq
to establish and maintain contracts guaranteeing that two values are equal. If they then try to usereflect_partial_eq
, the result may go against that supposed guarantee.In most cases, though, the results should be the same. This is because in most cases
PartialEq
is derived, which means it essentially performs the same operations thatreflect_partial_eq
does: ensure that all fields are equal recursively.However, if the user has a custom
PartialEq
implementation or marks their fields as#[reflect(skip_partial_eq)]
, then the divergence is more likely.User Responsibility
In either case, the best course of action would be for the user to decide which solution they want to use and be consistent in using it— or at least not rely on the results being identical across concrete and reflected implementations.
Transitivity
One aspect of
PartialEq
thatreflect_partial_eq
does not fully guarantee is transitivity. That is, ifA == B
andB == C
, thenA == C
.While concrete types and their proxies should respect transitivity, dynamic types (ones without a represented
TypeInfo
) do not.Example
As you can see, comparison between concrete values and proxy values are transitive. It's when comparing against a dynamic type that transitivity starts to no longer be guaranteed.
Hashing and Equality
The docs for
Hash
state the following:While the idea is that these
Reflect
methods uphold this property, there is a chance for this to be broken when skipping fields (see example below). This could be problematic for cases where users expect two "equal" values to result in the same hash.Currently, we just warn against it in the documentation. But there may be other solutions.
Additionally, it's not certain whether or not this truly applies to us since
reflect_partial_eq
is meant to be used likePartialEq
, andPartialEq
cannot make these same guarantees asEq
. If anything, we would probably need something likereflect_eq
to matchEq
behavior.Example
In the above case
foo1 == foo2
buthash(foo1) != hash(foo2)
. Again, this property might not be applicable due to the differences betweenPartialEq
andEq
, but it should nevertheless be noted.Open Questions
None
duringreflect_partial_eq
if a field has differing skip configuration (i.e. typeA
skips fieldx
while typeB
requires it)? Should we makereflect_partial_eq
return aResult
to allow users to branch on this versus other causes for failures?#[reflect(skip_hash)]
and#[reflect(skip_partial_eq)]
combined? Such as a#[reflect(skip_equivalence)]
attribute.Future Work
There are some things we could possibly consider adding in a future PR:
reflect_eq
method to mimicEq
Changelog
Changed
Reflect::reflect_hash
andReflect::reflect_partial_eq
are now consistent between concrete types and proxies of those types (proxies are simply dynamic types likeDynamicStruct
that contain theTypeInfo
for a concrete type )Reflect::reflect_hash
andReflect::reflect_partial_eq
no longer rely onHash
andPartialEq
implementations#[reflect(Hash)]
and#[reflect(PartialEq)]
for most types, and doing so will result in a compile errorReflect
now opt into this behavior by defaultStructInfo
) to its meta type (e.g.StructMeta
)Added
#[reflect(skip_hash)]
and#[reflect(skip_partial_eq)]
attributes for controlling the implementations ofReflect::reflect_hash
andReflect::reflect_partial_eq
, respectivelyStructInfo
):ValueMeta
TupleMeta
ArrayMeta
ListMeta
MapMeta
TupleStructMeta
StructMeta
EnumMeta
VariantMeta
FieldMeta
Migration Guide
Hash
andPartialEq
registrationsRegistering
Hash
and/orPartialEq
now results in a compile error. Such registrations will need to be removed.This does not apply to value types created with
#[reflect_value]
:New
Reflect::reflect_hash
behaviorReflect::reflect_hash
no longer relies on a concreteHash
implementation. All types that deriveReflect
come with a built-in implementation ofReflect::reflect_hash
that is purely based on reflection.If a particular field is not hashable, you will need to mark it with
#[reflect(skip_hash)]
:New
Reflect::reflect_partial_eq
behaviorReflect::reflect_partial_eq
no longer relies on a concretePartialEq
implementation. All types that deriveReflect
come with a built-in implementation ofReflect::reflect_partial_eq
that is purely based on reflection.If a particular field is not comparable or should not be considered for comparison, you will need to mark it with
#[reflect(skip_partial_eq)]
:This also means that some types that might have been considered equal or unequal before may no longer provide the same result. This is especially true for comparison against dynamic types since they now aim to be consistent with their concrete counterparts.
docs
accessThose using the
documentation
feature to access doc strings will now need to get it from a type's meta type rather than the info type directly: