Add missing_reflect lint

bevy_lint/src/lints/main_return_without_appexit.rs

@@ -9,6 +9,11 @@
 //! it from `main()` will set the exit code, which allows external processes to detect whether there
 //! was an error.
 //!
+//! # Known issues
+//!
+//! If you wish to silence this lint, you must add `#[allow(bevy::main_return_without_appexit)]` to
+//! `fn main()`, not the line that calls `App::run()`.
+//!
 //! # Example
 //!
 //! ```

bevy_lint/src/lints/missing_reflect.rs

@@ -0,0 +1,180 @@
+//! TODO
+
+use crate::declare_bevy_lint;
+use clippy_utils::{def_path_res, diagnostics::span_lint_hir};
+use rustc_hir::{
+    def::{DefKind, Res},
+    def_id::{DefId, LocalDefId},
+    Item, ItemKind, Node, OwnerId,
+};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::TyCtxt;
+use rustc_session::declare_lint_pass;
+
+declare_bevy_lint! {
+    pub MISSING_REFLECT,
+    RESTRICTION,
+    "defined a component, resource, or event without a `Reflect` implementation",
+}
+
+declare_lint_pass! {
+    MissingReflect => [MISSING_REFLECT.lint]
+}
+
+impl<'tcx> LateLintPass<'tcx> for MissingReflect {
+    // The lint can be summarized in a few steps:
+    //
+    // 1. Find all `impl` items for `Reflect`, `Component`, and `Resource`.
+    // 2. Find the types that these traits are implemented for.
+    // 3. If there's a type that implements `Component` or `Resource` but *not* `Reflect`, emit a
+    //    diagnostic.
+    //
+    // Because we need a list of all `impl` items, not just one at a time, we implement
+    // `check_crate()` and not `check_item()`.
+    fn check_crate(&mut self, cx: &LateContext<'tcx>) {
+        // Find all `impl` items for `Reflect` in the current crate, then from that find `T` in
+        // `impl Reflect for T`.
+        let reflect_types: Vec<_> = find_local_trait_impls(cx.tcx, &crate::paths::REFLECT)
+            .filter_map(|did| impl_to_source_type(cx.tcx, did))
+            .collect();
+
+        for checked_trait in CheckedTraits::all() {
+            // This is the same as the above `reflect_types`, but this time we are searching for
+            // one of the checked traits. (`Component` or `Resource`.)
+            let checked_types: Vec<_> = find_local_trait_impls(cx.tcx, checked_trait.def_path())
+                .filter_map(|did| impl_to_source_type(cx.tcx, did))
+                .collect();
+
+            // Check if any of the checked types do not implement `Reflect`. If so, emit the lint!
+            for def_id in checked_types {
+                if !reflect_types.contains(&def_id) {
+                    let ident = cx.tcx.opt_item_ident(def_id).unwrap();
+
+                    let owner_id = OwnerId {
+                        // This is guaranteed to be a `LocalDefId` because the trait `impl` that it
+                        // came from is also local.
+                        def_id: def_id.expect_local(),
+                    };
+
+                    span_lint_hir(
+                        cx,
+                        MISSING_REFLECT.lint,
+                        owner_id.into(),
+                        ident.span,
+                        checked_trait.diagnostic_message(),
+                    );
+                }
+            }
+        }
+    }
+}
+
+enum CheckedTraits {
+    Component,
+    Resource,
+}
+
+impl CheckedTraits {
+    fn all() -> [Self; 2] {
+        [Self::Component, Self::Resource]
+    }
+
+    fn def_path(&self) -> &'static [&'static str] {
+        match self {
+            Self::Component => &crate::paths::COMPONENT,
+            Self::Resource => &crate::paths::RESOURCE,
+        }
+    }
+
+    fn diagnostic_message(&self) -> &'static str {
+        match self {
+            Self::Component => "defined a component without a `Reflect` implementation",
+            Self::Resource => "defined a resource without a `Reflect` implementation",
+        }
+    }
+}
+
+/// Returns a list of [`LocalDefId`]s for `impl` blocks where a specified trait is implemented.
+///
+/// Note that sometimes multiple traits can be resolved from the same path. (If there are multiple
+/// versions of the same crate, for example.) When this is the case, the results for each trait are
+/// concatenated together.
+fn find_local_trait_impls<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    trait_def_path: &[&str],
+) -> impl Iterator<Item = LocalDefId> + 'tcx {
+    // Find the `DefId`s for a given trait.
+    let trait_def_ids = trait_def_ids(tcx, trait_def_path);
+
+    // Find a map of all trait `impl` items within the current crate. The key is the `DefId` of the
+    // trait, and the value is a `Vec<LocalDefId>` for all `impl` items.
+    let local_trait_impls = tcx.all_local_trait_impls(());
+
+    trait_def_ids
+        .filter_map(|trait_def_id| local_trait_impls.get(&trait_def_id))
+        .flatten()
+        .copied()
+}
+
+/// Finds all [`DefId`]s for a given trait path.
+///
+/// This returns an interator because multiple items can have the same name and path, such as
+/// traits and macros, and because there may be multiple versions of the same crate. The returned
+/// [`DefId`]s are guaranteed to point to traits, however, with all others skipped.
+fn trait_def_ids(tcx: TyCtxt<'_>, trait_def_path: &[&str]) -> impl Iterator<Item = DefId> {
+    def_path_res(tcx, trait_def_path)
+        .into_iter()
+        .filter_map(|res| match res {
+            Res::Def(DefKind::Trait, def_id) => Some(def_id),
+            _ => None,
+        })
+}
+
+/// This function locates the source structure definition for a `impl` item [`LocalDefId`].
+///
+/// Given the following code:
+///
+/// ```
+/// struct Foo; // ID: A
+///
+/// impl Foo {} // ID: B
+/// ```
+///
+/// This function will return a [`DefId`] of `A` when passed the [`LocalDefId`] `B`. It even works
+/// when the `impl` block implements a trait, such as `impl Bar for Foo`.
+///
+/// This function will return [`None`] if `def_id` does not correspond to an `impl` item, or if the
+/// type `T` in `impl T` is not an ADT[^0]. References are automatically peeled, so only the
+/// underlying type determines the result.
+///
+/// [^0]: An algebraic data type. These are most user-defined types, such as structs, enums, and
+/// unions. Notably, primitives are not ADTs. See [`TyKind`](rustc_middle::ty::TyKind) for a
+/// complete list.
+fn impl_to_source_type(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Option<DefId> {
+    let node = tcx.hir_node_by_def_id(def_id);
+
+    // Ensure the node is an `impl` item.
+    let Node::Item(Item {
+        kind: ItemKind::Impl(impl_),
+        ..
+    }) = node
+    else {
+        return None;
+    };
+
+    // This is the HIR representation of `T` for `impl T`. Note that the HIR representation does
+    // not contain actual type information, just the qualified path.
+    let hir_ty = impl_.self_ty;
+
+    // Convert the `rustc_hir::Ty` to a `rustc_middle::ty::Ty`, which is fully resolved with
+    // complete type information. Note that we can safely call `skip_binder()` because we are
+    // purely extrating the type's `DefId`, which does not depend on generic or lifetime data. Also
+    // note the call to `peel_refs()`, which removes references and returns the underlying type.
+    let ty_adt = tcx
+        .type_of(hir_ty.hir_id.owner)
+        .skip_binder()
+        .peel_refs()
+        .ty_adt_def()?;
+
+    Some(ty_adt.did())
+}

bevy_lint/src/lints/mod.rs

@@ -3,12 +3,14 @@ use rustc_lint::{Lint, LintStore};
 
 pub mod insert_event_resource;
 pub mod main_return_without_appexit;
+pub mod missing_reflect;
 pub mod panicking_methods;
 pub mod plugin_not_ending_in_plugin;
 
 pub(crate) static LINTS: &[&BevyLint] = &[
     insert_event_resource::INSERT_EVENT_RESOURCE,
     main_return_without_appexit::MAIN_RETURN_WITHOUT_APPEXIT,
+    missing_reflect::MISSING_REFLECT,
     panicking_methods::PANICKING_QUERY_METHODS,
     panicking_methods::PANICKING_WORLD_METHODS,
     plugin_not_ending_in_plugin::PLUGIN_NOT_ENDING_IN_PLUGIN,
@@ -22,6 +24,7 @@ pub(crate) fn register_lints(store: &mut LintStore) {
 pub(crate) fn register_passes(store: &mut LintStore) {
     store.register_late_pass(|_| Box::new(insert_event_resource::InsertEventResource));
     store.register_late_pass(|_| Box::new(main_return_without_appexit::MainReturnWithoutAppExit));
+    store.register_late_pass(|_| Box::new(missing_reflect::MissingReflect));
     store.register_late_pass(|_| Box::new(panicking_methods::PanickingMethods));
     store.register_late_pass(|_| Box::new(plugin_not_ending_in_plugin::PluginNotEndingInPlugin));
 }

bevy_lint/src/paths.rs

@@ -7,8 +7,11 @@
 //! [`match_def_path()`](clippy_utils::match_def_path).
 
 pub const APP: [&str; 3] = ["bevy_app", "app", "App"];
+pub const COMPONENT: [&str; 3] = ["bevy_ecs", "component", "Component"];
 pub const EVENTS: [&str; 3] = ["bevy_ecs", "event", "Events"];
 pub const PLUGIN: [&str; 3] = ["bevy_app", "plugin", "Plugin"];
 pub const QUERY: [&str; 4] = ["bevy_ecs", "system", "query", "Query"];
 pub const QUERY_STATE: [&str; 4] = ["bevy_ecs", "query", "state", "QueryState"];
+pub const REFLECT: [&str; 3] = ["bevy_reflect", "reflect", "Reflect"];
+pub const RESOURCE: [&str; 4] = ["bevy_ecs", "system", "system_param", "Resource"];
 pub const WORLD: [&str; 3] = ["bevy_ecs", "world", "World"];

bevy_lint/tests/ui/insert_event_resource/bug_94.rs

@@ -0,0 +1,22 @@
+//! This test tracks the bug reported in [#94]. When this starts failing, the bug has been fixed.
+//!
+//! [#94]: https://github.com/TheBevyFlock/bevy_cli/issues/94
+
+//@check-pass
+
+#![feature(register_tool)]
+#![register_tool(bevy)]
+#![deny(bevy::insert_event_resource)]
+
+use bevy::prelude::*;
+
+#[derive(Event)]
+struct Foo;
+
+fn main() {
+    let mut app = App::new();
+
+    // These both should error, but currently do not.
+    App::init_resource::<Events<Foo>>(&mut app);
+    App::insert_resource::<Events<Foo>>(&mut app, Default::default());
+}

bevy_lint/tests/ui/insert_event_resource/main.fixed

@@ -0,0 +1,26 @@
+#![feature(register_tool)]
+#![register_tool(bevy)]
+#![deny(bevy::insert_event_resource)]
+
+use bevy::prelude::*;
+
+#[derive(Event)]
+struct Foo;
+
+fn main() {
+    App::new().add_event::<Foo>();
+    //~^ ERROR: called `App::init_resource::<Events<T>>()` instead of `App::add_event::<T>()`
+
+    App::new().add_event::<Foo>();
+    //~^ ERROR: called `App::insert_resource(Events<T>)` instead of `App::add_event::<T>()`
+
+    // Make sure the correct type is detected, even when not explicitly passed to
+    // `insert_resource()`.
+    let implied_event: Events<Foo> = Default::default();
+    App::new().add_event::<Foo>();
+    //~^ ERROR: called `App::insert_resource(Events<T>)` instead of `App::add_event::<T>()`
+
+    // Ensure the lint can be muted by annotating the expression.
+    #[allow(bevy::insert_event_resource)]
+    App::new().init_resource::<Events<Foo>>();
+}

bevy_lint/tests/ui/insert_event_resource/main.rs

@@ -0,0 +1,26 @@
+#![feature(register_tool)]
+#![register_tool(bevy)]
+#![deny(bevy::insert_event_resource)]
+
+use bevy::prelude::*;
+
+#[derive(Event)]
+struct Foo;
+
+fn main() {
+    App::new().init_resource::<Events<Foo>>();
+    //~^ ERROR: called `App::init_resource::<Events<T>>()` instead of `App::add_event::<T>()`
+
+    App::new().insert_resource::<Events<Foo>>(Default::default());
+    //~^ ERROR: called `App::insert_resource(Events<T>)` instead of `App::add_event::<T>()`
+
+    // Make sure the correct type is detected, even when not explicitly passed to
+    // `insert_resource()`.
+    let implied_event: Events<Foo> = Default::default();
+    App::new().insert_resource(implied_event);
+    //~^ ERROR: called `App::insert_resource(Events<T>)` instead of `App::add_event::<T>()`
+
+    // Ensure the lint can be muted by annotating the expression.
+    #[allow(bevy::insert_event_resource)]
+    App::new().init_resource::<Events<Foo>>();
+}

bevy_lint/tests/ui/insert_event_resource/main.stderr

@@ -0,0 +1,40 @@
+error: called `App::init_resource::<Events<T>>()` instead of `App::add_event::<T>()`
+  --> tests/ui/insert_event_resource/main.rs:11:16
+   |
+11 |     App::new().init_resource::<Events<Foo>>();
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: the lint level is defined here
+  --> tests/ui/insert_event_resource/main.rs:3:9
+   |
+3  | #![deny(bevy::insert_event_resource)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: inserting an `Events` resource does not fully setup that event
+   |
+11 |     App::new().add_event::<Foo>();
+   |                ~~~~~~~~~~~~~~~~~~
+
+error: called `App::insert_resource(Events<T>)` instead of `App::add_event::<T>()`
+  --> tests/ui/insert_event_resource/main.rs:14:16
+   |
+14 |     App::new().insert_resource::<Events<Foo>>(Default::default());
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: inserting an `Events` resource does not fully setup that event
+   |
+14 |     App::new().add_event::<Foo>();
+   |                ~~~~~~~~~~~~~~~~~~
+
+error: called `App::insert_resource(Events<T>)` instead of `App::add_event::<T>()`
+  --> tests/ui/insert_event_resource/main.rs:20:16
+   |
+20 |     App::new().insert_resource(implied_event);
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: inserting an `Events` resource does not fully setup that event
+   |
+20 |     App::new().add_event::<Foo>();
+   |                ~~~~~~~~~~~~~~~~~~
+
+error: aborting due to 3 previous errors
+

bevy_lint/tests/ui/main_return_without_appexit/bug_87.rs

@@ -0,0 +1,17 @@
+//! This test tracks the bug reported in [#87]. When this starts failing, the bug has been fixed.
+//!
+//! [#87]: https://github.com/TheBevyFlock/bevy_cli/issues/87
+
+#![feature(register_tool)]
+#![register_tool(bevy)]
+#![deny(bevy::main_return_without_appexit)]
+
+use bevy::prelude::*;
+
+fn main() {
+    // This should not raise an error, since `AppExit` is not ignored.
+    let app_exit = App::new().run();
+    //~^ ERROR: an entrypoint that calls `App::run()` does not return `AppExit`
+
+    println!("{app_exit:?}");
+}

bevy_lint/tests/ui/main_return_without_appexit/bug_87.stderr

@@ -0,0 +1,18 @@
+error: an entrypoint that calls `App::run()` does not return `AppExit`
+  --> tests/ui/main_return_without_appexit/bug_87.rs:13:31
+   |
+11 | fn main() {
+   |          - help: try: `-> AppExit`
+12 |     // This should not raise an error, since `AppExit` is not ignored.
+13 |     let app_exit = App::new().run();
+   |                               ^^^^^
+   |
+   = note: `App::run()` returns `AppExit`, which can be used to determine whether the app exited successfully or not
+note: the lint level is defined here
+  --> tests/ui/main_return_without_appexit/bug_87.rs:7:9
+   |
+7  | #![deny(bevy::main_return_without_appexit)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 1 previous error
+