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

Shorten the 'world lifetime returned from QueryLens::query(). #17694

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chescock
Copy link
Contributor

@chescock chescock commented Feb 5, 2025

Objective

Fix unsoundness introduced by #15858. QueryLens::query() would hand out a Query with the full 'w lifetime, and the new _inner methods would let the results outlive the Query. This could be used to create aliasing mutable references, like

fn bad<'w>(mut lens: QueryLens<'w, EntityMut>, entity: Entity) {
    let one: EntityMut<'w> = lens.query().get_inner(entity).unwrap();
    let two: EntityMut<'w> = lens.query().get_inner(entity).unwrap();
    assert!(one.entity() == two.entity());
}

Fixes #17693

Solution

Restrict the 'world lifetime in the Query returned by QueryLens::query() to '_, the lifetime of the borrow of the QueryLens.

The model here is that Query<'w, 's, D, F> and QueryLens<'w, D, F> have permission to access their components for the lifetime 'w. So going from &'a mut QueryLens<'w> to Query<'w, 'a> would borrow the permission only for the 'a lifetime, but incorrectly give it out for the full 'w lifetime.

To handle any cases where users were calling get_inner() or iter_inner() on the Query and expecting the full 'w lifetime, we introduce a new QueryLens::query_inner() method. This is only valid for ReadOnlyQueryData, so it may safely hand out a copy of the permission for the full 'w lifetime. Since get_inner() and iter_inner() were only valid on ReadOnlyQueryData prior to #15858, that should cover any uses that relied on the longer lifetime.

Migration Guide

Users of QueryLens::query() who were calling get_inner() or iter_inner() will need to replace the call with QueryLens::query_inner().

Add a new `QueryLens::query_inner()` to handle cases that were relying on the longer lifetime.
@alice-i-cecile
Copy link
Member

@Victoronz @chescock @13ros27 take a look at this please :)

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 5, 2025
@13ros27
Copy link
Contributor

13ros27 commented Feb 5, 2025

I'll be honest the way Query uses lifetimes goes slightly over my head but this seems reasonable from what was discussed in #17693. Is it worth a compile fail test to ensure the lifetimes aren't changed at some point?

@chescock
Copy link
Contributor Author

chescock commented Feb 5, 2025

Is it worth a compile fail test to ensure the lifetimes aren't changed at some point?

Yes, definitely!

I have never written a compile-fail test, though. Can anyone point me in the right direction to get started?

@alice-i-cecile
Copy link
Member

https://github.com/bevyengine/bevy/tree/main/crates/bevy_ecs/compile_fail :)

Copy link
Contributor

@13ros27 13ros27 left a comment

Choose a reason for hiding this comment

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

As I mentioned I can't fully review the lifetime changes but the compile-fail test seems good and compiles successfully on main (where it shouldn't of course).

{
let mut query = system_state.get_mut(&mut world);
let mut lens = query.as_query_lens();
dbg!("hi");
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to leave these in?

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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide P-Unsound A bug that results in undefined compiler behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SOUNDNESS: Query should not be Copy
3 participants