Skip to content

add rustc_no_writable to mem::forget and structs it uses #159181

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.

Sign up for GitHub

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
rust-bors merged 1 commit into from
Jul 15, 2026
Merged

add rustc_no_writable to mem::forget and structs it uses #159181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/core/src/mem/manually_drop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ impl<T> ManuallyDrop<T> {
#[stable(feature = "manually_drop", since = "1.20.0")]
#[rustc_const_stable(feature = "const_manually_drop", since = "1.32.0")]
#[inline(always)]
#[rustc_no_writable]
pub const fn new(value: T) -> ManuallyDrop<T> {
ManuallyDrop { value: MaybeDangling::new(value) }
}
Expand Down
1 change: 1 addition & 0 deletions library/core/src/mem/maybe_dangling.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ pub struct MaybeDangling<P: ?Sized>(P);

impl<P: ?Sized> MaybeDangling<P> {
/// Wraps a value in a `MaybeDangling`, allowing it to dangle.
#[rustc_no_writable]
pub const fn new(x: P) -> Self
where
P: Sized,
Expand Down
3 changes: 3 additions & 0 deletions library/core/src/mem/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ pub mod type_info;
#[rustc_const_stable(feature = "const_forget", since = "1.46.0")]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_diagnostic_item = "mem_forget"]
#[rustc_no_writable]
pub const fn forget<T>(t: T) {
let _ = ManuallyDrop::new(t);
}
Expand Down Expand Up @@ -1170,6 +1171,7 @@ pub const unsafe fn transmute_copy<Src, Dst>(src: &Src) -> Dst {
/// let _: std::mem::MaybeUninit<u16> = unsafe { transmute_prefix(123_u8) };
/// ```
#[unstable(feature = "transmute_prefix", issue = "155079")]
#[rustc_no_writable]
pub const unsafe fn transmute_prefix<Src, Dst>(src: Src) -> Dst {
#[repr(C)]
union Transmute<A, B> {
Expand Down Expand Up @@ -1219,6 +1221,7 @@ pub const unsafe fn transmute_prefix<Src, Dst>(src: Src) -> Dst {
#[unstable(feature = "transmute_neo", issue = "155079")]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
#[inline]
#[rustc_no_writable]
pub const unsafe fn transmute_neo<Src, Dst>(src: Src) -> Dst {
const { assert!(Src::SIZE == Dst::SIZE) };

Expand Down
20 changes: 20 additions & 0 deletions src/tools/miri/tests/pass/tree_borrows/mem_forget-implicit_writes.rs
View file
Open in desktop
Comment thread
quiode marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Regression test. This failed before applying `#[rustc_no_writable]` to `mem::forget`, `ManuallyDrop::new`, and `MaybeDangling::new`.
//@compile-flags: -Zmiri-tree-borrows -Zmiri-tree-borrows-implicit-writes

@RalfJung RalfJung Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This currently doesn't test ManuallyDrop.

Also seems like transmute_prefix and transmute_neo should also get the attribute?

@quiode quiode Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly is meant with it doesn't test ManuallyDrop. That the code by asquared31415 (#159181 (comment)) does not cause UB (before and after) or that a similar function like transmute but with ManuallyDrop now also doesn't cause UB (but before did)?

@RalfJung RalfJung Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could remove #[rustc_no_writable] from ManuallyDrop and your test would still pass, wouldn't it?

@quiode quiode Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, when I remove it from the ManuallyDrop::new function, the test fails. Same goes for MaybeDangling::new.

@asquared31415 asquared31415 Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mem::forget uses ManuallyDrop::new which uses MaybeDangling::new, so all of the functions in this chain get passed the &mut.

@quiode quiode Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I should maybe have mentioned that 😅. That's why I added the attribute.

@RalfJung RalfJung Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense, thanks :)


use std::mem;

// This function is taken from the crate `derive_more`, from the file `into.rs`
unsafe fn transmute<From, To>(from: From) -> To {
let to = unsafe { mem::transmute_copy(&from) };
mem::forget(from);
to
}
Comment on lines +7 to +11

@asquared31415 asquared31415 Jul 12, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that we want to bless this pattern with mem::forget specifically, as opposed to guiding people to "pre-forgetting" to avoid reborrowing a reference after creating a derived reference? The following replacement of transmute, for example, does not report UB under the same miri settings.

unsafe fn transmute<From, To>(from: From) -> To {
    let from = ManuallyDrop::new(from); // effectively already forgotten, but happens first, so no reborrow after the copy
    unsafe { mem::transmute_copy(&from) }
}

View changes since the review

@saethlin saethlin Jul 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a test case is not the same as blessing a code pattern. We should still tell people that it is a bad idea to write code like this.

@asquared31415 asquared31415 Jul 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically adding the attribute to mem::forget was what I was considering "blessing" it, but the test case was the most obvious place I could think of to demonstrate the pattern. But if this attribute is mostly meant as "internally mitigate the real world effects" rather than a "this is fine actually", then it makes more sense.

@saethlin saethlin Jul 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing that should be taken as "this is fine actually" is documentation.

@JoJoDeveloping JoJoDeveloping Jul 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attribute is part of an experiment by @RalfJung and me and @quiode to see if we can unlock more optimizations w/o breaking too much real-world code and that includes figuring out which places need this attribute (and if that is "too many"). Since this is all highly experimental, we have not yet thought about how much "intentionality" the attribute ought to convey. Currently we're using it for both, at some later point we will need to decide where the attribute is "intended" and where not (if we decide we want to keep it at all). So I hope we do not need to decide that now :)

@quiode quiode Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is part of trying to minimize the amount of new UB introduced to existing crates, so the optimization could become useful. This function seemed like a good place because apparently it causes trouble in some code out there, independent if it's actually good code or not. Currently, under Tree Borrows at least, that pattern would be allowed so I took that as my point of reference.


fn main() {
let mut val = 10u32;
let r: &mut u32 = &mut val;

let to: &mut i32 = unsafe { transmute(r) };

assert_eq!(*to, 10);
}
Loading