[Merged by Bors] - yeet unsound lifetime annotations on `Query` methods #4243

BoxyUwU · 2022-03-17T17:06:02Z

Objective

Continuation of #2964 (I really should have checked other methods when I made that PR)

yeet unsound lifetime annotations on Query methods.
Example unsoundness:

use bevy::prelude::*;

fn main() {
    App::new().add_startup_system(bar).add_system(foo).run();
}

pub fn bar(mut cmds: Commands) {
    let e = cmds.spawn().insert(Foo { a: 10 }).id();
    cmds.insert_resource(e);
}

#[derive(Component, Debug, PartialEq, Eq)]
pub struct Foo {
    a: u32,
}
pub fn foo(mut query: Query<&mut Foo>, e: Res<Entity>) {
    dbg!("hi");
    {
        let data: &Foo = query.get(*e).unwrap();
        let data2: Mut<Foo> = query.get_mut(*e).unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.single();
        let data2: Mut<Foo> = query.single_mut();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.get_single().unwrap();
        let data2: Mut<Foo> = query.get_single_mut().unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.iter().next().unwrap();
        let data2: Mut<Foo> = query.iter_mut().next().unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let mut opt_data: Option<&Foo> = None;
        let mut opt_data_2: Option<Mut<Foo>> = None;
        query.for_each(|data| opt_data = Some(data));
        query.for_each_mut(|data| opt_data_2 = Some(data));
        assert_eq!(opt_data.unwrap(), &*opt_data_2.unwrap()); // oops UB
    }
    dbg!("bye");
}

Solution

yeet unsound lifetime annotations on Query methods

Guvante · 2022-03-17T17:10:27Z

Safety comments would help for the new unsafe calls otherwise LGTM

BoxyUwU · 2022-03-17T17:12:37Z

I do not know if the new unsafe code is sound or not 😃 I do not understand rendering code whatsoever, the code is exactly as sound as it was before this PR though all that's change is that we're more explicit about "oops this code might cause UB"

Guvante

git blame to explain the problem with the PR comment is as good as any other solution

Guvante · 2022-03-19T02:41:10Z

I arbitrarily chose 'w for the read only fetch I added... If we swapped it and the mutable ones we can satisfy the new tests. <WriteFetch<T> as Fetch<'w, 's>>::Item == Mut<'s, T> and <ReadOnlyWriteFetch<T> as Fetch<'w, 's>>::Item == & 's T

cart · 2022-03-19T03:47:37Z

This is definitely a welcome / needed fix, but forcing user-facing render code to use unsafe is "suboptimal". @BoxyUwU and I discussed a lot of solutions, but the one I'm most in favor of is adding "inner" method variants for "read only queries". This is safe, although @BoxyUwU rightly pointed out that allowing 'w lifetimes in read only queries might limit future apis (such as WorldCell-like apis). They also pointed out that allowing 'w does increase the burden on Query developers / Bevy ECS developers to maintain the proper invariants.

That being said, I still think the inner approach is the best interim solution (for the 0.7 release), as it doesn't require any changes to how Queries work (or how render code works). It is a "surgical" fix. Just a minor breaking change to add the _inner prefix where relevant in renderer code.

A lot of other ideas were thrown around, such Query wrappers, QueryMut variants, using QueryState instead of Queries on the render side, and others. But those are all much bigger / risker changes that deserve more time and thought.

Consider the latest commit "a proposal" though. We can revert it if anyone wants to push back.

Guvante · 2022-03-19T04:00:59Z

I was able to get the new tests to pass by making this change Guvante@0372566 to have a more concrete reference.

cart · 2022-03-19T04:08:01Z

I was able to get the new tests to pass by making this change Guvante@0372566 to have a more concrete reference.

That is "wrong" from my perspective. It is tying the components to the internal QueryState lifetime instead of the internal World lifetime. The components do not belong to QueryState, they belong to World. If we're going to do that, we should just give up on split lifetimes and go back to a "merged" lifetime.

cart · 2022-03-19T04:08:55Z

And I personally think we shouldn't give up on split lifetimes, because the renderer relies on getting components with world lifetimes.

Guvante · 2022-03-19T04:25:13Z

I assumed we didn't actually want to lift a mutable lifetime to 'w and that change only impacts mutable lifetimes. (I mean technically ReadOnlyWriteFetch is immutable but that is more how you can transform an &mut into &)

Guvante · 2022-03-19T04:28:03Z

Oh I accidentally revised out my other point. I assumed &self was the wanted lifetime for mutable things and it looked like most of the call sites were &'s self.

Guvante

I assumed that &'s self would shorten the lifetime of it similar to '_ but it seems that isn't always the case so a broader solution is better.

cart · 2022-03-22T02:43:19Z

bors r+

# Objective Continuation of #2964 (I really should have checked other methods when I made that PR) yeet unsound lifetime annotations on `Query` methods. Example unsoundness: ```rust use bevy::prelude::*; fn main() { App::new().add_startup_system(bar).add_system(foo).run(); } pub fn bar(mut cmds: Commands) { let e = cmds.spawn().insert(Foo { a: 10 }).id(); cmds.insert_resource(e); } #[derive(Component, Debug, PartialEq, Eq)] pub struct Foo { a: u32, } pub fn foo(mut query: Query<&mut Foo>, e: Res<Entity>) { dbg!("hi"); { let data: &Foo = query.get(*e).unwrap(); let data2: Mut<Foo> = query.get_mut(*e).unwrap(); assert_eq!(data, &*data2); // oops UB } { let data: &Foo = query.single(); let data2: Mut<Foo> = query.single_mut(); assert_eq!(data, &*data2); // oops UB } { let data: &Foo = query.get_single().unwrap(); let data2: Mut<Foo> = query.get_single_mut().unwrap(); assert_eq!(data, &*data2); // oops UB } { let data: &Foo = query.iter().next().unwrap(); let data2: Mut<Foo> = query.iter_mut().next().unwrap(); assert_eq!(data, &*data2); // oops UB } { let mut opt_data: Option<&Foo> = None; let mut opt_data_2: Option<Mut<Foo>> = None; query.for_each(|data| opt_data = Some(data)); query.for_each_mut(|data| opt_data_2 = Some(data)); assert_eq!(opt_data.unwrap(), &*opt_data_2.unwrap()); // oops UB } dbg!("bye"); } ``` ## Solution yeet unsound lifetime annotations on `Query` methods Co-authored-by: Carter Anderson <[email protected]>

bors · 2022-03-22T03:06:07Z

Pull request successfully merged into main.

Build succeeded:

# Objective Continuation of bevyengine#2964 (I really should have checked other methods when I made that PR) yeet unsound lifetime annotations on `Query` methods. Example unsoundness: ```rust use bevy::prelude::*; fn main() { App::new().add_startup_system(bar).add_system(foo).run(); } pub fn bar(mut cmds: Commands) { let e = cmds.spawn().insert(Foo { a: 10 }).id(); cmds.insert_resource(e); } #[derive(Component, Debug, PartialEq, Eq)] pub struct Foo { a: u32, } pub fn foo(mut query: Query<&mut Foo>, e: Res<Entity>) { dbg!("hi"); { let data: &Foo = query.get(*e).unwrap(); let data2: Mut<Foo> = query.get_mut(*e).unwrap(); assert_eq!(data, &*data2); // oops UB } { let data: &Foo = query.single(); let data2: Mut<Foo> = query.single_mut(); assert_eq!(data, &*data2); // oops UB } { let data: &Foo = query.get_single().unwrap(); let data2: Mut<Foo> = query.get_single_mut().unwrap(); assert_eq!(data, &*data2); // oops UB } { let data: &Foo = query.iter().next().unwrap(); let data2: Mut<Foo> = query.iter_mut().next().unwrap(); assert_eq!(data, &*data2); // oops UB } { let mut opt_data: Option<&Foo> = None; let mut opt_data_2: Option<Mut<Foo>> = None; query.for_each(|data| opt_data = Some(data)); query.for_each_mut(|data| opt_data_2 = Some(data)); assert_eq!(opt_data.unwrap(), &*opt_data_2.unwrap()); // oops UB } dbg!("bye"); } ``` ## Solution yeet unsound lifetime annotations on `Query` methods Co-authored-by: Carter Anderson <[email protected]>

BoxyUwU added the A-ECS Entities, components, systems, and events label Mar 17, 2022

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 17, 2022

BoxyUwU added C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention and removed S-Needs-Triage This issue needs to be labelled labels Mar 17, 2022

yeet

2fce25d

BoxyUwU force-pushed the query_lifetime_fix branch from 0004d01 to 2fce25d Compare March 17, 2022 17:11

Guvante approved these changes Mar 17, 2022

View reviewed changes

compile fail test

0cf02ba

BoxyUwU force-pushed the query_lifetime_fix branch from f8f7aca to 0cf02ba Compare March 17, 2022 20:21

cart added this to the Bevy 0.7 milestone Mar 19, 2022

Add "inner" lifetime Query variants and replace unsafe calls with them

224fb06

Guvante approved these changes Mar 19, 2022

View reviewed changes

bors bot changed the title ~~yeet unsound lifetime annotations on Query methods~~ [Merged by Bors] - yeet unsound lifetime annotations on Query methods Mar 22, 2022

bors bot closed this Mar 22, 2022

james7132 mentioned this pull request Feb 20, 2023

iter_inner is either accidentally public or undocumented #5290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - yeet unsound lifetime annotations on `Query` methods #4243

[Merged by Bors] - yeet unsound lifetime annotations on `Query` methods #4243

BoxyUwU commented Mar 17, 2022 •

edited

Loading

Guvante commented Mar 17, 2022

BoxyUwU commented Mar 17, 2022

Guvante left a comment

Guvante commented Mar 19, 2022

cart commented Mar 19, 2022 •

edited

Loading

Guvante commented Mar 19, 2022

cart commented Mar 19, 2022

cart commented Mar 19, 2022

Guvante commented Mar 19, 2022

Guvante commented Mar 19, 2022

Guvante left a comment

cart commented Mar 22, 2022

bors bot commented Mar 22, 2022

[Merged by Bors] - yeet unsound lifetime annotations on Query methods #4243

[Merged by Bors] - yeet unsound lifetime annotations on Query methods #4243

Conversation

BoxyUwU commented Mar 17, 2022 • edited Loading

Objective

Solution

Guvante commented Mar 17, 2022

BoxyUwU commented Mar 17, 2022

Guvante left a comment

Choose a reason for hiding this comment

Guvante commented Mar 19, 2022

cart commented Mar 19, 2022 • edited Loading

Guvante commented Mar 19, 2022

cart commented Mar 19, 2022

cart commented Mar 19, 2022

Guvante commented Mar 19, 2022

Guvante commented Mar 19, 2022

Guvante left a comment

Choose a reason for hiding this comment

cart commented Mar 22, 2022

bors bot commented Mar 22, 2022

[Merged by Bors] - yeet unsound lifetime annotations on `Query` methods #4243

[Merged by Bors] - yeet unsound lifetime annotations on `Query` methods #4243

BoxyUwU commented Mar 17, 2022 •

edited

Loading

cart commented Mar 19, 2022 •

edited

Loading