Fix soudness issue with Conflicts involving `read_all` and `write_all` #14579

cBournhonesque · 2024-08-01T23:42:35Z

Objective

Fixes Alias violations for EntityMut/EntityRef #14575
There is a soundness issue because we use conflicts() to check for system ambiguities + soundness issues. However since the current conflicts is a Vec<T>, we cannot express conflicts where there is no specific ComponentId at fault. For example q1: Query<EntityMut>, q2: Query<EntityMut>
There was a TODO to handle the write_all case but it was never resolved

Solution

Introduce an AccessConflict enum that is either a list of specific ids that are conflicting or All if all component ids are conflicting

Testing

Introduced a new unit test to check for the EntityMut case

Migration guide

The get_conflicts method of Access now returns an AccessConflict enum instead of simply a Vec of ComponentIds that are causing the access conflict. This can be useful in cases where there are no particular ComponentIds conflicting, but instead all of them are; for example fn system(q1: Query<EntityMut>, q2: Query<EntityRef>)

cBournhonesque · 2024-08-01T23:43:59Z

crates/bevy_ecs/src/schedule/schedule.rs

+                        AccessConflict::All => {
+                            // there is no specific component conflicting, but the systems are overall incompatible
+                            // for example 2 systems with `Query<EntityMut>`
+                            conflicting_systems.push((a, b, Vec::new()));


this is not ideal.. maybe we'd like to introduce a similar enum in the schedule graph conflicts? not sure if that's what we want

IMO this can wait until follow-up.

Out of curiousity, what happens with the ambiguity reporting in this case?

It says that the two systems are ambiguous and conflict on World: https://github.com/cBournhonesque/bevy/blob/944cc182d2265df2165ac03372d681d718e1723d/crates/bevy_ecs/src/schedule/schedule.rs#L1883-L1883

alice-i-cecile

I'm in favor of newtyping the conflicts here: more flexible and clearer.

This has tests for both the query and the system case which is great, but I'd also like to see EntityRef + EntityMut, and EntityMut + &World tested.

cBournhonesque · 2024-08-02T01:59:00Z

crates/bevy_ecs/src/query/access.rs

+        let mut conflicts = FixedBitSet::new();
        if self.reads_all {
-            // QUESTION: How to handle `other.writes_all`?
+            if other.writes_all {


this is the key change

alice-i-cecile · 2024-08-02T02:39:52Z

crates/bevy_ecs/src/query/access.rs


    /// Returns a vector of elements that this and `other` cannot access at the same time.
-    pub fn get_conflicts(&self, other: &FilteredAccess<T>) -> Vec<T> {
+    pub fn get_conflicts(&self, other: &FilteredAccess<T>) -> AccessConflict {


This is technically breaking, but I don't think it's worth refactoring to avoid this.

my general opinion is that fixing unsoundness takes priority over semver. there are of course ways to avoid both in the same release but getting the soundness fix out as quickly as possible is important imo.

github-actions · 2024-08-02T02:40:08Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

soqb

definitely a solid soundness fix; a couple of nits which can easily wait until a followup!

crates/bevy_ecs/src/lib.rs

crates/bevy_ecs/src/query/access.rs

alice-i-cecile · 2024-08-03T12:17:12Z

I like these little suggestions: let me know if you want to do them now or in a follow-up :)

cBournhonesque · 2024-08-03T12:52:31Z

I like these little suggestions: let me know if you want to do them now or in a follow-up :)

They make sense to me; I applied them now!

alice-i-cecile · 2024-08-06T01:47:29Z

@cBournhonesque let me know when the conflicts are resolved. I was pretty sure this would happen 😂

cBournhonesque · 2024-08-06T04:25:02Z

@cBournhonesque let me know when the conflicts are resolved. I was pretty sure this would happen 😂

Indeed! Updated :)

SkiFire13 · 2024-08-06T11:31:35Z

crates/bevy_ecs/src/query/access.rs

        if self.reads_all_resources {
-            // QUESTION: How to handle `other.writes_all`?
+            if other.reads_all_resources {
+                return AccessConflicts::All;
+            }
            conflicts.extend(other.resource_writes.ones());
        }

        if other.reads_all_resources {
-            // QUESTION: How to handle `self.writes_all`.
+            if self.reads_all_resources {
+                return AccessConflicts::All;
+            }
            conflicts.extend(self.resource_writes.ones());


These two new inner ifs should check for writes_all_resources and not reads_all_resources.

oh you'r right, that's a bug..
We don't have a unit test for this because there's nothing that queries all resources but not all components

Should we remove the AccessConflicts::ALL conflicts for resources entirely?

# Objective - I made a mistake when fixing the merge conflicts here: #14579 (comment) It wasn't caught because there's no easy way to trigger access conflicts with resources without triggering them with components first.

cBournhonesque added 2 commits August 1, 2024 17:05

wip

0b9da99

fix EntityMut soundness issue

74d0339

cBournhonesque commented Aug 1, 2024

View reviewed changes

cBournhonesque added 2 commits August 1, 2024 19:47

cargo fmt

517640e

clippy

e07acbf

cBournhonesque added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Aug 2, 2024

alice-i-cecile added this to the 0.14.1 milestone Aug 2, 2024

alice-i-cecile reviewed Aug 2, 2024

View reviewed changes

alice-i-cecile modified the milestones: 0.14.1, 0.14.2 Aug 2, 2024

cBournhonesque commented Aug 2, 2024

View reviewed changes

cBournhonesque added 2 commits August 1, 2024 22:07

add extra tests

a721ecb

fmt

944cc18

alice-i-cecile reviewed Aug 2, 2024

View reviewed changes

alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 2, 2024

alice-i-cecile modified the milestones: 0.14.2, 0.15 Aug 2, 2024

alice-i-cecile mentioned this pull request Aug 2, 2024

Separate component and resource access #14561

Merged

soqb approved these changes Aug 3, 2024

View reviewed changes

crates/bevy_ecs/src/lib.rs Show resolved Hide resolved

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

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

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 3, 2024

address comments

378eb03

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 3, 2024

alice-i-cecile modified the milestones: 0.15, 0.14.2 Aug 3, 2024

alice-i-cecile added this pull request to the merge queue Aug 6, 2024

alice-i-cecile approved these changes Aug 6, 2024

View reviewed changes

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 6, 2024

cBournhonesque added 2 commits August 6, 2024 00:16

Merge branch 'main' into cb/fix-entity-mut

fe4a49b

fix

4a092d3

alice-i-cecile added this pull request to the merge queue Aug 6, 2024

Merged via the queue into bevyengine:main with commit e85c072 Aug 6, 2024

SkiFire13 reviewed Aug 6, 2024

View reviewed changes

cBournhonesque mentioned this pull request Aug 6, 2024

Fix access conflicts for resources #14635

Merged

Uh oh!

Fix soudness issue with Conflicts involving read_all and write_all #14579

Fix soudness issue with Conflicts involving read_all and write_all #14579

Uh oh!

Conversation

cBournhonesque commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Migration guide

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hymm Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 2, 2024

Uh oh!

soqb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Aug 3, 2024

Uh oh!

cBournhonesque commented Aug 3, 2024

Uh oh!

alice-i-cecile commented Aug 6, 2024

Uh oh!

cBournhonesque commented Aug 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix soudness issue with Conflicts involving `read_all` and `write_all` #14579

Fix soudness issue with Conflicts involving `read_all` and `write_all` #14579

cBournhonesque commented Aug 1, 2024 •

edited

Loading

hymm Aug 2, 2024 •

edited

Loading