Allow querying multiple components from FilteredEntityMut

[cBournhonesque]

Objective

#20265 introduced a way to fetch multiple mutable components from an EntityMut, but it's still impossible to do so via an FilteredEntityMut.

I believe it is currently impossible to get two mutable components from a FilteredEntityMut, which somewhat limits use cases with dynamic queries.

Solution

A similar solution is harder to implement for FilteredEntityMut because the QueryData must go through the access checks, and it's not obvious to get the ComponentIds from a ReleaseStateQueryData.

Instead, I opt in to provide a similar abstraction as UnsafeEntityCell and UnsafeWorldCell, which are both public and Clone: an opt-in escape catch for advanced users that can guarantee that they are not causing any aliasing violations.

Here, instead we provide a method that copies the underlying UnsafeEntityCell, so the safety requirements are similar to UnsafeEntityCell and UnsafeWorldCell.

Testing

Added a doctest.

[chescock]

Why not an unsafe fn get_mut_unchecked(&self) -> Option<Mut<T>> method (similar to #20262)? That seems like it would be a little more convenient for this use case, and should have more understandable safety requirements than an unsafe clone.

I would expect the advantage of unsafe clone to be that it's more general. But it only helps with &mut self methods, and the only ones of those on FilteredEntityMut are get_mut and get_mut_by_id, so we should be able to cover all the use cases with just two methods. In contrast, Query has about ten different &mut self methods, so Query::reborrow_unsafe lets us avoid needing unchecked versions of all of those (although in practice we have most of them anyway).

---

If we do stick with this approach, I'd be inclined to name it something_unchecked or something_unsafe to make it clear that it's bypassing some safety checks. Maybe copy_unsafe since this is just a bitwise copy.

Alternately, we might want to copy Query::reborrow_unsafe and restrict the output lifetime like

    pub unsafe fn reborrow_unsafe(&self) -> FilteredEntityMut<'_, 's> {

That could help catch some errors, at the cost of being unable to return multiple items with the original 'w lifetime.

[cBournhonesque]

That's a great point, unsafe fn get_mut_unchecked(&self) -> Option<Mut<T>> is basically exactly what I want. I will implement it in a separate PR.

I didn't know that we had Query::reborrow_unsafe; it lends credence to the idea that these kinds of unsafe escape hatches that give you more freedom to manipulate ECS data as long as you promise to not cause aliasing violations are useful.

[alice-i-cecile]

Let's add the other method in the same PR please: I think both should exist, but most use cases should use the cheaper and more scoped form.

[cBournhonesque]

@chescock @alice-i-cecile I added the two extra methods; also instead of clone I created a separate Unsafe wrapper of FilteredEntityMut similar to UnsafeEntityCell or UnsafeWorldCell.
This is to address @hymm 's concern on discord:

UnsafeWorldCell mostly has unsafe methods for accessing data and the clone is safe. While FilteredEntityMut methods are safe, so making clone unsafe make safety a nonlocal concern. i.e. reasoning about the safety of the clone must consider all uses of the cloned value. While for UnsafeWorldCell the clone is safe and the safety needs to be upheld when accessing the data.

Or slightly less abstractly you might pass the FilteredEntityMut to a library function which you don't fully understand and then you get UB. While if you do that with an UnsafeWorldCell it is likely that the library function will be marked as unsafe too, since the safety has been passed up to the caller.

[chescock]

The implementations all seem good, but I don't think UnsafeFilteredEntityMut will actually address @hymm's concerns. A function that takes &FilteredEntityMut could call UnsafeFilteredEntityMut::new_readonly(e).into_mut() just as easily as it could call e.clone().

I'd be inclined to leave it out entirely, especially if we wind up making Query::reborrow_unsafe non-pub as @hymm suggests. But I also see that @alice-i-cecile asked you to include both methods in this PR.

[chescock]

Copying the existing doc comment seems good for this PR, but get_mut_by_id does not return a raw pointer, so we should probably do a follow-up to fix the comments everywhere. (Maybe it was originally written before MutUntyped existed?)

[cBournhonesque]

The implementations all seem good, but I don't think UnsafeFilteredEntityMut will actually address @hymm's concerns. A function that takes &FilteredEntityMut could call UnsafeFilteredEntityMut::new_readonly(e).into_mut() just as easily as it could call e.clone().

I'd be inclined to leave it out entirely, especially if we wind up making Query::reborrow_unsafe non-pub as @hymm suggests. But I also see that @alice-i-cecile asked you to include both methods in this PR.

You're right. I guess a function could also take a World and call unsafe {World.as_unsafe_world_cell().mut_world()}...
I would just like to have consistency: if we allow a public escape hatch like UnsafeWorldCell, then we should also provide them for other things like Query and FilteredEntityMut.

I could manually use UnsafeWorldCell to recreate my UnsafeFilteredEntityMut, it's just very tedious to do so since I would have to manually create the Access, etc. So it's not like we're giving users new unsafe capabilities, they already exist via UnsafeWorldCell.

As for this PR, I'm happy to do any of:

• keep the current impl
• revert to copy_unsafe
• remove copy_unsafe

[alice-i-cecile]

We should remove Copy here: it's far too much of a footgun for limited benefit.

[cBournhonesque]

I don't think so?
You can't use UnsafeFilteredEntityMut for anything else but to convert it back into a FilteredEntityMut, and you need an unsafe function call for that.
UnsafeWorldCell is also Copy, for instance

[alice-i-cecile]

A few things:

1. We should be adding breadcrumbs from FilteredEntityMut to this new type.
2. We should remove the Copy derive.
3. We should deduplicate the get_mut / get_mut_unchecked code paths.

Once that's done I'm happy with this PR. I do think that there's real value in making it easier and less error-prone to use our unsafe methods: paving the metaphorical cow paths will make migrations easier and reduce accidental UB.

[hymm]

If we want an escape hatch I think we should expose a method to convert a *EntityMut to an UnsafeEntityCell. That way anything using the UnsafeEntityCell has to pass the unsafety upwards.

[cBournhonesque]

If we want an escape hatch I think we should expose a method to convert a *EntityMut to an UnsafeEntityCell. That way anything using the UnsafeEntityCell has to pass the unsafety upwards.

Yes but then we would lose the information about access. The only 'unsafety' we want to not check is aliasing.

[hymm]

You're right. I guess a function could also take a World and call unsafe {World.as_unsafe_world_cell().mut_world()}...
I would just like to have consistency: if we allow a public escape hatch like UnsafeWorldCell, then we should also provide them for other things like Query and FilteredEntityMut.

This analogy isn't quite correct. UnsafeWorldCell has all the unsafe methods and World only has safe methods. This allows bevy to isolate the aliasing of world to clear points. In the multihreaded executor that means tracking the access of the systems and only converting back to a &mut World when no other systems are running. For an external library I would be surprised if any of them took a UnsafeWorldCell as an input, because it's very hard for the library to know the provenance of that UnsafeWorldCell and not cause UB.

So if we were following that model we should move any unsafe methods for FilteredEntityMut onto UnsafeFilteredEntityCell and then allow the unsafe conversions.

Query's are sort of in the situation where we had all the unsafe methods on &World before the creation of UnsafeWorldCell. The reason this isn't too much of a problem is that Query's are typed and it is hard for that reason for libraries to take a query as an input. Especially because of things like the orphan rule. Thinking about this a bit mixing things like query transmutes and the unsafe methods on Query would probably be bad, but I think they're both rare enough currently that it probably isn't a problem. If their use ever become more common then it certainly could be and we might want to consider making a UnsafeQuery type.

So what does this mean for FilteredEntityMut? I do think FilteredEntityMut is more similar to World as it is unsafe. And I can see libraries taking a &mut FilteredEntityMut as a input. So I see a couple options here:

1. We create a UnsafeFilteredEntityCell can reimplement all the safe methods from FilteredEntityMut onto it, but make it unsafe similar to UnsafeWorldCell and have no unsafe methods on
2. We expose a method to convert a FilteredEntityMut to a UnsafeEntityCell which already does much of this, just without the access checks.

My vote is for 2 due to all the api duplication. I think if you're already in unsafe land it wouldn't be that much of a burden not to access anything the FilteredEntityMut shouldn't and the user will probably already have a good sense of what the provenance of it is so you might as well do less work and skip the access checks too.

[hymm]

I thought of a third option. We could make a EntityCell that would be a fully runtime checked thing that would track all the borrows out of it. Potentially could be pretty useful.

[alice-i-cecile]

I thought of a third option. We could make a EntityCell that would be a fully runtime checked thing that would track all the borrows out of it. Potentially could be pretty useful.

This came up ages ago in an old RFC: bevyengine/rfcs#42 . I've softened on it now that there's better patterns for 99% of uses, so I'm definitely open to this :)