Replace unsafe usage of NonNull::new_unchecked with Box::into_non_null - #160251
Replace unsafe usage of NonNull::new_unchecked with Box::into_non_null#160251ArhanChaudhary wants to merge 1 commit into
NonNull::new_unchecked with Box::into_non_null#160251Conversation
rustbot
added
S-waiting-on-review
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rustbot reroll |
|
Unfortunately, all four cases here must be |
|
oh, it might not be forbidden entirely? it appears to be an open question: rust-lang/unsafe-code-guidelines#316 I think that the consensus from that issue is that it has to be sound for the global allocator, but might not be for custom allocators? I'm unsure, someone from opsem should probably weigh in on this. |
|
cc @rust-lang/opsem ^^ Personally, the PR looks fine as far as semantics, but that's not my area of expertise. I have to say that I do find the |
|
In addition to the above, there was recent discussion about this topic on Zulip: #t-opsem > Custom allocators vs Box::leak and "unleaking" |
|
It's pretty scary that clippy is now suggesting the more dangerous pattern. I would much prefer the lint be removed from clippy until the open questions are settled. |
Haha... I know right. Whoever contributed that lint clearly should have thought about it more. ( it was me :3 ) |
|
Yeah, the lint looks reasonable and I did see you cite a comment that predates rust-lang/unsafe-code-guidelines#316, and the opsem implications aren't obvious from the code because the impact isn't local. And we don't backlink to all previous discussions of a topic when we find a lurking problem. |
|
Agreed with @saethlin . Please use |
|
Thank you advising. How does this look? |
ArhanChaudhary
changed the title
NonNull::new_unchecked with NonNull::from_mutNonNull::new_unchecked with Box::into_non_null
|
|
|
Thank you! How about now? |
|
LGTM, thanks! |
This comment has been minimized.
This comment has been minimized.
…null` * replace unsafe usage of `NonNull::new_unchecked` with `NonNull::from_mut` * apply review feedback * apply review feedback again!
|
🔨 3 commits were squashed into c2d84bc. |
0151071 to
c2d84bc
Compare
|
@bors r=jhpratt,RalfJung rollup |
rust-bors
Bot
added
the
S-waiting-on-bors
rust-bors
Bot
removed
the
S-waiting-on-review
…=jhpratt,RalfJung
Replace unsafe usage of `NonNull::new_unchecked` with `Box::into_non_null`
~~We can avoid unsafe by replacing `unsafe { NonNull::new_unchecked(Box::into_raw(..)) }` with `NonNull::from_mut(Self::leak(..))`. A simple test to demonstrate the code generation is equivalent: https://godbolt.org/z/P5q9bzPPK~~
We can avoid unsafe by replacing `unsafe { NonNull::new_unchecked(Box::into_raw(..)) }` with the (soon-to-be-stable) `Box::into_non_null(..)`
Additionally, slightly tweak documentation.
…uwer Rollup of 10 pull requests Successful merges: - #157572 (stabilize size_of_val_raw, align_of_val_raw, Layout::for_value_raw) - #160012 (miri: ensure validity of references and pointers we dereference and cast) - #160294 (Update Enzyme to resolve one of the open bugs) - #159503 (allocations: document that they can be read-only) - #160250 (When issuing suggestions for missing trait items, label unstable items) - #160251 (Replace unsafe usage of `NonNull::new_unchecked` with `Box::into_non_null`) - #160311 (Remove final use of sealed traits from stdlib) - #160313 (Make the noundef-on-Cast size guard explicit) - #160323 (Box::leak: tell people to avoid unleaking) - #160328 (Move `check_track_caller` into the attribute parser)
…=jhpratt,RalfJung
Replace unsafe usage of `NonNull::new_unchecked` with `Box::into_non_null`
~~We can avoid unsafe by replacing `unsafe { NonNull::new_unchecked(Box::into_raw(..)) }` with `NonNull::from_mut(Self::leak(..))`. A simple test to demonstrate the code generation is equivalent: https://godbolt.org/z/P5q9bzPPK~~
We can avoid unsafe by replacing `unsafe { NonNull::new_unchecked(Box::into_raw(..)) }` with the (soon-to-be-stable) `Box::into_non_null(..)`
Additionally, slightly tweak documentation.
…uwer Rollup of 12 pull requests Successful merges: - #157572 (stabilize size_of_val_raw, align_of_val_raw, Layout::for_value_raw) - #160012 (miri: ensure validity of references and pointers we dereference and cast) - #160294 (Update Enzyme to resolve one of the open bugs) - #159503 (allocations: document that they can be read-only) - #160179 (std: Update `wasip3` crate dependency) - #160250 (When issuing suggestions for missing trait items, label unstable items) - #160251 (Replace unsafe usage of `NonNull::new_unchecked` with `Box::into_non_null`) - #160311 (Remove final use of sealed traits from stdlib) - #160313 (Make the noundef-on-Cast size guard explicit) - #160323 (Box::leak: tell people to avoid unleaking) - #160328 (Move `check_track_caller` into the attribute parser) - #160333 (Remove itertools dependency from `rustc_ast_pretty`)
7 participants
View all comments
We can avoid unsafe by replacingunsafe { NonNull::new_unchecked(Box::into_raw(..)) }withNonNull::from_mut(Self::leak(..)). A simple test to demonstrate the code generation is equivalent: https://godbolt.org/z/P5q9bzPPKWe can avoid unsafe by replacing
unsafe { NonNull::new_unchecked(Box::into_raw(..)) }with the (soon-to-be-stable)Box::into_non_null(..)Additionally, slightly tweak documentation.