Skip to content

Commit

Permalink
Check for conflicting accesses in assert_is_system (#8154)
Browse files Browse the repository at this point in the history
# Objective

The function `assert_is_system` is used in documentation tests to ensure
that example code actually produces valid systems. Currently,
`assert_is_system` just checks that each function parameter implements
`SystemParam`. To further check the validity of the system, we should
initialize the passed system so that it will be checked for conflicting
accesses. Not only does this enforce the validity of our examples, but
it provides a convenient way to demonstrate conflicting accesses via a
`should_panic` example, which is nicely rendered by rustdoc:

![should_panic
example](https://user-images.githubusercontent.com/21144246/226767682-d1c2f6b9-fc9c-4a4f-a4c4-c7f6070a115f.png)

## Solution

Initialize the system with an empty world to trigger its internal access
conflict checks.

---

## Changelog

The function `bevy::ecs::system::assert_is_system` now panics when
passed a system with conflicting world accesses, as does
`assert_is_read_only_system`.

## Migration Guide

The functions `assert_is_system` and `assert_is_read_only_system` (in
`bevy_ecs::system`) now panic if the passed system has invalid world
accesses. Any tests that called this function on a system with invalid
accesses will now fail. Either fix the system's conflicting accesses, or
specify that the test is meant to fail:

1. For regular tests (that is, functions annotated with `#[test]`), add
the `#[should_panic]` attribute to the function.
2. For documentation tests, add `should_panic` to the start of the code
block: ` ```should_panic`
  • Loading branch information
JoJoJet authored Mar 22, 2023
1 parent 47be369 commit daa1b02
Showing 1 changed file with 50 additions and 12 deletions.
62 changes: 50 additions & 12 deletions crates/bevy_ecs/src/system/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,33 +123,71 @@ pub use system::*;
pub use system_param::*;
pub use system_piping::*;

use crate::world::World;

/// Ensure that a given function is a [system](System).
///
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems.
pub fn assert_is_system<In, Out, Marker, S: IntoSystem<In, Out, Marker>>(sys: S) {
if false {
// Check it can be converted into a system
// TODO: This should ensure that the system has no conflicting system params
IntoSystem::into_system(sys);
}
///
/// # Examples
///
/// The following example will panic when run since the
/// system's parameters mutably access the same component
/// multiple times.
///
/// ```should_panic
/// # use bevy_ecs::{prelude::*, system::assert_is_system};
/// #
/// # #[derive(Component)]
/// # struct Transform;
/// #
/// fn my_system(query1: Query<&mut Transform>, query2: Query<&mut Transform>) {
/// // ...
/// }
///
/// assert_is_system(my_system);
/// ```
pub fn assert_is_system<In: 'static, Out: 'static, Marker>(
system: impl IntoSystem<In, Out, Marker>,
) {
let mut system = IntoSystem::into_system(system);

// Initialize the system, which will panic if the system has access conflicts.
let mut world = World::new();
system.initialize(&mut world);
}

/// Ensure that a given function is a [read-only system](ReadOnlySystem).
///
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems.
pub fn assert_is_read_only_system<In, Out, Marker, S: IntoSystem<In, Out, Marker>>(sys: S)
///
/// # Examples
///
/// The following example will fail to compile
/// since the system accesses a component mutably.
///
/// ```compile_fail
/// # use bevy_ecs::{prelude::*, system::assert_is_read_only_system};
/// #
/// # #[derive(Component)]
/// # struct Transform;
/// #
/// fn my_system(query: Query<&mut Transform>) {
/// // ...
/// }
///
/// assert_is_read_only_system(my_system);
/// ```
pub fn assert_is_read_only_system<In: 'static, Out: 'static, Marker, S>(system: S)
where
S: IntoSystem<In, Out, Marker>,
S::System: ReadOnlySystem,
{
if false {
// Check it can be converted into a system
// TODO: This should ensure that the system has no conflicting system params
IntoSystem::into_system(sys);
}
assert_is_system(system);
}

#[cfg(test)]
Expand Down

0 comments on commit daa1b02

Please sign in to comment.