Add missing_reflect lint (attempt 2)

[BD103]

Closes #54. See #137 for past attempt.

This adds a lint that lets users require all components, resources, and events derive Reflect. Reflect is often used by tools such as bevy-inspector-egui to view the state of the world while the game is running, but these tools are far less useful if they do not have access to reflection data.

This lint is off by default, and intended to be used to require Reflect for types within specific modules. For example:

#[deny(bevy::missing_reflect)]
mod components {
    use bevy::prelude::*;

    #[derive(Component)]
    struct MyComponent;
}

Developers are free to require it throughout the entire crate, however.

#![deny(bevy::missing_reflect)]

This lint gives lots of information when it is raised, such as where the offending trait was implemented and how to fix it:

Thanks to clippy_utils's suggest_item_with_attr(), the suggestion is machine applicable. This means that cargo fix can automatically fix this lint, making migrating a code base super easy.

[BD103]

I want to write a few UI tests and the module documentation for this lint, but everything else is ready for review. :)

[MrGVSV]

One question: How does this handle types that cannot be Reflect? For example:

struct NotReflect(f32);

#[derive(Component)]
struct MyComponent(NotReflect);

It seems like there's two options for users here:

1. Add #[allow(bevy::missing_reflect)] to the type
2. Derive Reflect but #[reflect(ignore)] the field

I think both are fine, but would it make sense (and/or be possible) to check if all fields are already Reflect before suggesting this? Or would that incur too much of a performance/complexity cost? (And maybe for this initial PR that would be over-complicating it.)

[BD103]

One question: How does this handle types that cannot be Reflect?

I must admit, I didn't realize there weren't types that could be reflected. (Though thinking through it, it does make sense.)

The lint currently doesn't care if it's impossible to implement Reflect, it just cares that the type doesn't currently implement it.

I think both are fine, but would it make sense (and/or be possible) to check if all fields are already Reflect before suggesting this?

I think checking that all fields can implement Reflect would work. If there are some that cannot, the lint can emit a note about #[reflect(ignore)] and make the suggestion MaybeIncorrect.

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

Or would that incur too much of a performance/complexity cost?

I'm not worried about performance too much right now. I'd much prefer the linter to be useful, smart, and feature-complete than zippy. (And there's some low-hanging fruit, should performance become arduous.)

[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)], Reflect¹.

Since FromReflect requires Reflect¹, 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 Reflect¹.

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. ↩ ↩² ↩³

[TimJentzsch]

I'm fine with merging this and moving the edge cases @MrGVSV described to a separate issue.
Their comment would serve as a great issue description.

Maybe we can even split this into two lints in the future: One for the simple cases where all fields implement Reflect, which would be machine applicable, and one for the cases where some fields require additional amnotations, which could be maybe incorrect.

[BD103]

I'm fine with merging this and moving the edge cases @MrGVSV described to a separate issue.
Their comment would serve as a great issue description.

Sounds good! I'm going to do some final touch-ups, then merge this as-is.

[BD103]

Unfortunately due to #141, I had to switch the suggestion to MaybeIncorrect by default. As nice as it was to use cargo fix to automatically apply suggestions, I'd much rather the implementation be correct.

With that, this PR is ready to go. Merging!