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 core::mem::copy to complement core::mem::drop. #95534

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 31, 2022

This is useful for combinators. I didn't add clone since you can already
use Clone::clone in its place; copy has no such corresponding function.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2022
@jyn514
Copy link
Member Author

jyn514 commented Mar 31, 2022

An alternative to this is to add Result::copied_err. But that's less general, and suggests adding Result::cloned_err, which seems unnecessary when map_err(Clone::clone) already exists.

@scottmcm
Copy link
Member

Spitballing: in the vein of Iterator::cloned vs Iterator::copied (and analogous things on Result and Option and such), what if it was .copy()? I don't like changing foo.bar().clone() to *foo.bar(), and copy(foo.bar()) isn't amazing either. But a clippy lint for "that's always copy, consider foo.bar().copy()" would be a nice diff. And passing, say, Copy::copy to an adapter instead of mem::copy is reasonable enough, IMHO.

I have no idea where it would live, though. It'd probably want to be sealed if it were a method, since anyone overloading it would always be wrong to do so.

@jyn514
Copy link
Member Author

jyn514 commented Mar 31, 2022

what if it was .copy()

  1. I'm not sure if this is possible at all - the only place I can think of putting it is a trait method on Copy, but that's not right because overriding Copy isn't allowed.
  2. If the trait is sealed, it doesn't help with the original use case of "I need a function pointer".

@scottmcm
Copy link
Member

As a sketch, I could imagine something like this:

// in the prelude

mod secret {
    pub trait CopyExt: Copy {
        fn copy(&self) -> Self { *self }
    }
    impl<T: Copy> CopyExt for T {}
}

pub use secret::{CopyExt as _};

But maybe that's overkill, I don't know.

If the trait is sealed, it doesn't help with the original use case of "I need a function pointer".

I don't follow this comment. You can still get a function pointer to a trait method the same as to a generic function.

@jyn514
Copy link
Member Author

jyn514 commented Mar 31, 2022

I don't follow this comment. You can still get a function pointer to a trait method the same as to a generic function.

But that fails if the trait is private: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f1d46b2fba47b3b492d1a5d2eafa8446

error[E0599]: no function or associated item named `copy` found for struct `S` in the current scope
  --> src/main.rs:16:22
   |
8  |     pub struct S;
   |     ------------- function or associated item `copy` not found for this
...
16 |     let f: fn() = S::copy;
   |                      ^^^^ function or associated item not found in `S`
   |
   = help: items from traits can only be used if the trait is implemented and in scope
note: `Sealed` defines an item `copy`, perhaps you need to implement it
  --> src/main.rs:3:9
   |
3  |         pub(super) trait Sealed {
   |         ^^^^^^^^^^^^^^^^^^^^^^^

@jyn514
Copy link
Member Author

jyn514 commented Mar 31, 2022

and you can't pub use a private trait either, even anonymously:

error[E0365]: `Sealed` is private, and cannot be re-exported
  --> src/main.rs:10:13
   |
10 |     pub use sealed::Sealed as _;
   |             ^^^^^^^^^^^^^^^^^^^ re-export of private `Sealed`
   |
   = note: consider declaring type or module `Sealed` with `pub`

@scottmcm
Copy link
Member

You'll note that I didn't seal the trait. (The overlap rules would need it from being implemented without specialization anyway.) Then imported it as _ in the prelude, so it's available to everyone, but nobody can name it.

That does seem to work: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0aea6bf318c412ab0fd029d2bb28179f

@jyn514
Copy link
Member Author

jyn514 commented Mar 31, 2022

Ah interesting, yes you're right that it does the same thing as copy, but also allows using an inherent method. But it seems more complicated than just mem::copy. I'm ok with implementing either.

@scottmcm
Copy link
Member

Yeah, I'm also not convinced by it. We'll see what libs-api thinks of stuff.

I think what I'd really want is this:

pub trait Clone {
    fn clone(&self) -> Self; // as today

    #[sealed]
    fn copy(&self) -> Self { *self }
}

so it's a small change, the method can be feature gated still, nobody can ever override it, thus unsafe code can still rely on it being actually a copy, and passing Clone::copy works instead of <_>::copy.

But that attribute doesn't exist today. Maybe I should (find someone else to) write an MCP/RFC for it. Arguably the Visitor::super_* methods would like it too...

@clarfonthey
Copy link
Contributor

I think that even if there were a postfix version of copy, having a standalone copy method (prelude 2024?) would help for a bunch of cases, I feel.

That said, I would very much love to see a way to do a postfix copy, although I'm less sold on the idea of it being a method as I am on a dedicated postfix deref operator, like zig has. This is a bit beside the point, though; while I think this version of copy is easy to add here in a PR, such an operator would require an RFC to be properly fleshed out.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2022
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 23, 2022
@the8472 the8472 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 28, 2022
@joshtriplett
Copy link
Member

Seems fine to experiment with; we can evaluate it more before stabilization.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 19, 2022

📌 Commit bbc39287a417b17feaf054c2b76f8aa982b7e38c has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2022
@clarfonthey
Copy link
Contributor

Shouldn't this have a tracking issue before merging? cc @joshtriplett

@joshtriplett
Copy link
Member

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 19, 2022
@joshtriplett
Copy link
Member

Yes, it should.

This is useful for combinators. I didn't add `clone` since you can already
use `Clone::clone` in its place; copy has no such corresponding function.
@jyn514
Copy link
Member Author

jyn514 commented Jun 19, 2022

Good catch, thanks, fixed.

@bors r=joshtriplett rollup

@bors
Copy link
Contributor

bors commented Jun 19, 2022

📌 Commit 9ac6277 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 19, 2022
Add `core::mem::copy` to complement `core::mem::drop`.

This is useful for combinators. I didn't add `clone` since you can already
use `Clone::clone` in its place; copy has no such corresponding function.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2022
Rollup of 4 pull requests

Successful merges:

 - rust-lang#95534 (Add `core::mem::copy` to complement `core::mem::drop`.)
 - rust-lang#97912 (Stabilize `Path::try_exists()` and improve doc)
 - rust-lang#98225 (Make debug_triple depend on target json file content rather than file path)
 - rust-lang#98257 (Fix typos in `IntoFuture` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9d4e08e into rust-lang:master Jun 20, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 20, 2022
@jyn514 jyn514 deleted the std-mem-copy branch June 20, 2022 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

10 participants