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

[Merged by Bors] - Add a method for converting MutUntyped -> Mut<T> #7113

Closed
wants to merge 11 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jan 6, 2023

Objective

MutUntyped is a struct that stores a PtrMut alongside change tick metadata. Working with this type is cumbersome, and has few benefits over storing the pointer and change ticks separately.

Related: #6430 (title is out of date)

Solution

Add a convenience method for transforming an untyped change detection pointer into its typed counterpart.


Changelog

  • Added the method MutUntyped::with_type.

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 6, 2023
crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
@JoJoJet JoJoJet requested a review from james7132 January 7, 2023 01:48
@JoJoJet JoJoJet added the C-Docs An addition or correction to our documentation label Jan 7, 2023
@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 Jan 7, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Jan 7, 2023

I relaxed the language on the *Ptr methods. Previously I wrote that the pointer must be "valid for reads/writes" or that it must be initialized. However, this is unnecessary, since it is unsound to create invalid instances of PtrMut and friends in the first place. Those invariants should be considered when constructing a pointer, not when consuming it.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 9, 2023

I disagree with this PR adding alignment safety invariants to read/deref. Ptr/PtrMut/OwningPtr etc are documented as being "type erased borrows" and should require alignment on construction of them not on read/writes.

@james7132
Copy link
Member

We should add the invariant to byte_add that it cannot make the pointer unaligned then. Since that would violate that invariant if the offset is not a multiple of the alignment.

@alice-i-cecile alice-i-cecile removed 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 Jan 9, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Jan 9, 2023

I disagree with this PR adding alignment safety invariants to read/deref. Ptr/PtrMut/OwningPtr etc are documented as being "type erased borrows" and should require alignment on construction of them not on read/writes.

The docs make no mention of any alignment requirement for these types:

/// This type tries to act "borrow-like" which means that:
/// - Pointer is considered exclusive and mutable. It cannot be cloned as this would lead to
/// aliased mutability.
/// - It must always points to a valid value of whatever the pointee type is.
/// - The lifetime 'a accurately represents how long the pointer is valid for.

The docs for Ptr*::new state this:

/// # Safety
/// The lifetime for the returned item must not exceed the lifetime inner is valid for

Again, no alignment requirements. Since these types have no alignment guarantee, you need to ensure they are aligned when you dereference them. This PR isn't adding any invariants, it's documenting invariants that used to be implicit.

We could add the requirement that these types must be aligned. But as discussed in #7117, it is useful to be able to create unaligned pointers. I don't think we should do this, it would make these types less valuable.

Also, to clarify one point that might be confusing:

/// - It must always points to a valid value of whatever the pointee type is.

This does not require the pointer to be aligned. Alignment is a property of a pointer, not the value it is pointing to.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 9, 2023

The docs make no mention of any alignment requirement for these types:

Again, no alignment requirements. Since these types have no alignment guarantee, you need to ensure they are aligned when you dereference them

I'm aware of this. This should be changed by documenting alignment requirements.

This PR isn't adding any invariants, it's documenting invariants that used to be implicit.

They could implicitly be on Ptr::new and the docs of Ptr rather than read/deref etc

This does not require the pointer to be aligned

This is reasonable if its enforced on construction, imo the only safety invariants on read or deref should be that when you provide the type of the pointee it is the same as whatever conceptually is the "underlying" erased type. That means you dont have to reason about send/sync or lifetimes or aliasing or alignment etc.

We could add the requirement that these types must be aligned

as mentioned in #7117 we could have separate types for when alignment is not being upheld (which if we were starting from a position of having read/deref have an invariant that the pointer is aligned I would consider this to be a good step forwards for safety)

This does not require the pointer to be aligned. Alignment is a property of a pointer, not the value it is pointing to.

A better way of wording the safety invaraints on read/deref etc would probably be to talk about T having to be the same as the "underlying" erased type although that is slightly overly restrictive. With all the other invariants required for these methods to be sound enforced on construction of the Ptr type.

@JoJoJet JoJoJet removed the C-Docs An addition or correction to our documentation label Jan 9, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Jan 9, 2023

I've reverted everything to use the incorrect invariants again, so we can have this discussion elsewhere.

@alice-i-cecile
Copy link
Member

I've reverted everything to use the incorrect invariants again, so we can have this discussion elsewhere.

Agreed, this deserves its own issue or PR.

@JoJoJet JoJoJet closed this Jan 11, 2023
@JoJoJet JoJoJet reopened this Jan 11, 2023
@JoJoJet JoJoJet 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 Jan 11, 2023
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.

bors r+

bors bot pushed a commit that referenced this pull request Jan 11, 2023
# Objective

`MutUntyped` is a struct that stores a `PtrMut` alongside change tick metadata. Working with this type is cumbersome, and has few benefits over storing the pointer and change ticks separately.

Related: #6430 (title is out of date)

## Solution

Add a convenience method for transforming an untyped change detection pointer into its typed counterpart.

---

## Changelog

- Added the method `MutUntyped::with_type`.
@bors bors bot changed the title Add a method for converting MutUntyped -> Mut<T> [Merged by Bors] - Add a method for converting MutUntyped -> Mut<T> Jan 11, 2023
@bors bors bot closed this Jan 11, 2023
bors bot pushed a commit that referenced this pull request Jan 11, 2023
…7117)

# Objective
Improve safety testing when using `bevy_ptr` types. This is a follow-up to #7113.

## Solution
Add a debug-only assertion that pointers are aligned when casting to a concrete type. This should very quickly catch any unsoundness from unaligned pointers, even without miri. However, this can have a large negative perf impact on debug builds.

---

## Changelog
Added: `Ptr::deref` will now panic in debug builds if the pointer is not aligned.
Added: `PtrMut::deref_mut` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::read` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::drop_as` will now panic in debug builds if the pointer is not aligned.
@JoJoJet JoJoJet deleted the mut-with-type branch January 13, 2023 02:29
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

`MutUntyped` is a struct that stores a `PtrMut` alongside change tick metadata. Working with this type is cumbersome, and has few benefits over storing the pointer and change ticks separately.

Related: bevyengine#6430 (title is out of date)

## Solution

Add a convenience method for transforming an untyped change detection pointer into its typed counterpart.

---

## Changelog

- Added the method `MutUntyped::with_type`.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…evyengine#7117)

# Objective
Improve safety testing when using `bevy_ptr` types. This is a follow-up to bevyengine#7113.

## Solution
Add a debug-only assertion that pointers are aligned when casting to a concrete type. This should very quickly catch any unsoundness from unaligned pointers, even without miri. However, this can have a large negative perf impact on debug builds.

---

## Changelog
Added: `Ptr::deref` will now panic in debug builds if the pointer is not aligned.
Added: `PtrMut::deref_mut` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::read` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::drop_as` will now panic in debug builds if the pointer is not aligned.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

`MutUntyped` is a struct that stores a `PtrMut` alongside change tick metadata. Working with this type is cumbersome, and has few benefits over storing the pointer and change ticks separately.

Related: bevyengine#6430 (title is out of date)

## Solution

Add a convenience method for transforming an untyped change detection pointer into its typed counterpart.

---

## Changelog

- Added the method `MutUntyped::with_type`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…evyengine#7117)

# Objective
Improve safety testing when using `bevy_ptr` types. This is a follow-up to bevyengine#7113.

## Solution
Add a debug-only assertion that pointers are aligned when casting to a concrete type. This should very quickly catch any unsoundness from unaligned pointers, even without miri. However, this can have a large negative perf impact on debug builds.

---

## Changelog
Added: `Ptr::deref` will now panic in debug builds if the pointer is not aligned.
Added: `PtrMut::deref_mut` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::read` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::drop_as` will now panic in debug builds if the pointer is not aligned.
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-Usability A targeted quality-of-life change that makes Bevy easier to use 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
Development

Successfully merging this pull request may close these issues.

4 participants