From 94ab2ab2aa31992e87fecd3372e3e7e5d546a509 Mon Sep 17 00:00:00 2001 From: Zachary Harrold Date: Sun, 4 Feb 2024 12:01:59 +1100 Subject: [PATCH] Deprecated Various Component Methods from `Query` and `QueryState` (#9920) # Objective - (Partially) Fixes #9904 - Acts on #9910 ## Solution - Deprecated the relevant methods from `Query`, cascading changes as required across Bevy. --- ## Changelog - Deprecated `QueryState::get_component_unchecked_mut` method - Deprecated `Query::get_component` method - Deprecated `Query::get_component_mut` method - Deprecated `Query::component` method - Deprecated `Query::component_mut` method - Deprecated `Query::get_component_unchecked_mut` method ## Migration Guide ### `QueryState::get_component_unchecked_mut` Use `QueryState::get_unchecked_manual` and select for the exact component based on the structure of the exact query as required. ### `Query::(get_)component(_unchecked)(_mut)` Use `Query::get` and select for the exact component based on the structure of the exact query as required. - For mutable access (`_mut`), use `Query::get_mut` - For unchecked access (`_unchecked`), use `Query::get_unchecked` - For panic variants (non-`get_`), add `.unwrap()` ## Notes - `QueryComponentError` can be removed once these deprecated methods are also removed. Due to an interaction with `thiserror`'s derive macro, it is not marked as deprecated. --- crates/bevy_ecs/src/query/mod.rs | 1 + crates/bevy_ecs/src/query/state.rs | 45 +++++++---- crates/bevy_ecs/src/system/mod.rs | 2 + crates/bevy_ecs/src/system/query.rs | 46 +++++++++-- .../tests/ui/query_lifetime_safety.rs | 2 + .../tests/ui/query_lifetime_safety.stderr | 80 +++++++++---------- crates/bevy_ui/src/focus.rs | 6 +- examples/ui/size_constraints.rs | 2 +- 8 files changed, 121 insertions(+), 63 deletions(-) diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 618c179f98570a..249485d005b9ef 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -739,6 +739,7 @@ mod tests { } } + #[allow(deprecated)] #[test] fn mut_to_immut_query_methods_have_immut_item() { #[derive(Component)] diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 666549d52a6f38..03de79f3d4f9b2 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -17,8 +17,8 @@ use fixedbitset::FixedBitSet; use std::{any::TypeId, borrow::Borrow, fmt, mem::MaybeUninit}; use super::{ - NopWorldQuery, QueryBuilder, QueryComponentError, QueryData, QueryEntityError, QueryFilter, - QueryManyIter, QuerySingleError, ROQueryItem, + NopWorldQuery, QueryBuilder, QueryData, QueryEntityError, QueryFilter, QueryManyIter, + QuerySingleError, ROQueryItem, }; /// Provides scoped access to a [`World`] state according to a given [`QueryData`] and [`QueryFilter`]. @@ -629,32 +629,37 @@ impl QueryState { /// Returns a shared reference to the component `T` of the given [`Entity`]. /// /// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is returned instead. + #[deprecated( + since = "0.13.0", + note = "Please use `get` and select for the exact component based on the structure of the exact query as required." + )] + #[allow(deprecated)] #[inline] pub(crate) fn get_component<'w, T: Component>( &self, world: UnsafeWorldCell<'w>, entity: Entity, - ) -> Result<&'w T, QueryComponentError> { + ) -> Result<&'w T, super::QueryComponentError> { let entity_ref = world .get_entity(entity) - .ok_or(QueryComponentError::NoSuchEntity)?; + .ok_or(super::QueryComponentError::NoSuchEntity)?; let component_id = world .components() .get_id(TypeId::of::()) - .ok_or(QueryComponentError::MissingComponent)?; + .ok_or(super::QueryComponentError::MissingComponent)?; let archetype_component = entity_ref .archetype() .get_archetype_component_id(component_id) - .ok_or(QueryComponentError::MissingComponent)?; + .ok_or(super::QueryComponentError::MissingComponent)?; if self .archetype_component_access .has_read(archetype_component) { // SAFETY: `world` must have access to the component `T` for this entity, // since it was registered in `self`'s archetype component access set. - unsafe { entity_ref.get::() }.ok_or(QueryComponentError::MissingComponent) + unsafe { entity_ref.get::() }.ok_or(super::QueryComponentError::MissingComponent) } else { - Err(QueryComponentError::MissingReadAccess) + Err(super::QueryComponentError::MissingReadAccess) } } @@ -663,6 +668,11 @@ impl QueryState { /// # Panics /// /// If given a nonexisting entity or mismatched component, this will panic. + #[deprecated( + since = "0.13.0", + note = "Please use `get` and select for the exact component based on the structure of the exact query as required." + )] + #[allow(deprecated)] #[inline] pub(crate) fn component<'w, T: Component>( &self, @@ -688,6 +698,13 @@ impl QueryState { /// /// This function makes it possible to violate Rust's aliasing guarantees. /// You must make sure this call does not result in multiple mutable references to the same component. + /// + /// [`QueryComponentError`]: super::QueryComponentError + #[deprecated( + since = "0.13.0", + note = "Please use QueryState::get_unchecked_manual and select for the exact component based on the structure of the exact query as required." + )] + #[allow(deprecated)] #[inline] pub unsafe fn get_component_unchecked_mut<'w, T: Component>( &self, @@ -695,18 +712,18 @@ impl QueryState { entity: Entity, last_run: Tick, this_run: Tick, - ) -> Result, QueryComponentError> { + ) -> Result, super::QueryComponentError> { let entity_ref = world .get_entity(entity) - .ok_or(QueryComponentError::NoSuchEntity)?; + .ok_or(super::QueryComponentError::NoSuchEntity)?; let component_id = world .components() .get_id(TypeId::of::()) - .ok_or(QueryComponentError::MissingComponent)?; + .ok_or(super::QueryComponentError::MissingComponent)?; let archetype_component = entity_ref .archetype() .get_archetype_component_id(component_id) - .ok_or(QueryComponentError::MissingComponent)?; + .ok_or(super::QueryComponentError::MissingComponent)?; if self .archetype_component_access .has_write(archetype_component) @@ -715,9 +732,9 @@ impl QueryState { // mutable reference to this entity's component `T`. let result = unsafe { entity_ref.get_mut_using_ticks::(last_run, this_run) }; - result.ok_or(QueryComponentError::MissingComponent) + result.ok_or(super::QueryComponentError::MissingComponent) } else { - Err(QueryComponentError::MissingWriteAccess) + Err(super::QueryComponentError::MissingWriteAccess) } } diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 630eb22bdff1d4..c5d1dfb5c535ed 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -395,6 +395,7 @@ mod tests { schedule.run(world); } + #[allow(deprecated)] #[test] fn query_system_gets() { fn query_system( @@ -1554,6 +1555,7 @@ mod tests { }); } + #[allow(deprecated)] #[test] fn readonly_query_get_mut_component_fails() { use crate::query::QueryComponentError; diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 7b7c5ebbc9e081..f13b38024451c0 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -2,9 +2,9 @@ use crate::{ component::{Component, Tick}, entity::Entity, query::{ - BatchingStrategy, QueryCombinationIter, QueryComponentError, QueryData, QueryEntityError, - QueryFilter, QueryIter, QueryManyIter, QueryParIter, QuerySingleError, QueryState, - ROQueryItem, ReadOnlyQueryData, + BatchingStrategy, QueryCombinationIter, QueryData, QueryEntityError, QueryFilter, + QueryIter, QueryManyIter, QueryParIter, QuerySingleError, QueryState, ROQueryItem, + ReadOnlyQueryData, }, world::{unsafe_world_cell::UnsafeWorldCell, Mut}, }; @@ -1132,8 +1132,16 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// - [`component`](Self::component) a panicking version of this function. /// - [`get_component_mut`](Self::get_component_mut) to get a mutable reference of a component. + #[deprecated( + since = "0.13.0", + note = "Please use `get` and select for the exact component based on the structure of the exact query as required." + )] + #[allow(deprecated)] #[inline] - pub fn get_component(&self, entity: Entity) -> Result<&T, QueryComponentError> { + pub fn get_component( + &self, + entity: Entity, + ) -> Result<&T, crate::query::QueryComponentError> { self.state.get_component(self.world, entity) } @@ -1165,11 +1173,18 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// - [`component_mut`](Self::component_mut) a panicking version of this function. /// - [`get_component`](Self::get_component) to get a shared reference of a component. + /// + /// [`QueryComponentError`]: crate::query::QueryComponentError + #[deprecated( + since = "0.13.0", + note = "Please use `get_mut` and select for the exact component based on the structure of the exact query as required." + )] + #[allow(deprecated)] #[inline] pub fn get_component_mut( &mut self, entity: Entity, - ) -> Result, QueryComponentError> { + ) -> Result, crate::query::QueryComponentError> { // SAFETY: unique access to query (preventing aliased access) unsafe { self.get_component_unchecked_mut(entity) } } @@ -1184,6 +1199,11 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// - [`get_component`](Self::get_component) a non-panicking version of this function. /// - [`component_mut`](Self::component_mut) to get a mutable reference of a component. + #[deprecated( + since = "0.13.0", + note = "Please use `get` and select for the exact component based on the structure of the exact query as required." + )] + #[allow(deprecated)] #[inline] #[track_caller] pub fn component(&self, entity: Entity) -> &T { @@ -1200,6 +1220,11 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// - [`get_component_mut`](Self::get_component_mut) a non-panicking version of this function. /// - [`component`](Self::component) to get a shared reference of a component. + #[deprecated( + since = "0.13.0", + note = "Please use `get_mut` and select for the exact component based on the structure of the exact query as required." + )] + #[allow(deprecated)] #[inline] #[track_caller] pub fn component_mut(&mut self, entity: Entity) -> Mut<'_, T> { @@ -1226,15 +1251,22 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// # See also /// /// - [`get_component_mut`](Self::get_component_mut) for the safe version. + /// + /// [`QueryComponentError`]: crate::query::QueryComponentError + #[deprecated( + since = "0.13.0", + note = "Please use `get_unchecked` and select for the exact component based on the structure of the exact query as required." + )] + #[allow(deprecated)] #[inline] pub unsafe fn get_component_unchecked_mut( &self, entity: Entity, - ) -> Result, QueryComponentError> { + ) -> Result, crate::query::QueryComponentError> { // This check is required to ensure soundness in the case of `to_readonly().get_component_mut()` // See the comments on the `force_read_only_component_access` field for more info. if self.force_read_only_component_access { - return Err(QueryComponentError::MissingWriteAccess); + return Err(crate::query::QueryComponentError::MissingWriteAccess); } // SAFETY: The above check ensures we are not a readonly query. diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_lifetime_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_lifetime_safety.rs index f1d6048e199959..11e04d46aee556 100644 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_lifetime_safety.rs +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_lifetime_safety.rs @@ -24,12 +24,14 @@ fn main() { assert_eq!(data, &mut *data2); // oops UB } + #[allow(deprecated)] { let data: &Foo = query.get_component::(e).unwrap(); let mut data2: Mut = query.get_component_mut(e).unwrap(); assert_eq!(data, &mut *data2); // oops UB } + #[allow(deprecated)] { let mut data2: Mut = query.get_component_mut(e).unwrap(); let data: &Foo = query.get_component::(e).unwrap(); diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_lifetime_safety.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_lifetime_safety.stderr index 042cb246f80198..5d425c1e9b6b04 100644 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_lifetime_safety.stderr +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_lifetime_safety.stderr @@ -19,101 +19,101 @@ error[E0502]: cannot borrow `query` as immutable because it is also borrowed as | ----- mutable borrow later used here error[E0502]: cannot borrow `query` as mutable because it is also borrowed as immutable - --> tests/ui/query_lifetime_safety.rs:29:39 + --> tests/ui/query_lifetime_safety.rs:30:39 | -28 | let data: &Foo = query.get_component::(e).unwrap(); +29 | let data: &Foo = query.get_component::(e).unwrap(); | ----- immutable borrow occurs here -29 | let mut data2: Mut = query.get_component_mut(e).unwrap(); +30 | let mut data2: Mut = query.get_component_mut(e).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here -30 | assert_eq!(data, &mut *data2); // oops UB +31 | assert_eq!(data, &mut *data2); // oops UB | ----------------------------- immutable borrow later used here error[E0502]: cannot borrow `query` as immutable because it is also borrowed as mutable - --> tests/ui/query_lifetime_safety.rs:35:30 + --> tests/ui/query_lifetime_safety.rs:37:30 | -34 | let mut data2: Mut = query.get_component_mut(e).unwrap(); +36 | let mut data2: Mut = query.get_component_mut(e).unwrap(); | ----- mutable borrow occurs here -35 | let data: &Foo = query.get_component::(e).unwrap(); +37 | let data: &Foo = query.get_component::(e).unwrap(); | ^^^^^ immutable borrow occurs here -36 | assert_eq!(data, &mut *data2); // oops UB +38 | assert_eq!(data, &mut *data2); // oops UB | ----- mutable borrow later used here error[E0502]: cannot borrow `query` as mutable because it is also borrowed as immutable - --> tests/ui/query_lifetime_safety.rs:41:39 + --> tests/ui/query_lifetime_safety.rs:43:39 | -40 | let data: &Foo = query.single(); +42 | let data: &Foo = query.single(); | ----- immutable borrow occurs here -41 | let mut data2: Mut = query.single_mut(); +43 | let mut data2: Mut = query.single_mut(); | ^^^^^^^^^^^^^^^^^^ mutable borrow occurs here -42 | assert_eq!(data, &mut *data2); // oops UB +44 | assert_eq!(data, &mut *data2); // oops UB | ----------------------------- immutable borrow later used here error[E0502]: cannot borrow `query` as immutable because it is also borrowed as mutable - --> tests/ui/query_lifetime_safety.rs:47:30 + --> tests/ui/query_lifetime_safety.rs:49:30 | -46 | let mut data2: Mut = query.single_mut(); +48 | let mut data2: Mut = query.single_mut(); | ----- mutable borrow occurs here -47 | let data: &Foo = query.single(); +49 | let data: &Foo = query.single(); | ^^^^^ immutable borrow occurs here -48 | assert_eq!(data, &mut *data2); // oops UB +50 | assert_eq!(data, &mut *data2); // oops UB | ----- mutable borrow later used here error[E0502]: cannot borrow `query` as mutable because it is also borrowed as immutable - --> tests/ui/query_lifetime_safety.rs:53:39 + --> tests/ui/query_lifetime_safety.rs:55:39 | -52 | let data: &Foo = query.get_single().unwrap(); +54 | let data: &Foo = query.get_single().unwrap(); | ----- immutable borrow occurs here -53 | let mut data2: Mut = query.get_single_mut().unwrap(); +55 | let mut data2: Mut = query.get_single_mut().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here -54 | assert_eq!(data, &mut *data2); // oops UB +56 | assert_eq!(data, &mut *data2); // oops UB | ----------------------------- immutable borrow later used here error[E0502]: cannot borrow `query` as immutable because it is also borrowed as mutable - --> tests/ui/query_lifetime_safety.rs:59:30 + --> tests/ui/query_lifetime_safety.rs:61:30 | -58 | let mut data2: Mut = query.get_single_mut().unwrap(); +60 | let mut data2: Mut = query.get_single_mut().unwrap(); | ----- mutable borrow occurs here -59 | let data: &Foo = query.get_single().unwrap(); +61 | let data: &Foo = query.get_single().unwrap(); | ^^^^^ immutable borrow occurs here -60 | assert_eq!(data, &mut *data2); // oops UB +62 | assert_eq!(data, &mut *data2); // oops UB | ----- mutable borrow later used here error[E0502]: cannot borrow `query` as mutable because it is also borrowed as immutable - --> tests/ui/query_lifetime_safety.rs:65:39 + --> tests/ui/query_lifetime_safety.rs:67:39 | -64 | let data: &Foo = query.iter().next().unwrap(); +66 | let data: &Foo = query.iter().next().unwrap(); | ----- immutable borrow occurs here -65 | let mut data2: Mut = query.iter_mut().next().unwrap(); +67 | let mut data2: Mut = query.iter_mut().next().unwrap(); | ^^^^^^^^^^^^^^^^ mutable borrow occurs here -66 | assert_eq!(data, &mut *data2); // oops UB +68 | assert_eq!(data, &mut *data2); // oops UB | ----------------------------- immutable borrow later used here error[E0502]: cannot borrow `query` as immutable because it is also borrowed as mutable - --> tests/ui/query_lifetime_safety.rs:71:30 + --> tests/ui/query_lifetime_safety.rs:73:30 | -70 | let mut data2: Mut = query.iter_mut().next().unwrap(); +72 | let mut data2: Mut = query.iter_mut().next().unwrap(); | ----- mutable borrow occurs here -71 | let data: &Foo = query.iter().next().unwrap(); +73 | let data: &Foo = query.iter().next().unwrap(); | ^^^^^ immutable borrow occurs here -72 | assert_eq!(data, &mut *data2); // oops UB +74 | assert_eq!(data, &mut *data2); // oops UB | ----- mutable borrow later used here error[E0502]: cannot borrow `query` as mutable because it is also borrowed as immutable - --> tests/ui/query_lifetime_safety.rs:79:13 + --> tests/ui/query_lifetime_safety.rs:81:13 | -78 | query.iter().for_each(|data| opt_data = Some(data)); +80 | query.iter().for_each(|data| opt_data = Some(data)); | ----- immutable borrow occurs here -79 | query.iter_mut().for_each(|data| opt_data_2 = Some(data)); +81 | query.iter_mut().for_each(|data| opt_data_2 = Some(data)); | ^^^^^^^^^^^^^^^^ mutable borrow occurs here -80 | assert_eq!(opt_data.unwrap(), &mut *opt_data_2.unwrap()); // oops UB +82 | assert_eq!(opt_data.unwrap(), &mut *opt_data_2.unwrap()); // oops UB | -------- immutable borrow later used here error[E0502]: cannot borrow `query` as immutable because it is also borrowed as mutable - --> tests/ui/query_lifetime_safety.rs:87:13 + --> tests/ui/query_lifetime_safety.rs:89:13 | -86 | query.iter_mut().for_each(|data| opt_data_2 = Some(data)); +88 | query.iter_mut().for_each(|data| opt_data_2 = Some(data)); | ----- mutable borrow occurs here -87 | query.iter().for_each(|data| opt_data = Some(data)); +89 | query.iter().for_each(|data| opt_data = Some(data)); | ^^^^^ immutable borrow occurs here -88 | assert_eq!(opt_data.unwrap(), &mut *opt_data_2.unwrap()); // oops UB +90 | assert_eq!(opt_data.unwrap(), &mut *opt_data_2.unwrap()); // oops UB | ---------- mutable borrow later used here diff --git a/crates/bevy_ui/src/focus.rs b/crates/bevy_ui/src/focus.rs index 5c3fc6594e8adc..4e7d6454c48cd9 100644 --- a/crates/bevy_ui/src/focus.rs +++ b/crates/bevy_ui/src/focus.rs @@ -162,7 +162,11 @@ pub fn ui_focus_system( // reset entities that were both clicked and released in the last frame for entity in state.entities_to_reset.drain(..) { - if let Ok(mut interaction) = node_query.get_component_mut::(entity) { + if let Ok(NodeQueryItem { + interaction: Some(mut interaction), + .. + }) = node_query.get_mut(entity) + { *interaction = Interaction::None; } } diff --git a/examples/ui/size_constraints.rs b/examples/ui/size_constraints.rs index 498e77329438bc..ff944c894c961b 100644 --- a/examples/ui/size_constraints.rs +++ b/examples/ui/size_constraints.rs @@ -366,7 +366,7 @@ fn update_radio_buttons_colors( children_query: Query<&Children>, ) { for &ButtonActivatedEvent(button_id) in event_reader.read() { - let target_constraint = button_query.get_component::(button_id).unwrap(); + let (_, target_constraint, _) = button_query.get(button_id).unwrap(); for (id, constraint, interaction) in button_query.iter() { if target_constraint == constraint { let (border_color, inner_color, text_color) = if id == button_id {