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 Sync bound on Component and Resource #4680

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/bevy_ecs/macros/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
ast.generics
.make_where_clause()
.predicates
.push(parse_quote! { Self: Send + Sync + 'static });
.push(parse_quote! { Self: Send + 'static });

let struct_name = &ast.ident;
let (impl_generics, type_generics, where_clause) = &ast.generics.split_for_impl();
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ use std::{any::TypeId, collections::HashMap};
/// bundle, in the _exact_ order that [`Bundle::get_components`] is called.
/// - [`Bundle::from_components`] must call `func` exactly once for each [`ComponentId`] returned by
/// [`Bundle::component_ids`].
pub unsafe trait Bundle: Send + Sync + 'static {
pub unsafe trait Bundle: Send + 'static {
/// Gets this [`Bundle`]'s component ids, in the order of this bundle's [`Component`]s
fn component_ids(components: &mut Components, storages: &mut Storages) -> Vec<ComponentId>;

Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::{
/// A component is data associated with an [`Entity`](crate::entity::Entity). Each entity can have
/// multiple different types of components, but only one of them per type.
///
/// Any type that is `Send + Sync + 'static` can implement `Component` using `#[derive(Component)]`.
/// Any type that is `Send + 'static` can implement `Component` using `#[derive(Component)]`.
///
/// In order to use foreign types as components, wrap them using a newtype pattern.
/// ```
Expand All @@ -31,7 +31,7 @@ use std::{
/// as one of the arguments.
///
/// Components can be grouped together into a [`Bundle`](crate::bundle::Bundle).
pub trait Component: Send + Sync + 'static {
pub trait Component: Send + 'static {
type Storage: ComponentStorage;
}

Expand Down
22 changes: 17 additions & 5 deletions crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ impl<'w> Fetch<'w> for EntityFetch<'w> {
}
}

impl<T: Component> WorldQuery for &T {
impl<T: Component + Sync> WorldQuery for &T {
type State = ReadState<T>;

fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> {
Expand All @@ -556,9 +556,13 @@ pub struct ReadState<T> {
marker: PhantomData<T>,
}

// SAFETY: ReadState only contains a ComponentId, should be safe to read
// from multiple threads concurrently
unsafe impl<T: Component + Sync> Sync for ReadState<T> {}

// SAFETY: component access and archetype component access are properly updated to reflect that T is
// read
unsafe impl<T: Component> FetchState for ReadState<T> {
unsafe impl<T: Component + Sync> FetchState for ReadState<T> {
fn init(world: &mut World) -> Self {
let component_id = world.init_component::<T>();
ReadState {
Expand Down Expand Up @@ -620,15 +624,15 @@ impl<T> Clone for ReadFetch<'_, T> {
}

/// SAFETY: access is read only
unsafe impl<'w, T: Component> ReadOnlyFetch for ReadFetch<'w, T> {}
unsafe impl<'w, T: Component + Sync> ReadOnlyFetch for ReadFetch<'w, T> {}

impl<'w, T: Component> WorldQueryGats<'w> for &T {
impl<'w, T: Component + Sync> WorldQueryGats<'w> for &T {
type Fetch = ReadFetch<'w, T>;
type ReadOnlyFetch = ReadFetch<'w, T>;
type _State = ReadState<T>;
}

impl<'w, T: Component> Fetch<'w> for ReadFetch<'w, T> {
impl<'w, T: Component + Sync> Fetch<'w> for ReadFetch<'w, T> {
type Item = &'w T;
type State = ReadState<T>;

Expand Down Expand Up @@ -793,6 +797,10 @@ pub struct WriteState<T> {
marker: PhantomData<T>,
}

// SAFETY: WriteState only contains a ComponentId, should be safe to read
// from multiple threads concurrently
unsafe impl<T: Component> Sync for WriteState<T> {}

// SAFETY: component access and archetype component access are properly updated to reflect that T is
// written
unsafe impl<T: Component> FetchState for WriteState<T> {
Expand Down Expand Up @@ -1259,6 +1267,10 @@ pub struct ChangeTrackersState<T> {
marker: PhantomData<T>,
}

// SAFETY: WriteState only contains a ComponentId, should be safe to read
// from multiple threads concurrently
unsafe impl<T: Component> Sync for ChangeTrackersState<T> {}

// SAFETY: component access and archetype component access are properly updated to reflect that T is
// read
unsafe impl<T: Component> FetchState for ChangeTrackersState<T> {
Expand Down
12 changes: 12 additions & 0 deletions crates/bevy_ecs/src/query/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ pub struct WithState<T> {
marker: PhantomData<T>,
}

// SAFETY: WithState only contains a ComponentId, should be safe to read
// from multiple threads concurrently
unsafe impl<T: Component> Sync for WithState<T> {}

// SAFETY: no component access or archetype component access
unsafe impl<T: Component> FetchState for WithState<T> {
fn init(world: &mut World) -> Self {
Expand Down Expand Up @@ -210,6 +214,10 @@ pub struct WithoutState<T> {
marker: PhantomData<T>,
}

// SAFETY: WithoutState only contains a ComponentId, should be safe to read
// from multiple threads concurrently
unsafe impl<T: Component> Sync for WithoutState<T> {}

// SAFETY: no component access or archetype component access
unsafe impl<T: Component> FetchState for WithoutState<T> {
fn init(world: &mut World) -> Self {
Expand Down Expand Up @@ -508,6 +516,10 @@ macro_rules! impl_tick_filter {
}
}

// SAFETY: The state only contains a ComponentId, should be safe to read
// from multiple threads concurrently
unsafe impl<T: Component> Sync for $state_name<T> {}

// SAFETY: this reads the T component. archetype component access and component access are updated to reflect that
unsafe impl<T: Component> FetchState for $state_name<T> {
fn init(world: &mut World) -> Self {
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_ecs/src/system/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::marker::PhantomData;
use super::Resource;

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

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

fn write(self, world: &mut World);
}

Expand Down Expand Up @@ -261,7 +261,7 @@ impl<'w, 's> Commands<'w, 's> {
/// worked out to share an ID space (which doesn't happen by default).
pub fn insert_or_spawn_batch<I, B>(&mut self, bundles_iter: I)
where
I: IntoIterator + Send + Sync + 'static,
I: IntoIterator + Send + 'static,
I::IntoIter: Iterator<Item = (Entity, B)>,
B: Bundle,
{
Expand Down Expand Up @@ -640,7 +640,7 @@ where

pub struct InsertOrSpawnBatch<I, B>
where
I: IntoIterator + Send + Sync + 'static,
I: IntoIterator + Send + 'static,
B: Bundle,
I::IntoIter: Iterator<Item = (Entity, B)>,
{
Expand All @@ -649,7 +649,7 @@ where

impl<I, B> Command for InsertOrSpawnBatch<I, B>
where
I: IntoIterator + Send + Sync + 'static,
I: IntoIterator + Send + 'static,
B: Bundle,
I::IntoIter: Iterator<Item = (Entity, B)>,
{
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> {
/// # bevy_ecs::system::assert_is_system(print_selected_character_name_system);
/// ```
#[inline]
pub fn get_component<T: Component>(&self, entity: Entity) -> Result<&T, QueryComponentError> {
pub fn get_component<T: Component + Sync>(&self, entity: Entity) -> Result<&T, QueryComponentError> {
let world = self.world;
let entity_ref = world
.get_entity(entity)
Expand Down
4 changes: 4 additions & 0 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,10 @@ pub struct RemovedComponentsState<T> {
marker: PhantomData<T>,
}

// SAFETY: RemovedComponentState only contains a ComponentId, should be safe to read
// from multiple threads concurrently
unsafe impl<T: Component> Sync for RemovedComponentsState<T> {}

impl<'a, T: Component> SystemParam for RemovedComponents<'a, T> {
type Fetch = RemovedComponentsState<T>;
}
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/world/entity_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ impl<'w> EntityRef<'w> {
}

#[inline]
pub fn get<T: Component>(&self) -> Option<&'w T> {
pub fn get<T: Component + Sync>(&self) -> Option<&'w T> {
// SAFE: entity location is valid and returned component is of type T
unsafe {
get_component_with_type(self.world, TypeId::of::<T>(), self.entity, self.location)
Expand Down Expand Up @@ -166,7 +166,7 @@ impl<'w> EntityMut<'w> {
}

#[inline]
pub fn get<T: Component>(&self) -> Option<&'_ T> {
pub fn get<T: Component + Sync>(&self) -> Option<&'_ T> {
// SAFE: lifetimes enforce correct usage of returned borrow
unsafe {
get_component_with_type(self.world, TypeId::of::<T>(), self.entity, self.location)
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ impl World {
/// assert_eq!(position.x, 0.0);
/// ```
#[inline]
pub fn get<T: Component>(&self, entity: Entity) -> Option<&T> {
pub fn get<T: Component + Sync>(&self, entity: Entity) -> Option<&T> {
self.get_entity(entity)?.get()
}

Expand Down
26 changes: 26 additions & 0 deletions crates/bevy_ecs_compile_fail_tests/tests/ui/non_sync_ref.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
use bevy_ecs::prelude::*;
use std::cell::Cell;

#[derive(Component, Eq, PartialEq, Debug)]
struct A(Cell<usize>);

fn main() {
let mut world = World::default();
let e = world.spawn().insert(A(Cell::new(10_usize))).id();

{
let query = QueryState::<&A>::new(&mut world);
}

{
let value = world.get::<A>(e);
}

{
let value = world.entity(e).get::<A>();
}

{
let value = world.entity_mut(e).get::<A>();
}
}
81 changes: 81 additions & 0 deletions crates/bevy_ecs_compile_fail_tests/tests/ui/non_sync_ref.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
error[E0599]: the function or associated item `new` exists for struct `bevy_ecs::query::QueryState<&A>`, but its trait bounds were not satisfied
--> tests/ui/non_sync_ref.rs:12:39
|
12 | let query = QueryState::<&A>::new(&mut world);
| ^^^ function or associated item cannot be called on `bevy_ecs::query::QueryState<&A>` due to unsatisfied trait bounds
|
= note: the following trait bounds were not satisfied:
`&A: WorldQuery`

error[E0277]: `Cell<usize>` cannot be shared between threads safely
--> tests/ui/non_sync_ref.rs:12:21
|
12 | let query = QueryState::<&A>::new(&mut world);
| ^^^^^^^^^^^^^^^^ `Cell<usize>` cannot be shared between threads safely
|
= help: within `A`, the trait `Sync` is not implemented for `Cell<usize>`
note: required because it appears within the type `A`
--> tests/ui/non_sync_ref.rs:5:8
|
5 | struct A(Cell<usize>);
| ^
= note: required because of the requirements on the impl of `WorldQuery` for `&A`
note: required by a bound in `bevy_ecs::query::QueryState`
--> C:/Users/James/Git/bevy/crates/bevy_ecs/src/query/state.rs:20:26
|
20 | pub struct QueryState<Q: WorldQuery, F: WorldQuery = ()> {
| ^^^^^^^^^^ required by this bound in `bevy_ecs::query::QueryState`

error[E0277]: `Cell<usize>` cannot be shared between threads safely
--> tests/ui/non_sync_ref.rs:16:27
|
16 | let value = world.get::<A>(e);
| ^^^ `Cell<usize>` cannot be shared between threads safely
|
= help: within `A`, the trait `Sync` is not implemented for `Cell<usize>`
note: required because it appears within the type `A`
--> tests/ui/non_sync_ref.rs:5:8
|
5 | struct A(Cell<usize>);
| ^
note: required by a bound in `bevy_ecs::world::World::get`
--> C:/Users/James/Git/bevy/crates/bevy_ecs/src/world/mod.rs:416:31
|
416 | pub fn get<T: Component + Sync>(&self, entity: Entity) -> Option<&T> {
| ^^^^ required by this bound in `bevy_ecs::world::World::get`

error[E0277]: `Cell<usize>` cannot be shared between threads safely
--> tests/ui/non_sync_ref.rs:20:37
|
20 | let value = world.entity(e).get::<A>();
| ^^^ `Cell<usize>` cannot be shared between threads safely
|
= help: within `A`, the trait `Sync` is not implemented for `Cell<usize>`
note: required because it appears within the type `A`
--> tests/ui/non_sync_ref.rs:5:8
|
5 | struct A(Cell<usize>);
| ^
note: required by a bound in `EntityRef::<'w>::get`
--> C:/Users/James/Git/bevy/crates/bevy_ecs/src/world/entity_ref.rs:67:31
|
67 | pub fn get<T: Component + Sync>(&self) -> Option<&'w T> {
| ^^^^ required by this bound in `EntityRef::<'w>::get`

error[E0277]: `Cell<usize>` cannot be shared between threads safely
--> tests/ui/non_sync_ref.rs:24:41
|
24 | let value = world.entity_mut(e).get::<A>();
| ^^^ `Cell<usize>` cannot be shared between threads safely
|
= help: within `A`, the trait `Sync` is not implemented for `Cell<usize>`
note: required because it appears within the type `A`
--> tests/ui/non_sync_ref.rs:5:8
|
5 | struct A(Cell<usize>);
| ^
note: required by a bound in `EntityMut::<'w>::get`
--> C:/Users/James/Git/bevy/crates/bevy_ecs/src/world/entity_ref.rs:169:31
|
169 | pub fn get<T: Component + Sync>(&self) -> Option<&'_ T> {
| ^^^^ required by this bound in `EntityMut::<'w>::get`
14 changes: 7 additions & 7 deletions crates/bevy_render/src/camera/camera.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ impl<T: Component + Default> Default for CameraTypePlugin<T> {
}
}

impl<T: Component + Default> Plugin for CameraTypePlugin<T> {
impl<T: Component + Sync + Default> Plugin for CameraTypePlugin<T> {
fn build(&self, app: &mut App) {
app.init_resource::<ActiveCamera<T>>()
.add_startup_system_to_stage(StartupStage::PostStartup, set_active_camera::<T>)
Expand All @@ -251,12 +251,12 @@ impl<T: Component + Default> Plugin for CameraTypePlugin<T> {

/// The canonical source of the "active camera" of the given camera type `T`.
#[derive(Debug)]
pub struct ActiveCamera<T: Component> {
pub struct ActiveCamera<T: Component + Sync> {
camera: Option<Entity>,
marker: PhantomData<T>,
}

impl<T: Component> Default for ActiveCamera<T> {
impl<T: Component + Sync> Default for ActiveCamera<T> {
fn default() -> Self {
Self {
camera: Default::default(),
Expand All @@ -265,7 +265,7 @@ impl<T: Component> Default for ActiveCamera<T> {
}
}

impl<T: Component> Clone for ActiveCamera<T> {
impl<T: Component + Sync> Clone for ActiveCamera<T> {
fn clone(&self) -> Self {
Self {
camera: self.camera,
Expand All @@ -274,7 +274,7 @@ impl<T: Component> Clone for ActiveCamera<T> {
}
}

impl<T: Component> ActiveCamera<T> {
impl<T: Component + Sync> ActiveCamera<T> {
/// Sets the active camera to the given `camera` entity.
pub fn set(&mut self, camera: Entity) {
self.camera = Some(camera);
Expand All @@ -286,7 +286,7 @@ impl<T: Component> ActiveCamera<T> {
}
}

pub fn set_active_camera<T: Component>(
pub fn set_active_camera<T: Component + Sync>(
mut active_camera: ResMut<ActiveCamera<T>>,
cameras: Query<Entity, (With<Camera>, With<T>)>,
) {
Expand All @@ -313,7 +313,7 @@ pub struct ExtractedCamera {
pub physical_size: Option<UVec2>,
}

pub fn extract_cameras<M: Component + Default>(
pub fn extract_cameras<M: Component + Sync + Default>(
mut commands: Commands,
windows: Res<Windows>,
images: Res<Assets<Image>>,
Expand Down
Loading