Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unsound SystemParam impl for DeferredWorld #17609

Closed
wants to merge 2 commits into from

Conversation

ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Jan 30, 2025

Objective

The following test currently passes (i.e. no panic) on main:

#[test]
fn deferred_world_last() {
    #[derive(Resource)]
    struct FooRes;

    fn bad_system(foo: ResMut<FooRes>, mut world: DeferredWorld) {
        let foo = foo.into_inner();
        let foo2 = world.get_resource_mut::<FooRes>().unwrap().into_inner();
    }

    let mut world = World::new();
    world.insert_resource(FooRes);
    let mut bad_system = IntoSystem::into_system(bad_system);
    bad_system.initialize(&mut world);
    bad_system.run((), &mut world);
}

Solution

Remove the unsound SystemParam impl for DeferredWorld.


Migration Guide

The SystemParam implementation for DeferredWorld was removed due to unsoundness. Consider using Commands instead.

@ItsDoot ItsDoot added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jan 30, 2025
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 30, 2025
@alice-i-cecile alice-i-cecile requested a review from hymm January 30, 2025 04:56
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jan 30, 2025
@JeanMertz
Copy link
Contributor

Does this mean DeferredWorld can no longer be used in regular systems?

@chescock
Copy link
Contributor

I think the only thing we need to do to make it sound is to check for existing access like

if system_meta.component_access_set.has_any_component_read()
    || system_meta.component_access_set.has_any_resource_read()
{
    panic!("Useful message")
}

Oh, and

system_meta.archetype_component_access.write_all();

Could we do that instead?

@hymm
Copy link
Contributor

hymm commented Jan 30, 2025

I think the only thing we need to do to make it sound is to check for existing access like

DeferredWorld pushes into the World's command queue, not one specific to the system and that requires exclusive access to the world.

@chescock
Copy link
Contributor

Well, it was a little more complicated than I expected, but I created a competing PR that tries to leave the impl in place but make it sound: #17616

@chescock
Copy link
Contributor

DeferredWorld pushes into the World's command queue, not one specific to the system and that requires exclusive access to the world.

Oh! I missed that. It might still be sound, though. If we declare that accessing the World's command queue requires write-all-components access, then no more than one system can claim it.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 30, 2025
@alice-i-cecile
Copy link
Member

On second thought, I'd prefer fixing this in 0.16 cycle.

@hymm
Copy link
Contributor

hymm commented Jan 30, 2025

Oh! I missed that. It might still be sound, though. If we declare that accessing the World's command queue requires write-all-components access, then no more than one system can claim it.

It might be sound, but when do the command queue get applied? I don't think there's a determined place when they ever will get applied. Just whenever the world's queue happen's to get applied. Fixing this properly would be to somehow pass a system stored command queue to the constructor of DeferredWorld.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My pref here is to remove this for now and it can just be readded once an appropriate pr shows up.

@hymm hymm added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 30, 2025
@alice-i-cecile
Copy link
Member

Closing in favor of #17616.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants