Reduce the API surface of bevy_ecs::Query

[memoryruins]

Query::new and QuerySet::new functions were marked and documented as unsafe in # 798, which essentially closes #796. Before closing #796, I wanted to see what would happen if these were marked as pub(crate). Everything in bevy's repo continues to build since the usages are only in bevy_ecs. As mentioned in the issue, it is unlikely a user will create their own IntoSystem impl.

In addition, Query::iter_unsafe, Query::get_unsafe and Query::get_component_unsafe are not used in any of bevy's crates. I imagine the latter methods were added to mirror some of World's methods like World::query_unchecked, but I believe there is some value in reducing the API surface of bevy_ecs::Query for learnability and leaving the unsafe interfaces to direct interactions with World.

These changes are split into two commits. I would understand if only the first or none of these changes are desired, but I wanted to open this to float the idea.

[cart]

Cool I support making the constructors pub(crate) (although its worth pointing out that this prevents people from building their own IntoSystem implementations that use Query/QuerySet).

I think I want to keep the unsafe methods around. There will be some patterns (like nested conflicting query iterations) that are safe in practice, but that can't be statically enforced. I don't want to close off the option to drop down to unsafe code if a consumer is willing to take the risk / self validate.

[memoryruins]

Ah yea, I respect the motivations of not trying to prevent something potentially useful for a user.

Before I opened this, I wasn't able to create a motivating example in a query system where safe methods did not already handle it and where the unsafe methods were used correctly.

like nested conflicting query iterations

Do you have an example in mind where this would be okay to do? The following tests a few variations:

use bevy_ecs::{Schedule, prelude::*};

#[derive(Debug)]
struct Foo(pub i32);

fn main() {
    let mut world = World::default();
    let mut resources = Resources::default();

    world.spawn((Foo(42),));

    let mut schedule = Schedule::default();
    schedule.add_stage("update");

    schedule.add_system_to_stage("update", s0.system());
    schedule.add_system_to_stage("update", s1.system());
    schedule.add_system_to_stage("update", s2.system());

    schedule.initialize(&mut world, &mut resources);
    schedule.run(&mut world, &mut resources);
}

// This is fine with safe interface to read only components.
// It will build and print `Foo(42)`.
fn s0(query: Query<&Foo>) {
    for x in query.iter() {
        for _y in query.iter() {
            println!("{:?}", x);
        }
    }
}

// This will build, but will thankfully panic at runtime anyway.
// "&mut test::Foo conflicts with the component access [test::Foo] in this prior query: &mut test::Foo'"
fn s1(q0: Query<&mut Foo>, q1: Query<&mut Foo>) {
    unsafe {
        for x in q0.iter_unsafe() {
            for _y in q1.iter_unsafe() {
                println!("{:?}", x);
            }
        }
    }
}

// This will build and run, but can acquire two mutable borrows
// to the same component and thus is incorrect usage.
// `cargo miri run` will report undefined behavior.
fn s2(query: Query<&mut Foo>) {
    unsafe {
        for x in query.iter_unsafe() {
            for _y in query.iter_unsafe() {
                println!("{:?}", x);
            }
        }
    }
}

[cart]

Something like this:

fn system(mut queries: QuerySet<(Query<&mut A>, Query<(&mut A, &B)>)>) {
  for x in queries.q0().iter_unsafe() {
     if some_criteria_that_ensures_no_conflict {
         if let Ok((a, b)) = queries.q1().get_unsafe() {
         }
     }
  }
}

[memoryruins]

I imagine a common criteria would be checking IDs beforehand in some form.

fn system(q0: Query<With<A, Entity>>, q1: Query<&mut A>) {
    for e0 in q0.iter() {
        for e1 in q0.iter().filter(|&e1| e1 != e0) {
            let entities = unsafe { (q1.get_unsafe(e0), q1.get_unsafe(e1)) };
            if let (Ok(a0), Ok(a1)) = entities {
                print!("{:?}: {:?}", e0, a0);
                println!(", {:?}: {:?}", e1, a1);
            }
        }
    }
}

Anyway, thanks for patiently hearing me out and sharing the motivations.
The commit that removed the methods has now been reverted ^^