Question regarding mutability of Query::iter_mut()

[Bromeon]

Hello everyone!
On https://bevyengine.org/news/bevy-0-4/, there's this code snippet:

// on each update this system runs once and internally iterates over each entity
fn system(time: Res<Time>, query: Query<(Entity, &mut Transform)>) {
    for (entity, mut transform) in query.iter_mut() {
        // do per-entity logic here
    }
}

Since iter_mut() takes &mut self, the function parameter should be mut query, no?

---

Given that, I'll try to fill out the feature template, although it's more of a general API discussion than a concrete request 🙂

What problem does this solve or what need does it fill?

For me, it's not immediately obvious that Query::iter_mut() operates on &mut self, as logically, the query itself is not mutated, but rather the components being queried.
Also, iter() already provides a specific trait bound ReadOnlyFetch, so it cannot be used for mutable queries:

/// Iterates over the query results. This can only be called for read-only queries
#[inline]
pub fn iter(&self) -> QueryIter<'_, Q, F>
    where
        Q::Fetch: ReadOnlyFetch, // <-- this bound
{
    // SAFE: system runs without conflicts with other systems.
    // same-system queries have runtime borrow checks when they conflict
    unsafe { self.world.query_unchecked() }
}

/// Iterates over the query results
#[inline]
pub fn iter_mut(&mut self) -> QueryIter<'_, Q, F> {
    // SAFE: system runs without conflicts with other systems.
    // same-system queries have runtime borrow checks when they conflict
    unsafe { self.world.query_unchecked() }
}

Is the actual reason to enforce unique access (non-aliasing), so that no two mutable queries can run at the same time? The only thing I can imagine here is doing that in a nested loop, and doing so would probably not make sense for iter() queries either.

What solution would you like?

If compatible with safety, allowing iter_mut() on &self could be considered.
If not, we can document the background why &mut self is used.

What alternative(s) have you considered?

Leaving everything as is, fixing the 0.4 release notes and improving documentation.

Additional context

I looked at #753 (aligning with Rust conventions) and #796 (a bug related to safety, although not directly related to this).

[Bromeon]

Maybe an example: a small system that checks which gardeners have planted a flower, and updates a tilemap accordingly.

fn plant_flower_system(
    query: Query<(&Gardener, )>,
    mut tilemap: ResMut<TileMap>,
) {
    for (gardener,) in query.iter() {
        if gardener.has_flower {
            tilemap.set_tile(/* ... */);
        }
    }
}

Now I extend this system and want to play an animation when the flower is planted, so I need mutable access to an Animation component:

fn plant_flower_system(
    mut query: Query<(&Gardener, &mut Animation)>,
    mut tilemap: ResMut<TileMap>,
) {
    for (gardener, mut anim) in query.iter_mut() {
        if gardener.has_flower {
            tilemap.set_tile(/* ... */);
            anim.play("plant_flower");
        }
    }
}

As you see, this requires changing the iterator as well as the parameter's mutability.
The fact that something is mutated now appears in 4 (!) different places:

• mut query
• &mut Animation
• mut anim
• iter_mut()

This is rather repetitive and not the nicest from an ergonomics point of view; I can understand if this is strictly required for safety however. But maybe there's another way?

[cart]

Yeah this is a safety issue. We can't allow multiple mutable queries because it could result in aliased mutability. Nesting query iterations is one way this could happen, but there are other more common patters, such as for (e, x) in q1.iter_mut() { let y = q2.get_mut(e) } and collecting specific items from a query, then using the results alongside a second query. The alternative to the "lifetime" approach is to add runtime safety checks, which has a measurable cost (see the Bevy 0.2 release blog post).

Instead we made it possible to define multiple queries with separate "lifetimes". As long as they don't conflict, you can mutably borrow all of them at the same time. If they do conflict, you need to group them together in QuerySets (to ensure a single borrow can happen at a given time). This is a "runtime check" but it only needs to happen once instead of on every access.

There is no better way to do this that I'm aware of without incurring a significant runtime cost. Legion uses "world splitting", which is less ergonomic and harder to reason about (imo). But the general idea is similar to the approach we use now.

I'll leave this open for a bit in case anyone has ideas, but I'm inclined to close this as I've already put a lot of thought into the problem. We can open new issues if someone finds a better alternative to the current approach, but I'm skeptical that it will happen.

[Bromeon]

Thank you very much for your answer! So, indeed the issue is exclusive/unique access to the components, which can only be statically enforced using &mut.

It seems like bevy_ecs::Query is mostly split in two groups of methods: those that operate on &mut self, and those that operate on &self and have a ReadOnlyFetch bound, to prevent them from being called on queries over mutable components.

---

One idea that came to my mind is the type-state pattern, which is an alternative way of modeling mutually exclusive "feature-sets" -- here the read-only and read-write accessors in a query:

// Type tags to denote a query "read-only" or "mutable"
trait State {}
struct Mutable;
struct ReadOnly;
impl State for Mutable {}
impl State for ReadOnly {}

pub struct Query<T, S: State> {
    _ph: PhantomData<(T, S)>,
}

impl<T, S: State> Query<T, S> {
    pub fn new() -> Self { Self { _ph: PhantomData } }
}

The query could have distinct implementations, depending on the State type-tag (note that the method has the same name):

impl<T> Query<T, Mutable> {
    pub fn iter(&mut self) { println!("mutable iter()"); }
}

impl<T> Query<T, ReadOnly> {
    pub fn iter(&self) { println!("read-only iter()"); }
}

And some metaprogramming could even allow to infer the State type-tag depending on the generic argument T, so that the user would never have to mention it explicitly:

trait Decide {
    type State: State;
}

impl<T> Decide for &T {
    type State = ReadOnly;
}

impl<T> Decide for &mut T {
    type State = Mutable;
}

// so the declaration becomes:
pub struct Query<T, S: State = <T as Decide>::State> {
    _ph: PhantomData<(T, S)>,
}

Now applied, this would look as follows:

pub fn test() {
    let mut qmut = Query::<&mut i32>::new(); // Mutable inferred
    let qread = Query::<&i32>::new(); // ReadOnly inferred

    qmut.iter(); // prints "mutable iter()"
    qread.iter(); // prints "read-only iter()"
}

---

Applied to this specific issue, it would allow a small ergonomic improvement (mostly allow to write iter() instead of iter_mut(), which would also address #753). But the question is, if such a pattern could be beneficial for other reasons? Just brainstorming here 🙂

[Lythenas]

Personally I think calling iter_mut is clearer than calling just iter on a mutable query. Especially if it is somewhere in a larger system function. So I would keep the current names.

But maybe the error messages are a bit easier to understand if you try to call iter on a mutable query (or iter_mut on a read only query). E.g. the above code (with iter renamed to iter_mut for the mutable query) would produce errors like this:

error[E0599]: no method named `iter` found for struct `Query<&mut i32, Mutable>` in the current scope
  --> src/main.rs:41:10
   |
21 | pub struct Query<T, S: State = <T as Decide>::State> {
   | ---------------------------------------------------- method `iter` not found for this
...
41 |     qmut.iter(); // prints "mutable iter()"
   |          ^^^^ method not found in `Query<&mut i32, Mutable>`

error[E0599]: no method named `iter_mut` found for struct `Query<&i32, ReadOnly>` in the current scope
  --> src/main.rs:42:11
   |
21 | pub struct Query<T, S: State = <T as Decide>::State> {
   | ---------------------------------------------------- method `iter_mut` not found for this
...
42 |     qread.iter_mut(); // prints "read-only iter()"
   |           ^^^^^^^^ method not found in `Query<&i32, ReadOnly>`

(https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6ef1859681df5af2daddda30d2c65ef8)

Instead of something like:

error[E0277]: the trait bound `bevy_ecs::core::query::FetchMut<RenderableMap>: ReadOnlyFetch` is not satisfied
  --> src/map.rs:78:11
   |
78 |     query.iter();
   |           ^^^^ the trait `ReadOnlyFetch` is not implemented for `bevy_ecs::core::query::FetchMut<RenderableMap>`
   |
   = note: required because of the requirements on the impl of `ReadOnlyFetch` for `(bevy_ecs::core::query::EntityFetch, bevy_ecs::core::query::FetchMut<RenderableMap>)`

[Bromeon]

Personally I think calling iter_mut is clearer than calling just iter on a mutable query. Especially if it is somewhere in a larger system function. So I would keep the current names.

As mentioned above, there's already 4 places where "mut" appears in one form or another, so there's enough repetition to not miss it. Furthermore, the important part to convey is which components are mutated by a system -- the query itself is a means to get those components, and the fact that it needs &mut access is mostly a technical issue (uniqueness).

If I have

for (spatial, damage, mut health) in query.iter() { ... }

, I know immediately that this system mutates health but not any other components. This mutation is additionally expressed twice in the function signature, which is the first thing you see if you look at a system:

fn system(
    mut query: Query<(&Spatial, &Damage &mut Health)> )
//  ^                                    ^
//  mutates anything                     mutates this component

(ok, the top-level mut is strictly not part of the function signature, but in practice it's often there in the code).

Or, to compare it with resources: you specify only at the function signature whether they are Res (read-only) or ResMut (mutable). At the point where you access them, you implicitly dereference them, either mutably or immutably, but you don't see it from the code. For components, you at least have the mut health keyword in the for loop to express that.

[alice-i-cecile]

@NathanSWard, as discussed in #2271, would you like me to assign this to you with the understanding that it will likely be a couple weeks until you get to it?

[cart]

I don't think we should assign work until someone has "started" on it. It should be first-come-first serve by default.