bevy_reflect: Type parameter bounds

[MrGVSV]

Objective

Fixes #8965.

Background

For convenience and to ensure everything is setup properly, we automatically add certain bounds to the derived types. The current implementation does this by taking the types from all active fields and adding them to the where-clause of the generated impls. I believe this method was chosen because it won't add bounds to types that are otherwise ignored.

#[derive(Reflect)]
struct Foo<T, U: SomeTrait, V> {
  t: T,
  u: U::Assoc,
  #[reflect(ignore)]
  v: [V; 2]
}

// Generates something like:
impl<T, U: SomeTrait, V> for Foo<T, U, V>
where
  // Active:
  T: Reflect,
  U::Assoc: Reflect,

  // Ignored:
  [V; 2]: Send + Sync + Any
{
  // ...
}

The self-referential type fails because it ends up using itself as a type bound due to being one of its own active fields.

#[derive(Reflect)]
struct Foo {
  foo: Vec<Foo>
}

// Foo where Vec<Foo>: Reflect -> Vec<T> where T: Reflect -> Foo where Vec<Foo>: Reflect -> ...

Solution

We can't simply parse all field types for the name of our type. That would be both complex and prone to errors and false-positives. And even if it wasn't, what would we replace the bound with?

Instead, I opted to go for a solution that only adds the bounds to what really needs it: the type parameters. While the bounds on concrete types make errors a bit cleaner, they aren't strictly necessary. This means we can change our generated where-clause to only add bounds to generic type parameters.

Doing this, though, returns us back to the problem of over-bounding parameters that don't need to be bounded. To solve this, I added a new container attribute (based on this comment and @nicopap's comment) that allows us to pass in a custom where clause to modify what bounds are added to these type parameters.

This allows us to do stuff like:

trait Trait {
  type Assoc;
}

// We don't need `T` to be reflectable since we only care about `T::Assoc`.
#[derive(Reflect)]
#[reflect(where T::Assoc: FromReflect)]
struct Foo<T: Trait>(T::Assoc);

#[derive(TypePath)]
struct Bar;

impl Trait for Bar {
  type Assoc = usize;
}

#[derive(Reflect)]
struct Baz {
  a: Foo<Bar>,
}

Note
I also tried allowing #[reflect(ignore)] to be used on the type parameters themselves, but that proved problematic since the derive macro does not consume the attribute. This is why I went with the container attribute approach.

Alternatives

One alternative could possibly be to just not add reflection bounds automatically (i.e. only add required bounds like Send, Sync, Any, and TypePath).

The downside here is we add more friction to using reflection, which already comes with its own set of considerations. This is a potentially viable option, but we really need to consider whether or not the ergonomics hit is worth it.

If we did decide to go the more manual route, we should at least consider something like #5772 to make it easier for users to add the right bounds (although, this could still become tricky with FromReflect also being automatically derived).

Open Questions

1. Should we go with this approach or the manual alternative?
2. Should we add a skip_params attribute to avoid the T: 'static trick? Decided to go with custom_where() as it's the simplest Scratch that, went with a normal where clause
3. custom_where bikeshedding? No longer needed since we are using a normal where clause

TODO

• Add compile-fail tests

---

Changelog

• Fixed issue preventing recursive types from deriving Reflect
• Changed how where-clause bounds are generated by the Reflect derive macro
  • They are now only applied to the type parameters, not to all active fields
• Added #[reflect(where T: Trait, U::Assoc: Trait, ...)] container attribute

Migration Guide

When deriving Reflect, generic type params that do not need the automatic reflection bounds (such as Reflect) applied to them will need to opt-out using a custom where clause like: #[reflect(where T: Trait, U::Assoc: Trait, ...)].

The attribute can define custom bounds only used by the reflection impls. To simply opt-out all the type params, we can pass in an empty where clause: #[reflect(where)].

// BEFORE:
#[derive(Reflect)]
struct Foo<T>(#[reflect(ignore)] T);

// AFTER:
#[derive(Reflect)]
#[reflect(where)]
struct Foo<T>(#[reflect(ignore)] T);

[nicopap]

I like the approach. It's also more predictable, as it's in line with how the std lib traits work. But because — unlike with std lib traits — it's extremely onerous to implement yourself the Reflect trait, we need to give more controls to users of the Reflect derive macro.

Here is my suggestion: We keep the derive stupid simple by default (ie: do as std, ie: as this PR) But the controls should be:

// We don't need `T` to be `Reflect` since we only care about `T::Assoc`
#[derive(Reflect)]
#[reflect(custom_trait_bounds(T::Assoc: Reflect)]
struct Foo<T: Trait>(T::Assoc);

This would disable the default generic trait bounds and replace them with the provided value. Just paste as-is the bounds from the custom_trait_bounds meta.

Would this help? Being able to provide custom bounds is valuable (I think, right?). And in term of implementation, treating the meta attribute as a "discard all default behavior" rather than "customize default behavior" should simplify the code.

[MrGVSV]

But because — unlike with std lib traits — it's extremely onerous to implement yourself the Reflect trait, we need to give more controls to users of the Reflect derive macro.

Yes, hard agree! The last thing we want is to force people to have to manually implement any of the reflection traits themselves since it's pretty easy to implement something the wrong way.

I just realized I wasn't thinking this out clearly enough. My thought was that if we don't go with this PR, we just require users to add the bounds themselves. But I forgot about the fact that users might not want to always have T: Reflect and only use that for when the type itself is meant to be reflected.

For other readers' reference, it's like when you do this:

#[derive(Default)]
struct Foo<T>(Option<T>);

But then you realize that this adds the T: Default bound to the generated Default impl. Which prevents you from doing this (even though it would be perfectly valid otherwise):

struct Bar;

// Should be `Foo(None)`
let foo = Foo::<Bar>::default(); // ERROR! `Bar` does not implement `Default`

The fix there is to manually implement Default. But like @nicopap pointed out, we really do not want people to have to implement all these traits manually.

Here is my suggestion: We keep the derive stupid simple by default (ie: do as std, ie: as this PR) But the controls should be:

// We don't need `T` to be `Reflect` since we only care about `T::Assoc`
#[derive(Reflect)]
#[reflect(custom_trait_bounds(T::Assoc: Reflect)]
struct Foo<T: Trait>(T::Assoc);

This would disable the default generic trait bounds and replace them with the provided value. Just paste as-is the bounds from the custom_trait_bounds meta.

Would this help? Being able to provide custom bounds is valuable (I think, right?). And in term of implementation, treating the meta attribute as a "discard all default behavior" rather than "customize default behavior" should simplify the code.

With the above consideration in mind, I think this would extremely beneficial. However, we might still need both because I don't know if custom_trait_bounds has an obvious way to opt-out for particular type parameters like ignore_params can.

Would we want to use both or is there a better way to handle both opt-out and custom bound cases? I can probably add that to this PR (unless you really wanted to code it yourself haha).

Also, bikeshed: what about #[reflect(where(T::Assoc: Reflect + FromReflect + ...))] or #[reflect(bound_params(T::Assoc: Reflect + FromReflect + ...))] instead of #[reflect(custom_trait_bounds(T::Assoc: Reflect + FromReflect + ...))]?

[nicopap]

The custom_trait_bounds would basically overwrite anything the macro does. (ie, no bound at all, appart the ones given as argument to custom_trait_bounds ). Giving full control to the user.

The advantage over any alternative, is that there is no bound-specific logic, no need to keep track of which bound is disabled etc. It's either "all generic parameters of the struct" or "what the user specified". It's KISS and fully flexible.

A potential issue is that it might generate bad error messages that are difficult to untangle as a user.

[nicopap]

Ok #[reflect(where(Foo: Bar))] is just amazing, I'm 98% for it. Others might object.

Edit: Maybe overwrite_where? set_where? Though I still like where way more

[MrGVSV]

The custom_trait_bounds would basically overwrite anything the macro does. (ie, no bound at all, appart the ones given as argument to custom_trait_bounds ). Giving full control to the user.

The advantage over any alternative, is that there is no bound-specific logic, no need to keep track of which bound is disabled etc. It's either "all generic parameters of the struct" or "what the user specified". It's KISS and fully flexible.

Are you saying that to opt-out of the normal bounds, you'd need to do #[reflect(custom_trait_bounds(T))]? If so, I'm not sure if that's the most intuitive.

I think the dedicated attribute is the clearest (so far).

Ok #[reflect(where(Foo: Bar))] is just amazing, I'm 98% for it. Others might object.

Edit: Maybe overwrite_where? set_where? Though I still like where way more

Yeah I like where as well (or bound_params to match ignore_params). I'll do that for now and we can continue to bikeshed later.

[nicopap]

Well, you would use #[reflect(where())] to remove all Reflect bounds, only problem is you need to know the mechanism. But whether it's #[reflect(where())] or #[reflect(skip_bounds)] you still need to know that either where exists or skip_bounds exists, and what they do. So might as well keep to a single attribute.

[MrGVSV]

Okay so unfortunately you can't use keywords like where in attributes. So instead I went with custom_where. I think I like that better than overwrite_where (mainly because I tend to get confused between overwrite and override lol).

I made it so that it only overrides type parameters explicitly called out in the attribute. This is so that types with tons of generics don't need to redefine the same reflection bounds on all of them even though they really only intended to override one.

But here's my question: should we still have something like skip_params?

Without skip_params, you can only opt-out of the default params by specifying a new bound in the custom_where (e.g. T: 'static).

Here are some examples:

// Use `'static` to opt-out of the default bounds for `T`
#[derive(Reflect)]
#[reflect(custom_where(T: 'static))]
struct Foo<T> {
  a: String,
  #[reflect(ignore)] 
  b: PhantomData<T>,
  #[reflect(ignore)] 
  c: usize,
}

trait Trait {
  type Assoc;
}

// We don't need `T` to be `Reflect` since we only care about `T::Assoc`.
// To prevent `T` from getting the default bounds, we can use the same `'static` trick
#[derive(Reflect)]
#[reflect(custom_where(T: 'static, T::Assoc: FromReflect))]
struct Foo<T: Trait>(T::Assoc);

[nicopap]

I re-iterate that I'd prefer to see a dumb simple solution rather than a more advanced one that tries to conform to user expectations (and IMO fails, or actually sets the expectations itself).

This is so that types with tons of generics don't need to redefine the same reflection bounds on all of them

Do you have an example of such type? My generic structs don't usually have more than 3 type parameters.

I'll refer to "only overwriting parameters referred to by the custom clause" by "named-only".

IMO "erase and replace" is more predictable for users:

1. With named-only, users need to mix in their head the bounds generated by the macro (therefore not visible in code) and the ones in the custom_where
2. With erase-and-replace, the full where clause is visible by the end user, when needed
3. with named-only, you need to do that weird dance of setting existing parameters to a dummy bound, in addition to adding your own bounds
4. erase-and-replace is simpler to implement, more maintainable: you don't have to inspect the content of the where clause to selectively replace bounds.

With erase-and-replace, the user might be surprised that setting the bound of T suddenly unsets the bound on U. But named-only has similar user-facing problems. So absent a clear advantage, I'd advocate for the simpler to implement solution.

IMO if we go with named-only a "disable all bound" is not needed, because you can always do custom_where(T:, U:, V:, …) (fairly certain this is valid syntax).

[MrGVSV]

I re-iterate that I'd prefer to see a dumb simple solution rather than a more advanced one that tries to conform to user expectations (and IMO fails, or actually sets the expectations itself).

I think you raise some good points, and I agree that having custom_where just replace the non-essential bounds is a lot easier to make sense of than what I have now. I'll remove the param detection and just have it replace the bounds. Thanks for the feedback!

[MrGVSV]

Simplified the attribute as per @nicopap's suggestions (Discord thread).

Now the entire where-clause is replaced when the attribute is present (minus the absolutely required bounds). This was decided on due to its simplicity. We can always return to the more granular approach in the future if needed.

So now we can achieve the examples above like so:

// Use `'static` to opt-out of the default bounds for `T`
#[derive(Reflect)]
#[reflect(custom_where())]
struct Foo<T> {
  a: String,
  #[reflect(ignore)] 
  b: PhantomData<T>,
  #[reflect(ignore)] 
  c: usize,
}

trait Trait {
  type Assoc;
}

// We don't need `T` to be `Reflect` since we only care about `T::Assoc`.
#[derive(Reflect)]
#[reflect(custom_where(T::Assoc: FromReflect))]
struct Foo<T: Trait>(T::Assoc);

[nicopap]

Some question regarding certain implementation details.

[MrGVSV]

@nicopap I went ahead and just refactored the entire WhereClauseOptions struct. It had a lot of stuff left over from previous work that is no longer needed. I think this is a bit simpler now.

Let me know what you think and if there is anything I should add/remove/change!

[nicopap]

Lovely!

[soqb]

it's true that we need an attribute to opt-out of bounds and the way custom_where works now certainly provides the most flexibility.

however, i'm not sure i'm in favour of the changes to the default bound resolution logic: our current algorithm i think is less prone to false positives overall, being better tailored to Reflect's complex requirements.

on the other hand, it is true that this method provides an antidote for some of the arcane components of the derive macro and matches the behaviour of all the standard library's macros, so i'm not wholly against the change.

i would recommend slicing the breaking changes from this PR out into a separate one where we can discuss, but if i'm fighting the consensus that won't be necessary.

[nicopap]

Serde has a different syntax for type bounds. What about re-using it?

https://serde.rs/container-attrs.html#bound

[soqb]

i think my favourite possible syntax is to force (in a separate attribute definition from any other configuration) #[reflect(where T: Reflect)] which i'm sure could be achieved by matching on the path of the lazily-parsed meta to find where and then parsing the bounds directly from the rest.

[nicopap]

My (extremely lazy) solution was, for set_params: Option<syn::Generics> do the following:

            () if meta.path.is_ident("set_params") => {
                self.set_params = Some(meta.input.parse()?);
            }

Results in #[parse_dsl(set_params <Foo: ParseDsl>)] which is a bit weird, but it's one line of code 😅

[MrGVSV]

i think my favourite possible syntax is to force (in a separate attribute definition from any other configuration) #[reflect(where T: Reflect)] which i'm sure could be achieved by matching on the path of the lazily-parsed meta to find where and then parsing the bounds directly from the rest.

How would we opt-out of default bounds? In the above examples they have the custom_where() syntax to remove all non-required bounds (not the prettiest but works haha). With this syntax would the user just have an empty where?

Serde has a different syntax for type bounds. What about re-using it?

https://serde.rs/container-attrs.html#bound

I actually do sorta like this. It gives us the option to modify the where clause on specific impls if we find we can/should add that sort of configuration in the future. Although, I guess it doesn't matter too much right now since I don't think we have any impls we want to allow the user to control separately (yet).

[MrGVSV]

I added a new commit (be18a8f) that replaces custom_where with just a regular where clause as suggested by @soqb. I'd like to get feedback on which one we might want to go with.

Example:

// Change parameter bound
#[derive(Reflect)]
#[reflect(where T: Default)]
struct Foo<T>(String, #[reflect(ignore)] PhantomData<T>);

// Opt-out parameter
#[derive(Reflect)]
#[reflect(where)] // Yeah, I'm surprised a lone `where` is valid syntax too lol
struct Bar<T>(String, #[reflect(ignore)] PhantomData<T>);

[MrGVSV]

This is the best I could come up with for peeking into a TokenStream. If anyone else has a better way of doing this, please let me know!

[alice-i-cecile]

Great docs, good explanation, sensible code changes and lots more tests.

[alice-i-cecile]

I'm going to leave this to Monday, but I'm content to merge this without further review. Added back to the milestone as a result.

[soqb]

i'm still not in favour of the new default bounds (on type parameters instead of active fields).

however the implementation is solid so i'll defer to y'all on this one.

[soqb]

don't we want Any + Send + Sync on T::Assoc instead?

[soqb]

also, you'll need to add TypePath to the list for T::Assoc.

[soqb]

these checks seem redundant given what's below them.

[nicopap]

Took a while to regain context here. I completely forgot what I thought was important…

I don't love the new syntax. I think the serde approach is better¹ (if only to follow established norms). Though it's not a dealbreaker for me.

Couldn't take a very deep look at the code this time, but from a quick glance it looks good.

LGTM.

Footnotes

1. https://serde.rs/container-attrs.html#bound ↩

[alice-i-cecile]

I'm going to merge this now then: we can clean it up further in follow-ups.