Allow query transmutation with immutable query references

[kaphula]

Consider the following example:

#[derive(Component)]
struct A { }
#[derive(Component)]
struct B { }

fn system1(
    my_query: Query<(&A, &B)>
) {
    fn immutable_helper(subquery: &Query<(&A)>) {
        for x in subquery.iter() {
            // bla bla
        }
    }
    let lens_a = my_query.transmute_lens::<(&A)>();
    immutable_helper(&lens_a.query())
}

Why does using transmute_lens() and query() require me to have mutable reference to the query if the query itself is not mutating anything? This prevents me from using transmute lens if the destination of the transmute result accepts only immutable query references. Is there a way around this without forcing the user to perform painful refactorings for all helper functions to accept mutable references of the query, assuming that is even always possible?

Going down the path of definitions from transmute_lens, it would also seem that final transmute function call takes immutable reference anyway, so maybe this feature could be implemented without much hassle?

[kaphula]

So I added immutable variations of transmute_lens and transmute_lens_filtered without any more changes to the bevy source code and it seems it works as this kind of test passes:

    /// ```rust
    /// # use bevy_ecs::prelude::*;
    /// # use bevy_ecs::system::QueryLens;
    /// #
    /// # #[derive(Component)]
    /// # struct A(usize);
    /// #
    /// # #[derive(Component)]
    /// # struct B(usize);
    /// #
    /// # let mut world = World::new();
    /// #
    /// # world.spawn((A(10), B(5)));
    /// #
    /// fn reusable_function(q: &Query<&A>) {
    ///     assert_eq!(q.single().0, 10);
    /// }
    ///
    /// fn system1(query: Query<(&A, &B)>) {
    ///     let mut lens = query.transmute_lens_i::<&A>();
    ///     reusable_function(&lens.query());
    /// }
    ///
    /// # let mut schedule = Schedule::default();
    /// # schedule.add_systems((system1));
    /// # schedule.run(&mut world);
    /// ```
    pub fn transmute_lens_i<NewD: QueryData>(&self) -> QueryLens<'_, NewD> {
        self.transmute_lens_filtered_i::<NewD, ()>()
    }

Therefore it seems to me that it is just a matter of adding immutable and mutable variations of the transmute_lens and transmute_lens_filtered functions to solve this issue? Should I make a PR for it? And if yes, how should the functions be named? Would it be wise to name the new additions with _immutable postfix, or rename the existing functions with _mut postfix (that would be a breaking change from previous version though).

[hymm]

It's unsound to use immutable references. If you do that you can get 2 query lens out of one query and get two mutable references to the same value. If you run the ecs compile fail tests there should be a test checking for this.

It might be valid to allow using &self for read only queries, but it's late here and I haven't thought it through all the way.

[kaphula]

So the problem essentially is that the user can get access to two mutable queries that contain mutable references to same component data? I created a test case here. It compiles and does not crash, but I don't know if this is something that could cause issues otherwise. If this is unwanted behavior, could it be possible to create a compile time mechanism to prevent this kind of access and yet still allow the usage of transmuting read only queries?

I created a fork and started using this feature on my project for now regardless and it works, no issues so far. The changes are here in the last two commits.

[hymm]

Your immutable helper in that code doesn't actually alias the data. It'd be more like:

fn immutable_helper(subquery: &mut Query<(&mut A)>, subquery2: &mut Query<(&mut A)>) {
    let a1 = subquery.iter_mut().next();
    let a2 = subquery.iter_mut().next();
    // oops we violated rusts safety rules since we have 2 &mut references to the same data. This is UB.
    assert_eq!(a1 as *const A, a2 as *const B);
}

Note that just because something doesn't crash that it doesn't mean that it doesn't have undefined behavior.

[alice-i-cecile]

This is an advanced case of #3774.