[Merged by Bors] - Remove Sync bound from Command

[Demiu]

Unless I'm mistaken it is unnecessary, Commands are never accessed from two threads simultaneously. It unnecessarily restricts Command structs

[alice-i-cecile]

It compiles! This lines up with my mental model of commands: they're dispatched across threads to various systems and collected back from them (and so need a Send bound), but there's no time where a Sync bound would be required.

@DJMcNab if you're in agreement I'm happy to merge this right away.

[DJMcNab]

I'd need to do an audit of CommandQueue to be sure.

iirc it unsafely implements Send and Sync, referencing this impl? I think SystemParam's state need to be Send/Sync, so this compiling seems suspect to me without any actual checks.

For example, this compiling without the Send bound would definitely be wrong, but I somewhat suspect it might do. We probably should have a safety comment here that that supertrait is required for soundness.

Also, I thought there was already another pr which did this, although I don't have a reference to hand. It might have started, but the plumbing required to actually do it correctly overwhelmed the author - not sure?

Always happy to tidy up our send/sync requirements, but I need to check this is actually correct.

[alice-i-cecile]

Also, I thought there was already another pr which did this, although I don't have a reference to hand. It might have started, but the plumbing required to actually do it correctly overwhelmed the author - not sure?

A quick skim of the PRs didn't show anything for me, and I don't remember this.

It appears that the SAFETY comments of CommandQueue here are still valid: https://github.com/bevyengine/bevy/blob/d38a8dfdd79f6fd8e1e915aa5e2ddb03ffdaa76a/crates/bevy_ecs/src/system/commands/command_queue.rs

Definitely needs a close look though.

[Demiu]

I did look into this beforehand and didn't just remove Sync.
The CommandQueue doesn't make it's contents pub and the only 2 associated functions it has take mutable reference, so I do not think there is a way to mutate anything behind a shared reference to a queue. CommandQueue::apply just goes through commands in a queue sequentially on a single thread. Edit: A Command can't be run on two threads simultaneously because the only function it has a) takes &mut World b) consumes self.
Yes, you technically can also remove Send bound on Command, which is wrong, but it will still compile because CommandQueue erases the type of a command and stores them as just bytes.
And since this type erasure happens the unsafe impls for Send and Sync on CommandQueue are superfluous because to the compiler all of it's elements are Send + Sync.

[JoJoJet]

I like it

[cart]

Yeah this aligns with my view of the world

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12