Update transmute_copy to ub_checks and ?Sized#155989
Update transmute_copy to ub_checks and ?Sized#155989scottmcm wants to merge 1 commit intorust-lang:mainfrom
transmute_copy to ub_checks and ?Sized#155989Conversation
scottmcm
added
T-libs-api
rustbot
added
the
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Currently, this UB is detected at run time when |
|
Yeah I agree, making this an non-unwinding panic seems like an obvious improvement, but disabling the check when UB checks are disabled seems strictly worse. The idea of UB checks being optional is that they can be expensive, but this check is constant and therefore guaranteed to be free, so why not always? |
|
The check isn't constant when the source is unsized. |
|
Also, the thing with the "well it's a 'free' check" is that that's only true if it's Plus, it's often not language UB to violate this (at least under TB), since often the reference is into a larger allocation anyway. The interesting and difficult part of the UB remains fundamentally uncheckable. The assert was useful before because people never noticed that they were doing something always-UB. But the ubcheck is absolutely sufficient to cover that need. If you have code that you never run in debug ever and you're pervasively getting your |
scottmcm
added
S-waiting-on-t-libs-api
|
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rust-rfcbot
added
the
proposed-final-comment-period
Amanieu
removed
the
I-libs-api-nominated
rust-rfcbot
added
the
disposition-merge
8 participants
The
assert!intransmute_copyis from #98839. However, it was controversial at the time because it's possible to writetransmute_copys that were UB by the description but didn't pass the documented conditions. It's also sub-optimal to have possiblepanics in unsafe code where the panic can actually make things worse by dropping things in the middle of what the programmer expected to be panic-free. Now that we have theUbCheckspossibility, though, we can do this better: this PR makes it a non-unwinding panic when not meeting the documented condition when UB checks are enabled, but which very clearly emphasizes that said check isn't a documented behaviour of the function.Also, I was staring at the signature and couldn't figure out why it needs
Src: Sized. Since it always takes a reference, there seems to be no downside to acceptingSrc: ?Sized. That would also give it more of a reason to exist even if https://doc.rust-lang.org/nightly/std/mem/fn.transmute_prefix.html exists (astransmute_prefix(x)existing will no longer need thetransmute_copy(&ManuallyDrop::new(x))polyfill, cc rust-lang/rfcs#3844). That definitely needs an FCP, though.(The change to
assert_unsafe_preconditionhere doesn't technically need libs-api assent, but I'd still like to hear your thoughts since it's different from the "promise the assert" that IIRC you were also asked to consider elsewhere.)A partial replacement for #154665