Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reflexive world queries #68

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 174 additions & 0 deletions rfcs/68-reflexive-world-queries.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
# Feature Name: `reflexive-world-queries`

## Summary

Types created using `#[derive(WorldQuery)]` are now reflexive,
meaning we no longer generate 'Item' structs for each custom WorldQuery.
This makes the API for these types much simpler.

## Motivation

You, a humble Bevy user, wish to define your own custom `WorldQuery` type.
It should display the name of an entity if it has one, and fall back
to showing the entity's ID if it doesn't.

```rust
#[derive(WorldQuery)]
pub struct DebugName {
pub name: Option<&'static Name>,
pub id: Entity,
}

impl Debug for DebugName {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt:Result {
if let Some(name) = self.name {
write!(f, "{name}")
} else {
write!(f, "Entity {:?}", self.id)
}
}
}
```

This type is easy define, and should be very useful.
However, you notice a strange error when trying to use it:

```rust
fn my_system(q: Query<DebugName>) {
for dbg in &q {
println!("{dbg:?}");
}
}
```

```
error[E0277]: `DebugNameItem<'_>` doesn't implement `Debug`
--> crates\bevy_core\src\name.rs:194:20
|
194 | println!("{dbg:?}");
| ^^^ `DebugNameItem<'_>` cannot be formatted using `{:?}`
|
= help: the trait `Debug` is not implemented for `DebugNameItem<'_>`
= note: add `#[derive(Debug)]` to `DebugNameItem<'_>` or manually `impl Debug for DebugNameItem<'_>`
= note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
```

"What do you mean rustc?!" you plead as you bang your head against the wall. You just implemented `Debug`!

The problem here is that the type returned from the query is *not* the type we just defined
-- it's a hidden macro-generated type with near-identical fields.
Here's what it looks like in the docs:

```rust
pub struct DebugNameItem<'__w> {
pub name: <Option<&'static Name> as WorldQuery>::Item<'__w>,
pub id: <Entity as WorldQuery>::Item<'__w>,
}
```

In order to fix the error, we need to implement `Debug` for *this* type:

```rust
impl Debug for DebugNameItem<'_> { ... }
```

This avoids the compile error, but it results in an awkward situation where our documentation,
trait impls, and methods are fragmented between two very similar types with a non-obvious distinction between them.

Since the `DebugNameItem` is generated in a macro, it is very awkard to flesh out its API.
Its documentation is necessarily vague since you can't write it yourself, and deriving traits
requires the special syntax `#[world_query(derive(PartialEq))]`.

## User-facing explanation

### **Reflexive**

1. (Adjective) *In reference to oneself.*

Any `WorldQuery` type defined with `#[derive(WorldQuery)]` must be reflexive,
meaning it returns itself when used in a query.
Most `WorldQuery` types are reflexive, and may be used without consideration of this property.
A notable exception is `&mut T`, which is incompatible with the derive macro.
`Mut<T>` must be used instead.

Examples:

```rust
#[derive(WorldQuery)]
struct DebugName<'w> {
name: Option<&'w Name>,
id: Entity,
}

impl Debug for DebugName<'_> { ... }

#[derive(WorldQuery)]
#[world_query(mutable)]
struct PhysicsComponents<'w> {
mass: &'w Mass,
transform: Mut<'w, Transform>,
velocity: Mut<'w, Velocity>,
}
```

## Implementation strategy

Changing the derive macro to be reflexive should be a trivial change
-- the `SystemParam` derive already works this way,
so the details will not be discussed in this RFC.

To support this change, we will need to rework some of our built-in
`WorldQuery` types to be reflexive.

```rust
fn any_system(q: Query<AnyOf<(&A, &B, &C)>>) {
// Before:
for (a, b, c) in &q {
...
}

// After:
for AnyOf((a, b, c)) in &q {
...
}
}

fn changed_system(q: Query<Changed<A>>) {
// Before:
for changed_a in &q {
...
}

// After:
for Changed(changed_a) in &q {
...
}
}
```

Since `&mut T` is not reflexive, we will have to implement `WorldQuery` for `Mut<T>`.

## Drawbacks

This will add slightly more friction in some cases, since
types such as `&mut T` are forbidden from being used in the derive macro.
Copy link
Member

@james7132 james7132 Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my biggest gripe with this approach. This sacrifices the simpler mental model for normal users and happy path and forces any mutative query to use Mut<T> over &mut T, and that mapping from &T to &mut T is now lost.

We can address this by implementing &mut T: World Query, and just forcibly mark everything matched as mutated, but that would make the distinction between &mut T and Mut<T> extremely nuanced.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm nervous about that, especially because of how subtly it breaks existing code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my biggest gripe with this approach. This sacrifices the simpler mental model for normal users and happy path and forces any mutative query to use Mut<T> over &mut T, and that mapping from &T to &mut T is now lost.

Not sure if we're on the same page, but I want to clarify that you can still write Query<&mut T> and have it return Mut<T> -- the reflexive constraint is only for named WorldQuery structs made using the derive macro. Very little user code will be affected in practice. I think this constraint will surprise users at first, but I also think it will make sense when they think about it, or read the docs and have it explained to them (we'll have to think of a good way of explaining this that would make it click more easily). Also, this behavior is consistent with how the SystemParam derive macro works, which might make it easier to understand for some users.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another point I want to add: using &mut T in a derived WorldQuery already fails with a confusing type error, until you read the docs and see that you need the special attribute #[world_query(mutable)]. Since we're already relying on the user to read the docs for this macro to be usable, I don't think there's much of a regression caused by requiring world queries to be reflexive.

However, these types can still be used in anonymous `WorldQuery`s or types
such as `type MyQuery = (&'static mut A, &'static mut U)`.

Since the `WorldQuery` derive is only used in cases where the user wishes
to expose a public API, we should prioritize the cleanliness of that API
over the ease of implementing it.

## Rationale and alternatives

Do nothing. The current situation is workable, but it's not ideal.
Generated 'Item' structs add signifcant noise to a plugin's API,
and make it more difficult to understand due to having several very similar types.

## Unresolved questions

None.

## Future possibilities

We should explore ways to make the `#[world_query(mutable)]` attribute unnecessary.