Skip to content

SystemParamBuilder - Enable type inference of closure parameter when building dynamic systems #14820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions crates/bevy_ecs/src/system/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,21 @@ mod tests {
assert_eq!(result, 3);
}

#[test]
fn multi_param_builder_inference() {
let mut world = World::new();

world.spawn(A);
world.spawn_empty();

let system = (LocalBuilder(0u64), ParamBuilder::local::<u64>())
.build_state(&mut world)
.build_system(|a, b| *a + *b + 1);

let result = world.run_system_once(system);
assert_eq!(result, 1);
}

#[test]
fn param_set_builder() {
let mut world = World::new();
Expand Down
50 changes: 49 additions & 1 deletion crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,52 @@ pub struct SystemState<Param: SystemParam + 'static> {
archetype_generation: ArchetypeGeneration,
}

// Allow closure arguments to be inferred.
// For a closure to be used as a `SystemParamFunction`, it needs to be generic in any `'w` or `'s` lifetimes.
// Rust will only infer a closure to be generic over lifetimes if it's passed to a function with a Fn constraint.
// So, generate a function for each arity with an explicit `FnMut` constraint to enable higher-order lifetimes,
// along with a regular `SystemParamFunction` constraint to allow the system to be built.
macro_rules! impl_build_system {
($($param: ident),*) => {
impl<$($param: SystemParam),*> SystemState<($($param,)*)> {
/// Create a [`FunctionSystem`] from a [`SystemState`].
/// This method signature allows type inference of closure parameters for a system with no input.
/// You can use [`SystemState::build_system_with_input()`] if you have input, or [`SystemState::build_any_system()`] if you don't need type inference.
pub fn build_system<
Out: 'static,
Marker,
F: FnMut($(SystemParamItem<$param>),*) -> Out
+ SystemParamFunction<Marker, Param = ($($param,)*), In = (), Out = Out>
>
(
self,
func: F,
) -> FunctionSystem<Marker, F>
{
self.build_any_system(func)
}

/// Create a [`FunctionSystem`] from a [`SystemState`].
/// This method signature allows type inference of closure parameters for a system with input.
/// You can use [`SystemState::build_system()`] if you have no input, or [`SystemState::build_any_system()`] if you don't need type inference.
pub fn build_system_with_input<
Input,
Out: 'static,
Marker,
F: FnMut(In<Input>, $(SystemParamItem<$param>),*) -> Out
+ SystemParamFunction<Marker, Param = ($($param,)*), In = Input, Out = Out>,
>(
self,
func: F,
) -> FunctionSystem<Marker, F> {
self.build_any_system(func)
}
}
}
}

all_tuples!(impl_build_system, 0, 16, P);

impl<Param: SystemParam> SystemState<Param> {
/// Creates a new [`SystemState`] with default state.
///
Expand Down Expand Up @@ -242,7 +288,9 @@ impl<Param: SystemParam> SystemState<Param> {
}

/// Create a [`FunctionSystem`] from a [`SystemState`].
pub fn build_system<Marker, F: SystemParamFunction<Marker, Param = Param>>(
/// This method signature allows any system function, but the compiler will not perform type inference on closure parameters.
/// You can use [`SystemState::build_system()`] or [`SystemState::build_system_with_input()`] to get type inference on parameters.
pub fn build_any_system<Marker, F: SystemParamFunction<Marker, Param = Param>>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're implementing build_system for a FnMut<Param1, Param2, ...>
which calls build_any_system, which is implemented for SystemParamFunction
and SystemParamFunction is implemented for all FnMut<Param1, Param2...>

I get your overall logic to add type inference, but would there be a way to get rid of SystemParamFunction altogether and just use macros everywhere to deal directly with FnMut<Param1, Param2...>?
It seems weird to have this extra indirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SystemParamFunction is the existing trait that powers FunctionSystem. So I can't get rid of that!

I agree it's weird, but given that we need a SystemParamFunction to make a system, I think this is the simplest way to make parameter type inference work in the current rust compiler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like someone like @SkiFire13 would have a better opinion than mine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that getting rid of SystemParamFunction is likely not an option, as it power all the other system trait machinery. The duplicated FnMut bounds on these methods do feel a bit unfortunate given that SystemParamFunction's impls already have them (and twice!). However I don't think you can avoid them, as closure type inference is very finicky and requires a direct FnMut bound on the function that the closure is passed to, which in this case is build_system. So in the end:

  • SystemParamFunction is needed for the existing system machinery (e.g. App::add_systems)
  • the FnMut bounds here are needed because closure inference won't look at them if they are anywhere else

self,
func: F,
) -> FunctionSystem<Marker, F> {
Expand Down