Skip to content

Commit

Permalink
Make derived SystemParam readonly if possible (#4650)
Browse files Browse the repository at this point in the history
Required for #4402.

# Objective

- derived `SystemParam` implementations were never `ReadOnlySystemParamFetch`
- We want them to be, e.g. for `EventReader`

## Solution

- If possible, 'forward' the impl of `ReadOnlySystemParamFetch`.
  • Loading branch information
DJMcNab committed May 9, 2022
1 parent 96078c7 commit 583175b
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 1 deletion.
5 changes: 4 additions & 1 deletion crates/bevy_ecs/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}),
Expand Down Expand Up @@ -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<TSystemParamState: #path::system::SystemParamState + #path::system::ReadOnlySystemParamFetch, #punctuated_generics> #path::system::ReadOnlySystemParamFetch for FetchState <TSystemParamState, #punctuated_generic_idents> #where_clause {}
};
})
}
Expand Down
14 changes: 14 additions & 0 deletions crates/bevy_ecs/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,8 @@ impl<E: Event> std::iter::Extend<E> for Events<E> {

#[cfg(test)]
mod tests {
use crate::{prelude::World, system::SystemState};

use super::*;

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
Expand Down Expand Up @@ -753,4 +755,16 @@ mod tests {
vec![EmptyTestEvent::default()]
);
}

#[test]
fn ensure_reader_readonly() {
fn read_for<E: Resource>() {
let mut world = World::new();
world.init_resource::<Events<E>>();
let mut state = SystemState::<EventReader<E>>::new(&mut world);
// This can only work if EventReader only reads the world
let _reader = state.get(&world);
}
read_for::<EmptyTestEvent>();
}
}
Original file line number Diff line number Diff line change
@@ -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::<Mutable>::new(&mut world);
// state.get(&world);
// But that makes the test show absolute paths
assert_readonly::<Mutable>();
}

fn assert_readonly<P: SystemParam>()
where
<P as SystemParam>::Fetch: ReadOnlySystemParamFetch,
{
}
Original file line number Diff line number Diff line change
@@ -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::<Mutable>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ 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<P: SystemParam>()
| --------------- required by a bound in this
22 | where
23 | <P as SystemParam>::Fetch: ReadOnlySystemParamFetch,
| ^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `assert_readonly`

0 comments on commit 583175b

Please sign in to comment.