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

Using Without in Or filters is unsound #4657

Closed
infmagic2047 opened this issue May 4, 2022 · 1 comment
Closed

Using Without in Or filters is unsound #4657

infmagic2047 opened this issue May 4, 2022 · 1 comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention P-Unsound A bug that results in undefined compiler behavior

Comments

@infmagic2047
Copy link
Contributor

Bevy version

Current main (but should also happen with 0.7 release)

What you did

use bevy::prelude::*;

#[derive(Component, Debug)]
struct Foo(u32);

#[derive(Component, Debug)]
struct Bar(u32);

fn setup(mut commands: Commands) {
    commands.spawn().insert(Foo(1));
}

fn test(
    mut query_1: Query<&mut Foo, Or<(Without<Foo>, Without<Bar>)>>,
    mut query_2: Query<&mut Foo, Without<Bar>>,
) {
    let mut x = query_1.single_mut();
    let mut y = query_2.single_mut();
    x.0 = 1;
    println!("{:?} {:?}", *x, *y);
    y.0 = 2;
    println!("{:?} {:?}", *x, *y);
}

fn main() {
    App::new()
        .add_plugins(MinimalPlugins)
        .add_startup_system(setup)
        .add_system(test)
        .run();
}

What you expected to happen

test should be disallowed as it has conflicting queries.

What actually happened

The code runs successfully and prints Foo(1) Foo(1) and Foo(2) Foo(2) repeatedly.

Additional information

Current Or<Without<A>, Without<B>> get recorded as access.add_without(A); access.add_without(B);, which is the cause of problems here. It is probably too difficult to express the "or" condition precisely (filters can encode arbitrary boolean expressions), and a good solution is to handle such a case as if no filters are given.

@infmagic2047 infmagic2047 added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels May 4, 2022
@DJMcNab DJMcNab added A-ECS Entities, components, systems, and events P-High This is particularly urgent, and deserves immediate attention and removed S-Needs-Triage This issue needs to be labelled labels May 4, 2022
@alice-i-cecile
Copy link
Member

Thanks for spotting this, and the fantastic issue writeup.

@cart cart added the P-Unsound A bug that results in undefined compiler behavior label May 5, 2022
@bors bors bot closed this as completed in 1320818 May 18, 2022
exjam pushed a commit to exjam/bevy that referenced this issue May 22, 2022
…ine#4659)

# Objective

Fixes bevyengine#4657

Example code that wasnt panic'ing before this PR (and so was unsound):
```rust
    #[test]
    #[should_panic = "error[B0001]"]
    fn option_has_no_filter_with() {
        fn sys(_1: Query<(Option<&A>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }

    #[test]
    #[should_panic = "error[B0001]"]
    fn any_of_has_no_filter_with() {
        fn sys(_1: Query<(AnyOf<(&A, ())>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }

    #[test]
    #[should_panic = "error[B0001]"]
    fn or_has_no_filter_with() {
        fn sys(_1: Query<&mut B, Or<(With<A>, With<B>)>>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }
```
## Solution

- Only add the intersection of `with`/`without` accesses of all the elements in `Or/AnyOf` to the world query's `FilteredAccess<ComponentId>` instead of the union.
- `Option`'s fix can be thought of the same way since its basically `AnyOf<T, ()>` but its impl is just simpler as `()` has no `with`/`without` accesses
---

## Changelog

- `Or`/`AnyOf`/`Option` will now report more query conflicts in order to fix unsoundness

## Migration Guide

- If you are now getting query conflicts from `Or`/`AnyOf`/`Option` rip to you and ur welcome for it now being caught
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…ine#4659)

# Objective

Fixes bevyengine#4657

Example code that wasnt panic'ing before this PR (and so was unsound):
```rust
    #[test]
    #[should_panic = "error[B0001]"]
    fn option_has_no_filter_with() {
        fn sys(_1: Query<(Option<&A>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }

    #[test]
    #[should_panic = "error[B0001]"]
    fn any_of_has_no_filter_with() {
        fn sys(_1: Query<(AnyOf<(&A, ())>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }

    #[test]
    #[should_panic = "error[B0001]"]
    fn or_has_no_filter_with() {
        fn sys(_1: Query<&mut B, Or<(With<A>, With<B>)>>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }
```
## Solution

- Only add the intersection of `with`/`without` accesses of all the elements in `Or/AnyOf` to the world query's `FilteredAccess<ComponentId>` instead of the union.
- `Option`'s fix can be thought of the same way since its basically `AnyOf<T, ()>` but its impl is just simpler as `()` has no `with`/`without` accesses
---

## Changelog

- `Or`/`AnyOf`/`Option` will now report more query conflicts in order to fix unsoundness

## Migration Guide

- If you are now getting query conflicts from `Or`/`AnyOf`/`Option` rip to you and ur welcome for it now being caught
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants