Small owning ref refactoring #109948
Small owning ref refactoring #109948
Conversation
... which make `new_assert_stable_address` quite useless
rustbot
added
S-waiting-on-review
| @@ -1117,41 +1111,41 @@ impl<T: 'static> ToHandleMut for RefCell<T> { | |||
| // about which handle creation to use (i.e., read() vs try_read()) as well as | |||
| // what to do with error results. | |||
|
|
|||
| /// Typedef of an owning reference that uses a `Box` as the owner. | |||
| /// Type alias of an owning reference that uses a [`Box`] as the owner. | |||
There was a problem hiding this comment.
While we're at it, could you delete this impl for Box? It's unsound. If it's used we need to find a replacement. We can probably just use Arc without regressing perf.
There was a problem hiding this comment.
What impl? This is a type alias...
There was a problem hiding this comment.
Maybe the impl lives in the stable_deref_trait crate. But at least the type alias should be deleted, we should absolutely not be using an owning_ref with a Box, that's unsound because of boxes aliasing guarantees.
There was a problem hiding this comment.
Would it be okay if we wrap the owner in MaybeUninit to hide the aliasing? 🤔
At least it convinces the miri I think:
- Current version (miri detects ub ❌)
- With
MaybeUninit(miri doesn't see wrong ✅)
Although I'm not sure if this is just a bug in MIRI...
Also/1: why do we have a copy of the owning_ref crate? 🤔
Also/2: wait owning_ref crate (same as with this module) is straight up unsound? 🤔
There was a problem hiding this comment.
MaybeUninit would work but that'd lose the niche and it's also kinda hacky. I'd prefer just deleting the Box and using an Arc, it's the easiest solution. If we really want a Box, then we can roll or own or use the one from ouroboros. Or we can make T-opsem give up on giving Box these bad and confusing rules :D
I think owning_ref had some unrelated unsoundnesses and is unmaintained which is why the fixed version is copied here.
There was a problem hiding this comment.
I've searched for uses of (Owning|Box)Ref and we appear to only have 3 uses:
- With
Mmap(which is itself unsound 💀 ) inrustc_codegen_ssa::back::metadata::DefaultMetadataLoader(uses box to erase type) - With
Mmap( 💀 ) andVec<u8>inrustc_metadata::locator(uses box to erase type) - With
MetadataBlobinrustc_metadata::rmeta::def_path_hash_map
Given that all of these use T = [u8] and most erase U... maybe we can "just" yeet owning_ref with all it's unsoundness altogether and instead have something like this?
There was a problem hiding this comment.
I tried an overcomplicated version of it in #97770. Your example sounds great, you should try it :).
|
Closing in favor of #109971 |
…strieb Yeet `owning_ref` Based on the discussions from rust-lang#109948 This replaces `owning_ref` with a far simpler & safer abstraction. Fixes rust-lang#109974
…strieb Yeet `owning_ref` Based on the discussions from rust-lang#109948 This replaces `owning_ref` with a far simpler & safer abstraction. Fixes rust-lang#109974
…rieb Yeet `owning_ref` Based on the discussions from rust-lang#109948 This replaces `owning_ref` with a far simpler & safer abstraction. Fixes rust-lang#109974
Yeet `owning_ref` Based on the discussions from rust-lang/rust#109948 This replaces `owning_ref` with a far simpler & safer abstraction. Fixes #109974
r? @Nilstrieb