[Merged by Bors] - Pointerfication followup: Type safety and cleanup #4621

james7132 · 2022-04-28T01:44:14Z

Objective

The Ptr types gives free access to the underlying NonNull<u8>, which adds more publicly visible pointer wrangling than there needs to be. There are also a few edge cases where Ptr types could be more readily utilized for properly validating the soundness of ECS operations.

Solution

Replace *Ptr(Mut)::inner with cast which requires a concrete type to give the pointer. This function could also have a debug_assert with an alignment check to ensure that the pointer is aligned properly, but is currently not included.
Use OwningPtr::read in ECS macros over casting the inner pointer around.

alice-i-cecile · 2022-04-28T01:48:25Z

Adding the Controversial tag due to the pointery trickery involved, to make sure we get Cart to review this before merging.

I don't expect much pushback on these changes though; they're very nice.

alice-i-cecile · 2022-04-28T01:49:14Z

crates/bevy_ecs/src/ptr.rs

            #[inline]
-            pub fn inner(&self) -> NonNull<u8> {
-                self.0
+            pub fn cast<T>(&self) -> NonNull<T> {


Oh this is dramatically nicer. I love this change.

alice-i-cecile

Quite straightforward. I really like the increased clarity and type safety we get here.

BoxyUwU · 2022-04-28T02:37:05Z

All the use sites you changed seem to immediately .as_ptr() and also all seem to be using T = u8 because of it being blobvec internals. I think we ought to change inner(&self) -> NonNull<u8> to as_ptr(self) -> *mut u8 instead. I like the changes to use .read though :)

james7132 · 2022-04-28T12:06:46Z

Main reason I chose cast was to effectively treat Ptr types like void*: you need to give it an underlying type to have any meaningful operation on it. The fact that BlobVec uses u8 internally is just an artifact of the interface it uses. The uses in Deref(Mut) and read likewise enforce this usage too. This is in the crate's public interface, and IMO we should try to smooth out the rough edges on anything we mark pub before someone starts relying on it.

Ideally, moving the pointer casts and conversions into Ptr types behind a carefully curated public interface also allows us to provide wider runtime safety checks in debug mode (as the comment in the OP above suggests) to ensure the lower level safety invariants don't get broken.

BoxyUwU · 2022-04-28T13:04:44Z

If you know the underlying type you should be calling deref/deref_mut/read and working with borrows instead. The only reason these types exist instead of using borrows is because we do not know the underlying type. There should never be a reason to do .cast::<RealType>.

Now that ptrification is merged we should add debug checks to those 3 methods, storing Option<TypeId> and a Layout inside of the Ptr* types when debug assertions are on.

Put another way- inner() exists solely for working with the data in an untyped fashion because if you know the type you should just call deref/deref_mut/read and work with those instead. Therefore its not a coincidence that all the callers of cast are using u8 its rather the point of the method.

james7132 · 2022-05-01T02:20:40Z

@BoxyUwU made the changes requested, ended up adding OwningPtr::drop_as and making to_ptr into pub(crate). This should at least disable using raw *mut u8 in userspace/non-ECS engine code and force use of the typed APIs.

~~With that said, is there a reason why OwningPtr does not have deref(_mut)?~~ Ah nevermind, brainfart.

TheRawMeatball · 2022-05-01T04:04:39Z

IMO we should revert to_ptr -> as_ptr, as to_* methods signify more active operations, e.g. to_string wheras as_* operations signify zero-cost casting like operations. Beyond this, we should also not mark these methods pub(crate) to make them useful for public abstractions, such as #4447.

BoxyUwU

I do somewhat think we ought to rename the offset methods to byte_offset to make it clear at use sites whats going on but that could be done in a separate PR

BoxyUwU · 2022-05-01T11:00:11Z

crates/bevy_ecs/src/ptr.rs

+    /// associated lifetime.
+    #[inline]
+    #[allow(clippy::wrong_self_convention)]
+    pub unsafe fn as_ptr(self) -> *mut u8 {


this doesn't really need to be an unsafe fn it could be fn as_ptr(&self) -> *mut u8 to be maximally flexible but it probably doesn't matter

BoxyUwU · 2022-05-01T11:01:38Z

crates/bevy_ecs/src/component.rs

@@ -186,7 +186,7 @@ impl std::fmt::Debug for ComponentDescriptor {
 impl ComponentDescriptor {
    // SAFETY: The pointer points to a valid value of type `T` and it is safe to drop this value.
    unsafe fn drop_ptr<T>(x: OwningPtr<'_>) {


I'm not sure we really need ComponentDescriptor::drop_ptr anymore considering you can just do OwningPtr::drop_as

I actually tried removing this and there was a lifetime mismatch. Using OwningPtr::drop_as::<T> doesn't satisfy for<'a> unsafe fn.... Is there a way to make this work?

oh... that makes sense I guess, oh well ._.

alice-i-cecile · 2022-05-02T12:01:35Z

bors r+

# Objective The `Ptr` types gives free access to the underlying `NonNull<u8>`, which adds more publicly visible pointer wrangling than there needs to be. There are also a few edge cases where Ptr types could be more readily utilized for properly validating the soundness of ECS operations. ## Solution - Replace `*Ptr(Mut)::inner` with `cast` which requires a concrete type to give the pointer. This function could also have a `debug_assert` with an alignment check to ensure that the pointer is aligned properly, but is currently not included. - Use `OwningPtr::read` in ECS macros over casting the inner pointer around.

alice-i-cecile · 2022-05-02T12:21:49Z

bors r-

bors · 2022-05-02T12:21:50Z

Canceled.

cart · 2022-05-03T20:05:21Z

bors r+

# Objective The `Ptr` types gives free access to the underlying `NonNull<u8>`, which adds more publicly visible pointer wrangling than there needs to be. There are also a few edge cases where Ptr types could be more readily utilized for properly validating the soundness of ECS operations. ## Solution - Replace `*Ptr(Mut)::inner` with `cast` which requires a concrete type to give the pointer. This function could also have a `debug_assert` with an alignment check to ensure that the pointer is aligned properly, but is currently not included. - Use `OwningPtr::read` in ECS macros over casting the inner pointer around.

bors · 2022-05-03T20:23:28Z

Pull request successfully merged into main.

Build succeeded:

# Objective The `Ptr` types gives free access to the underlying `NonNull<u8>`, which adds more publicly visible pointer wrangling than there needs to be. There are also a few edge cases where Ptr types could be more readily utilized for properly validating the soundness of ECS operations. ## Solution - Replace `*Ptr(Mut)::inner` with `cast` which requires a concrete type to give the pointer. This function could also have a `debug_assert` with an alignment check to ensure that the pointer is aligned properly, but is currently not included. - Use `OwningPtr::read` in ECS macros over casting the inner pointer around.

Followup: inner -> cast.

b74ab7d

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 28, 2022

james7132 added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Apr 28, 2022

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 28, 2022

alice-i-cecile reviewed Apr 28, 2022

View reviewed changes

alice-i-cecile approved these changes Apr 28, 2022

View reviewed changes

alice-i-cecile requested review from TheRawMeatball and BoxyUwU April 28, 2022 01:50

Fix tests

600f60a

james7132 added 3 commits April 30, 2022 05:39

Revert inner -> cast.

46ca843

as_ptr and drop_as

2510881

Formatting

8693468

james7132 added 3 commits April 30, 2022 19:29

as_ptr -> to_ptr

7416fd3

Inline

6e939e1

Shut up clippy

afdd684

james7132 added 4 commits April 30, 2022 21:35

Revert back to as_ptr, make pub again.

62da3fd

Update docs

114bc59

Fix CI

405fb89

More docs fixes

c62d2e5

TheRawMeatball approved these changes May 1, 2022

View reviewed changes

BoxyUwU approved these changes May 1, 2022

View reviewed changes

BoxyUwU reviewed May 1, 2022

View reviewed changes

BoxyUwU 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 May 1, 2022

bors bot changed the title ~~Pointerfication followup: Type safety and cleanup~~ [Merged by Bors] - Pointerfication followup: Type safety and cleanup May 3, 2022

bors bot closed this May 3, 2022

james7132 deleted the ptrfication-followup branch June 22, 2022 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Pointerfication followup: Type safety and cleanup #4621

[Merged by Bors] - Pointerfication followup: Type safety and cleanup #4621

james7132 commented Apr 28, 2022

alice-i-cecile commented Apr 28, 2022 •

edited

Loading

alice-i-cecile Apr 28, 2022

alice-i-cecile left a comment

BoxyUwU commented Apr 28, 2022

james7132 commented Apr 28, 2022

BoxyUwU commented Apr 28, 2022 •

edited

Loading

james7132 commented May 1, 2022 •

edited

Loading

TheRawMeatball commented May 1, 2022

BoxyUwU left a comment

BoxyUwU May 1, 2022

BoxyUwU May 1, 2022

james7132 May 1, 2022 •

edited

Loading

BoxyUwU May 1, 2022

alice-i-cecile commented May 2, 2022

alice-i-cecile commented May 2, 2022

bors bot commented May 2, 2022

cart commented May 3, 2022

bors bot commented May 3, 2022

[Merged by Bors] - Pointerfication followup: Type safety and cleanup #4621

[Merged by Bors] - Pointerfication followup: Type safety and cleanup #4621

Conversation

james7132 commented Apr 28, 2022

Objective

Solution

alice-i-cecile commented Apr 28, 2022 • edited Loading

alice-i-cecile Apr 28, 2022

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

BoxyUwU commented Apr 28, 2022

james7132 commented Apr 28, 2022

BoxyUwU commented Apr 28, 2022 • edited Loading

james7132 commented May 1, 2022 • edited Loading

TheRawMeatball commented May 1, 2022

BoxyUwU left a comment

Choose a reason for hiding this comment

BoxyUwU May 1, 2022

Choose a reason for hiding this comment

BoxyUwU May 1, 2022

Choose a reason for hiding this comment

james7132 May 1, 2022 • edited Loading

Choose a reason for hiding this comment

BoxyUwU May 1, 2022

Choose a reason for hiding this comment

alice-i-cecile commented May 2, 2022

alice-i-cecile commented May 2, 2022

bors bot commented May 2, 2022

cart commented May 3, 2022

bors bot commented May 3, 2022

alice-i-cecile commented Apr 28, 2022 •

edited

Loading

BoxyUwU commented Apr 28, 2022 •

edited

Loading

james7132 commented May 1, 2022 •

edited

Loading

james7132 May 1, 2022 •

edited

Loading