bevy_reflect: Contextual serialization error messages

[MrGVSV]

Objective

Reflection serialization can be difficult to debug. A lot of times a type fails to be serialized and the user is left wondering where that type came from.

This is most often encountered with Bevy's scenes. Attempting to serialize all resources in the world will fail because some resources can't be serialized.

For example, users will often get complaints about bevy_utils::Instant not registering ReflectSerialize. Well, Instant can't be serialized, so the only other option is to exclude the resource that contains it. But what resource contains it? This is where reflection serialization can get a little tricky (it's Time<Real> btw).

Solution

Add the debug_stack feature to bevy_reflect. When enabled, the reflection serializers and deserializers will keep track of the current type stack. And this stack will be used in error messages to help with debugging.

Now, if we unknowingly try to serialize Time<Real>, we'll get the following error:

type `bevy_utils::Instant` did not register the `ReflectSerialize` type data. For certain types, this may need to be registered manually using `register_type_data` (stack: `bevy_time::time::Time<bevy_time::real::Real>` -> `bevy_time::real::Real` -> `bevy_utils::Instant`)

Implementation

This makes use of thread_local! to manage an internal TypeInfoStack which holds a stack of &'static TypeInfo. We push to the stack before a type is (de)serialized and pop from the stack afterwards.

Using a thread-local should be fine since we know two (de)serializers can't be running at the same time (and if they're running on separate threads, then we're still good).

The only potential issue would be if a user went through one of the sub-serializers, like StructSerializer. However, I don't think many users are going through these types (I don't even know if we necessarily want to keep those public either, but we'll save that for a different PR). Additionally, this is just a debug feature that only affects error messages, so it wouldn't have any drastically negative effect. It would just result in the stack not being cleared properly if there were any errors.

Lastly, this is not the most performant implementation since we now fetch the TypeInfo an extra time. But I figured that for a debug tool, it wouldn't matter too much.

Feature

This also adds a debug feature, which enables the debug_stack feature.

I added it because I think we may want to potentially add more debug tools in the future, and this gives us a good framework for adding those. Users who want all debug features, present and future, can just set debug. If they only want this feature, then they can just use debug_stack.

I also made the debug feature default to help capture the widest audience (i.e. the users who want this feature but don't know they do). However, if we think it's better as a non-default feature, I can change it!

And if there's any bikeshedding around the name debug_stack, let me know!

Testing

Run the following command:

cargo test --package bevy_reflect --features debug_stack

---

Changelog

• Added the debug and debug_stack features to bevy_reflect
• Updated the error messages returned by the reflection serializers and deserializers to include more contextual information when the debug_stack or debug feature is enabled

[bushrat011899]

This looks great, I love the idea of trying to provide better error messages to users when possible. I have only a couple of non-blocking comments that I thought I'd mention, otherwise LGTM!

[bushrat011899]

Since TypedReflectSerializer is responsible for creating/clearing then TypeInfoStack, I feel like it should be possible to store the stack within this serializer, and pass references to it around like the TypeRegistry, perhaps wrapping it all in a ReflectContext.

I don't see any problems with the thread_local implementation as-is beyond a general ick around global state; you're justification is perfectly valid. Just making a note here for a possible future PR.

[MrGVSV]

I don't fully remember unfortunately.

I believe the reason was that the serializer would need to hold onto a &mut TypeInfoStack but we don't want to require the user to define this themselves.

We might be able to get around this by creating an InternalReflectSerializer. Then ReflectSerializer can create and own the stack, and then pass a mutable reference to it to the internal serializer.

That would be a breaking change though. I'll try to look into it and see if it will actually work.

[MrGVSV]

Just looked into this and while I believe it's possible to do this with an internal serializer, it wouldn't really work properly across threads.

This is because a Serialize impl only requires a &self reference. This means that two references to the same serializer could be sent to different threads and be serialized at the same time (maybe to send multiple network packets, save to separate files, etc.). Doing so would cause the stack to receive entries from both, throwing it completely off.

So for now, I think using a thread-local is probably the safest option.

[alice-i-cecile]

Gentle reminder to fix merge conflicts here.

[alice-i-cecile]

@MrGVSV I regret to inform you that someone has caused merge conflicts for this again in #15107 ;) 😩

[MrGVSV]

@MrGVSV I regret to inform you that someone has caused merge conflicts for this again in #15107 ;) 😩

Now what kind of dummy would perform a huge refactor on the exact set of modules he just put up a PR for this PR is touching and urge the refactor to get merged first?!

...

[alice-i-cecile]

CI failures look real: you seem to have forgotten to feature flag an import.