Implement System::system_metas(&self) and System::system_metas_mut(&mut self) for system exploration and configuration

[Rajveer100]

Part of #16168

Objective

Implement System::system_metas(&self) and System::system_metas_mut(&mut self) for system exploration and configuration.

Solution

Introduce System::system_metas(&self) and System::system_metas_mut(&mut self) which would return iterator/vector/slice that we can work with.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[Rajveer100]

@alice-i-cecile
I am not quite sure if this is in the right direction.

[alice-i-cecile]

@MiniaczQ, opinions?

[alice-i-cecile]

In all of the cases other than for piped systems, this method should return a vector with one element :)

[MiniaczQ]

Yeah, I didn't want to tackle this, because it's actually pretty technical.
I'd say the actual scope of this issue can be split into 3 steps.
If you can get the first one down, the rest is relatively easy as follow-up PRs.

Step 1

We definitely need a trait that has system_metas(&self) -> Vec<&SystemMeta> and system_metas_mut(&mut self) -> Vec<&mut SystemMeta>.
Implement it for all of the system types, sort of like what you did, just as a trait instead.

Now, to make this actually work, we need another set of methods (in that trait): extend_with_system_metas(&self, &mut Vec<&SystemMeta>) and extend_with_system_metas_mut(&mut self, &mut Vec<&mut SystemMeta>).
The original two methods will create a new Vec and fill it using the new methods.
Non-primitive systems like adapters and combinators will call the new methods on their sub-systems, while primitive systems will just append their own meta.
I'm not sure if the borrow checker will freak out here or not with the &mut.

Step 2

Then there is the matter of in-lining it.
This is not very appealing:

let mut my_system = IntoSystem::into_system(my_system);
my_system.system_metas_mut()[0].name = "abba".into();
app.add_systems(Update, my_system);

so we want a map_system_metas method (again, in the trait) that can do the same thing inline.

app.add_systems(Update, my_system.map_system_metas(|metas| { metas[0].name = "abba".into(); }));

no need for immutable variant.

Step 3

Sadly this isn't the end for this issue, we also should expose SystemMetas for schedule-configuration wrappers as well.
So implementing aforementioned trait on I believe SystemConfig and SystemSetConfig, I'd need to look it up to be sure.
This would allow us to do my_system.after(other_system).map_system_metas(...).

[Rajveer100]

Thanks for the detailed overview :)

[Rajveer100]

@MiniaczQ
Let me know if the latest changes is the intended way. I think only the non-primitive systems remain:

• AdapterSystems
• CombinatorSystems
• PipeSystems

[MiniaczQ]

Looks good!
Looking through CI there seem to be no issues with borrow checker so far, hopefully it stays that way 🤞

[Rajveer100]

Done with non-primitive systems, if this looks fine we can work with the next steps.

[Rajveer100]

@alice-i-cecile @MiniaczQ
Got back to this after a long time, review needed :)

CI looks good.

[chescock]

Welcome back!

I'm worried the use case for this change may have disappeared in the meantime, though. The linked issue says this is "needed for system param warning, which currently can be configured only for function systems", but system param validation has been reworked since then, with ParamWarnPolicy removed in #18454.

Following that and #19477, all SystemMeta has is name, last_run, and flags. They can all be read using the System trait, last_run can be set, and the flags aren't safe to modify, so the only API that's really missing is set_name().

Do we really still need this API?

[Rajveer100]

At a first glance, looks like this may not be required, but do we have the same way of direct access this API provides?

It's more like a user's choice which allows one to get a deeper access. I can't think of a use case that this especially provides at the moment, but I feel if we introduce this and let users sort of play with it in the next release we can get a better idea.

If not, we could always remove it?

[Rajveer100]

@chescock
Any final thoughts?

[chescock]

@chescock Any final thoughts?

Whoops, sorry, missed this message for a while!

At a first glance, looks like this may not be required, but do we have the same way of direct access this API provides?

I think my argument here is that we do! Looking at the fields on SystemMeta, nearly all of them are exposed on the System trait already. The only exception is being able to set the name ... but combinator systems store their name separately from the inner SystemMeta, so setting the name on those won't change the system name anyway.

But I'm also just a random reviewer and not a SME or maintainer :). If you can get some more consensus that this is a good direction then I'm happy to give it a proper review! I just don't want to waste your time nitpicking the implementation if we aren't going to do this anyway.

[Rajveer100]

Cool, thanks for the feedback, maybe in the future if there's a critical discussion in this area we can come back to it.

Looking into other issues :)