Some nested queries now cause ECS to crash (introduced in commit 3efbaca10452427385822f79208ff3a78ab9edda)

[ashneverdawn]

Introduced in commit 3efbaca

Error occurs when a system has nested queries that involve the same component.
thread 'Compute Task Pool (1)' panicked at 'client::ui::components::focus::Focus already borrowed', /Users/ashneverdawn/.cargo/git/checkouts/bevy-f7ffde730c324c74/8106f77/crates/bevy_ecs/hecs/src/archetype.rs:196:13

Example code that panics:

pub fn focus_system(
  mut interaction_query: Query<(Entity, Mutated<Interaction>, Mut<Focus>)>,
  mut focused_query: Query<(Entity, Mut<Focus>)>,
) {
  for (entity, interaction, mut new_focus) in &mut interaction_query.iter() {
    if *interaction == Interaction::Clicked {
      for (entity, mut focus) in &mut focused_query.iter() {
        focus.active = false;
      }
      new_focus.active = true;
      break;
    }
  }
}

[BoxyUwU]

Before that commit queries wouldn't get their archetype accesses set properly and so when borrowing from world they were sometimes borrowing stuff without marking it as borrowed. Which let you do do stuff like run two systems that borrow T mutably in parallel (which is UB). And likewise in that example code you're borrowing Focus twice as mutable which violates borrow checking rules. Please don't hesitate to correct me if I'm misunderstanding your issue ^^

[ashneverdawn]

So if I understand correctly, previously there was a race condition and now that it is fixed this is actually expected to break, correct?
If that is the case, I think this should be detected at compile-time somehow rather than just panic. Is there a way we could make the borrow checker detect this? or make it somehow fail otherwise at compile time?

[BoxyUwU]

It wasn't a race condition (the parallel system was a bad example), just us not setting what data a query borrows which meant that it couldn't mark components as borrowed which meant that the runtime borrow check enforcement couldn't work(...I think? I haven't had a chance to properly look into why it happens but I think this is probably why). But yeah, long story short is that this code is expected to panic.

I've absolutely no idea if it's possible to make this kind of thing compile time enforceable though x)

[chemicstry]

Legion doesn't have this problem because iterating a query requires borrowing of World. In case of two nested queries, you can split world into two disjoint sets and borrow each.

Since bevy hides World borrow under query type it is impossible to borrow check system at compile time. One alternative would be to enforce all system's queries to be disjoint, but it would cause inconvenience.

I think the current panic behavior is the best what can be done without sacrificing API usability.

[ashneverdawn]

ok I will close this ticket in that case. Thanks for the info!