Remove Sync bound on Component and Resource by james7132 · Pull Request #4680 · bevyengine/bevy

james7132 · 2022-05-06T20:15:59Z

Objective

Fixes #2487. Remove the Sync bound on Component and Resource to allow Send-only components.

Solution

Remove Sync bound from Component and Resource.
Address the compilation errors.
Changed the PhantomData<T> in FetchState and SystemParamState to use PhantomData<fn() -> T> to ensure they're Sync.
Add a compile_fail_test.
Add Sync bound on the WorldQuery impl for &T.
Add Sync bound on Res<T> and Option<Res<T>>
Add Sync bound on Query::get_component, World::get, EntityRef::get, and EntityMut::get.

Changelog

Removed: Sync bound on Component and Resource.

Migration Guide

Component and Resource no longer requires Sync. Any generic code using Component as a bound, &T as Query parameter, or Res<T> might require an additional Sync bound.

DJMcNab · 2022-05-06T20:20:29Z

It's worth noting that at the time #2487 was written, Component also meant Resource (because #[derive(Component)] hadn't landed).

It's actually more interesting and useful to do the same transformation for resources.

That being said, if we can get this incrementally added I'm not too unhappy.

crates/bevy_ecs/src/query/fetch.rs

DJMcNab · 2022-05-06T20:24:07Z

crates/bevy_ecs/src/system/commands/mod.rs


 /// A [`World`] mutation.
-pub trait Command: Send + Sync + 'static {
+pub trait Command: Send + 'static {


I think we'll also want to do the same transformation for Send at some point.

Note that doing that requires we keep proper main thread management - it requires a lot of care.

Is the Send + Sync necessary when the target is wasm?

Eventually we'll want Web Worker-based multithreading, so it will still be required.

crates/bevy_ecs_compile_fail_tests/tests/ui/non_sync_ref.rs

crates/bevy_render/src/camera/camera.rs

james7132 · 2022-05-07T00:36:25Z

This is basically ready for review. I still need to figure out how to make the compile tests work.

crates/bevy_ecs/src/event.rs

alice-i-cecile · 2022-05-07T01:32:36Z

examples/games/game_menu.rs

    // This system updates the settings when a new value for a setting is selected, and marks
    // the button as the one currently selected
-    fn setting_button<T: Component + PartialEq + Copy>(
+    fn setting_button<T: Component + Sync + PartialEq + Copy>(


This is an unfortunate bound to need to add in end-user code.

What happens if it's omitted?

The bound on &T fails to compile. This honestly only comes up in generic code though.

james7132 · 2022-05-09T16:58:04Z

Following @DJMcNab's suggestion, I've been thinking on how to make &T and Res<T> safe for !Sync types, and concluded we basically just need &T to have the same effect as &mut T or ResMut<T> does for Sync types. However, this seems like the actual implementation needs either negative trait bounds to split the Component/Resource implementation, or rely on specialization of some kind, both of which are currently unstable or still very early in the RFC process for Rust itself. This seems like something we'll just have to come back to later when there's better language support.

JohnTheCoolingFan · 2022-07-15T04:34:41Z

This PR would allow me to make a game similar to screeps but with lua using mlua crate. Hopefully this gets merged soon.

alice-i-cecile · 2022-07-15T04:40:39Z

@JohnTheCoolingFan can you link to the mlua struct that needs this so we can try to understand a bit more about how this would help?

JohnTheCoolingFan · 2022-07-15T04:42:28Z

https://docs.rs/mlua/latest/mlua/struct.Lua.html

alice-i-cecile · 2022-07-16T13:40:21Z

crates/bevy_ecs/src/query/fetch.rs

 pub struct ReadState<T> {
    component_id: ComponentId,
-    marker: PhantomData<T>,
+    marker: PhantomData<fn() -> T>,


This aligns with my mental model of how this should be modelled, but this is subtle enough that it deserves a comment.

Hmm, I don't really think it's needed. This is very much the standard way to express 'this represents something which can produce a T'.

It's no more strange than the initial use of PhantomData imo - maybe we should have a comment that we need to be able to 'reconstruct' the original T, so store it here. But we don't need to comment specifically on it being fn()->T

alice-i-cecile · 2022-07-16T13:43:50Z

crates/bevy_ecs/src/query/fetch.rs

 /// # bevy_ecs::system::assert_is_system(my_system);
 /// ```
+///
+/// ## Non-Sync Queries


These docs are great; add a copy of this to Res and NonSend?

We should also try to deduplicate these docs with the include_str! doc macro trick; these are subtle and critical enough that I really don't want that duplication.

alice-i-cecile

Core changes are good, and make sense to me.

I have some doc nits that should be addressed, and I think that doc deduplication is unusually important here.

alice-i-cecile · 2022-09-04T20:48:11Z

Closing in favor of the revived #5879.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label May 6, 2022

james7132 added 3 commits May 6, 2022 13:19

Bare minimum: Get it to compile

87ec70a

Update bounds on Query/World/Entity{Ref/Mut}

16e86ac

Add compile_fail_test and update Component derive macro

998b4bf

james7132 force-pushed the relax-sync-bound branch from 50fce3c to 998b4bf Compare May 6, 2022 20:19

james7132 added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels May 6, 2022

Formatting

dd6fcd3

DJMcNab reviewed May 6, 2022

View reviewed changes

james7132 added 8 commits May 6, 2022 14:18

unsafe impl -> PhantomData<fn() -> T>

f101e84

Fix examples

61f4d7c

Fix examples

01669da

Update paths for compile tests

62d0c4b

Attempt removing Sync on Resource

4feca9d

Formatting k

3f7a0a3

Remaining unsafe impl

6deee18

Use TRYBUILD='overwrite'

7b40740

james7132 marked this pull request as ready for review May 7, 2022 00:36

alice-i-cecile reviewed May 7, 2022

View reviewed changes

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed May 7, 2022

View reviewed changes

alice-i-cecile mentioned this pull request May 7, 2022

Add trivial Event trait #4685

Closed

james7132 changed the title ~~Remove Sync bound on Component~~ Remove Sync bound on Component and Resource May 8, 2022

Convert compile_fail_tests to doctests

dd63b22

james7132 requested review from DJMcNab and alice-i-cecile May 9, 2022 00:43

Add error codes for compile_fail tests

e6cb45c

james7132 added 2 commits May 9, 2022 10:02

Merge branch 'main' into relax-sync-bound

f5b2b4b

Remove unecessary bounds

f1e3983

Nilirad mentioned this pull request May 11, 2022

[Merged by Bors] - Entities and components section for ECS chapter bevyengine/bevy-website#294

Closed

3 tasks

is_send_and_sync -> is_send

6251294

james7132 added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label May 12, 2022

alice-i-cecile reviewed Jul 16, 2022

View reviewed changes

alice-i-cecile requested changes Jul 16, 2022

View reviewed changes

Weibye added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Aug 10, 2022

JohnTheCoolingFan mentioned this pull request Sep 4, 2022

Remove Sync bound on Component and Resource (adopted) #5879

Closed

alice-i-cecile closed this Sep 4, 2022

james7132 deleted the relax-sync-bound branch December 12, 2022 06:38

Uh oh!

Conversation

james7132 commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Migration Guide

Uh oh!

DJMcNab commented May 6, 2022

Uh oh!

Uh oh!

DJMcNab May 6, 2022

Choose a reason for hiding this comment

Uh oh!

kauly May 26, 2022

Choose a reason for hiding this comment

Uh oh!

james7132 May 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

james7132 commented May 7, 2022

Uh oh!

Uh oh!

alice-i-cecile May 7, 2022

Choose a reason for hiding this comment

Uh oh!

james7132 May 7, 2022 • 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!

james7132 commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohnTheCoolingFan commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jul 15, 2022

Uh oh!

JohnTheCoolingFan commented Jul 15, 2022

Uh oh!

alice-i-cecile Jul 16, 2022

Choose a reason for hiding this comment

Uh oh!

DJMcNab Jul 16, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jul 16, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Sep 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

james7132 commented May 6, 2022 •

edited

Loading

james7132 May 7, 2022 •

edited

Loading

james7132 commented May 9, 2022 •

edited

Loading

JohnTheCoolingFan commented Jul 15, 2022 •

edited

Loading