Repository: bevyengine/bevy
File: crates/bevy_ecs/src/system/combinator.rs (Rust)

Code before:
{
    type In = Func::In;
    type Out = Func::Out;

    fn name(&self) -> DebugName {
        self.name.clone()
    }

    #[inline]
    fn flags(&self) -> super::SystemStateFlags {
        self.a.flags() | self.b.flags()
    }

    unsafe fn run_unsafe(
        &mut self,
        input: SystemIn<'_, Self>,
        mut world: UnsafeWorldCell,
    ) -> Result<Self::Out, RunSystemError> {
        Func::combine(
            input,
            &mut world,
            // SAFETY: The world accesses for both underlying systems have been registered,
            // so the caller will guarantee that no other systems will conflict with `a` or `b`.
            // If either system has `is_exclusive()`, then the combined system also has `is_exclusive`.
            // Since we require a `combine` to pass in a mutable reference to `world` they can never be
            // called in parallel or re-entrantly, so their world accesses will not conflict with each other.
            |input, &mut world| unsafe { self.a.run_unsafe(input, world) },
            // `Self::validate_param_unsafe` already validated the first system,
            // but we still need to validate the second system once the first one runs.
            // SAFETY: See the comment above.
            |input, &mut world| unsafe {
                self.b.validate_param_unsafe(world)?;
                self.b.run_unsafe(input, world)
            },
        )
    }

    #[cfg(feature = "hotpatching")]
    #[inline]
    fn refresh_hotpatch(&mut self) {
        self.a.refresh_hotpatch();
        self.b.refresh_hotpatch();
    }

    #[inline]
    fn apply_deferred(&mut self, world: &mut World) {
        self.a.apply_deferred(world);
        self.b.apply_deferred(world);
    }

    #[inline]

Review comment (refactor):
I agree that this is sound, but I think the proof should mention that it relies on `combine` being generic in a non-`'static` `T`.  Otherwise a future developer might try to simplify `fn combine()` by by replacing the generic `T` with `UnsafeWorldCell` when they see that's the only type that is ever used.  

I'm not quite sure how to phrase that, though.  Maybe something like 

```suggestion
            // Since we require a `combine` to pass in a mutable reference to `world` they can never be
            // called in parallel or re-entrantly, so their world accesses will not conflict with each other.
            // A `fn combine` implementation cannot pass a different `UnsafeWorldCell` instance because
            // it is an unbound generic type.
```

?

... or maybe we actually want a comment on `fn combine`, since a developer changing the signature there might not even see the comment here.  

Code after:
{
    type In = Func::In;
    type Out = Func::Out;

    fn name(&self) -> DebugName {
        self.name.clone()
    }

    #[inline]
    fn flags(&self) -> super::SystemStateFlags {
        self.a.flags() | self.b.flags()
    }

    unsafe fn run_unsafe(
        &mut self,
        input: SystemIn<'_, Self>,
        world: UnsafeWorldCell,
    ) -> Result<Self::Out, RunSystemError> {
        struct PrivateUnsafeWorldCell<'w>(UnsafeWorldCell<'w>);

        Func::combine(
            input,
            &mut PrivateUnsafeWorldCell(world),
            // SAFETY: The world accesses for both underlying systems have been registered,
            // so the caller will guarantee that no other systems will conflict with `a` or `b`.
            // If either system has `is_exclusive()`, then the combined system also has `is_exclusive`.
            // Since we require a `combine` to pass in a mutable reference to `world` and that's a private type
            // passed to a function as an unbound non-'static generic argument, they can never be called in parallel
            // or re-entrantly because that would require forging another instance of `PrivateUnsafeWorldCell`.
            // This means that the world accesses in the two closures will not conflict with each other.
            |input, world| unsafe { self.a.run_unsafe(input, world.0) },
            // `Self::validate_param_unsafe` already validated the first system,
            // but we still need to validate the second system once the first one runs.
            // SAFETY: See the comment above.
            |input, world| unsafe {
                self.b.validate_param_unsafe(world.0)?;
                self.b.run_unsafe(input, world.0)
            },
        )
    }

    #[cfg(feature = "hotpatching")]
    #[inline]
    fn refresh_hotpatch(&mut self) {
        self.a.refresh_hotpatch();
        self.b.refresh_hotpatch();
    }

    #[inline]
    fn apply_deferred(&mut self, world: &mut World) {
        self.a.apply_deferred(world);