Cleanup observer system input: Trigger reborrowing

[ItsDoot]

Objective

Simplify the safety story of On and Trigger, and reduce the number of lifetimes on On from two to one.

This is a cleanup pass in preparation to support observer target queries, On<'world, 'state, 'input, E: EventPattern, D: QueryData = (), F: QueryFilter = ()>.

Solution

Add and require support for Trigger reborrowing:

• Removed the lifetime from Event::Trigger
  • That lets us add the E: Event<Trigger = Self> requirement directly to Trigger's trait, removing the unsafe requirement for it.
• Added Trigger::State<'a>, which is the type that's passed to World::trigger and Commands::trigger
• Added Trigger::View<'a>, which is the type passed into observer systems as a mutable view of the Trigger::State
  • This type currently only differs from State for PropagateEntityTrigger, in order to provide access to a propagate: &mut bool
• Added Trigger::reborrow to convert from a &'a mut Trigger::State<'_> to a Trigger::View<'a> (owned)
• Added EventTriggerState and EventTriggerView type aliases for ease of access (replacing EventPatternTrigger)

Testing

No new tests were added, suggestions welcome.

[SkiFire13]

I can't review the PR right now, but I want to note that reinterpreting State<'long> as State<'short> also means that it cannot expose a way to get &mut Something<'short> (which includes public fields of type Something<'short>

I cannot check what this PR does, but that was the original issue and reason why On/Trigger had this lifetime hack.

(Also sorry for commenting only on this line, Github on mobile is terrible)

[chescock]

I can't review the PR right now, but I want to note that reinterpreting State<'long> as State<'short> also means that it cannot expose a way to get &mut Something<'short> (which includes public fields of type Something<'short>

I cannot check what this PR does, but that was the original issue and reason why On/Trigger had this lifetime hack.

I think the fn reborrow() method having the extra lifetime parameter in Self::State<'_> is what addresses this issue. Reviewing your comments at #20731 (comment), you said:

Here however we're casting it to a &mut E::Trigger<'w> for some arbitrary lifetime 'w. This is fine only as long as nothing about the lifetime 'w that 's different from 'a is ever exposed to safe code, effectively forcing the safe code to be universally quantifed over a set of lifetimes that must include the original 'a.

I think that extra lifetime parameter gives us that universal quantification. The clever part is that the parameter is on a trait method on Trigger now, so it doesn't leak into On!

[SkiFire13]

I've been looking at the PR a bit more and I think it should be good (don't quote me on that yet!) but I think the motivation is not quite right.

I think what makes it safe is that On has only one lifetime thanks to reborrowing, and the trigger ref also has only one lifetime thanks to reborrowing.

I don't see a reason for requiring the State to be transmutable though. The reborrowing is done by reborrow, there seems to be no transmute going on.

[cart]

While this does simplify the lifetimes in the top level API, it also complicates the "type story" for triggers generally. I don't fully understand the "trigger target queries" stuff. Do you have a proposal link and/or a practical example of the API you're aiming for?

[chescock]

I like all the places where we got to get rid of for<'a>!

[chescock]

I can't review the PR right now, but I want to note that reinterpreting State<'long> as State<'short> also means that it cannot expose a way to get &mut Something<'short> (which includes public fields of type Something<'short>

I cannot check what this PR does, but that was the original issue and reason why On/Trigger had this lifetime hack.

I think the fn reborrow() method having the extra lifetime parameter in Self::State<'_> is what addresses this issue. Reviewing your comments at #20731 (comment), you said:

Here however we're casting it to a &mut E::Trigger<'w> for some arbitrary lifetime 'w. This is fine only as long as nothing about the lifetime 'w that 's different from 'a is ever exposed to safe code, effectively forcing the safe code to be universally quantifed over a set of lifetimes that must include the original 'a.

I think that extra lifetime parameter gives us that universal quantification. The clever part is that the parameter is on a trait method on Trigger now, so it doesn't leak into On!

[ItsDoot]

Updated based on feedback; I think it's much clearer now.

[chescock]

Yeah, this looks nice and clear!

There is a lot of type complexity, but I think the complexity is fundamental to allowing both mutable state in PropagateEntityTrigger::propagate and borrowed data in EntityComponentsTrigger, and this hides it away from the user a bit better than all the HRTBs, while also needing less unsafe.

[chescock]

Nit, but it might be more clear to use the Self alias for these associated types where possible (so, every impl but EntityComponentsTrigger).

Suggested change

	type State<'input> = PropagateEntityTrigger<AUTO_PROPAGATE, E, T>;
	type View<'input> = &'input mut PropagateEntityTrigger<AUTO_PROPAGATE, E, T>;
	type State<'input> = Self;
	type View<'input> = &'input mut Self;

[Trashtalk217]

It all looks good and it's a nice little bit of cleanup of the lifetime soup.