[Merged by Bors] - Avoid making Fetchs Clone
#5593
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BoxyUwU
added
A-ECS
| /// While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure | ||
| /// that the returned value is not used in any way that would cause two `QueryItem<Self>` for the same | ||
| /// `archetype_index` or `table_row` to be alive at the same time. | ||
| unsafe fn clone_fetch<'w>( |
There was a problem hiding this comment.
I feel like we probably want a safe read-only version of this too.
I'm not sure if it's immediately useful though.
There was a problem hiding this comment.
I don't think it really matters, none of the call sites seem to know that Self: ReadOnlyWorldQuery holds, it'd be trivial to write a safe function later if we need it so i'm not bothered about this
alice-i-cecile
approved these changes
Aug 7, 2022
BoxyUwU
force-pushed
the
stop_deriving_clone_fetches
branch
from
750d894 to
bec0fea
Compare
alice-i-cecile
added
the
P-Unsound
BoxyUwU
force-pushed
the
stop_deriving_clone_fetches
branch
from
bec0fea to
9110910
Compare
james7132
reviewed
Oct 25, 2022
Co-authored-by: Alice Cecile <[email protected]>
james7132
approved these changes
Oct 26, 2022
james7132
added
the
S-Ready-For-Final-Review
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Oct 26, 2022
# Objective - Do not implement `Copy` or `Clone` for `Fetch` types as this is kind of sus soundness wise (it feels like cloning an `IterMut` in safe code to me). Cloning a fetch seems important to think about soundness wise when doing it so I prefer this over adding a `Clone` bound to the assoc type definition (i.e. `type Fetch: Clone`) even though that would also solve the other listed things here. - Remove a bunch of `QueryFetch<'w, Q>: Clone` bounds from our API as now all fetches can be "cloned" for use in `iter_combinations`. This should also help avoid the type inference regression ptrification introduced where `for<'a> QueryFetch<'a, Q>: Trait` bounds misbehave since we no longer need any of those kind of higher ranked bounds (although in practice we had none anyway). - Stop being able to "forget" to implement clone for fetches, we've had a lot of issues where either `derive(Clone)` was used instead of a manual impl (so we ended up with too tight bounds on the impl) or flat out forgot to implement Clone at all. With this change all fetches are able to be cloned for `iter_combinations` so this will no longer be possible to mess up. On an unrelated note, while making this PR I realised we probably want safety invariants on `archetype/table_fetch` that nothing aliases the table_row/archetype_index according to the access we set. --- ## Changelog `Clone` and `Copy` were removed from all `Fetch` types. ## Migration Guide - Call `WorldQuery::clone_fetch` instead of `fetch.clone()`. Make sure to add safety comments :)
|
Pull request successfully merged into main. Build succeeded:
|
bors
bot
changed the title
Fetchs CloneFetchs Clone
james7132
pushed a commit
to james7132/bevy
that referenced
this pull request
Oct 28, 2022
# Objective - Do not implement `Copy` or `Clone` for `Fetch` types as this is kind of sus soundness wise (it feels like cloning an `IterMut` in safe code to me). Cloning a fetch seems important to think about soundness wise when doing it so I prefer this over adding a `Clone` bound to the assoc type definition (i.e. `type Fetch: Clone`) even though that would also solve the other listed things here. - Remove a bunch of `QueryFetch<'w, Q>: Clone` bounds from our API as now all fetches can be "cloned" for use in `iter_combinations`. This should also help avoid the type inference regression ptrification introduced where `for<'a> QueryFetch<'a, Q>: Trait` bounds misbehave since we no longer need any of those kind of higher ranked bounds (although in practice we had none anyway). - Stop being able to "forget" to implement clone for fetches, we've had a lot of issues where either `derive(Clone)` was used instead of a manual impl (so we ended up with too tight bounds on the impl) or flat out forgot to implement Clone at all. With this change all fetches are able to be cloned for `iter_combinations` so this will no longer be possible to mess up. On an unrelated note, while making this PR I realised we probably want safety invariants on `archetype/table_fetch` that nothing aliases the table_row/archetype_index according to the access we set. --- ## Changelog `Clone` and `Copy` were removed from all `Fetch` types. ## Migration Guide - Call `WorldQuery::clone_fetch` instead of `fetch.clone()`. Make sure to add safety comments :)
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective - Do not implement `Copy` or `Clone` for `Fetch` types as this is kind of sus soundness wise (it feels like cloning an `IterMut` in safe code to me). Cloning a fetch seems important to think about soundness wise when doing it so I prefer this over adding a `Clone` bound to the assoc type definition (i.e. `type Fetch: Clone`) even though that would also solve the other listed things here. - Remove a bunch of `QueryFetch<'w, Q>: Clone` bounds from our API as now all fetches can be "cloned" for use in `iter_combinations`. This should also help avoid the type inference regression ptrification introduced where `for<'a> QueryFetch<'a, Q>: Trait` bounds misbehave since we no longer need any of those kind of higher ranked bounds (although in practice we had none anyway). - Stop being able to "forget" to implement clone for fetches, we've had a lot of issues where either `derive(Clone)` was used instead of a manual impl (so we ended up with too tight bounds on the impl) or flat out forgot to implement Clone at all. With this change all fetches are able to be cloned for `iter_combinations` so this will no longer be possible to mess up. On an unrelated note, while making this PR I realised we probably want safety invariants on `archetype/table_fetch` that nothing aliases the table_row/archetype_index according to the access we set. --- ## Changelog `Clone` and `Copy` were removed from all `Fetch` types. ## Migration Guide - Call `WorldQuery::clone_fetch` instead of `fetch.clone()`. Make sure to add safety comments :)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 25, 2023
…8246) # Objective Cloning a `WorldQuery` type's "fetch" struct was made unsafe in #5593, by adding the `unsafe fn clone_fetch` to `WorldQuery`. However, as that method's documentation explains, it is not the right place to put the safety invariant: > While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure that the returned value is not used in any way that would cause two `QueryItem<Self>` for the same `archetype_index` or `table_row` to be alive at the same time. You can clone a fetch struct all you want and it will never cause undefined behavior -- in order for something to go wrong, you need to improperly call `WorldQuery::fetch` with it (which is marked unsafe). Additionally, making it unsafe to clone a fetch struct does not even prevent undefined behavior, since there are other ways to incorrectly use a fetch struct. For example, you could just call fetch more than once for the same entity, which is not currently forbidden by any documented invariants. ## Solution Document a safety invariant on `WorldQuery::fetch` that requires the caller to not create aliased `WorldQueryItem`s for mutable types. Remove the `clone_fetch` function, and add the bound `Fetch: Clone` instead. --- ## Changelog - Removed the associated function `WorldQuery::clone_fetch`, and added a `Clone` bound to `WorldQuery::Fetch`. ## Migration Guide ### `fetch` invariants The function `WorldQuery::fetch` has had the following safety invariant added: > If this type does not implement `ReadOnlyWorldQuery`, then the caller must ensure that it is impossible for more than one `Self::Item` to exist for the same entity at any given time. This invariant was always required for soundness, but was previously undocumented. If you called this function manually anywhere, you should check to make sure that this invariant is not violated. ### Removed `clone_fetch` The function `WorldQuery::clone_fetch` has been removed. The associated type `WorldQuery::Fetch` now has the bound `Clone`. Before: ```rust struct MyFetch<'w> { ... } unsafe impl WorldQuery for MyQuery { ... type Fetch<'w> = MyFetch<'w> unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> { MyFetch { field1: fetch.field1, field2: fetch.field2.clone(), ... } } } ``` After: ```rust #[derive(Clone)] struct MyFetch<'w> { ... } unsafe impl WorldQuery for MyQuery { ... type Fetch<'w> = MyFetch<'w>; } ``` --------- Co-authored-by: Alice Cecile <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
CopyorCloneforFetchtypes as this is kind of sus soundness wise (it feels like cloning anIterMutin safe code to me). Cloning a fetch seems important to think about soundness wise when doing it so I prefer this over adding aClonebound to the assoc type definition (i.e.type Fetch: Clone) even though that would also solve the other listed things here.QueryFetch<'w, Q>: Clonebounds from our API as now all fetches can be "cloned" for use initer_combinations. This should also help avoid the type inference regression ptrification introduced wherefor<'a> QueryFetch<'a, Q>: Traitbounds misbehave since we no longer need any of those kind of higher ranked bounds (although in practice we had none anyway).derive(Clone)was used instead of a manual impl (so we ended up with too tight bounds on the impl) or flat out forgot to implement Clone at all. With this change all fetches are able to be cloned foriter_combinationsso this will no longer be possible to mess up.On an unrelated note, while making this PR I realised we probably want safety invariants on
archetype/table_fetchthat nothing aliases the table_row/archetype_index according to the access we set.Changelog
CloneandCopywere removed from allFetchtypes.Migration Guide
WorldQuery::clone_fetchinstead offetch.clone(). Make sure to add safety comments :)