[Merged by Bors] - Add get_multiple and get_multiple_mut APIs for Query and QueryState #4298

alice-i-cecile · 2022-03-22T22:47:36Z

Objective

The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users.
This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique.
This could already by bypassed with get_unchecked, but that is an extremely unsafe API.
Closes Safe method for multiple mutable references to components from Query #2042.

Solution

Add get_multiple, get_multiple_mut and their unchecked equivalents (multiple and multiple_mut) to Query and QueryState.
Improve the QueryEntityError type to provide more useful error information.

Changelog

Added get_multiple, get_multiple_mut and their unchecked equivalents (multiple and multiple_mut) to Query and QueryState.

Migration Guide

The QueryEntityError enum now has a `AliasedMutability variant, and returns the offending entity.

Context

This is a fresh attempt at #3333; rebasing was behaving very badly and it was important to rebase on top of the recent query soundness fixes. Many thanks to all the reviewers in that thread, especially @BoxyUwU for the help with lifetimes.

To-do

Add compile fail tests
Successfully deduplicate code
Decide what to do about failing doc tests
Get some reviews for lifetime soundness

crates/bevy_ecs/src/query/state.rs

crates/bevy_ecs/src/system/query.rs

BoxyUwU

Hopefully this doesnt result in a bunch of unreadable compiler errors 🤦‍♀️

crates/bevy_ecs/src/system/query.rs

Co-authored-by: Boxy <supbscripter@gmail.com>

nakedible · 2022-03-24T07:59:26Z

I can just say that looks good and even better than the last one, thanks for continuing this. I did read through the pull once to know exactly what was in there, but don't have time for a proper review now.

cart

This looks good to me! Just one comment on world validation.

cart · 2022-03-30T00:29:59Z

crates/bevy_ecs/src/query/state.rs

+        last_change_tick: u32,
+        change_tick: u32,
+    ) -> Result<[<Q::Fetch as Fetch<'w, 's>>::Item; N], QueryEntityError> {
+        self.validate_world(world);


The _unchecked_manual variants don't validate World in any other cases. This saves us on a lot of redundant work, as functions like Query::get() might get called hundreds of thousands of times, even in a single system. Instead, we (currently) put the burden on the scheduler to do this.

Sadly I didn't include this in the Safety docs in QueryState (or in System::run_unsafe) and theres a gap here because the "safe" System::run doesn't validate world.

Obviously thats not good. But I'd prefer to solve the problem holistically / leave the current pattern in-tact. I think validating world at the System::run_unsafe level (and updating the _unchecked_manual safety docs) has my preference here, given that we are embracing the "just run a system" pattern.

Barring any conflicting thoughts on this (this feels like it will be a divisive topic), can you move validate_world "up a level" and out of the _unchecked_manual variants?

All of the other _manual stuff validates world so I think it would be weird for this PR to go and update everything to work differently

manual validates. manual_unchecked does not. I'm only talking about manual_unchecked here.

Oops: unchecked_manual, not manual_unchecked

"manual" means you're on the hook to update archetypes (but by default should be assumed to be "safe"). "unchecked" is "fast / unvalidated / unsafe baseline impl"

I'm definitely not asking for changes to unrelated code here. Just for consistency (despite the fact that the pattern being used has some holes, as previously stated).

Hmm okay yeah I guess we are missing a lot of methods here so its hard to see a pattern 🤣 _unchecked not validating world seems fine I guess 🤔

Barring any conflicting thoughts on this (this feels like it will be a divisive topic), can you move validate_world "up a level" and out of the _unchecked_manual variants?

Yep, can do.

(leaving unresolved so the link in #4363 works nicely)

crates/bevy_ecs/src/query/state.rs

crates/bevy_ecs/src/system/query.rs

cart · 2022-03-30T02:23:16Z

So, I fixed this @cart, by making the underlying QueryState::get_read_only_manual unsafe.

You didn't fix that. Query::get_multiple will still call validate_world() internally, when it definitely shouldn't. Query::get_multiple_mut is fixed though.

crates/bevy_ecs/src/query/state.rs

Co-authored-by: Boxy <supbscripter@gmail.com>

alice-i-cecile · 2022-03-30T02:26:22Z

There we go... Thanks @BoxyUwU.

cart · 2022-03-30T19:16:32Z

bors r+

@BoxyUwU

…4298) # Objective - The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users. - This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique. - This could already by bypassed with get_unchecked, but that is an extremely unsafe API. - Closes #2042. ## Solution - Add `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to `Query` and `QueryState`. - Improve the `QueryEntityError` type to provide more useful error information. ## Changelog - Added `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to Query and QueryState. ## Migration Guide - The `QueryEntityError` enum now has a `AliasedMutability variant, and returns the offending entity. ## Context This is a fresh attempt at #3333; rebasing was behaving very badly and it was important to rebase on top of the recent query soundness fixes. Many thanks to all the reviewers in that thread, especially @BoxyUwU for the help with lifetimes. ## To-do - [x] Add compile fail tests - [x] Successfully deduplicate code - [x] Decide what to do about failing doc tests - [x] Get some reviews for lifetime soundness

bors · 2022-03-30T19:33:39Z

Pull request successfully merged into main.

Build succeeded:

@BoxyUwU

…evyengine#4298) # Objective - The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users. - This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique. - This could already by bypassed with get_unchecked, but that is an extremely unsafe API. - Closes bevyengine#2042. ## Solution - Add `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to `Query` and `QueryState`. - Improve the `QueryEntityError` type to provide more useful error information. ## Changelog - Added `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to Query and QueryState. ## Migration Guide - The `QueryEntityError` enum now has a `AliasedMutability variant, and returns the offending entity. ## Context This is a fresh attempt at bevyengine#3333; rebasing was behaving very badly and it was important to rebase on top of the recent query soundness fixes. Many thanks to all the reviewers in that thread, especially @BoxyUwU for the help with lifetimes. ## To-do - [x] Add compile fail tests - [x] Successfully deduplicate code - [x] Decide what to do about failing doc tests - [x] Get some reviews for lifetime soundness

@BoxyUwU

…evyengine#4298) # Objective - The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users. - This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique. - This could already by bypassed with get_unchecked, but that is an extremely unsafe API. - Closes bevyengine#2042. ## Solution - Add `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to `Query` and `QueryState`. - Improve the `QueryEntityError` type to provide more useful error information. ## Changelog - Added `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to Query and QueryState. ## Migration Guide - The `QueryEntityError` enum now has a `AliasedMutability variant, and returns the offending entity. ## Context This is a fresh attempt at bevyengine#3333; rebasing was behaving very badly and it was important to rebase on top of the recent query soundness fixes. Many thanks to all the reviewers in that thread, especially @BoxyUwU for the help with lifetimes. ## To-do - [x] Add compile fail tests - [x] Successfully deduplicate code - [x] Decide what to do about failing doc tests - [x] Get some reviews for lifetime soundness

alice-i-cecile added 10 commits March 22, 2022 16:39

Basic function signatures

a78a231

Add AliasedMutability variant to QueryEntityError

2a22d98

QueryState methods

f822094

Query methods

f47b7ea

Infallible variants

854bb1e

Fix safety comment

712abda

Make QueryEntityError more useful

34e92c7

Basic docs

063e616

Basic doc tests

2423515

Doc tests for unhappy paths

f04c390

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Mar 22, 2022

alice-i-cecile added this to the Bevy 0.7 milestone Mar 22, 2022

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

alice-i-cecile added C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Triage This issue needs to be labelled labels Mar 22, 2022

alice-i-cecile requested a review from BoxyUwU March 22, 2022 22:48

alice-i-cecile commented Mar 22, 2022

View reviewed changes

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved

alice-i-cecile commented Mar 22, 2022

View reviewed changes

crates/bevy_ecs/src/system/query.rs Show resolved Hide resolved

BoxyUwU suggested changes Mar 22, 2022

View reviewed changes

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

alice-i-cecile mentioned this pull request Mar 22, 2022

Query::get_multiple and get_multiple_mut #3333

Closed

Fix lifetimes on self

3bc9432

Co-authored-by: Boxy <supbscripter@gmail.com>

alice-i-cecile force-pushed the get_multiple-again branch from 0d3b765 to 3bc9432 Compare March 22, 2022 23:08

alice-i-cecile added 2 commits March 22, 2022 19:12

Use unwrap_err to fix broken doc test

fce42c7

Deduplicate logic

1932ee7

alice-i-cecile force-pushed the get_multiple-again branch from 6972e2b to 1932ee7 Compare March 22, 2022 23:55

alice-i-cecile added 2 commits March 22, 2022 20:08

Split logic into mutable and non-mutable variants to reduce unsafety

1544dbb

Use read-write Fetch type for get_multiple_unchecked_manual

0f0d8ae

alice-i-cecile marked this pull request as draft March 23, 2022 00:12

Add compile-fail tests to verify lifetimes

f5bb379

alice-i-cecile added 5 commits March 23, 2022 21:33

Remove from_raw from doc tests

2edcb87

More robust error handling

d2ad4fb

Explicitly test for invalid entities

5871b10

Also return the Entity for QueryEntityError::QueryDoesNotMatch

1cd2963

Fix tests

22337a8

BoxyUwU approved these changes Mar 24, 2022

View reviewed changes

yilinwei approved these changes Mar 25, 2022

View reviewed changes

BoxyUwU added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 25, 2022

cart reviewed Mar 30, 2022

View reviewed changes

Move world validation into QueryState::get_mulitple_mut

86f1521

alice-i-cecile mentioned this pull request Mar 30, 2022

System::run does not validate which world it is being run on #4363

Closed

BoxyUwU reviewed Mar 30, 2022

View reviewed changes

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved

World is already validated by update_archetypes call

be22235

cart reviewed Mar 30, 2022

View reviewed changes

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

Avoid repeated calls to validate_world in Query::get_multiple

f7aa741

alice-i-cecile force-pushed the get_multiple-again branch from c142f40 to f7aa741 Compare March 30, 2022 01:49

BoxyUwU reviewed Mar 30, 2022

View reviewed changes

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved

Yeet validation

141c01d

Co-authored-by: Boxy <supbscripter@gmail.com>

Cargo fmt

c0e5903

bors bot changed the title ~~Add get_multiple and get_multiple_mut APIs for Query and QueryState~~ [Merged by Bors] - Add get_multiple and get_multiple_mut APIs for Query and QueryState Mar 30, 2022

bors bot closed this Mar 30, 2022

alice-i-cecile mentioned this pull request Mar 30, 2022

[Merged by Bors] - unsafeify World::entities_mut #4093

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add get_multiple and get_multiple_mut APIs for Query and QueryState #4298

[Merged by Bors] - Add get_multiple and get_multiple_mut APIs for Query and QueryState #4298

alice-i-cecile commented Mar 22, 2022 •

edited

Loading

BoxyUwU left a comment

nakedible commented Mar 24, 2022

cart left a comment

cart Mar 30, 2022

BoxyUwU Mar 30, 2022

cart Mar 30, 2022

cart Mar 30, 2022

cart Mar 30, 2022 •

edited

Loading

cart Mar 30, 2022

BoxyUwU Mar 30, 2022

alice-i-cecile Mar 30, 2022

alice-i-cecile Mar 30, 2022

cart commented Mar 30, 2022

alice-i-cecile commented Mar 30, 2022

cart commented Mar 30, 2022

bors bot commented Mar 30, 2022

[Merged by Bors] - Add get_multiple and get_multiple_mut APIs for Query and QueryState #4298

[Merged by Bors] - Add get_multiple and get_multiple_mut APIs for Query and QueryState #4298

Conversation

alice-i-cecile commented Mar 22, 2022 • edited Loading

Objective

Solution

Changelog

Migration Guide

Context

To-do

BoxyUwU left a comment

Choose a reason for hiding this comment

nakedible commented Mar 24, 2022

cart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cart Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cart commented Mar 30, 2022

alice-i-cecile commented Mar 30, 2022

cart commented Mar 30, 2022

bors bot commented Mar 30, 2022

alice-i-cecile commented Mar 22, 2022 •

edited

Loading

cart Mar 30, 2022 •

edited

Loading