reflect: improve container attributes

[soqb]

Objective

• Remove confusion around #[reflect(Debug, PartialEq, Hash, Default)].
• Allow specifying paths like #[reflect(my_crate::foo::MyTrait)].

Solution

• Remove the special casing around #[reflect(Default)].
• Change the syntax to #[reflect(debug, partial_eq, hash)] to make obvious the distinction between normal and "special" idents.
  • #[reflect(Hash(my_hash_fn))] is now #[reflect(hash = "my_hash_fn")].
• Implement parsing for paths in #[reflect] attributes.

---

Changelog

• Replaced #[reflect(Debug, PartialEq, Hash, Default)] syntax with #[reflect(debug, partial_eq, hash)] syntax.

Migration Guide

• Replaced #[reflect(Debug, PartialEq, Hash, Default)] syntax with #[reflect(debug, partial_eq, hash)] syntax.
• Derived FromReflect::from_reflect implementations no longer can create a value from a partial set of fields if it implements default. All fields are required.

[alice-i-cecile]

Information about the special trait-like attributes needs to be prominent and clear in the docs for the derive macro, or the syntax needs to be more distinct.

I agree with the change, but the UX is quite confusing with some traits being capitalized and others seemingly not.

[MrGVSV]

Still need to give a proper review, but I'm wondering if— while we're changing this up anyways— we could separate out the type data registrations from everything else.

As we start to add more container attributes (e.g. #[reflect(hash = "my_hash_fn", partial_eq)], etc), it might be better to give type data registration a defined syntax. Being able to do #[reflect(debug, Serialize, hash = "my_hash_fn", Component, partial_eq)] feels wrong to me. Not only that, but it makes it harder for lints/build scripts to identify the meaning of each attribute. And lastly it means that adding a new attribute is a breaking change. For example, we might add a #[reflect(foo)] attribute, but that might conflict with the person who decided they wanted to create a custom type data struct with the all-lowercased name foo (of course, in reality that's probably not a very likely scenario).

It might be nice to break it up like:

• #[reflect(hash, partial_eq, debug)]
• #[reflect(register(Serialize, Component))] where register is the attribute name that auto-registers the given type data

Thoughts?

[cart]

Hmm this might make sense from an implementation perspective, but from a user perspective I would find this pretty confusing. We train people to think "when I want reflected TraitX behaviors, write reflect(TraitX)". The fact that some built in traits have special internal implementations / fast paths seems inconsequential relative to retaining that mental model. We're forcing users to be aware that PartialEq and Hash are "special" from a reflection perspective. That makes this all more confusing from an "average user" perspective.

What is the specific real world confusion we're solving here? Ex: is it that people are reaching for ReflectPartialEq in the registry and it is missing? If so, I would point out that (1) this is not a common case and (2) if we really want to solve that problem we probably can / should insert ReflectPartialEq into the registry.

[MrGVSV]

The fact that some built in traits have special internal implementations / fast paths seems inconsequential relative to retaining that mental model.

Hm, true. I think the difficulty comes when we want to provide customization for these attributes. All of these allow users to specify custom implementations like #[reflect(Hash(my_custom_hash_fn)]. This cannot be done for any other "true" type data.

We could optimize for users not needing to specify custom trait functions for these types and keep things as is.

Default is still a bit weird in that it's both changing how FromReflect works and registering ReflectDefault. And if we eventually want to be consistent with the field attributes by allowing a custom default function, then that complicates things further. Of course, this PR also aims to remove that special casing entirely, but we may eventually want to have the ability to construct a default instance stored on TypeInfo as well rather than just in the registry.

Clone will face a similar problem if #13432 lands.

Additionally, these aren't strictly "internal implementations" in the sense that the user doesn't ever need to be fully aware of them. This is required knowledge if one wishes to use the affected Reflect methods (i.e. Reflect::reflect_hash, Reflect::reflect_partial_eq). In order for those to work and/or do anything, the user needs to know that the attribute is needed.

At this point in time, I'm not too sure which way is the best to move forward. As much as I'd like to change these attributes for consistency with the field attributes, the user-facing argument is a good one.

I'd be curious to hear thoughts from others on the matter, especially those unfamiliar with reflection. Is it unintuitive to have a few traits that control Reflect methods be special-cased? How could we make this clearer (either the current system or the one presented in this PR)? Are there alternatives we haven't considered?

[cart]

This cannot be done for any other "true" type data.

Not yet. But on that note, maybe we should build a mechanism for passing in parameters to constructors of "true" type data? Then this could still "feel" unified.

This is required knowledge if one wishes to use the affected Reflect methods (i.e. Reflect::reflect_hash, Reflect::reflect_partial_eq). In order for those to work and/or do anything, the user needs to know that the attribute is needed.

Agreed, but I would argue these are still "reflect configuration/behaviors associated with a specific trait (Hash, PartialEq, etc)".

At this point in time, I'm not too sure which way is the best to move forward. As much as I'd like to change these attributes for consistency with the field attributes, the user-facing argument is a good one.

I feel pretty strongly that we should use "reflect trait syntax" for these cases (while trying to unify behaviors, syntax, and code paths to the maximum extent that makes sense). My gut reaction to seeing the new syntax is "this is weird and confusing" (both as a user of Bevy and as an informed developer of it). I think ideologically the current trait syntax makes sense. We are still reflecting a specific trait/behavior, it just manifests slightly differently. In the context of trait "type data" (which adds additional behaviors and data on top of exposing the trait itself), I think something like a "Hash behavior override" fits in perfectly.

Changing the syntax has multiple negative impacts:

1. It forces users to know which traits are "special". This is the biggest one.
2. It immediately creates questions about why they are different, even for the users that dont need to care.
3. It looks funky, especially next to "normal" reflected traits. These are high-traffic traits, so we're making a bunch of impls look funkier than they need to.