Add Lint: camera_modification_in_fixed_update #417
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,196 @@ | ||
| //! Checks for systems added to the `FixedUpdate` schedule that mutably query entities with a | ||
| //! `Camera` component. | ||
| //! | ||
| //! # Motivation | ||
| //! | ||
| //! Modifying camera components in the `FixedUpdate` schedule can cause jittery or inconsistent | ||
| //! visuals because `FixedUpdate` runs at a fixed timestep mainly for physics and deterministic | ||
| //! logic. The camera should be updated in the last variable timestep before `FixedUpdate` which is | ||
| //! the `Update` schedule. | ||
| //! | ||
| //! # Known Issues | ||
| //! | ||
| //! - The lint only detects systems that explicitly use the `With<Camera>` query filter. | ||
| //! | ||
| //! # Example | ||
| //! | ||
| //! ```rust | ||
| //! use bevy::prelude::*; | ||
| //! | ||
| //! fn move_camera(mut query: Query<&mut Transform, With<Camera>>) { | ||
| //! // ... | ||
| //! } | ||
| //! | ||
| //! fn main() { | ||
| //! App::new() | ||
| //! .add_systems(FixedUpdate, move_camera); | ||
| //! } | ||
| //! ``` | ||
| //! | ||
| //! Use instead: | ||
| //! | ||
| //! ```rust | ||
| //! use bevy::prelude::*; | ||
| //! | ||
| //! fn move_camera(mut query: Query<&mut Transform, With<Camera>>) { | ||
| //! // ... | ||
| //! } | ||
| //! | ||
| //! fn main() { | ||
| //! App::new() | ||
| //! .add_systems(Update, move_camera); | ||
| //! } | ||
| //! ``` | ||
BD103 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| use clippy_utils::{diagnostics::span_lint_and_help, sym, ty::match_type}; | ||
| use rustc_hir::{ExprKind, QPath, def::Res}; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_middle::ty::{Adt, GenericArgKind, TyKind}; | ||
| use rustc_span::Symbol; | ||
|
|
||
| use crate::{declare_bevy_lint, declare_bevy_lint_pass, utils::hir_parse::MethodCall}; | ||
|
|
||
| declare_bevy_lint! { | ||
| pub(crate) CAMERA_MODIFICATION_IN_FIXED_UPDATE, | ||
| super::Nursery, | ||
| "camera modified in the `FixedUpdate` schedule", | ||
| } | ||
|
|
||
| declare_bevy_lint_pass! { | ||
| pub(crate) CameraModificationInFixedUpdate => [CAMERA_MODIFICATION_IN_FIXED_UPDATE], | ||
| @default = { | ||
| add_systems: Symbol = sym!(add_systems), | ||
| }, | ||
| } | ||
|
|
||
| impl<'tcx> LateLintPass<'tcx> for CameraModificationInFixedUpdate { | ||
| fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx>) { | ||
| if expr.span.in_external_macro(cx.tcx.sess.source_map()) { | ||
| return; | ||
| } | ||
|
|
||
| let Some(MethodCall { | ||
| method_path, | ||
| args, | ||
| receiver, | ||
| .. | ||
| }) = MethodCall::try_from(cx, expr) | ||
| else { | ||
| return; | ||
| }; | ||
|
|
||
| let receiver_ty = cx.typeck_results().expr_ty(receiver).peel_refs(); | ||
|
|
||
| // Match calls to `App::add_systems(schedule, systems)` | ||
| if match_type(cx, receiver_ty, &crate::paths::APP) | ||
| && method_path.ident.name != self.add_systems | ||
BD103 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| return; | ||
| } | ||
|
|
||
| let [schedule, systems] = args else { | ||
| return; | ||
| }; | ||
|
|
||
| let schedule_ty = cx.typeck_results().expr_ty(schedule).peel_refs(); | ||
|
|
||
| // Skip if the schedule is not `FixedUpdate` | ||
| if !match_type(cx, schedule_ty, &crate::paths::FIXED_UPDATE) { | ||
| return; | ||
| } | ||
|
|
||
| // Collect all added system expressions | ||
| let system_exprs = if let ExprKind::Tup(inner) = systems.kind { | ||
| inner.iter().collect() | ||
DaAlbrecht marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } else { | ||
| vec![systems] | ||
| }; | ||
|
|
||
| // Resolve the function definition for each system | ||
| for system_expr in system_exprs { | ||
| if let ExprKind::Path(QPath::Resolved(_, path)) = system_expr.kind | ||
| && let Res::Def(_, def_id) = path.res | ||
|
Comment on lines
+140
to
+141
There was a problem hiding this comment. For a follow-up: This doesn't handle nested tuples. Meaning I don't think this will lint on: app.add_systems(FixedUpdate, (unrelated, (also_unrelated, modifies_camera)));
// ^^^^^^^^^^^^^^^ Won't see thisThere was a problem hiding this comment. Yep! I think I want to solve this first before merging, otherwise it could be confusing. There was a problem hiding this comment. It is a nursery lint, so you can list it in "Known Issues" if you want this shipped first. There was a problem hiding this comment. You may be able to use |
||
| { | ||
| let system_fn_sig = cx.tcx.fn_sig(def_id); | ||
| // Iterate over the function parameter types of the system function | ||
| for ty in system_fn_sig.skip_binder().inputs().skip_binder() { | ||
| let Adt(adt_def_id, args) = ty.kind() else { | ||
| continue; | ||
| }; | ||
|
|
||
| // Check if the parameter is a `Query` | ||
| let adt_ty = cx.tcx.type_of(adt_def_id.did()).skip_binder(); | ||
| if !match_type(cx, adt_ty, &crate::paths::QUERY) { | ||
| continue; | ||
| } | ||
|
|
||
| // Get the type arguments and ignore Lifetimes | ||
| let mut query_type_arguments = | ||
| args.iter() | ||
| .filter_map(|generic_arg| match generic_arg.unpack() { | ||
| GenericArgKind::Type(ty) => Some(ty), | ||
| _ => None, | ||
| }); | ||
|
|
||
| let Some(query_data) = query_type_arguments.next() else { | ||
| return; | ||
| }; | ||
|
|
||
| let Some(query_filters) = query_type_arguments.next() else { | ||
| return; | ||
| }; | ||
|
|
||
| // Determine mutability of each queried component | ||
| let query_data_mutability = match query_data.kind() { | ||
| TyKind::Tuple(tys) => tys | ||
| .iter() | ||
| .filter_map(|ty| match ty.kind() { | ||
| TyKind::Ref(_, _, mutability) => Some(mutability), | ||
| _ => None, | ||
| }) | ||
| .collect(), | ||
| TyKind::Ref(_, _, mutability) => vec![mutability], | ||
| _ => return, | ||
| }; | ||
|
|
||
| // collect all query filters | ||
| let query_filters = if let TyKind::Tuple(inner) = query_filters.kind() { | ||
| inner.iter().collect() | ||
| } else { | ||
| vec![query_filters] | ||
| }; | ||
|
|
||
| // Check for `With<Camera>` filter on a mutable query | ||
| for query_filter in query_filters { | ||
| // Check if the `With` `QueryFilter` was used. | ||
| if match_type(cx, query_filter, &crate::paths::WITH) | ||
| // Get the generic argument of the Filter | ||
| && let TyKind::Adt(_, with_args) = query_filter.kind() | ||
| // There can only be exactly one argument | ||
| && let Some(filter_component_arg) = with_args.iter().next() | ||
| // Get the type of the component the filter should filter for | ||
| && let GenericArgKind::Type(filter_component_ty) = | ||
| filter_component_arg.unpack() | ||
| // Check if Filter is of type `Camera` | ||
| && match_type(cx, filter_component_ty, &crate::paths::CAMERA) | ||
| // Emit lint if any `Camera` component is mutably borrowed | ||
| && query_data_mutability.iter().any(|mutability|match mutability { | ||
| rustc_ast::Mutability::Not => false, | ||
| rustc_ast::Mutability::Mut => true, | ||
| }) | ||
| { | ||
| span_lint_and_help( | ||
| cx, | ||
| CAMERA_MODIFICATION_IN_FIXED_UPDATE, | ||
| path.span, | ||
| CAMERA_MODIFICATION_IN_FIXED_UPDATE.desc, | ||
| None, | ||
| "insert the system in the `Update` schedule instead", | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| #![feature(register_tool)] | ||
| #![register_tool(bevy)] | ||
| #![deny(bevy::camera_modification_in_fixed_update)] | ||
|
|
||
| use bevy::prelude::*; | ||
|
|
||
| #[derive(Component)] | ||
| struct Hp; | ||
|
|
||
| #[derive(Component)] | ||
| struct Player; | ||
|
|
||
| fn main() { | ||
| App::new() | ||
| .add_plugins(DefaultPlugins) | ||
| .add_systems(Startup, spawn_camera) | ||
| // This should not raise an error since its in `Startup` | ||
| .add_systems(Startup, move_camera) | ||
| .add_systems(FixedUpdate, move_camera) | ||
| //~^ ERROR: camera modified in the `FixedUpdate` schedule | ||
| //~| HELP: insert the system in the `Update` schedule instead | ||
| .add_systems(FixedUpdate, move_camera_tuple_data) | ||
| //~^ ERROR: camera modified in the `FixedUpdate` schedule | ||
| //~| HELP: insert the system in the `Update` schedule instead | ||
| // This should not raise an error since its in not mutably borrowing any data | ||
| .add_systems(FixedUpdate, dont_mut_camera) | ||
| //~| ERROR: camera modified in the `FixedUpdate` schedule | ||
| //~v HELP: insert the system in the `Update` schedule instead | ||
| .add_systems(FixedUpdate, (move_camera, move_camera_tuple_data)) | ||
| //~^ ERROR: camera modified in the `FixedUpdate` schedule | ||
| //~| HELP: insert the system in the `Update` schedule instead | ||
| .add_systems(FixedUpdate, multiple_queries) | ||
| //~^ ERROR: camera modified in the `FixedUpdate` schedule | ||
| //~| HELP: insert the system in the `Update` schedule instead | ||
| .add_systems(FixedUpdate, multiple_none_mut_queries) | ||
| .add_systems(FixedUpdate, multiple_query_filters) | ||
| //~^ ERROR: camera modified in the `FixedUpdate` schedule | ||
| //~| HELP: insert the system in the `Update` schedule instead | ||
| .add_systems(FixedUpdate, multiple_none_mut_query_filters) | ||
| .run(); | ||
| } | ||
|
|
||
| fn spawn_camera(mut commands: Commands) { | ||
| commands.spawn((Name::new("Camera"), Camera::default())); | ||
| } | ||
|
|
||
| fn move_camera(mut _query: Query<&mut Transform, With<Camera>>) {} | ||
|
|
||
| fn dont_mut_camera(_query: Query<&Transform, With<Camera>>) {} | ||
|
|
||
| fn move_camera_tuple_data( | ||
| mut _query: Query<(&mut Transform, &Hp, Entity), With<Camera>>, | ||
| mut _commands: Commands, | ||
| _time: Res<Time>, | ||
| ) { | ||
| } | ||
|
|
||
| fn multiple_queries( | ||
| mut _query: Query<(&mut Transform, Entity), With<Camera>>, | ||
| mut _query2: Query<(&mut Hp, Entity), With<Player>>, | ||
| ) { | ||
| } | ||
|
|
||
| fn multiple_none_mut_queries( | ||
| _query: Query<(&Transform, Entity), With<Camera>>, | ||
| mut _query2: Query<(&mut Hp, Entity), With<Player>>, | ||
| ) { | ||
| } | ||
|
|
||
| fn multiple_query_filters( | ||
| mut _query: Query<(&mut Transform, Entity), (With<Camera>, Without<Player>)>, | ||
| ) { | ||
| } | ||
|
|
||
| fn multiple_none_mut_query_filters( | ||
| _query: Query<(&Transform, Entity), (With<Camera>, Without<Player>)>, | ||
| ) { | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| error: camera modified in the `FixedUpdate` schedule | ||
| --> tests/ui/camera_modification_in_fixed_update/main.rs:36:35 | ||
| | | ||
| 36 | .add_systems(FixedUpdate, multiple_query_filters) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: insert the system in the `Update` schedule instead | ||
| note: the lint level is defined here | ||
| --> tests/ui/camera_modification_in_fixed_update/main.rs:3:9 | ||
| | | ||
| 3 | #![deny(bevy::camera_modification_in_fixed_update)] | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| error: camera modified in the `FixedUpdate` schedule | ||
| --> tests/ui/camera_modification_in_fixed_update/main.rs:32:35 | ||
| | | ||
| 32 | .add_systems(FixedUpdate, multiple_queries) | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: insert the system in the `Update` schedule instead | ||
|
|
||
| error: camera modified in the `FixedUpdate` schedule | ||
| --> tests/ui/camera_modification_in_fixed_update/main.rs:29:36 | ||
| | | ||
| 29 | .add_systems(FixedUpdate, (move_camera, move_camera_tuple_data)) | ||
| | ^^^^^^^^^^^ | ||
| | | ||
| = help: insert the system in the `Update` schedule instead | ||
|
|
||
| error: camera modified in the `FixedUpdate` schedule | ||
| --> tests/ui/camera_modification_in_fixed_update/main.rs:29:49 | ||
| | | ||
| 29 | .add_systems(FixedUpdate, (move_camera, move_camera_tuple_data)) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: insert the system in the `Update` schedule instead | ||
|
|
||
| error: camera modified in the `FixedUpdate` schedule | ||
| --> tests/ui/camera_modification_in_fixed_update/main.rs:22:35 | ||
| | | ||
| 22 | .add_systems(FixedUpdate, move_camera_tuple_data) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: insert the system in the `Update` schedule instead | ||
|
|
||
| error: camera modified in the `FixedUpdate` schedule | ||
| --> tests/ui/camera_modification_in_fixed_update/main.rs:19:35 | ||
| | | ||
| 19 | .add_systems(FixedUpdate, move_camera) | ||
| | ^^^^^^^^^^^ | ||
| | | ||
| = help: insert the system in the `Update` schedule instead | ||
|
|
||
| error: aborting due to 6 previous errors | ||
|
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.