From b96cc0dfa7d6138016e36d722ba301d86b41d48a Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 3 May 2022 21:49:50 +0100 Subject: [PATCH 1/3] Make SystemParam readonly if possible --- crates/bevy_ecs/macros/src/lib.rs | 5 ++++- crates/bevy_ecs/src/event.rs | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 0f57f448a18e7..3273faf9ab1a2 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -324,7 +324,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { } Ok(()) }) - .expect("Invalid 'render_resources' attribute format."); + .expect("Invalid 'system_param' attribute format."); attributes }), @@ -420,6 +420,9 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { } } } + + // Safety: The `ParamState` is `ReadOnlySystemParamFetch`, so this can only read from the `World` + unsafe impl #path::system::ReadOnlySystemParamFetch for FetchState #where_clause {} }; }) } diff --git a/crates/bevy_ecs/src/event.rs b/crates/bevy_ecs/src/event.rs index b28e036c4060c..0ed437deedd09 100644 --- a/crates/bevy_ecs/src/event.rs +++ b/crates/bevy_ecs/src/event.rs @@ -513,6 +513,8 @@ impl std::iter::Extend for Events { #[cfg(test)] mod tests { + use crate::{prelude::World, system::SystemState}; + use super::*; #[derive(Copy, Clone, PartialEq, Eq, Debug)] @@ -753,4 +755,16 @@ mod tests { vec![EmptyTestEvent::default()] ); } + + #[test] + fn ensure_reader_readonly() { + fn read_for() { + let mut world = World::new(); + world.init_resource::>(); + let mut state = SystemState::>::new(&mut world); + // This can only work if EventReader only reads the world + let _reader = state.get(&world); + } + read_for::(); + } } From ba0fdbccb8c6ad412636d04418aef1eb07652230 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Sat, 7 May 2022 15:15:37 +0100 Subject: [PATCH 2/3] Add a compile_fail test --- .../tests/ui/system_param_derive_readonly.rs | 25 +++++++++++++++++++ .../ui/system_param_derive_readonly.stderr | 25 +++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.rs create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.stderr diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.rs new file mode 100644 index 0000000000000..048a1557693f8 --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.rs @@ -0,0 +1,25 @@ +use bevy_ecs::prelude::*; +use bevy_ecs::system::{ReadOnlySystemParamFetch, SystemParam, SystemState}; + +#[derive(Component)] +struct Foo; + +#[derive(SystemParam)] +struct Mutable<'w, 's> { + a: Query<'w, 's, &'static mut Foo>, +} + +fn main() { + // Ideally we'd use: + // let mut world = World::default(); + // let state = SystemState::::new(&mut world); + // state.get(&world); + // But that makes the test show absolute paths + assert_readonly::(); +} + +fn assert_readonly() +where +

::Fetch: ReadOnlySystemParamFetch, +{ +} diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.stderr new file mode 100644 index 0000000000000..870e2fb3131c5 --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.stderr @@ -0,0 +1,25 @@ +warning: unused import: `SystemState` + --> tests/ui/system_param_derive_readonly.rs:2:63 + | +2 | use bevy_ecs::system::{ReadOnlySystemParamFetch, SystemParam, SystemState}; + | ^^^^^^^^^^^ + | + = note: `#[warn(unused_imports)]` on by default + +error[E0277]: the trait bound `for<'x> WriteFetch<'x, Foo>: ReadOnlyFetch` is not satisfied + --> tests/ui/system_param_derive_readonly.rs:18:5 + | +18 | assert_readonly::(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `for<'x> ReadOnlyFetch` is not implemented for `WriteFetch<'x, Foo>` + | + = note: required because of the requirements on the impl of `ReadOnlySystemParamFetch` for `QueryState<&'static mut Foo>` + = note: 2 redundant requirements hidden + = note: required because of the requirements on the impl of `ReadOnlySystemParamFetch` for `_::FetchState<(QueryState<&'static mut Foo>,)>` +note: required by a bound in `assert_readonly` + --> tests/ui/system_param_derive_readonly.rs:23:32 + | +21 | fn assert_readonly() + | --------------- required by a bound in this +22 | where +23 |

::Fetch: ReadOnlySystemParamFetch, + | ^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `assert_readonly` From 73c11fcf79577ada69395d660cbcfd6aad358f5a Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Mon, 9 May 2022 15:00:29 +0100 Subject: [PATCH 3/3] Update to use `Event` trait --- crates/bevy_ecs/src/event.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/event.rs b/crates/bevy_ecs/src/event.rs index 0ed437deedd09..0a4a459073a5d 100644 --- a/crates/bevy_ecs/src/event.rs +++ b/crates/bevy_ecs/src/event.rs @@ -758,7 +758,7 @@ mod tests { #[test] fn ensure_reader_readonly() { - fn read_for() { + fn read_for() { let mut world = World::new(); world.init_resource::>(); let mut state = SystemState::>::new(&mut world);