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

Tracking issue for #![feature(maybe_uninit_extra,const_maybe_uninit_write)] #63567

Open
Tracked by #16
Centril opened this issue Aug 14, 2019 · 67 comments
Open
Tracked by #16
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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. Libs-Tracked Libs issues that are tracked on the team's project board. requires-nightly This issue requires a nightly compiler in some way. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Aug 14, 2019

This is a tracking issue for the RFC "Deprecate uninitialized in favor of a new MaybeUninit type" (rust-lang/rfcs#1892).

This issue specifically tracks the following unstable methods:

impl<T> MaybeUninit<T> {
    pub unsafe fn assume_init_read(&self) -> T { ... }
    pub unsafe fn assume_init_drop(&mut self) { ... }
}

and it tracks const-ness of

impl<T> MaybeUninit<T> {
    pub const fn write(&mut self, val: T) -> &mut T { ... }
}
@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. B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. requires-nightly This issue requires a nightly compiler in some way. labels Aug 14, 2019
@RalfJung
Copy link
Member

Latest status here:

I am beginning to think that read should probably be renamed to read_init or read_assume_init or so, something that indicates that this may only be done after initialization is complete.

Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
@SimonSapin
Copy link
Contributor

@RalfJung Should the same logic apply to get_ref and get_mut? #63568

@RalfJung
Copy link
Member

That depends on whether references to invalid data are UB or not. If yes, then certainly. If not, then maybe.

Currently they are UB but that's just because it is the more conservative choice.

@KodrAus KodrAus added Libs-Tracked Libs issues that are tracked on the team's project board. A-raw-pointers Area: raw pointers, MaybeUninit, NonNull labels Jul 30, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 12, 2020
…it-drop, r=RalfJung

Add MaybeUninit::assume_init_drop.

`ManuallyDrop`'s documentation tells the user to use `MaybeUninit` instead when handling uninitialized data. However, the main functionality of `ManuallyDrop` (`drop`) is not available directly on `MaybeUninit`. Adding it makes it easier to switch from one to the other.

I re-used the `maybe_uninit_extra` feature and tracking issue number (rust-lang#63567), since it seems very related. (And to avoid creating too many features tracking issues for `MaybeUninit`.)
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 12, 2020
…it-drop, r=RalfJung

Add MaybeUninit::assume_init_drop.

`ManuallyDrop`'s documentation tells the user to use `MaybeUninit` instead when handling uninitialized data. However, the main functionality of `ManuallyDrop` (`drop`) is not available directly on `MaybeUninit`. Adding it makes it easier to switch from one to the other.

I re-used the `maybe_uninit_extra` feature and tracking issue number (rust-lang#63567), since it seems very related. (And to avoid creating too many features tracking issues for `MaybeUninit`.)
@ejmahler
Copy link

What stands in the way of stabilizing this?

@RalfJung
Copy link
Member

Hm, good question. I'd list these:

  • Figuring out if the return type of write makes sense and is useful as-is
  • Figuring out if these methods are useful enough to justify adding them to the API

@RalfJung
Copy link
Member

See #80376 for a potential concern around the return type of write.

@mgeier you mentioned this to be a potential problem for write_slice_cloned; do you think the same issue also applies to write?

@mgeier
Copy link
Contributor

mgeier commented Dec 27, 2020

I don't know whether this is an actual problem, but yes, it has the same issue: playground link

BTW, the underlying assume_init_mut() has the same issue: playground link

In the latter case it is arguably worse, because the method name contains assume_init, which suggests to me that this "activates" the destructor, which it doesn't. But I guess that has a different tracking issue: #63568.

For me, the real question is: what are the semantics of assigning to a &mut T?

I assume that when I'm assigning a new value, the old one will be guaranteed to be dropped. But what about the new value? Is the new value guaranteed to be dropped at some point? If yes, this issue is an actual problem.

But AFAICT, it is allowed in safe Rust to call std::mem::forget() on anything, so a &mut T is simply unable to guarantee that an assigned value will ever be dropped.

Still, I'm not sure whether accidental leaking should be made that easy?

@SimonSapin
Copy link
Contributor

We already have stable impl DerefMut for ManuallyDrop which makes this accidental leaking just as easy.

@RalfJung
Copy link
Member

In the latter case it is arguably worse, because the method name contains assume_init, which suggests to me that this "activates" the destructor, which it doesn't. But I guess that has a different tracking issue: #63568.

Why do you think assume_init has anything to do with destructors? That was never the intention. The only thing it is intended to convey is "if this is not yet initialized, there's UB".

@mgeier
Copy link
Contributor

mgeier commented Dec 28, 2020

Why do you think assume_init has anything to do with destructors? That was never the intention. The only thing it is intended to convey is "if this is not yet initialized, there's UB".

I guess the problem is that the function name assume_init() doesn't contain what it actually does.

Therefore, in my (limited) experience with stable Rust, I've come to imagine that assume_init() is in reality a shortcut that in its full form means "activate the destructor, assuming the memory has been properly initialized before".

That is fine as long as there is only a single function containing the words "assume init".
Now that multiple functions with "assume init" are proposed to be added, this becomes inconsistent and confusing.

I've looked back to the discussions about naming assume_init() (which happened before I was a Rust user) and I found #53491 (comment) which argues "Putting both the emphasis that it has to be initialized and a description of what it does (unwrap/into_inner/etc.) into one name gets rather unwieldy".
I think this was a fine argument as long as there was only one function containing "assume init", but it doesn't work with multiple "assume init" functions.

Now that we have a stable function assume_init(), I think we have to accept that the name assume_init contains more meaning than simply the two words "assume init", and I think we should use the words "assume init" in newly added functions only if they carry the full meaning of the already existing assume_init() function.

@RalfJung
Copy link
Member

I am rather surprised by this. Whether or not the destructor runs is fully encoded in the type -- when a T goes out of scope, its destructor runs; when a MaybeUninit<T> goes out of scope, nothing happens. This is the first time that I hear of the idea of "activating a destructor".

Now that we have a stable function assume_init(), I think we have to accept that the name assume_init contains more meaning than simply the two words "assume init", and I think we should use the words "assume init" in newly added functions only if they carry the full meaning of the already existing assume_init() function.

I disagree -- I think having more of these methods will actually make this more clear. Once assume_init_read and assume_init_ref/mut are stable, it seems unlikely that people would associate assume_init with "activating drop".

@Coder-256
Copy link
Contributor

Coder-256 commented Dec 28, 2020

I think in other words, the confusion is that assume_init/assume_init_read move the value, meaning it will be dropped, but assume_init_ref/assume_init_mut only reference the value, meaning it will not be dropped. I see how this can be confusing but honestly I actually like design because it's pretty similar to how pointers already work: the former two are like std::ptr::read(), and the latter two are like casting from a pointer to a reference.

@mgeier
Copy link
Contributor

mgeier commented Dec 30, 2020

@RalfJung

This is the first time that I hear of the idea of "activating a destructor".

Sorry that I'm using such naive and hand-wavey terminology.

But that's how I, as a humble Rust user, think about it. I have a MaybeUninit<T> variable which I, let's say initialize with some FFI function. Now the bits in memory have already the correct values, but in order to further work with it, I'd like to turn it into a T, which logically only does one thing: it "activates" the destructor.

I think having more of these methods will actually make this more clear. Once assume_init_read and assume_init_ref/mut are stable, it seems unlikely that people would associate assume_init with "activating drop".

That's true, but also because assume_init() will be stripped of any meaning with regards to what it actually does.

But anyway, I'm wondering: why do we even need assume_init_read and assume_init_ref/mut?

Aren't they just wrappers for one-liners?

I have the feeling that adding all those function will make Rust more complicated without really gaining anything.

I think using the one-liners directly makes it much more obvious to see what actually happens in the code.

@Coder-256

I see how this can be confusing but honestly I actually like design because it's pretty similar to how pointers already work: the former two are likestd::ptr::read(), and the latter two are like casting from a pointer to a reference.

I don't think this is similar at all. The function name std::ptr::read() contains a verb that tells me what the function actually does. In contrast, assume_init() does not tell me what it actually does (but granted, I can get quite a good idea by looking at the involved types).

Casting from a pointer to a reference isn't similar either. It is done by using sigils alone and doesn't involve any function names at all.

@RalfJung
Copy link
Member

That's true, but also because assume_init() will be stripped of any meaning with regards to what it actually does.

Well, the main meaning (I'd say) is that initialization is done and the compiler may now assume that the data is initialized. So I don't think meaning is "stripped".

But anyway, this is useful feedback, even if I don't quite know what to do with it. ;) Thanks!

@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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 9, 2021
@rfcbot
Copy link

rfcbot commented Nov 10, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 20, 2021
@rfcbot
Copy link

rfcbot commented Nov 20, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 9, 2021
@ojeda
Copy link
Contributor

ojeda commented Jan 11, 2022

AFAICT, no PR was sent for this. Sending one for the maybe_uninit_extra bit.

ojeda added a commit to ojeda/rust that referenced this issue Jan 11, 2022
This covers:

    impl<T> MaybeUninit<T> {
        pub unsafe fn assume_init_read(&self) -> T { ... }
        pub unsafe fn assume_init_drop(&mut self) { ... }
    }

It does not cover the const-ness of `write` under
`const_maybe_uninit_write` nor the const-ness of
`assume_init_read` (this commit adds
`const_maybe_uninit_assume_init_read` for that).

FCP: rust-lang#63567 (comment).

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 14, 2022
… r=Mark-Simulacrum

Partially stabilize `maybe_uninit_extra`

This covers:

```rust
impl<T> MaybeUninit<T> {
    pub unsafe fn assume_init_read(&self) -> T { ... }
    pub unsafe fn assume_init_drop(&mut self) { ... }
}
```

It does not cover the const-ness of `write` under `const_maybe_uninit_write` nor the const-ness of `assume_init_read` (this commit adds `const_maybe_uninit_assume_init_read` for that).

FCP: rust-lang#63567 (comment).

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 14, 2022
… r=Mark-Simulacrum

Partially stabilize `maybe_uninit_extra`

This covers:

```rust
impl<T> MaybeUninit<T> {
    pub unsafe fn assume_init_read(&self) -> T { ... }
    pub unsafe fn assume_init_drop(&mut self) { ... }
}
```

It does not cover the const-ness of `write` under `const_maybe_uninit_write` nor the const-ness of `assume_init_read` (this commit adds `const_maybe_uninit_assume_init_read` for that).

FCP: rust-lang#63567 (comment).

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 14, 2022
… r=Mark-Simulacrum

Partially stabilize `maybe_uninit_extra`

This covers:

```rust
impl<T> MaybeUninit<T> {
    pub unsafe fn assume_init_read(&self) -> T { ... }
    pub unsafe fn assume_init_drop(&mut self) { ... }
}
```

It does not cover the const-ness of `write` under `const_maybe_uninit_write` nor the const-ness of `assume_init_read` (this commit adds `const_maybe_uninit_assume_init_read` for that).

FCP: rust-lang#63567 (comment).

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@SOF3
Copy link
Contributor

SOF3 commented Feb 2, 2022

Why is it called assume_init_read? Are there similarly named concepts anywhere else (in the stdlib or the community)? This is just looks like a memcpy to me.

@RalfJung
Copy link
Member

RalfJung commented Feb 4, 2022

Yes, there are other assume_init functions on MaybeUninit. They are all just memcpys, but they are type-changing memcpys and that makes all the difference.

@Kixunil
Copy link
Contributor

Kixunil commented Feb 4, 2022

To me it's a natural combination of ptr::read() and assume_init.

@Finomnis
Copy link
Contributor

Finomnis commented Aug 28, 2023

What about the stabilization of const_maybe_uninit_write? It seems like the feature leads to this tracking issue, yet it doesn't get discussed here anywhere. Unless I'm missing something.

@RalfJung
Copy link
Member

That's blocked on stabilizing any use of &mut in const.

@DaniPopes
Copy link
Contributor

Opened #116233 to const-stabilize assume_init_read since it looks like it was only blocked by ptr::read (#92768 (comment)), which has since been stabilized in 1.71.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 13, 2023
…it_assume_init_read, r=dtolnay

Stabilize `const_maybe_uninit_assume_init_read`

AFAICT the only reason this was not included in the `maybe_uninit_extra` stabilization was because `ptr::read` was unstable (rust-lang#92768 (comment)), which has since been stabilized in 1.71.

Needs a separate FCP from the [original `maybe_uninit_extra` one](rust-lang#63567 (comment)).

Tracking issue: rust-lang#63567
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Oct 14, 2023
…_init_read, r=dtolnay

Stabilize `const_maybe_uninit_assume_init_read`

AFAICT the only reason this was not included in the `maybe_uninit_extra` stabilization was because `ptr::read` was unstable (rust-lang/rust#92768 (comment)), which has since been stabilized in 1.71.

Needs a separate FCP from the [original `maybe_uninit_extra` one](rust-lang/rust#63567 (comment)).

Tracking issue: #63567
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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. Libs-Tracked Libs issues that are tracked on the team's project board. requires-nightly This issue requires a nightly compiler in some way. 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