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

stabilize core parts of MaybeUninit #60445

Merged
merged 6 commits into from
May 20, 2019
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented May 1, 2019

and deprecate mem::uninitialized in the future (1.40.0). This is part of implementing rust-lang/rfcs#1892.

Also expand the documentation a bit.

This type is currently primarily useful when dealing with partially initialized arrays. In libstd, it is used e.g. in BTreeMap (with some unstable APIs that however can all be replaced, less ergonomically, by stable ones). What we stabilize should also be enough for SmallVec (Cc @bluss).

Making this useful for structs requires rust-lang/rfcs#2582 or a commitment that references to uninitialized data are not insta-UB.

@rust-highfive
Copy link
Collaborator

r? @Kimundi

(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 May 1, 2019
@Centril
Copy link
Contributor

Centril commented May 1, 2019

r? @Centril

@rust-highfive rust-highfive assigned Centril and unassigned Kimundi May 1, 2019
@Centril Centril 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. relnotes Marks issues that should be documented in the release notes of the next release. labels May 1, 2019
@Centril Centril added this to the 1.36 milestone May 1, 2019
@rust-highfive

This comment has been minimized.

src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
#[inline]
#[rustc_deprecated(since = "2.0.0", reason = "use `mem::MaybeUninit::uninit` instead")]
#[rustc_deprecated(since = "1.40.0", reason = "use `mem::MaybeUninit` instead")]
Copy link
Member

Choose a reason for hiding this comment

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

Why not 1.36?

Copy link
Member Author

Choose a reason for hiding this comment

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

This function is widely used so I figured we'd give it some time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest 1.38 instead as a middle ground. 1.34.0 seems rather long (~6 months from 1.36).

Copy link
Member Author

@RalfJung RalfJung May 2, 2019

Choose a reason for hiding this comment

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

The precedent with #52994 is to keep it future-deprecated for 3 releases, that would be 1.39.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

It's looks pretty great overall but here are some things that would be good to improve on.

I'll work up an FCP when I wake up tomorrow :)

src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
src/libcore/mem.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented May 2, 2019

Stabilization proposal

I propose that we stabilize the following API:

#[derive(Copy)]
pub union MaybeUninit<T> {
    uninit: (),
    value: ManuallyDrop<T>,
}

impl<T: Copy> Clone for MaybeUninit<T> { ... }

impl<T> MaybeUninit<T> {
    pub const fn new(val: T) -> MaybeUninit<T> { ... }

    pub const fn uninit() -> MaybeUninit<T> { ... }

    pub fn zeroed() -> MaybeUninit<T> { ... }

    pub fn as_ptr(&self) -> *const T { ... }

    pub fn as_mut_ptr(&mut self) -> *mut T { ... }

    pub unsafe fn assume_init(self) -> T { ... }
}

The proposed interface is minimal yet still useful as noted in the PR description:

This type is currently primarily useful when dealing with partially initialized arrays. In libstd, it is used e.g. in BTreeMap (with some unstable APIs that however can all be replaced, less ergonomically, by stable ones). What we stabilize should also be enough for SmallVec (Cc @bluss).

As the description notes, the API has also been tested inside the standard library and elsewhere.

The name of the type, MaybeUninit, has already gone through the FCP process and the other parts have been extensively bikeshedded. As such, I believe we are ready to commit to this today with more to come eventually.

The documentation for MaybeUninit<T> is already thorough, with more documentation being added in this PR.

A further note: rust-lang/rfcs#2582 has not been accepted yet and therefore we leave in place a warning in the documentation for anyone that tries to rely on those semantics with current surface syntax. Or as the PR description notes:

Making this useful for structs requires rust-lang/rfcs#2582 or a commitment that references to uninitialized data are not insta-UB.

As for the deprecation of mem::uninitialized, I propose that we schedule to deprecate uninitialized in 1.38. The function is widely used, but 3 months on stable should still be ample time to allow people to migrate.

With all that said:

@rfcbot merge

@rfcbot
Copy link

rfcbot commented May 2, 2019

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 2, 2019
src/libcore/mem.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Small nit; otherwise it looks great.

src/libcore/mem.rs Outdated Show resolved Hide resolved
@withoutboats
Copy link
Contributor

Is there a crater run on how much of the ecosystem currently uses mem::uninitialized?

@Centril
Copy link
Contributor

Centril commented May 2, 2019

@withoutboats I don't believe so.

@Centril
Copy link
Contributor

Centril commented May 2, 2019

We discussed this briefly on the lang team meeting. Mainly, the subject was the deprecation schedule. We agreed that clear communication was important. We should also try to do a crater run to see when deprecation is most opportune. However, (in my personal view) we can stabilize and tweak the deprecation date later based on data.

@RalfJung
Copy link
Member Author

@bors r=Centril

@bors
Copy link
Contributor

bors commented May 20, 2019

📌 Commit 1916391 has been approved by Centril

@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 May 20, 2019
@RalfJung
Copy link
Member Author

@bors p=1

@bors
Copy link
Contributor

bors commented May 20, 2019

⌛ Testing commit 1916391 with merge 6dabc5d1c3298e8fede2f41503d30e27e48e6236...

@pietroalbini
Copy link
Member

@bors retry

Yielding priority to the stable release.

@SimonSapin
Copy link
Contributor

@pietroalbini By the way, https://github.com/rust-lang/rust/blob/beta/src/bootstrap/channel.rs mentioning 1.35 suggests that 1.36 beta has not been cut yet, is that right? It’d be nice if this PR can make it in.

@RalfJung
Copy link
Member Author

The process says "Promote master to beta (T-2 days, Tuesday)", so I take it if we can land this today we are good?

That was the expectation anyways. ;)

@RalfJung
Copy link
Member Author

@bors p=50

@pietroalbini
Copy link
Member

@SimonSapin yep, beta will be cut tomorrow. I'll try to remember to check this PR before cutting it.

@bors
Copy link
Contributor

bors commented May 20, 2019

⌛ Testing commit 1916391 with merge d35181a...

bors added a commit that referenced this pull request May 20, 2019
stabilize core parts of MaybeUninit

and deprecate mem::uninitialized in the future (1.40.0). This is part of implementing rust-lang/rfcs#1892.

Also expand the documentation a bit.

This type is currently primarily useful when dealing with partially initialized arrays. In libstd, it is used e.g. in `BTreeMap` (with some unstable APIs that however can all be replaced, less ergonomically, by stable ones). What we stabilize should also be enough for `SmallVec` (Cc @bluss).

Making this useful for structs requires rust-lang/rfcs#2582 or a commitment that references to uninitialized data are not insta-UB.
@bors
Copy link
Contributor

bors commented May 20, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Centril
Pushing d35181a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 20, 2019
@bors bors merged commit 1916391 into rust-lang:master May 20, 2019
Centril added a commit to Centril/rust that referenced this pull request May 21, 2019
adjust deprecation date of mem::uninitialized

In rust-lang#60445 we [decided](rust-lang#60445 (comment)) that we'd deprecate for 1.38 instead of 1.40, but I forgot to adjust for that.
Centril added a commit to Centril/rust that referenced this pull request May 21, 2019
adjust deprecation date of mem::uninitialized

In rust-lang#60445 we [decided](rust-lang#60445 (comment)) that we'd deprecate for 1.38 instead of 1.40, but I forgot to adjust for that.
Centril added a commit to Centril/rust that referenced this pull request May 21, 2019
adjust deprecation date of mem::uninitialized

In rust-lang#60445 we [decided](rust-lang#60445 (comment)) that we'd deprecate for 1.38 instead of 1.40, but I forgot to adjust for that.
Centril added a commit to Centril/rust that referenced this pull request May 22, 2019
adjust deprecation date of mem::uninitialized

In rust-lang#60445 we [decided](rust-lang#60445 (comment)) that we'd deprecate for 1.38 instead of 1.40, but I forgot to adjust for that.
Centril added a commit to Centril/rust that referenced this pull request May 22, 2019
adjust deprecation date of mem::uninitialized

In rust-lang#60445 we [decided](rust-lang#60445 (comment)) that we'd deprecate for 1.38 instead of 1.40, but I forgot to adjust for that.
lawliet89 added a commit to lawliet89/rocket_cors that referenced this pull request Nov 13, 2019
lawliet89 added a commit to lawliet89/rocket_cors that referenced this pull request Nov 13, 2019
- Fix clippy lints
- Bump minimum Rust version beyond Rocket required for
  - `std::mem::MaybeUninit` (cf. rust-lang/rust#60445)
  - `alloc` crate
v1olen pushed a commit to v1olen/rocket_cors that referenced this pull request Mar 18, 2021
- Fix clippy lints
- Bump minimum Rust version beyond Rocket required for
  - `std::mem::MaybeUninit` (cf. rust-lang/rust#60445)
  - `alloc` crate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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

Successfully merging this pull request may close these issues.