Lint missing_reflect does not check for fields that cannot be reflected

[BD103]

Please see #139 (comment), #139 (comment), and #139 (comment) for context.

In summary: the missing_reflect lint pass currently emits the #[derive(Reflect)] suggestion as MachineApplicable, but this may not always be the case. Some fields of structs cannot be reflected, so deriving Reflect will fail.

Quoting @MrGVSV:

For checking all fields, should I just check that they also implement Reflect? Or is there other criteria that I should look for?

So this is where it can get a little confusing 😅

Always Required

The traits that are always mandatory are Any, Send, and Sync.

https://github.com/bevyengine/bevy/blob/8c0fcf02d0095061f8756c8551b08e99542afe92/crates/bevy_reflect/derive/src/where_clause_options.rs#L194-L196

Active Fields

Then for active fields (i.e. fields not marked #[reflect(ignore)]), we require that they implement TypePath, MaybeTyped, and RegisterForReflection. The latter two are hidden traits that allow us to handle dynamic types, which can't implement Typed and GetTypeRegistration, respectively.

https://github.com/bevyengine/bevy/blob/8c0fcf02d0095061f8756c8551b08e99542afe92/crates/bevy_reflect/derive/src/where_clause_options.rs#L148-L171

Reflection Bound

Lastly, all active fields must implement either FromReflect or, if #[reflect(from_reflect = false)], Reflect1.

Since FromReflect requires Reflect1, it should be fine to just check for PartialReflect. But if any field doesn't implement FromReflect, the container type will need to also add the #[reflect(from_reflect = false)] opt-out.

https://github.com/bevyengine/bevy/blob/8c0fcf02d0095061f8756c8551b08e99542afe92/crates/bevy_reflect/derive/src/where_clause_options.rs#L173-L182

Generic Types

And generic type parameters require TypePath unless the container is marked with #[reflect(type_path = false)].

https://github.com/bevyengine/bevy/blob/8c0fcf02d0095061f8756c8551b08e99542afe92/crates/bevy_reflect/derive/src/where_clause_options.rs#L132-L146

So if we did want to handle cases with un-reflectable fields, it would be best if we checked for all of these. But if it's simpler to start with, I'd say the biggest one to check for is just Reflect1.

There are other attributes to help users control these bounds (e.g. #[reflect(no_field_bounds)], #[reflect(where T: Foo)], etc.), but I don't think a linter should recommend them just generally.

Footnotes

1. The `Reflect` bound in the derive macro was changed to `PartialReflect` in 0.15. [↩](#user-content-fnref-1-30df4f76ad0aee04816f34f9092b17c4) [↩2](#user-content-fnref-1-2-30df4f76ad0aee04816f34f9092b17c4) [↩3](#user-content-fnref-1-3-30df4f76ad0aee04816f34f9092b17c4)

For a short-term solution, the lint pass should check that all fields of a struct implement Reflect, and use that to determine the Applicability. In the future, the lint should do the more complicated checks that @MrGVSV described and suggest using #[reflect(ignore)] for bad fields.