Skip to content
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: enforce that type data is only inserted for the correct type #4567

Conversation

jakobhellermann
Copy link
Contributor

Objective

The TypeRegistry in bevy_reflect has a HashMap<TypeId, TypeRegistration> where each type registration can contain arbitrary "TypeData", usually created for the type T from <TypeData as FromType<T>>::from_type()).
This is what #[reflect(Deserialize, MyTrait, Component)] expands to.

The problem is that nobody enforces that the data inserted into the TypeRegistration of type T is actually created using FromType for the same T.

You could currently do

let registration = TypeRegistration::of::<f32>();
registration.insert(<ReflectDeserialize as FromType<u128>>::from_type())`;

or

let type_registration = ...;
*type_registration.data_mut::<ReflectDeserialize>() = <ReflectDeserialize as FromType<u128>>::from_type();

This is unexpected and may lead to panics, but makes it impossible to do stuff like #4475 that depends on the correct type of the type data for soundness.

Solution

Change the insert method to be used like

let registration = TypeRegistration::of::<f32>();
registration.insert::<f32, ReflectDeserialize>();

which panics when used with the wrong type.

Also remove the data_mut method, I don't know of any use case and if you want to overwrite it you can just insert it again to overwrite it.

This does remove the ability to insert TypeData not created using FromType but as any arbitrary instance, but that's actually a feature IMO since the type data should not depend on any information other than the type T. And if there are use cases I'm not thinking of right now we can always add them later.

@bjorn3
Copy link
Contributor

bjorn3 commented Apr 23, 2022

Is isn't enough if we get a way to build a TypeRegistration for a type created at runtime without rust counterpart. For example for scripting.

@jakobhellermann jakobhellermann added A-Reflection Runtime information about types and removed S-Needs-Triage This issue needs to be labelled labels Apr 23, 2022
@jakobhellermann
Copy link
Contributor Author

It isn't enough if we get a way to build a TypeRegistration for a type created at runtime without rust counterpart. For example for scripting.

That's true, however the whole TypeRegistry and Reflect trait is currently not usable for "types" without type ids.

When/if that is eventually implemented, we can add a way to add type registration data that does not use FromType, but that is something for future PRs imo.

self.data.insert(TypeId::of::<T>(), Box::new(data));
/// If another instance of `D` was previously inserted, it is replaced.
pub fn insert<T: 'static, D: TypeData + FromType<T>>(&mut self) {
if self.type_id != std::any::TypeId::of::<T>() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

Suggested change
if self.type_id != std::any::TypeId::of::<T>() {
if self.type_id != TypeId::of::<T>() {

Comment on lines +194 to +196
/// Inserts an instance of `D` into this registration's type data.
/// The value is created using the [`FromType<T>`] impl for `D`
/// and `T` must match the type that this [`TypeRegistration`] was created for.
Copy link
Member

@MrGVSV MrGVSV Apr 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe a Panics section to make it absolutely clear to users that this method can and will panic if the wrong type is used?

if self.type_id != std::any::TypeId::of::<T>() {
panic!(
"called `TypeRegistration::insert` on a registration for `{}` with data for type `{}`",
self.short_name,
Copy link
Member

@MrGVSV MrGVSV Apr 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe we use the full name here for clarity? There may be odd instances where a user is trying to insert for type my_crate::Foo but is accidentally using lib_crate::Foo (which would result in called 'TypeRegistration::insert' on a registration for 'Foo' with data for type 'lib_crate::Foo'.

Admittedly, this isn't that bad already, but it could be a bit clearer imo.

(Would need to update the panicking test as well.)

@oddfacade
Copy link
Contributor

Also remove the data_mut method, [...] if you want to overwrite it you can just insert it again to overwrite it.

This does remove the ability to insert TypeData not created using FromType

Unless I'm misunderstanding something, these two statements seem contradictory. If you can only insert TypeData created using FromType then you have entirely removed the ability to mutate it, since you can only ever "overwrite" a type data object with another instance with the exact same value.

@jakobhellermann
Copy link
Contributor Author

jakobhellermann commented Apr 24, 2022

Unless I'm misunderstanding something, these two statements seem contradictory. If you can only insert TypeData created using FromType then you have entirely removed the ability to mutate it, since you can only ever "overwrite" a type data object with another instance with the exact same value.

Oh, true. So while you can replace its value, there is only one valid value for you to replace it with...

Anyways, I just thought of another way to resolve the issue with #4475. We can store the TypeId in ReflectFromPtr and add the requirement to check the type id to the Safety docs in ReflectFromPtr::as_reflect_ptr.

let type_registration = ...;
let reflect_from_ptr = type_registration.data::<ReflectFromPtr>().unwrap();
assert_eq!(type_registration.type_id(), reflect_from_ptr.type_id());
...

// SAFE: the type id has been checked, `val` is valid
unsafe { reflect_from_ptr.as_reflect_ptr(&val as *const ()) };

That way bevy is sound and user has to make sure that nobody did weird things to the registration.

EDIT: that doesn't help us in the case of ComponentIds without a type id...

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Apr 25, 2022
@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Apr 25, 2022
@MrGVSV
Copy link
Member

MrGVSV commented May 12, 2022

Ping @jakobhellermann this branch likely has merge conflicts from #4712. To resolve, simply move your line of code in bevy_reflect_derive to the new registration.rs file.

@jakobhellermann
Copy link
Contributor Author

I thought about this some more and found a use case where the type data shouldn't just be FromType. Say you have a ReflectMethods struct that lists functions callable from a scripting language, and you want to add those for a type that didn't have reflection in mind. It would be nice to be able to get a reference to that ReflectMethods struct and insert any custom methods from another crate.

I think this the design of this PR is too restrictive and we can solve the issue in #4475 without it.

@jakobhellermann jakobhellermann deleted the bevy-reflect-guarantee-correct-type branch May 13, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants