Skip to content
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.

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

Add mem::{take, replace, swap} to the prelude #126785

Closed
wants to merge 2 commits into from

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Jun 21, 2024

This adds the following 3 functions from core::mem to the prelude:

fn replace<T>(dest: &mut T, src: T) -> T;
fn take<T: Default>(dest: &mut T) -> T;
fn swap<T>(x: &mut T, y: &mut T);

The main goal is to make Rust easier to teach, while also making common code patterns easier to write. Many new Rust users coming from other languages find Rust's ownership system hard to use and a part of that is because it does not allow leaving values in an uninitialized state after moving them, even temporarily. While the tools to work around this are available in core::mem, they are not very discoverable for new users. By treating these functions more like keywords (similar to drop), we can encourage the inclusion of these functions in teaching materials and expose new Rust users to them.

These 3 functions are also very widely used in production Rust code so even experienced Rust users will benefit from this change. For example, in the Rust compiler there are:

  • 74 uses of mem::replace.
  • 94 uses of mem::take.
  • 17 uses of mem::swap.

@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 21, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@jdonszelmann
Copy link
Contributor

As an educator, I can definitely see the value here. I think people tend to forget these functions, buried in std::mem, while talking about them gives (I think) a lot of insight into how ownership and moving works.

@flip1995
Copy link
Member

Not a blocker, but I personally like to prefix those functions with mem::. As those are pretty common function names, it's nice to have the context from where they are from. But YMMV.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 46)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 49)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 61)
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
+   --> $SRC_DIR/core/src/mem/mod.rs:LL:COL
9    |
+    = note: similarly named function `take` defined here
+    |
10    = note: this error originates in the macro `nested_expr` which comes from the expansion of the macro `call_nested_expr` (in Nightly builds, run with -Z macro-backtrace for more info)
12 error[E0425]: cannot find value `fake` in this scope

13   --> $DIR/macro-backtrace-nested.rs:5:12
14    |
14    |
15 LL |     () => (fake)
-    |            ^^^^ not found in this scope
+    |            ^^^^ help: a function with a similar name exists: `take`
17 ...
18 LL |     call_nested_expr_sum!();

+   --> $SRC_DIR/core/src/mem/mod.rs:LL:COL
+    |
+    = note: similarly named function `take` defined here
+    = note: similarly named function `take` defined here
20    |
21    = note: this error originates in the macro `nested_expr` which comes from the expansion of the macro `call_nested_expr_sum` (in Nightly builds, run with -Z macro-backtrace for more info)


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macros/macro-backtrace-nested/macro-backtrace-nested.stderr
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macros/macro-backtrace-nested/macro-backtrace-nested.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args macros/macro-backtrace-nested.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/macros/macro-backtrace-nested.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macros/macro-backtrace-nested" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macros/macro-backtrace-nested/auxiliary"
--- stderr -------------------------------
error[E0425]: cannot find value `fake` in this scope
##[error]  --> /checkout/tests/ui/macros/macro-backtrace-nested.rs:5:12
   |
   |
LL |     () => (fake) //~ ERROR cannot find
   |            ^^^^ help: a function with a similar name exists: `take`
LL |     1 + call_nested_expr!();
   |         ------------------- in this macro invocation
  --> /rustc/FAKE_PREFIX/library/core/src/mem/mod.rs:791:1
   |
   |
   = note: similarly named function `take` defined here
   |
   = note: this error originates in the macro `nested_expr` which comes from the expansion of the macro `call_nested_expr` (in Nightly builds, run with -Z macro-backtrace for more info)
error[E0425]: cannot find value `fake` in this scope
##[error]  --> /checkout/tests/ui/macros/macro-backtrace-nested.rs:5:12
   |
   |
LL |     () => (fake) //~ ERROR cannot find
   |            ^^^^ help: a function with a similar name exists: `take`
...
LL |     call_nested_expr_sum!();
  --> /rustc/FAKE_PREFIX/library/core/src/mem/mod.rs:791:1
   |
   = note: similarly named function `take` defined here
   |
   |
   = note: this error originates in the macro `nested_expr` which comes from the expansion of the macro `call_nested_expr_sum` (in Nightly builds, run with -Z macro-backtrace for more info)
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0425`.
------------------------------------------

@Amanieu Amanieu added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jun 21, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Jun 21, 2024

I don't think we should do this. At the very least, we need to consider alternative directions in language evolution.

Yeah, the compiler's name-res might be able to handle it, but what about the human's? ptr::replace and ptr::swap exist, and there are proposals to allow importing type-qualified or trait-qualified functions to use directly, in lieu of having additional free functions.

If such happens, the fact that replace and swap are common function names on types provide the opportunity for additional confusion.

@tgross35
Copy link
Contributor

Prior art, adding std::{mem, fmt} was proposed in RFC3090. Some dicsussion specific to those modules at rust-lang/rfcs#3090 (comment)

@Lee-Janggun
Copy link
Contributor

I personally actively avoid importing mem functions so that when I use them, I am sure this is the mem functions, and I don't have to consider the possibility it is some other function, e.g., Option::take. Of course, associated functions are typically used with the struct.function() syntax, but I would much rather keep these explicit. I do use drop as just drop(x), but I think the wording of drop in highlighted enough in rust that this is no ambiguity.

@jhpratt
Copy link
Member

jhpratt commented Jun 23, 2024

I personally like to prefix those functions with mem::. As those are pretty common function names

This is the case for myself and countless others as well. If anything, I would like to see core::mem/std::mem added to the prelude. Unless I'm mistaken this was discussed in the run-up to the 2021 edition.

For what it's worth, I'm not positive the compiler is currently able to handle the name resolution shadowing for modules the same way it can for functions. I say this because there are not currently any modules in the prelude.

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 23, 2024
@bors
Copy link
Contributor

bors commented Jun 26, 2024

☔ The latest upstream changes (presumably #126951) made this pull request unmergeable. Please resolve the merge conflicts.

@Amanieu
Copy link
Member Author

Amanieu commented Jul 2, 2024

Given the feedback (which I agree with!), I'm going to close this PR.

@Amanieu Amanieu closed this Jul 2, 2024
@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.