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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Avoid making Fetchs Clone #5593

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Avoid making Fetchs Clone #5593

wants to merge 3 commits into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Aug 6, 2022 •
edited

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 :)

@BoxyUwU BoxyUwU added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use labels Aug 6, 2022
@BoxyUwU BoxyUwU changed the title Avoid making mutable Fetchs Clone Avoid making Fetchs Clone Aug 6, 2022
alice-i-cecile
alice-i-cecile reviewed Aug 7, 2022
View reviewed changes
crates/bevy_ecs/src/query/fetch.rs
/// 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>(
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we probably want a safe read-only version of this too.

I'm not sure if it's immediately useful though.

Copy link
Member Author

Choose a reason for hiding this comment

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

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
alice-i-cecile reviewed Aug 7, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'm on board with these changes; I agree with you that the trait impls are suspicious and tricky to use.

I'd be interested in seeing if we can special-case read-only fetches a little harder, but I'm not sure if it would actually have any immediate payoff.

alice-i-cecile
alice-i-cecile approved these changes Aug 7, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Alright, I'm happy to leave writing safe methods for future work if and when we have a motivating use case.

@alice-i-cecile alice-i-cecile added the P-Unsound A bug that results in undefined compiler behavior label Sep 21, 2022
@james7132 james7132 self-requested a review October 20, 2022 19:56
@BoxyUwU @alice-i-cecile
Update crates/bevy_ecs/src/query/fetch.rs
173bcc8 
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 26, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 26, 2022
@BoxyUwU
Avoid making Fetchs Clone (#5593)
54cf45c 
# 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 :)
@bors
Copy link
Contributor

bors bot commented Oct 26, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Avoid making Fetchs Clone [Merged by Bors] - Avoid making Fetchs Clone Oct 26, 2022
@bors bors bot closed this Oct 26, 2022
@rparrett rparrett mentioned this pull request Oct 26, 2022
Closed
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@BoxyUwU @james7132
Avoid making Fetchs Clone (bevyengine#5593)
f440ca5 
# 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
@BoxyUwU @ItsDoot
Avoid making Fetchs Clone (bevyengine#5593)
07bf602 
# 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 :)
@JoJoJet JoJoJet mentioned this pull request Mar 29, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jul 25, 2023
@JoJoJet @alice-i-cecile
Fix safety invariants for WorldQuery::fetch and simplify cloning (#…
02688a9 
…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 <alice.i.cecile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@james7132 james7132 james7132 approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@BoxyUwU @alice-i-cecile @james7132