Compile Time Query checking and System Parameters #17837

MalekiRe · 2025-02-13T01:04:20Z

Objective

Compile-Time Query Validation

This introduces compile-time validation for Query and System Parameters to catch invalid system configurations during compilation rather than at runtime. This provides earlier feedback to developers and eliminates a class of potential runtime errors.

Solution

Solution Architecture

Core Mechanism

The solution leverages Rust's const evaluation capabilities to detect invalid query and system parameter combinations at compile time through a tree-based validation system. Three key structures form the backbone of this validation:

pub struct ComponentAccessTree {
    pub this: ComponentAccess,
    pub left: Option<&'static ComponentAccessTree>,
    pub right: Option<&'static ComponentAccessTree> 
}

pub struct WithFilterTree {
    pub this: WithId,
    pub left: &'static Option<WithFilterTree>,
    pub right: &'static Option<WithFilterTree>
}

pub struct WithoutFilterTree {
    pub this: WithoutId,
    pub left: &'static Option<WithoutFilterTree>,
    pub right: &'static Option<WithoutFilterTree>
}

These trees are built up recursively through const evaluation to track:

Component access patterns (&T vs &mut T)
With/Added/Changed filters
Without filters

Implementation Details

The validation system is built on a set of traits that provide default no-op implementations and allow system parameters to opt-in to validation:

// Base trait with default opt-out behavior
pub trait SystemParam: Sized {
    const COMPONENT_ACCESS_TREE: ComponentAccessTree = ComponentAccessTree {
        this: ComponentAccess::Ignore,
        left: None,
        right: None,
    };
    const WITH_FILTER_TREE: Option<WithFilterTree> = None;
    const WITHOUT_FILTER_TREE: Option<WithoutFilterTree> = None;
}

// Components must now provide a unique ID
impl Component for MyComponent {
    const UNSTABLE_TYPE_ID: u128 = /* unique random number */;
    #[cfg(feature = "diagnostic_component_names")]
    const STRUCT_NAME: Option<&'static str> = Some("MyComponent");
}

The system builds up validation trees recursively through generic implementations:

// For immutable references
unsafe impl<T: Component> QueryData for &T {
    const COMPONENT_ACCESS_TREE_QUERY_DATA: ComponentAccessTree = Self::COMPONENT_ACCESS_TREE;
}

// For filters
unsafe impl<T: Component> QueryFilter for With<T> {
    const WITH_FILTER_TREE_QUERY_DATA: Option<WithFilterTree> = Some(WithFilterTree {
        this: WithId(Some(T::UNSTABLE_TYPE_ID)),
        left: &None,
        right: &None,
    });
}

Complex queries automatically build their validation trees through modifying the existing and building up their recursive trees:

macro_rules! impl_tuple_query_data {
    ($($param: ident),*) => {
        unsafe impl<$($param: QueryData),*> QueryData for ($($param,)*) {
            const COMPONENT_ACCESS_TREE_QUERY_DATA: ComponentAccessTree = 
                impl_tuple_query_data!(@tree $($param),*);
        }
    };
}

Example Usage

The system can detect invalid query combinations at compile time:

// This compiles - queries are disjoint due to With/Without
fn valid(query1: Query<&mut Age, With<Awa>>, query2: Query<&mut Age, Without<Awa>>) {
}

// This fails to compile - potential mutable access conflict
fn invalid(query1: Query<&mut Age, With<Awa>>, query2: Query<&mut Age>) {
}

With diagnostic_component_names enabled, you get clear error messages:

error: Invalid System Queries, &mut Age conflicts with &mut Age in same system

Performance Impact

Benchmarks show minimal impact:

Binary size increase: ~0.006% (1020710736 -> 1020710736 bytes)
Runtime overhead: ~2-3 seconds on alien cake addict compiled from scratch
- Main branch: (1m10s,1m10s,1m10s)
- My branch: (1m12s, 1m12s, 1m13s)

Cool Things

Earlier Error Detection: Invalid query combinations are caught at compile time rather than runtime
Zero Runtime Cost: All validation happens during compilation, which means we can later rip out our runtime access checks!!!! ( We will have to make the Component trait unsafe in order to do so however )
Clear Error Messages: With diagnostic names enabled, errors clearly identify conflicting components
Opt-In System: Default implementations mean existing code works without modification
Complex Validation: Can detect conflicts in complex scenarios involving multiple queries and filters

Testing

Validated against alien cake addict benchmark + whole bevy engine
I will work on a comprehensive test suite for various invalid query combinations

Migration Guide

When implementing Component manually you must now include a value for CONST_UNSTABLE_TYPEID: u128

Make sure it's unique by randomly generating it before hand, if we rip out access checks then not making sure it's unique will be Undefined Behavior 🙀.

BD103 · 2025-02-13T03:27:26Z

crates/bevy_ecs/macros/src/component.rs

    // This puts `register_required` before `register_recursive_requires` to ensure that the constructors of _all_ top
    // level components are initialized first, giving them precedence over recursively defined constructors for the same component type
    TokenStream::from(quote! {
        impl #impl_generics #bevy_ecs_path::component::Component for #struct_name #type_generics #where_clause {
            const STORAGE_TYPE: #bevy_ecs_path::component::StorageType = #storage;
+            const UNSTABLE_TYPE_ID: u128 = #unstable_type_id;


Is it possible to make this value deterministic? I think that's preferable from it randomly changing whenever cargo clean && cargo build is ran.

I don't think so. Using something like type_id::<Self>() would be ideal, but you'd need a nightly feature flag to make it const.

I guess you could do some sort of hashing that takes for input the component's ast, but that still probably isn't enough to avoid collisions with other components. It would also drastically increase the complexity of this pr.

I agree it'd be preferable, but what benefits are there to making it deterministic?

Yeah using type_id:: would be ideal, but we don't want to use any sort of deterministic count because I've heard from the grape vine that rustc might mess with things so we could end up with UB. Unless I have a guarantee that IDs are never going to repeat I would rather safely stick with a random u128.

cBournhonesque · 2025-02-13T14:24:13Z

Personally I am not a fan of this. The PR is technically impressive, and the fact that you reached this point is amazing, but I think that this PR adds a lot of extra complexity that not many people can maintain. On top of that the compile-time benefits are probably limited to a subset of cases as I assume that you weren't able to encode every possible combination of QueryData/QueryFilters.
We also get increases in compilation time, which are probably small when compiling examples but can get large with bigger apps.

Also I would say that writing invalid queries is relatively rare, rare enough that I wouldn't get any benefit from it compared to the app simply panicking at runtime. The usual tricky case is forgetting to add a Without to a query after adding With to another param, and I think it's good enough to catch this at runtime

JeanMertz · 2025-02-13T15:19:36Z

Also I would say that writing invalid queries is relatively rare, rare enough that I wouldn't get any benefit from it compared to the app simply panicking at runtime.

I assume the end-goal would be for a lot less (or none?) of these checks to be needed at runtime, which would mean we could be looking at getting back runtime performance for the cost of additional code maintenance and a bit of upfront compilation time cost.

I don't have the numbers, and I don't know the end-goal here, but depending on how far this can be pushed with as little impact to (non-engine) developers, if we can squeeze out measurable runtime performance by reducing the number of checks done at runtime, then that seems worth exploring.

Also, hat off to @MalekiRe for getting this to work with a relatively small diff (not to discount the experimentation it probably took to get to this final diff). 🎩

My proposal would be to let this cook for a bit, let's see how far this can be pushed, and measure the pro's and con's in a week, month or even months from now. There's no need to rush this, but if the numbers stack in favour of this change, then I presume this becomes a lot less "controversial".

As for some technical feedback from my side: The UNSTABLE_TYPE_ID associated const on Component is unfortunate, but I wonder how many people manually implement the trait. I do agree that having that ID be generated in a deterministic way would be favourable.

cBournhonesque · 2025-02-13T15:26:48Z

If I understand correctly the runtime checks are done only at query/system creation time, so only once and not every frame.
And for sure please do keep cooking :)
The work done here could potentially unblock #13375 for the mutable variants

MalekiRe · 2025-02-14T03:27:46Z

@cBournhonesque

Personally I am not a fan of this. The PR is technically impressive, and the fact that you reached this point is amazing, but I think that this PR adds a lot of extra complexity that not many people can maintain. On top of that the compile-time benefits are probably limited to a subset of cases as I assume that you weren't able to encode every possible combination of QueryData/QueryFilters. We also get increases in compilation time, which are probably small when compiling examples but can get large with bigger apps.

The increase in compilation time here seems pretty small, but I don't have any larger codebases up to date with main bevy to test it on.

I don't think the actual complexity is very much. The idea is complex but the code itself is simple, if that makes sense?

I was indeed able to encode every possible combination of QueryData and QueryFilters 😊

chescock

It's really really cool that this is even possible! Nice work!!

crates/bevy_ecs/macros/src/component.rs

chescock · 2025-02-14T14:50:46Z

crates/bevy_ecs/src/query/fetch.rs

-unsafe impl<T: Component> QueryData for &T {
+unsafe impl<T: Component> QueryData for &T
+where
+    for<'a> &'a T: AccessTreeContainer,


Why do these bounds need to be higher-ranked, and not just Self: AccessTreeContainer? It looks like they are only used for Self::COMPONENT_ACCESS_TREE.

For that matter, do you need AccessTreeContainer at all? Given that UNSTABLE_TYPE_ID and STRUCT_NAME are already on Component, it looks like all it does is share a bit of code between &T and Ref<T> and between &mut T and Mut<T>. It might be simpler to remove the trait and do

const COMPONENT_ACCESS_TREE_QUERY_DATA: ComponentAccessTree = <&T as QueryData>::COMPONENT_ACCESS_TREE;

to share the code.

I don't think this works? We have an implementation ON &mut T where T is component because we wanna be able to tell if a component access is a ref or a mut.

Doing where Self: AccessTreeContainer seems to compile when I try it.

And copy-pasting the definition from impl<T: Component> AccessTreeContainer for &T to unsafe impl<T: Component> QueryData for &T works other than privacy, which should let you get rid of AccessTreeContainer completely. If you want to leave them private then I expect you could make a const fn new, although I didn't try that.

... And I don't see a definition of COMPONENT_ACCESS_TREE at all in QueryData for Ref or Mut? Well, if I add

const COMPONENT_ACCESS_TREE_QUERY_DATA: ConstTree<ComponentAccess> = <&T as QueryData>::COMPONENT_ACCESS_TREE_QUERY_DATA;

to unsafe impl<'__w, T: Component> QueryData for Ref<'__w, T> { then it seems to work, so you could still share the definitions between &T and Ref<T>.

chescock · 2025-02-14T14:56:00Z

crates/bevy_ecs/src/query/fetch.rs

+        )
+    };
+    // Handle three or more items case
+    (@tree $t0:ident, $t1:ident, $($rest:ident),+) => {


Why do you need separate cases for two and three items? It seems like you could make the two-item case take (@tree $t0:ident, $($rest:ident),+) and do the induction there.

Alternately, I think you can do this without recursion. It might be simpler as something like

const COMPONENT_ACCESS_TREE_QUERY_DATA: ComponentAccessTree = const { let tree = ComponentAccessTree { this: ComponentAccess::Ignore, left: None, right: None }; $( let tree = ComponentAccessTree::combine(tree, $name); )* tree };

let tree = &ConstTreeInner::<ComponentAccess>::combine(tree, $name::COMPONENT_ACCESS_TREE_QUERY_DATA); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | | | creates a temporary value which is freed while still in use | argument requires that borrow lasts for `'static`

Sadly it seems like this doesn't work. But if you could elaborate on how to do the two item case instead I would like to switch to that!

Oh, I see. I was expecting it to do static promotion... but that's only possible if the inner values are themselves const, and tree isn't const. And if you try to make a bunch of nested const TREE = then it complains that they "can't use generic parameters from outer item". Yuck.

But if you could elaborate on how to do the two item case instead I would like to switch to that!

The simplest change is

(@tree) => { &ConstTreeInner::Empty }; (@tree $t0:ident) => { $t0::COMPONENT_ACCESS_TREE_QUERY_DATA }; (@tree $t0:ident, $($rest:ident),+) => { &ConstTreeInner::<ComponentAccess>::combine( $t0::COMPONENT_ACCESS_TREE_QUERY_DATA, impl_tuple_query_data!(@tree $($rest),+) ) };

But you could go one further with

// Base case (@tree) => { &ConstTreeInner::Empty }; // Inductive case (@tree $t0:ident, $($rest:ident),*) => { &ConstTreeInner::<ComponentAccess>::combine( $t0::COMPONENT_ACCESS_TREE_QUERY_DATA, impl_tuple_query_data!(@tree $($rest),*) ) };

for the cost of an extra combine(Empty).

... and for that matter, since we're already generating that same code for smaller tuples, you could use their trees rather than doing a second recursion:

// Base case (@tree) => { &ConstTreeInner::Empty }; // Inductive case (@tree $t0:ident, $($rest:ident),*) => { &ConstTreeInner::<ComponentAccess>::combine( $t0::COMPONENT_ACCESS_TREE_QUERY_DATA, <($($rest,)*)>::COMPONENT_ACCESS_TREE_QUERY_DATA ) };

That should generate a little less code.

chescock · 2025-02-14T14:57:33Z

crates/bevy_ecs/src/system/system_param.rs

+    /// A compile-time representation of how this system parameter accesses components
+    /// Used for validating access patterns during const evaluation
+    const COMPONENT_ACCESS_TREE: ComponentAccessTree = ComponentAccessTree {
+        this: ComponentAccess::Ignore,


You build this empty tree in three places. Is it worth making a const ComponentAccessTree::EMPTY?

Updated the code so I don't think this comment is relevant anymore. Resolve at your discretion.

chescock · 2025-02-14T15:05:06Z

crates/bevy_ecs/src/schedule/config.rs

@@ -570,6 +579,15 @@ macro_rules! impl_system_collection {
        where
            $($sys: IntoSystemConfigs<$param>),*
        {
+            const INTO_SYSTEM_CONFIGS_PANIC_CHECKER: Option<SystemPanicMessage> = const {
+                let mut val = None;
+                $(if let Some(awa) = $sys::INTO_SYSTEM_CONFIGS_PANIC_CHECKER {


This will only report one error, even if multiple systems have errors. Is it worth trying to aggregate them?

I don't think so? If we really want to then we can by doing a similar tree structure with SystemPanicMessages but I don't think it's worth it. What do you think?

Yeah, reporting only one is fine. It's unlikely someone will have multiple bad systems, and I think we'll panic on the first one today, so it's still a strict improvement!

crates/bevy_ecs/src/system/const_param_checking.rs

chescock · 2025-02-14T15:47:33Z

crates/bevy_ecs/src/system/const_param_checking.rs

+            &'static Option<WithoutFilterTree>,
+        ),
+    ) -> Option<SystemPanicMessage> {
+        if let Some(left_without_tree) = left.2 {


This is essentially a const version of FilteredAccess::get_conflicts, right? The model here is slightly different, and doesn't look like it can handle Option, Or, AnyOf, or Changed/Added correctly.

Do you want to handle those cases?

If you do, then I think you want to move all three trees from QueryData and QueryFilter up to WorldQuery.

Changed and Added perform read access, so, for example, Query<&mut T> and Query<(), Changed<T>> should conflict. Moving the component access to WorldQuery will let you populate that on those filters. (Note that Query<&mut T, Changed<T>> is fine, since the filter is evaluated before the item.)

Option<T> performs read access, but doesn't filter. So, for example Query<(Option<&T>, &mut U)> and Query<&mut U, Without<T>> do conflict, but I think your current check can't express that. To be consistent with the run-time check, you'll want to have &T/&mut T/Ref<T>/Mut<T> also register a With filter, and then have Option ignore the nested With filters. Moving the "with" filters to WorldQuery will let you populate those.

Or and AnyOf are a little more complicated. The model here has a list of filters that are "and"ed together, but the full access model supports a list of those that is "or"ed together. If you want to support those, I think you need a new kind of tree whose leaves have both a tree of "with" filters and a tree of "without" filters, just as FilteredAccess has filter_sets: Vec<AccessFilters<T>>, and AccessFilters has a list of "with" and "without" filters.

Oh, and if you want to handle EntityRef and EntityMut, then you need to express "all access". And if you want to handle EntityRefExcept<T> then you need to express "all access except".

If you reread the code now it might be clearer. It does indeed handle things like Changed and Added and Option, and it doesn't look for conflicts on the same query with the same query filter so things like Query<&mut Age, With<Age>> don't conflict but things like query1: Query<&mut Age>, query2: Query<With<Age>> do.

I'll have to look at it more. It looked like With<T> and Changed<T> had the same implementation, though. Is that right? Because this should be fine:

fn good_system(query1: Query<&mut Age>, query2: Query<(), With<Age>>)

But this should conflict:

fn bad_system(query1: Query<&mut Age>, query2: Query<(), Changed<Age>>)

And it looked like Option<T> just uses the access from T. Is that right? Because this should be fine:

fn good_system(query1: Query<&mut Name, Without<Age>>, query2: Query<(&mut Name, &Age)>)

But this should conflict:

fn bad_system(query1: Query<&mut Name, Without<Age>>, query2: Query<(&mut Name, Option<&Age>)>)

crates/bevy_ecs/src/system/function_system.rs

chescock · 2025-02-14T16:29:46Z

crates/bevy_ecs/src/system/const_param_checking.rs

+    Mut,
+}
+
+/// A tree structure representing the `With<T>`, `Added<T>`, and `Changed<T>` filters


If I understand correctly, these are semantically unordered sets, and are implemented as trees rather than lists to allow efficient concatenation. Is that right? It might be useful to have a comment to that effect.

MalekiRe · 2025-02-15T23:01:13Z

@chescock I'm gonna request a re-review now that I updated the code structure significantly, because perhaps you have new thoughts. I am going to continue to work through your other comments though.

MalekiRe · 2025-02-16T01:15:03Z

I'm currently going through and porting over all the tests that should fail now to the ecs compile fail tester

:

MalekiRe added 2 commits February 12, 2025 18:19

working but not with query filters

b374ef0

query filters work

20f086a

MalekiRe marked this pull request as draft February 13, 2025 01:36

BD103 reviewed Feb 13, 2025

View reviewed changes

got stuff more working

f2d8a31

MalekiRe added 6 commits February 13, 2025 19:14

good error messages

214e84c

progress

a99b68c

made component diagonostic names a configurable feature

7b1e737

more doc comments

747b881

finished comments

e9866ad

fixed typo

c81744b

MalekiRe marked this pull request as ready for review February 14, 2025 03:25

MalekiRe added 4 commits February 13, 2025 22:37

removed un-needed dependancies

44aa6fb

reverted change_detection example

d00b235

okay now ACTUALLY reverted change detection

a556045

made the name feature actually work yay

e1a20c6

MalekiRe requested review from chescock and Victoronz February 14, 2025 03:46

formatting

7517ac8

chescock reviewed Feb 14, 2025

View reviewed changes

made the code way more succinct and understandable

47e4414

MalekiRe requested a review from chescock February 15, 2025 23:01

MalekiRe added 3 commits February 15, 2025 18:07

cleaned up macro code and made systems go to 16 not 15

bd6b324

some docs and foramtting

1cd9270

doc comments

ad0b84e

MalekiRe added 4 commits February 15, 2025 20:39

ported over some of the runtime tests to compile time

148a1f3

forgot to format the macro crate

5d2cb10

:

saving this here, swtiching to another branch

8f798a4

uwu

48a92ac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile Time Query checking and System Parameters #17837

Compile Time Query checking and System Parameters #17837

MalekiRe commented Feb 13, 2025 •

edited

Loading

BD103 Feb 13, 2025

MarcGuiselin Feb 13, 2025 •

edited

Loading

MalekiRe Feb 14, 2025

cBournhonesque commented Feb 13, 2025 •

edited

Loading

JeanMertz commented Feb 13, 2025 •

edited

Loading

cBournhonesque commented Feb 13, 2025

MalekiRe commented Feb 14, 2025

chescock left a comment

chescock Feb 14, 2025

MalekiRe Feb 15, 2025

chescock Feb 16, 2025

chescock Feb 14, 2025

MalekiRe Feb 15, 2025

chescock Feb 16, 2025

chescock Feb 14, 2025

MalekiRe Feb 15, 2025

chescock Feb 14, 2025

MalekiRe Feb 15, 2025

chescock Feb 16, 2025

chescock Feb 14, 2025

MalekiRe Feb 15, 2025 •

edited

Loading

chescock Feb 16, 2025

chescock Feb 14, 2025

MalekiRe commented Feb 15, 2025

MalekiRe commented Feb 16, 2025

Compile Time Query checking and System Parameters #17837

Are you sure you want to change the base?

Compile Time Query checking and System Parameters #17837

Conversation

MalekiRe commented Feb 13, 2025 • edited Loading

Objective

Compile-Time Query Validation

Solution

Solution Architecture

Core Mechanism

Implementation Details

Example Usage

Performance Impact

Cool Things

Testing

Migration Guide

Choose a reason for hiding this comment

MarcGuiselin Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cBournhonesque commented Feb 13, 2025 • edited Loading

JeanMertz commented Feb 13, 2025 • edited Loading

cBournhonesque commented Feb 13, 2025

MalekiRe commented Feb 14, 2025

chescock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalekiRe Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalekiRe commented Feb 15, 2025

MalekiRe commented Feb 16, 2025

MalekiRe commented Feb 13, 2025 •

edited

Loading

MarcGuiselin Feb 13, 2025 •

edited

Loading

cBournhonesque commented Feb 13, 2025 •

edited

Loading

JeanMertz commented Feb 13, 2025 •

edited

Loading

MalekiRe Feb 15, 2025 •

edited

Loading