From 1270ce0beb772516aaf6f6905256594d4a645a92 Mon Sep 17 00:00:00 2001
From: Boxy <supbscripter@gmail.com>
Date: Tue, 10 Jan 2023 18:55:23 +0000
Subject: [PATCH] Make `Query` fields private (#7149)

`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
---
 crates/bevy_ecs/src/system/mod.rs          |  2 +-
 crates/bevy_ecs/src/system/query.rs        | 31 ++++++++++++----------
 crates/bevy_ecs/src/system/system_param.rs |  8 +++++-
 3 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs
index 300752dc31e7b..aaa46d1209cbd 100644
--- a/crates/bevy_ecs/src/system/mod.rs
+++ b/crates/bevy_ecs/src/system/mod.rs
@@ -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();
     }
 }
diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs
index c376223b4eff8..9f822ca786eb0 100644
--- a/crates/bevy_ecs/src/system/query.rs
+++ b/crates/bevy_ecs/src/system/query.rs
@@ -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> {
@@ -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,
@@ -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,
+            )
         }
     }
 
diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs
index d346972056078..a229925eca6fc 100644
--- a/crates/bevy_ecs/src/system/system_param.rs
+++ b/crates/bevy_ecs/src/system/system_param.rs
@@ -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,
+        )
     }
 }