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

[chescock]

Objective

When building a system from SystemParamBuilders and defining the system as a closure, the compiler should be able to infer the parameter types from the builder types.

Solution

Create methods for each arity that take an argument that implements both SystemParamFunction as well as FnMut(SystemParamItem<P>,...). The explicit FnMut constraint will allow the compiler to infer the necessary higher-ranked lifetimes along with the parameter types.

I wanted to show that this was possible, but I can't tell whether it's worth the complexity. It requires a separate method for each arity, which pollutes the docs a bit:

Example

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

[alice-i-cecile]

Good to see that there's a test and excellent internal docs, thank you.

[cBournhonesque]

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.

[chescock]

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.

[cBournhonesque]

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

[SkiFire13]

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

[alice-i-cecile]

Oh BTW, before I merge this, do you want to try the fake_variadic trick here and see if this resolves the docs regression?

[chescock]

do you want to try the fake_variadic trick here and see if this resolves the docs regression?

I'm happy to try it! I don't expect it to work, though. I think that only works for trait impls, and this is a bunch of impl blocks.

How do I test that? Just running cargo doc doesn't seem to get me any collapsed impls, even for traits where I see them collapsed on dev-docs.bevyengine.org.

[alice-i-cecile]

@bash, the author of #14703 may have advice for you there.

[alice-i-cecile]

Regardless, I'll merge this in as-is once merge conflicts are resolved :)

[bash]

How do I test that? Just running cargo doc doesn't seem to get me any collapsed impls, even for traits where I see them collapsed on dev-docs.bevyengine.org.

You can build the docs with fake_variadic enabled by using the nightly compiler and custom RUSTFLAGS / RUSTDOCFLAGS (See https://github.com/bevyengine/bevy/blob/main/.github/workflows/docs.yml#L59).

This should do the trick:

RUSTFLAGS='--cfg docsrs_dep' RUSTDOCFLAGS='--cfg=docsrs' cargo +nightly doc

If you don't want to remember the correct incantation there's a handy cargo-docs-rs tool that pulls the right flags directly from Cargo.toml.

[alice-i-cecile]

Let me know if you want to experiment more, or if I should merge this as is. If I don't hear back I'll merge this on Monday :)

[chescock]

Nope, putting the attribute on the impl block fails with

error: `#[doc(fake_variadic)]` must be used on the first of a set of tuple or fn pointer trait impls with varying arity

so I don't think it will work for this.

(And the reason I had trouble before is that I forgot how to set environment variables in PowerShell :).)

I can make it work for impl<P: SystemParam, B: SystemParamBuilder<P>> SystemParamBuilder<(P₁, P₂, …, Pₙ)> for (B₁, B₂, …, Bₙ), though, which got missed because the SystemParamBuilder and fake_variadic PRs were open at the same time. I'll push up a separate PR for that!