Skip to content

Commit

Permalink
Make Query fields private (bevyengine#7149)
Browse files Browse the repository at this point in the history
`Query`'s fields being `pub(crate)` means that the struct can be constructed via safe code from anywhere in `bevy_ecs` . This is Not Good since it is intended that all construction of this type goes through `Query::new` which is an `unsafe fn` letting various `Query` methods rely on those invariants holding even though they can be trivially bypassed.

This has no user facing impact
  • Loading branch information
BoxyUwU authored and alradish committed Jan 22, 2023
1 parent fcd5cf7 commit 1270ce0
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 16 deletions.
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1210,7 +1210,7 @@ mod tests {
let world2 = World::new();
let qstate = world1.query::<()>();
// SAFETY: doesnt access anything
let query = unsafe { Query::new(&world2, &qstate, 0, 0) };
let query = unsafe { Query::new(&world2, &qstate, 0, 0, false) };
query.iter();
}
}
31 changes: 17 additions & 14 deletions crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,16 +274,16 @@ use std::{any::TypeId, borrow::Borrow, fmt::Debug};
/// [`With`]: crate::query::With
/// [`Without`]: crate::query::Without
pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> {
pub(crate) world: &'world World,
pub(crate) state: &'state QueryState<Q, F>,
pub(crate) last_change_tick: u32,
pub(crate) change_tick: u32,
world: &'world World,
state: &'state QueryState<Q, F>,
last_change_tick: u32,
change_tick: u32,
// SAFETY: This is used to ensure that `get_component_mut::<C>` properly fails when a Query writes C
// and gets converted to a read-only query using `to_readonly`. Without checking this, `get_component_mut` relies on
// QueryState's archetype_component_access, which will continue allowing write access to C after being cast to
// the read-only variant. This whole situation is confusing and error prone. Ideally this is a temporary hack
// until we sort out a cleaner alternative.
pub(crate) force_read_only_component_access: bool,
force_read_only_component_access: bool,
}

impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> std::fmt::Debug for Query<'w, 's, Q, F> {
Expand All @@ -309,11 +309,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> {
state: &'s QueryState<Q, F>,
last_change_tick: u32,
change_tick: u32,
force_read_only_component_access: bool,
) -> Self {
state.validate_world(world);

Self {
force_read_only_component_access: false,
force_read_only_component_access,
world,
state,
last_change_tick,
Expand All @@ -329,14 +330,16 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> {
pub fn to_readonly(&self) -> Query<'_, 's, Q::ReadOnly, F::ReadOnly> {
let new_state = self.state.as_readonly();
// SAFETY: This is memory safe because it turns the query immutable.
Query {
// SAFETY: this must be set to true or `get_component_mut` will be unsound. See the comments
// on this field for more details
force_read_only_component_access: true,
world: self.world,
state: new_state,
last_change_tick: self.last_change_tick,
change_tick: self.change_tick,
unsafe {
Query::new(
self.world,
new_state,
self.last_change_tick,
self.change_tick,
// SAFETY: this must be set to true or `get_component_mut` will be unsound. See the comments
// on this field for more details
true,
)
}
}

Expand Down
8 changes: 7 additions & 1 deletion crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,13 @@ unsafe impl<Q: WorldQuery + 'static, F: ReadOnlyWorldQuery + 'static> SystemPara
world: &'w World,
change_tick: u32,
) -> Self::Item<'w, 's> {
Query::new(world, state, system_meta.last_change_tick, change_tick)
Query::new(
world,
state,
system_meta.last_change_tick,
change_tick,
false,
)
}
}

Expand Down

0 comments on commit 1270ce0

Please sign in to comment.