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

MaybeUninit<T> could be Copy for all T #62835

Open
Diggsey opened this issue Jul 20, 2019 · 10 comments
Open

MaybeUninit<T> could be Copy for all T #62835

Diggsey opened this issue Jul 20, 2019 · 10 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Diggsey
Copy link
Contributor

Diggsey commented Jul 20, 2019

At the moment it only implements Copy when T: Copy, but there's no memory-safety reason for it not to always be Copy.

@jonas-schievink jonas-schievink added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 20, 2019
@ExpHP
Copy link
Contributor

ExpHP commented Jul 20, 2019

I do agree it sounds safe, but perhaps the lack of this serves as a useful lint.

E.g. somebody could do this:

let place = MaybeUninit::<Vec<()>>::uninit();

place.write(vec![]);

// because they're calling "clone", user might think this is safe;
// but in reality, the same vec is read twice.

let vec_1 = unsafe { place.clone().assume_init() };
let vec_2 = unsafe { place.clone().assume_init() };

@Diggsey
Copy link
Contributor Author

Diggsey commented Jul 20, 2019

True, but then we probably need another way to make a non-Copy type Copy.

@nagisa
Copy link
Member

nagisa commented Jul 20, 2019

The example above could be reproduced even without Clone:

let place = MaybeUninit::<Vec<()>>::uninit();
place.write(vec![]);
// implicit Copy
let vec_1 = unsafe { place.assume_init() };
// Boom.
let vec_2 = unsafe { place.assume_init() };

@Diggsey what’s your use-case that’s not served by the APIs currently available?

@Diggsey
Copy link
Contributor Author

Diggsey commented Jul 20, 2019

It came up whilst I was looking at ways to get slotmap to work with non-Copy types on stable rust, although that's not a particularly compelling use-case. Mostly it's just a missing escape hatch. We have:

  • UnsafeCell for making the immutable, mutable
  • MaybeUninit for making the initialised, uninitialised
  • unsafe impl Send for making the unsendable, sendable
  • unsafe impl Sync for making the not-thread-safe, thread-safe

But there is no escape hatch for making non-Copy data Copy.

@RalfJung
Copy link
Member

RalfJung commented Jul 21, 2019

True, but then we probably need another way to make a non-Copy type Copy.

There was talk about an unsafe impl Copy in the context of #25053.

@cuviper
Copy link
Member

cuviper commented Jul 23, 2019

Maybe there could be an explicit method to copy MaybeUninit contents?

impl<T> MaybeUninit<T> {
    fn copy(&self) -> Self {
        unsafe { ptr::read(self) }
    }
}

@RalfJung
Copy link
Member

RalfJung commented Jul 23, 2019

@cuviper you mean something like this?

EDIT: Ah, no you meant it should return a MaybeUninit<T>.
Given that this does not actually help for the stated motivation of "unsafely asserting that a type is Copy", I am not sure what that would achieve.

@cuviper
Copy link
Member

cuviper commented Jul 23, 2019

It wouldn't literally be Copy, but this would still be an "escape hatch" to copy the thing. I don't know if that's sufficient for what slotmap wants, and of course they can do the same ptr::read "copy" anyway.

@Diggsey
Copy link
Contributor Author

Diggsey commented Jul 23, 2019

Yeah, the issue is not with actually being able to do a memcpy on the type - that is easy enough. The goal is to be able to use in generic contexts which have a Copy bound.

@timvermeulen
Copy link
Contributor

Ran into this today when I tried to use <[MaybeUninit<T>]>::copy_within.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants