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 a new std::mem::copied function. #78

Closed
jyn514 opened this issue Aug 1, 2022 · 6 comments
Closed

Add a new std::mem::copied function. #78

jyn514 opened this issue Aug 1, 2022 · 6 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@jyn514
Copy link
Member

jyn514 commented Aug 1, 2022

Proposal

Add a new std::mem::copied function.

Problem statement, Motivation, use-cases

Right now if you want to pass a function pointer to a combinator, you need to define a new closure. This is a very small paper cut, but it comes up quite frequently. Having a named function reduces the line noise and makes the code easier to read.

let result_from_ffi_function: Result<(), &i32> = Err(&1);
let result_copied: Result<(), i32> = result_from_ffi_function.map_err(|x| *x);

Solution sketches

// core::mem
pub fn copied<T: Copy>(x: &T) -> T { *x }

The new function will make the code simpler:

use std::mem::copied;
let result_copied: Result<(), i32> = result_from_ffi_function.map_err(copied);

Links and related work

This mirrors core::mem::drop and core::mem::take.

rust-lang/rust#95534, rust-lang/rust#98262, rust-lang/rust#100006

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@jyn514 jyn514 added T-libs-api api-change-proposal A proposal to add or alter unstable APIs in the standard libraries labels Aug 1, 2022
@thomcc
Copy link
Member

thomcc commented Aug 1, 2022

Note: the actual proposal here is to rename the unstable core::mem::copy function to core::mem::copied, and make it a const fn.

Background: this is a function which simply takes a &T where T: Copy and dereferences it, copying it. According to rust-lang/rust#95534, the main use case for this function is for use in combinators, which seems reasonable1, as a.map(core::mem::copied) does seem clearer than a.map(|b| *b).

The motivation for making it a const fn is that you can already dereference a Copy reference in const contexts. That is, this grants no extra capabilities and it would seem inconsistent if not allowed.

The motivation for the rename to core::mem::copied is to distinguish it from core::ptr::copy, which performs a very different operation. The name copied is consistent with Iterator::copied, Option::copied, as those combinators are morally equivalent to something like thing.map(core::mem::copied)2. Given that the use case is combinators, I do think this name makes sense, although I was initially opposed until I realized this. So I'm mostly in favor, especially given that it's unstable.

I think the main thing I'd wonder about this is if there's some module besides core::mem it could live in. It's hard for me to act like mem::copied doesn't bring memcpy to my mind (even if the name of the function on its own makes a lot of sense, and is an improvement).

Footnotes

  1. I don't feel that strongly about its usefulness, and don't think I'd go to bat for the functions existence, as I don't know that I've needed to write a closure like |b| *b very many times (at least not since we added .copied() to things).

  2. This is of course less true for Iterator than Option (due to the return type), but you get the point.

@jyn514
Copy link
Member Author

jyn514 commented Aug 1, 2022

at least not since we added .copied() to things

Note that copied() only exists for Iterator and Option; for it to be as useful as the standalone function there would need to be a Result::copied and Result::copied_err. And that still wouldn't support custom data structures or enums.

I agree mem::copied seems a little confusing, but not sure a better name for it ...

@scottmcm
Copy link
Member

scottmcm commented Oct 16, 2022

Copying from rust-lang/rust#95534 (comment):

I think this might really want to be something like

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

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

And I think someone might be working on an RFC for such an attribute...

@ChayimFriedman2
Copy link

@scottmcm This has the disadvantage that you can't use it fully.

@mbartlett21
Copy link

mbartlett21 commented Oct 17, 2022

But it then has the advantage that it is always in scope, since Clone is in the prelude, and can always* be called as a method.

* Actually, it isn't quite always, since types can have a .copy() method, but those methods should be few and very far between...

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 29, 2022
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
@joshtriplett
Copy link
Member

👍 for the rename and constification.

(There's still a fair bit of debate about whether we want to stabilize this function, but we all agreed that the new name is better.)

@joshtriplett joshtriplett added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

6 participants