Alias violations for EntityMut/EntityRef

[re0312]

Bevy version

main

What you did

use bevy::prelude::*;

fn sys(_a: Query<EntityMut>, _b: Query<EntityMut>) {}

fn main() {
    let mut app = App::new();
    app.add_systems(Update, sys).run();
}

What went wrong

this code should panic due to runtime alias violation checks, but it didn't.

[thebluefish]

My train of thought is that multiple EntityMut should theoretically only fail if they attempt to alias the same data - so that filters work, like in the example docs for EntityMut - but they technically don't in an empty world with no data to conflict on. I wrote this MVCE to test aliasing of real data, and it does appear to break aliasing rules. This MVCE works as far back as bevy 0.12, when EntityMut became a query parameter.

fn main() {
    use bevy::prelude::*;

    #[derive(Component, Debug)]
    struct Foo(pub bool);

    App::new()
        .add_systems(Startup, |mut commands: Commands| { commands.spawn(Foo(true)); })
        .add_systems(Update, |mut foo: Query<EntityMut>, mut bar: Query<EntityMut>| {
            let mut foo = foo.single_mut();
            let foo_id = foo.id();

            let mut bar = bar.single_mut();
            let bar_id = bar.id();

            // Entity { index: 0, generation: 1 } == Entity { index: 0, generation: 1 }
            println!("{:?} == {:?}", foo_id, bar_id);

            let foo_component = foo.get_mut::<Foo>().unwrap();
            let mut bar_component = bar.get_mut::<Foo>().unwrap();

            // Entity { index: 0, generation: 1 }: Foo(true)
            println!("{:?}: {:?}", foo_id, *foo_component);

            bar_component.0 = false;

            // Entity { index: 0, generation: 1 }: Foo(false)
            println!("{:?}: {:?}", foo_id, *foo_component);
        })
        .run();
}

[SkiFire13]

Ok, so:

• Query checks that it is compatible with previous SystemParams using assert_component_access_compatibility
  

  bevy/crates/bevy_ecs/src/system/system_param.rs

  Line 222 in 0c7df88

  assert_component_access_compatibility(

• assert_component_access_compatibility in turn checks that the previous access is compatible with the new one by checking if FilteredAccessSet::get_conflicts_single returns an empty Vec
  

  bevy/crates/bevy_ecs/src/system/system_param.rs

  Lines 295 to 298 in 0c7df88

  	let conflicts = system_access.get_conflicts_single(current);
  	if conflicts.is_empty() {
  	return;
  	}

• FilteredAccessSet::get_conflicts_single in turn checks if the FilteredAccess for the whole set is compatible with the new one, and if not it gathers the conflicts. Note that in this case the access are actually not compatible, so it does end up gathering the conflicts with each FilteredAccess in the set using FilteredAccess::get_conflicts
  

  bevy/crates/bevy_ecs/src/query/access.rs

  Lines 697 to 706 in 0c7df88

  	pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess<T>) -> Vec<T> {
  	// if the unfiltered access is incompatible, must check each pair
  	let mut conflicts = HashSet::new();
  	if !self.combined_access.is_compatible(filtered_access.access()) {
  	for filtered in &self.filtered_accesses {
  	conflicts.extend(filtered.get_conflicts(filtered_access).into_iter());
  	}
  	}
  	conflicts.into_iter().collect()
  	}

• FilteredAccess::get_conflicts in turn checks again if they are compatible, which in this case again they are not, then proceeds to gather the conflicts using Access::get_conflicts
  

  bevy/crates/bevy_ecs/src/query/access.rs

  Lines 497 to 502 in 0c7df88

  	pub fn get_conflicts(&self, other: &FilteredAccess<T>) -> Vec<T> {
  	if !self.is_compatible(other) {
  	// filters are disjoint, so we can just look at the unfiltered intersection
  	return self.access.get_conflicts(&other.access);
  	}
  	Vec::new()

• Finally, Access::get_conflicts tries to gather a list of components that both Access want to access. The problem however is that neither of them has an explicit list of such components, instead they both have a read_all = true and write_all = true. Notice how this method does have some unresolved questions about how write_all should be handled! How did they remain there unanswered?
  

  bevy/crates/bevy_ecs/src/query/access.rs

  Lines 272 to 298 in 0c7df88

  	pub fn get_conflicts(&self, other: &Access<T>) -> Vec<T> {
  	let mut conflicts = FixedBitSet::default();
  	if self.reads_all {
  	// QUESTION: How to handle `other.writes_all`?
  	conflicts.extend(other.writes.ones());
  	}
  	if other.reads_all {
  	// QUESTION: How to handle `self.writes_all`.
  	conflicts.extend(self.writes.ones());
  	}
  	if self.writes_all {
  	conflicts.extend(other.reads_and_writes.ones());
  	}
  	if other.writes_all {
  	conflicts.extend(self.reads_and_writes.ones());
  	}
  	conflicts.extend(self.writes.intersection(&other.reads_and_writes));
  	conflicts.extend(self.reads_and_writes.intersection(&other.writes));
  	conflicts
  	.ones()
  	.map(SparseSetIndex::get_sparse_set_index)
  	.collect()
  	}

So finally, there are two solutions:

• somehow fix Access::get_conflicts to return something (but what?) in the write_all cases;
• add a is_compatible check in assert_component_access_compatibility, just like it's used in all the other methods seen here; again it's unclear what the error should be in case there's no list of components they conflict on.

[hymm]

edit: whoops. Just noticed the linked pr.

Maybe get_conflicts should return an enum?

enum AccessConflicts<T> {
  All // if self and other set reads_all or writes_all
  Components<Vec<T>>
}