Cleanup observer system input: `Trigger` reborrowing by ItsDoot · Pull Request #25668 · bevyengine/bevy

ItsDoot · 2026-09-03T07:59:52Z

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 · 2026-09-03T17:07:50Z

-pub unsafe trait Trigger<E: Event> {
+/// For any `'long: 'short`, `State<'long>` must have the same layout as
+/// `State<'short>` and must be valid to reinterpret as `State<'short>` for
+/// the duration of the shorter borrow.


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)

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!

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 · 2026-09-04T05:39:45Z

-// - The implementation abides by the other safety constraints defined in [`Trigger`]
-unsafe impl<E: AnimationEvent + for<'a> Event<Trigger<'a> = AnimationEventTrigger>> Trigger<E>
+// SAFETY: `AnimationEventTrigger` has no lifetimes.
+unsafe impl<E: AnimationEvent + Event<Trigger = AnimationEventTrigger>> Trigger<E>


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 · 2026-09-03T23:54:35Z

+/// For any `'long: 'short`, `State<'long>` must have the same layout as
+/// `State<'short>` and must be valid to reinterpret as `State<'short>` for
+/// the duration of the shorter borrow.


I think you get this for free from parametricity, and don't need this to be a safety requirement. So Trigger can be a safe trait!

The only place the lifetime in the transmuted State<'_> is used is to call fn reborrow(). That's generic in the lifetime, and lifetime parameters are erased at runtime, so there is no way for it it depend on the particular lifetime. That is, you can declare that you are transmuting to the original 'long lifetime, since fn reborrow() has no way to tell it apart from any other lifetime.

The trouble with the old code was that On<...> as SystemInput had type Param<'i> = On<'i, 'i, E>, which wound up forcing that lifetime ('t) to equal a bunch of others ('w). Those other lifetimes should have been shorter, so code that relied on them being equal could be unsound. But this PR only uses the lifetime once, as a parameter to reborrow, so external code can't make any incorrect assumptions about it.

Unsafeness removed!

chescock · 2026-09-04T00:21:41Z

+    T: Traversal<E>,
 {
+    type State<'input> = PropagateEntityTrigger<AUTO_PROPAGATE, E, T>;
+    type View<'input> = PropagateEntityTriggerItem<'input, AUTO_PROPAGATE, E, T>;


This could alternately have been &'input mut PropagateEntityTrigger<AUTO_PROPAGATE, E, T> instead of a new type, right? That would have been slightly less breaking, since wouldn't need for the extra * when using trigger.propagate... although breaking the ability to modify original_event_target is probably for the best :).

Yep, replaced!

chescock · 2026-09-04T00:28:35Z

 ///
 /// This trait can be derived.
-pub trait AnimationEvent: Clone + for<'a> Event<Trigger<'a> = AnimationEventTrigger> {}
+pub trait AnimationEvent: Clone + Event<Trigger = AnimationEventTrigger> {}


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

chescock · 2026-09-04T00:36:27Z

-    pub fn trigger<'a, E: Event<Trigger<'a>: Default>>(&mut self, mut event: E) {
+    pub fn trigger<E: Event>(&mut self, mut event: E)
+    where
+        for<'a> EventTriggerState<'a, E>: Default,


I don't love the places where we had to add a for<'a>, though.

... or, wait, could this be

Suggested change

for<'a> EventTriggerState<'a, E>: Default,

EventTriggerState<'static, E>: Default,

here and elsewhere? The weaker constraint is still enough to call default(), and it avoids the for<'a>.

(I don't think it's even any less general than the old API. Event: 'static, so there aren't any lifetimes in E, which means any lifetime in an impl is either 'static or a free lifetime that can be substituted with it.)

Yep that worked across the board pretty much!

chescock · 2026-09-04T13:22:28Z

-pub unsafe trait Trigger<E: Event> {
+/// For any `'long: 'short`, `State<'long>` must have the same layout as
+/// `State<'short>` and must be valid to reinterpret as `State<'short>` for
+/// the duration of the shorter borrow.


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 · 2026-09-04T22:56:00Z

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

Trigger reborrowing

f1a6aa9

ItsDoot requested review from chescock and hymm September 3, 2026 08:00

github-project-automation Bot added this to ECS Sep 3, 2026

github-project-automation Bot moved this to Needs SME Triage in ECS Sep 3, 2026

fix example

16018f0

ItsDoot force-pushed the ecs/trigger-reborrow branch from 4664ee0 to 16018f0 Compare September 3, 2026 08:20

SkiFire13 reviewed Sep 3, 2026

View reviewed changes

cart reviewed Sep 4, 2026

View reviewed changes

chescock reviewed Sep 4, 2026

View reviewed changes

ItsDoot added 2 commits September 4, 2026 17:28

cleanup based on feedback

9265601

further cleanup

13b9461

	for<'a> EventTriggerState<'a, E>: Default,
	EventTriggerState<'static, E>: Default,

Uh oh!

Uh oh!

Conversation

ItsDoot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cart Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ItsDoot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ItsDoot commented Sep 3, 2026 •

edited

Loading

cart Sep 4, 2026 •

edited

Loading