Add borrowed_reborrowable lint (#164)

Fixes #127

Adds the pedantic `borrowed_reborrowable` lint to suggest that
parameters using a mutable reference to a re-borrowable type instead
switch to an owned instance of that type.

See #127 for details.

---------

Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>

bevy_lint/src/lints/borrowed_reborrowable.rs

@@ -0,0 +1,272 @@
+//! Checks for function parameters that take a mutable reference to a
+//! re-borrowable type.
+//!
+//! # Motivation
+//!
+//! Several Bevy types look like they are owned, when in reality they contain an `&mut` reference
+//! to the data owned by the ECS. `Commands` and `Query` are examples of such types that _pretend_
+//! to own data for better user ergonomics.
+//!
+//! This can be an issue when a user writes a function that takes a mutable reference to one of
+//! these types, not realizing that it itself is _already_ a reference. These mutable references
+//! can almost always be readily converted back to an owned instance of the type, which is a cheap
+//! operation that avoids nested references.
+//!
+//! The only time a re-borrowable type cannot be re-borrowed is when the function returns
+//! referenced data that is bound to the mutable reference of the re-borrowable type.
+//!
+//! # Known Issues
+//!
+//! This lint does not currently support the [`Fn`] traits or function pointers. This means the
+//! following types will not be caught by the lint:
+//!
+//! - `impl FnOnce(&mut Commands)`
+//! - `Box<dyn FnMut(&mut Commands)>`
+//! - `fn(&mut Commands)`
+//!
+//! # Example
+//!
+//! ```
+//! # use bevy::prelude::*;
+//! #
+//! fn system(mut commands: Commands) {
+//!     helper_function(&mut commands);
+//! }
+//!
+//! // This takes `&mut Commands`, but it doesn't need to!
+//! fn helper_function(commands: &mut Commands) {
+//!     // ...
+//! }
+//! #
+//! # bevy::ecs::system::assert_is_system(system);
+//! ```
+//!
+//! Use instead:
+//!
+//! ```
+//! # use bevy::prelude::*;
+//! #
+//! fn system(mut commands: Commands) {
+//!     // Convert `&mut Commands` to `Commands`.
+//!     helper_function(commands.reborrow());
+//! }
+//!
+//! fn helper_function(mut commands: Commands) {
+//!     // ...
+//! }
+//! #
+//! # bevy::ecs::system::assert_is_system(system);
+//! ```
+//!
+//! The following is an example where a type cannot be re-borrowed, for which this lint will not
+//! emit any warning:
+//!
+//! ```
+//! # use bevy::{prelude::*, ecs::system::EntityCommands};
+//! #
+//! fn system(mut commands: Commands) {
+//!     let entity_commands = helper_function(&mut commands);
+//! }
+//!
+//! // Note how this function returns a reference with the same lifetime as `Commands`.
+//! fn helper_function<'a>(commands: &'a mut Commands) -> EntityCommands<'a> {
+//!     commands.spawn_empty()
+//! }
+//! #
+//! # bevy::ecs::system::assert_is_system(system);
+//! ```
+
+use std::ops::ControlFlow;
+
+use crate::declare_bevy_lint;
+use clippy_utils::{diagnostics::span_lint_and_sugg, ty::match_type};
+use rustc_errors::Applicability;
+use rustc_hir::{intravisit::FnKind, Body, FnDecl, Mutability};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::{Interner, Ty, TyKind, TypeVisitable, TypeVisitor};
+use rustc_session::declare_lint_pass;
+use rustc_span::{
+    def_id::LocalDefId,
+    symbol::{kw, Ident},
+    Span,
+};
+
+declare_bevy_lint! {
+    pub BORROWED_REBORROWABLE,
+    PEDANTIC,
+    "parameter takes a mutable reference to a re-borrowable type",
+}
+
+declare_lint_pass! {
+    BorrowedReborrowable => [BORROWED_REBORROWABLE.lint]
+}
+
+impl<'tcx> LateLintPass<'tcx> for BorrowedReborrowable {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'tcx>,
+        _: &'tcx Body<'tcx>,
+        _: Span,
+        def_id: LocalDefId,
+    ) {
+        let fn_sig = match kind {
+            FnKind::Closure => cx.tcx.closure_user_provided_sig(def_id).value,
+            // We use `instantiate_identity` to discharge the binder since we don't
+            // mind using placeholders for any bound arguments
+            _ => cx.tcx.fn_sig(def_id).instantiate_identity(),
+        };
+
+        let arg_names = cx.tcx.fn_arg_names(def_id);
+
+        let args = fn_sig.inputs().skip_binder();
+
+        for (arg_index, arg_ty) in args.iter().enumerate() {
+            let TyKind::Ref(region, ty, Mutability::Mut) = arg_ty.kind() else {
+                // We only care about `&mut` parameters
+                continue;
+            };
+
+            let arg_ident = arg_names[arg_index];
+
+            // This lint would emit a warning on `&mut self` if `self` was reborrowable. This isn't
+            // useful, though, because it would hurt the ergonomics of using methods of
+            // reborrowable types.
+            //
+            // To avoid this, we skip any parameter named `self`. This won't false-positive on
+            // other function arguments named `self`, since it is a special keyword that is
+            // disallowed in other positions.
+            if arg_ident.name == kw::SelfLower {
+                continue;
+            }
+
+            let Some(reborrowable) = Reborrowable::try_from_ty(cx, *ty) else {
+                // The type is not one of our known re-borrowable types
+                continue;
+            };
+
+            let is_output_bound_to_arg = fn_sig
+                .output()
+                .visit_with(&mut ContainsRegion(*region))
+                .is_break();
+
+            if is_output_bound_to_arg {
+                // We don't want to suggest re-borrowing if the return type's
+                // lifetime is bound to the argument's reference.
+                // This is because it's impossible to convert something like:
+                // `for<'a> (&'a mut Commands<'_, '_>) -> EntityCommands<'a>`
+                // to something like:
+                // `for<'a> (Commands<'_, '_>) -> EntityCommands<'a>`
+                // without getting: `error[E0515]: cannot return value referencing function
+                // parameter `commands` ``
+                continue;
+            }
+
+            let span = decl.inputs[arg_index].span.to(arg_ident.span);
+
+            span_lint_and_sugg(
+                cx,
+                BORROWED_REBORROWABLE.lint,
+                span,
+                reborrowable.message(),
+                reborrowable.help(),
+                reborrowable.suggest(arg_ident, ty.to_string()),
+                // Not machine-applicable since the function body may need to
+                // also be updated to account for the removed ref
+                Applicability::MaybeIncorrect,
+            );
+        }
+    }
+}
+
+#[derive(Debug, Copy, Clone)]
+enum Reborrowable {
+    Commands,
+    Deferred,
+    DeferredWorld,
+    EntityCommands,
+    EntityMut,
+    FilteredEntityMut,
+    Mut,
+    MutUntyped,
+    NonSendMut,
+    PtrMut,
+    Query,
+    ResMut,
+}
+
+impl Reborrowable {
+    fn try_from_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Self> {
+        use crate::paths::*;
+
+        const PATH_MAP: &[(&[&str], Reborrowable)] = &[
+            (&COMMANDS, Reborrowable::Commands),
+            (&DEFERRED, Reborrowable::Deferred),
+            (&DEFERRED_WORLD, Reborrowable::DeferredWorld),
+            (&ENTITY_COMMANDS, Reborrowable::EntityCommands),
+            (&ENTITY_MUT, Reborrowable::EntityMut),
+            (&FILTERED_ENTITY_MUT, Reborrowable::FilteredEntityMut),
+            (&MUT, Reborrowable::Mut),
+            (&MUT_UNTYPED, Reborrowable::MutUntyped),
+            (&NON_SEND_MUT, Reborrowable::NonSendMut),
+            (&PTR_MUT, Reborrowable::PtrMut),
+            (&QUERY, Reborrowable::Query),
+            (&RES_MUT, Reborrowable::ResMut),
+        ];
+
+        for &(path, reborrowable) in PATH_MAP {
+            if match_type(cx, ty, path) {
+                return Some(reborrowable);
+            }
+        }
+
+        None
+    }
+
+    fn message(&self) -> String {
+        let name = self.name();
+        format!("parameter takes `&mut {name}` instead of a re-borrowed `{name}`",)
+    }
+
+    fn name(&self) -> &'static str {
+        match self {
+            Self::Commands => "Commands",
+            Self::Deferred => "Deferred",
+            Self::DeferredWorld => "DeferredWorld",
+            Self::EntityCommands => "EntityCommands",
+            Self::EntityMut => "EntityMut",
+            Self::FilteredEntityMut => "FilteredEntityMut",
+            Self::Mut => "Mut",
+            Self::MutUntyped => "MutUntyped",
+            Self::NonSendMut => "NonSendMut",
+            Self::PtrMut => "PtrMut",
+            Self::Query => "Query",
+            Self::ResMut => "ResMut",
+        }
+    }
+
+    fn help(&self) -> String {
+        let name = self.name();
+        format!("use `{name}` instead")
+    }
+
+    fn suggest(&self, ident: Ident, ty: String) -> String {
+        format!("mut {ident}: {ty}")
+    }
+}
+
+/// [`TypeVisitor`] for checking if the given region is contained in the type.
+struct ContainsRegion<I: Interner>(pub I::Region);
+
+impl<I: Interner> TypeVisitor<I> for ContainsRegion<I> {
+    type Result = ControlFlow<()>;
+
+    fn visit_region(&mut self, r: I::Region) -> Self::Result {
+        if self.0 == r {
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(())
+        }
+    }
+}

bevy_lint/src/lints/mod.rs

@@ -7,6 +7,7 @@
 use crate::lint::BevyLint;
 use rustc_lint::{Lint, LintStore};
 
+pub mod borrowed_reborrowable;
 pub mod insert_event_resource;
 pub mod main_return_without_appexit;
 pub mod missing_reflect;
@@ -15,6 +16,7 @@ pub mod plugin_not_ending_in_plugin;
 pub mod zst_query;
 
 pub(crate) static LINTS: &[&BevyLint] = &[
+    borrowed_reborrowable::BORROWED_REBORROWABLE,
     insert_event_resource::INSERT_EVENT_RESOURCE,
     main_return_without_appexit::MAIN_RETURN_WITHOUT_APPEXIT,
     panicking_methods::PANICKING_QUERY_METHODS,
@@ -30,6 +32,7 @@ pub(crate) fn register_lints(store: &mut LintStore) {
 }
 
 pub(crate) fn register_passes(store: &mut LintStore) {
+    store.register_late_pass(|_| Box::new(borrowed_reborrowable::BorrowedReborrowable));
     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));

bevy_lint/src/paths.rs

@@ -7,13 +7,24 @@
 //! [`match_def_path()`](clippy_utils::match_def_path).
 
 pub const APP: [&str; 3] = ["bevy_app", "app", "App"];
+pub const COMMANDS: [&str; 4] = ["bevy_ecs", "system", "commands", "Commands"];
 pub const COMPONENT: [&str; 3] = ["bevy_ecs", "component", "Component"];
+pub const DEFERRED: [&str; 4] = ["bevy_ecs", "system", "system_param", "Deferred"];
+pub const DEFERRED_WORLD: [&str; 4] = ["bevy_ecs", "world", "deferred_world", "DeferredWorld"];
+pub const ENTITY_COMMANDS: [&str; 4] = ["bevy_ecs", "system", "commands", "EntityCommands"];
+pub const ENTITY_MUT: [&str; 4] = ["bevy_ecs", "world", "entity_ref", "EntityMut"];
 // Note that this moves to `bevy_ecs::event::base::Event` in 0.15.
 pub const EVENT: [&str; 3] = ["bevy_ecs", "event", "Event"];
 pub const EVENTS: [&str; 3] = ["bevy_ecs", "event", "Events"];
+pub const FILTERED_ENTITY_MUT: [&str; 4] = ["bevy_ecs", "world", "entity_ref", "FilteredEntityMut"];
+pub const MUT: [&str; 3] = ["bevy_ecs", "change_detection", "Mut"];
+pub const MUT_UNTYPED: [&str; 3] = ["bevy_ecs", "change_detection", "MutUntyped"];
+pub const NON_SEND_MUT: [&str; 3] = ["bevy_ecs", "change_detection", "NonSendMut"];
 pub const PLUGIN: [&str; 3] = ["bevy_app", "plugin", "Plugin"];
+pub const PTR_MUT: [&str; 2] = ["bevy_ptr", "PtrMut"];
 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 RES_MUT: [&str; 3] = ["bevy_ecs", "change_detection", "ResMut"];
 pub const RESOURCE: [&str; 4] = ["bevy_ecs", "system", "system_param", "Resource"];
 pub const WORLD: [&str; 3] = ["bevy_ecs", "world", "World"];

bevy_lint/tests/ui/borrowed_reborrowable/bug_174.rs

@@ -0,0 +1,26 @@
+//! This test tracks the bug reported in [#174]. When this starts failing, the bug has been fixed.
+//!
+//! [#174]: https://github.com/TheBevyFlock/bevy_cli/issues/174
+
+//@check-pass
+
+#![feature(register_tool)]
+#![register_tool(bevy)]
+#![deny(bevy::borrowed_reborrowable)]
+
+use bevy::prelude::*;
+
+fn main() {
+    let mut world = World::new();
+
+    closure_wrapper(world.commands(), |commands| commands.spawn_empty().id());
+    closure_wrapper2(world.commands(), |commands| commands.spawn_empty().id());
+}
+
+fn closure_wrapper(mut commands: Commands, f: impl FnOnce(&mut Commands) -> Entity) {
+    f(&mut commands);
+}
+
+fn closure_wrapper2(mut commands: Commands, f: fn(&mut Commands) -> Entity) {
+    f(&mut commands);
+}

bevy_lint/tests/ui/borrowed_reborrowable/closures.rs

@@ -0,0 +1,20 @@
+//! This tests the `borrowed_reborrowable` lint, specifically when triggered on closure types.
+
+#![feature(register_tool)]
+#![register_tool(bevy)]
+#![deny(bevy::borrowed_reborrowable)]
+
+use bevy::prelude::*;
+
+fn main() {
+    let mut world = World::new();
+    let mut commands = world.commands();
+
+    //~| HELP: use `Commands` instead
+    //~v ERROR: parameter takes `&mut Commands` instead of a re-borrowed `Commands`
+    let closure = |commands: &mut Commands| {
+        commands.spawn_empty();
+    };
+
+    closure(&mut commands);
+}

bevy_lint/tests/ui/borrowed_reborrowable/closures.stderr

@@ -0,0 +1,14 @@
+error: parameter takes `&mut Commands` instead of a re-borrowed `Commands`
+  --> tests/ui/borrowed_reborrowable/closures.rs:15:20
+   |
+15 |     let closure = |commands: &mut Commands| {
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^ help: use `Commands` instead: `mut commands: bevy::prelude::Commands<'_, '_>`
+   |
+note: the lint level is defined here
+  --> tests/ui/borrowed_reborrowable/closures.rs:5:9
+   |
+5  | #![deny(bevy::borrowed_reborrowable)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 1 previous error
+