Skip to content

[do not merge] core::mem::replace using core::mem::swap #154575

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

Open
GrigorenkoPV wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[do not merge] core::mem::replace using core::mem::swap #154575

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
21 changes: 3 additions & 18 deletions library/core/src/mem/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,24 +912,9 @@ pub const fn take<T: [const] Default>(dest: &mut T) -> T {
#[must_use = "if you don't need the old value, you can just assign the new value directly"]
#[rustc_const_stable(feature = "const_replace", since = "1.83.0")]
#[rustc_diagnostic_item = "mem_replace"]
pub const fn replace<T>(dest: &mut T, src: T) -> T {
// It may be tempting to use `swap` to avoid `unsafe` here. Don't!
// The compiler optimizes the implementation below to two `memcpy`s
// while `swap` would require at least three. See PR#83022 for details.

// SAFETY: We read from `dest` but directly write `src` into it afterwards,
// such that the old value is not duplicated. Nothing is dropped and
// nothing here can panic.
unsafe {
// Ideally we wouldn't use the intrinsics here, but going through the
// `ptr` methods introduces two unnecessary UbChecks, so until we can
// remove those for pointers that come from references, this uses the
// intrinsics instead so this stays very cheap in MIR (and debug).

let result = crate::intrinsics::read_via_copy(dest);
crate::intrinsics::write_via_move(dest, src);
result
}
pub const fn replace<T>(dest: &mut T, mut src: T) -> T {
swap(dest, &mut src);
src
}

/// Disposes of a value.
Expand Down
16 changes: 9 additions & 7 deletions src/tools/miri/tests/fail/both_borrows/aliasing_mut4.tree.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: Undefined Behavior: write access through <TAG> at ALLOC[0x0] is forbidden
--> RUSTLIB/core/src/mem/mod.rs:LL:CC
|
LL | crate::intrinsics::write_via_move(dest, src);
| ^^^ Undefined Behavior occurred here
LL | unsafe { intrinsics::typed_swap_nonoverlapping(x, y) }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
|
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/tree-borrows.md for further information
Expand All @@ -20,15 +20,17 @@ help: the protected tag <TAG> was created here, in the initial state Frozen
LL | fn safe(x: &i32, y: &mut Cell<i32>) {
| ^
= note: stack backtrace:
0: std::mem::replace
0: std::mem::swap
at RUSTLIB/core/src/mem/mod.rs:LL:CC
1: std::cell::Cell::replace
1: std::mem::replace
at RUSTLIB/core/src/mem/mod.rs:LL:CC
2: std::cell::Cell::replace
at RUSTLIB/core/src/cell.rs:LL:CC
2: std::cell::Cell::set
3: std::cell::Cell::set
at RUSTLIB/core/src/cell.rs:LL:CC
3: safe
4: safe
at tests/fail/both_borrows/aliasing_mut4.rs:LL:CC
4: main
5: main
at tests/fail/both_borrows/aliasing_mut4.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
Expand Down
9 changes: 0 additions & 9 deletions tests/codegen-llvm/dead_on_return.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,6 @@ pub fn test_str_new(mut s: String) {
s = String::new();
}

#[unsafe(no_mangle)]
pub fn test_str_take(mut x: String) -> String {
// CHECK-LABEL: @test_str_take
// CHECK-NEXT: {{.*}}:
// CHECK-NEXT: call void @llvm.memcpy
// CHECK-NEXT: ret
core::mem::take(&mut x)
}
Copy link
Copy Markdown
Member

@bjorn3 bjorn3 Mar 30, 2026

Choose a reason for hiding this comment

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

Instead of removing this subtest, maybe fix the test expectation?

Copy link
Copy Markdown
Contributor Author

@GrigorenkoPV GrigorenkoPV Mar 30, 2026

Choose a reason for hiding this comment

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

From what I can tell, this is somewhat of a regression. If I understand correctly, the test checks that we just do a memcpy the pointers/sizes from x into the return value, because we no longer care about x after the function returns. But this is no longer the case after the change to mem::replace. This sounds like it might be a good place to stop right there and say "this is a regression, we're not doing it", but a maybe a perf run is still worth it?


#[unsafe(no_mangle)]
pub fn test_array_store(mut x: [u32; 100]) {
// CHECK-LABEL: @test_array_store
Expand Down
41 changes: 28 additions & 13 deletions tests/mir-opt/inline_coroutine_body.run2-{closure#0}.Inline.panic-abort.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,17 @@
+ scope 17 (inlined Option::<()>::take) {
+ let mut _42: std::option::Option<()>;
+ scope 18 (inlined std::mem::replace::<Option<()>>) {
+ scope 19 {
+ let _43: ();
+ let mut _44: &mut std::option::Option<()>;
+ scope 19 (inlined std::mem::swap::<Option<()>>) {
+ let mut _45: *mut std::option::Option<()>;
+ let mut _46: *mut std::option::Option<()>;
+ }
+ }
+ }
+ scope 20 (inlined #[track_caller] Option::<()>::expect) {
+ let mut _43: isize;
+ let mut _44: !;
+ let mut _47: isize;
+ let mut _48: !;
+ scope 21 {
+ }
+ }
Expand Down Expand Up @@ -191,7 +195,8 @@
+ StorageDead(_24);
+ StorageLive(_37);
+ StorageLive(_39);
+ StorageLive(_44);
+ StorageLive(_43);
+ StorageLive(_48);
+ StorageLive(_34);
+ StorageLive(_35);
+ StorageLive(_40);
Expand All @@ -201,12 +206,11 @@
+ StorageDead(_40);
+ StorageLive(_42);
+ _42 = Option::<()>::None;
+ _35 = copy ((*_37).0: std::option::Option<()>);
+ ((*_37).0: std::option::Option<()>) = move _42;
+ StorageDead(_42);
+ StorageLive(_43);
+ _43 = discriminant(_35);
+ switchInt(move _43) -> [0: bb11, 1: bb12, otherwise: bb5];
+ StorageLive(_45);
+ _45 = &raw mut ((*_37).0: std::option::Option<()>);
+ StorageLive(_46);
+ _46 = &raw mut _42;
+ _43 = typed_swap_nonoverlapping::<Option<()>>(move _45, move _46) -> [return: bb11, unwind unreachable];
}
+
+ bb5: {
Expand Down Expand Up @@ -266,16 +270,27 @@
+ }
+
+ bb11: {
+ _44 = option::expect_failed(const "`Ready` polled after completion") -> unwind unreachable;
+ StorageDead(_46);
+ StorageDead(_45);
+ _35 = move _42;
+ StorageDead(_42);
+ StorageLive(_47);
+ _47 = discriminant(_35);
+ switchInt(move _47) -> [0: bb12, 1: bb13, otherwise: bb5];
+ }
+
+ bb12: {
+ _48 = option::expect_failed(const "`Ready` polled after completion") -> unwind unreachable;
+ }
+
+ bb13: {
+ _34 = move ((_35 as Some).0: ());
+ StorageDead(_43);
+ StorageDead(_47);
+ StorageDead(_35);
+ _18 = Poll::<()>::Ready(move _34);
+ StorageDead(_34);
+ StorageDead(_44);
+ StorageDead(_48);
+ StorageDead(_43);
+ StorageDead(_39);
+ StorageDead(_37);
+ StorageDead(_22);
Expand Down
41 changes: 28 additions & 13 deletions tests/mir-opt/inline_coroutine_body.run2-{closure#0}.Inline.panic-unwind.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,17 @@
+ scope 17 (inlined Option::<()>::take) {
+ let mut _42: std::option::Option<()>;
+ scope 18 (inlined std::mem::replace::<Option<()>>) {
+ scope 19 {
+ let _43: ();
+ let mut _44: &mut std::option::Option<()>;
+ scope 19 (inlined std::mem::swap::<Option<()>>) {
+ let mut _45: *mut std::option::Option<()>;
+ let mut _46: *mut std::option::Option<()>;
+ }
+ }
+ }
+ scope 20 (inlined #[track_caller] Option::<()>::expect) {
+ let mut _43: isize;
+ let mut _44: !;
+ let mut _47: isize;
+ let mut _48: !;
+ scope 21 {
+ }
+ }
Expand Down Expand Up @@ -202,7 +206,8 @@
+ StorageDead(_24);
+ StorageLive(_37);
+ StorageLive(_39);
+ StorageLive(_44);
+ StorageLive(_43);
+ StorageLive(_48);
+ StorageLive(_34);
+ StorageLive(_35);
+ StorageLive(_40);
Expand All @@ -212,12 +217,11 @@
+ StorageDead(_40);
+ StorageLive(_42);
+ _42 = Option::<()>::None;
+ _35 = copy ((*_37).0: std::option::Option<()>);
+ ((*_37).0: std::option::Option<()>) = move _42;
+ StorageDead(_42);
+ StorageLive(_43);
+ _43 = discriminant(_35);
+ switchInt(move _43) -> [0: bb16, 1: bb17, otherwise: bb7];
+ StorageLive(_45);
+ _45 = &raw mut ((*_37).0: std::option::Option<()>);
+ StorageLive(_46);
+ _46 = &raw mut _42;
+ _43 = typed_swap_nonoverlapping::<Option<()>>(move _45, move _46) -> [return: bb16, unwind unreachable];
}

- bb6 (cleanup): {
Expand Down Expand Up @@ -299,16 +303,27 @@
+ }
+
+ bb16: {
+ _44 = option::expect_failed(const "`Ready` polled after completion") -> bb11;
+ StorageDead(_46);
+ StorageDead(_45);
+ _35 = move _42;
+ StorageDead(_42);
+ StorageLive(_47);
+ _47 = discriminant(_35);
+ switchInt(move _47) -> [0: bb17, 1: bb18, otherwise: bb7];
+ }
+
+ bb17: {
+ _48 = option::expect_failed(const "`Ready` polled after completion") -> bb11;
+ }
+
+ bb18: {
+ _34 = move ((_35 as Some).0: ());
+ StorageDead(_43);
+ StorageDead(_47);
+ StorageDead(_35);
+ _18 = Poll::<()>::Ready(move _34);
+ StorageDead(_34);
+ StorageDead(_44);
+ StorageDead(_48);
+ StorageDead(_43);
+ StorageDead(_39);
+ StorageDead(_37);
+ StorageDead(_22);
Expand Down
Loading