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_ref)]` #63568

Open
Centril opened this issue Aug 14, 2019 · 15 comments

Comments

@Centril
Copy link
Member

@Centril 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 get_ref(&self) -> &T { ... }
    pub unsafe fn get_mut(&mut self) -> &mut T { ... }
}

TODO: Update the tracking issue in the code.

@RalfJung

This comment has been minimized.

Copy link
Member

@RalfJung RalfJung commented Aug 14, 2019

The reason these are unstable is that most of the time, people should use raw pointers for as long as possible. So the question remains whether these methods carry their weight.

Of course, without rust-lang/rfcs#2582 it can be hard to avoid creating references.

Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
…fJung

Adjust tracking issues for `MaybeUninit<T>` gates

cc rust-lang#63566  rust-lang#63567 rust-lang#63568 rust-lang#63569

r? @RalfJung
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
…fJung

Adjust tracking issues for `MaybeUninit<T>` gates

cc rust-lang#63566  rust-lang#63567 rust-lang#63568 rust-lang#63569

r? @RalfJung
@danielhenrymantilla

This comment has been minimized.

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Aug 14, 2019

I'll repost here something that I posted in the old tracker issue, for the sake of visibility, and because it is something that remains, imho, relevant to the topic at hand:

Unstable get_ref / get_mut are definitely adviseable because the validity of references to uninitialized data (specially integer and floating point data) hasn't yet been settled. However, there are cases where get_ref / get_mut may be used when the MaybeUninit has been init: to get a safe handle to the (now known initialised) data while avoiding any memcpy (instead of assume_init, which may trigger a memcpy).

  • this may seem like a particularly specific situation, but the main reason people (may want to) use uninitialised data is precisely for this kind of cheap savings.

Because of this, I imagine assume_init_by_ref / assume_init_by_mut could be nice to have (since into_inner has been called assume_init, it seems plausible that the ref / ref mut getters also get a special name to reflect this).

There are two / three options for this, related to the Drop interaction:

  1. Exact same API as get_ref and get_mut, which can lead to memory leaks when there is drop glue;

    • (Variant): same API as get_ref / get_mut, but with a Copy bound;
  2. Closure style API, to guarantee drop:

impl<T> MaybeUninit<T> {
    /// # Safety
    ///
    ///   - the contents must have been initialised
    unsafe
    fn assume_init_with_mut<R, F> (mut self: MaybeUninit<T>, f: F) -> R
    where
        F : FnOnce(&mut T) -> R,
    {
        if mem::needs_drop::<T>().not() {
            return f(unsafe { self.get_mut() });
        }
        let mut this = ::scopeguard::guard(self, |mut this| {
            ptr::drop_in_place(this.as_mut_ptr());
        });
        f(unsafe { MaybeUninit::<T>::get_mut(&mut *this) })
    }
}

(Where scopeguard's logic can easily be reimplemented, so there is no need to depend on it)


These could be stabilized faster than get_ref / get_mut, given the explicit assume_init requirement.

Drawbacks

If a variant of option .1 was chosen, and get_ref / get_mut were to become usable without the assume_init situation, then this API would become almost strictly inferior (I say almost because with the proposed API, reading from the reference would be okay, which it can never be in the case of get_ref and get_mut)

@SimonSapin

This comment has been minimized.

Copy link
Contributor

@SimonSapin SimonSapin commented Aug 16, 2019

I agree that these methods are useful after the value has been fully initialized, at which points references are valid not matter which way future language decisions go. However I think that dropping the value should be an entirely separate concern, and should not be conflated in the same APIs as obtaining a reference.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

@SimonSapin SimonSapin commented Oct 18, 2019

Based on my previous comment, let’s?

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

@rfcbot rfcbot commented Oct 18, 2019

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

Concerns:

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.

@danielhenrymantilla

This comment has been minimized.

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Oct 18, 2019

So, despite .get_ref / .get_mut being currently unsound when applied on an uninit value, we want to stabilize them with that name? My comment maybe went too far thinking about drops but my point about them being more aptly named as assume_init_by_ref / ..._by_mut still stands. This is an easy but important detail to fix before stabilization: the whole point of MaybeUninit is too avoid asserting "init-ness" too early, and I feel that there will be more misuages with .get_ref/mut methods than with the assume_init_by_ref/mut counterparts.

@Centril

This comment has been minimized.

Copy link
Member Author

@Centril Centril commented Oct 19, 2019

Naming

I basically agree with @RalfJung's point in #63567 (comment) and I feel the answer to the question in #63567 (comment) is "yes". It seems appropriate to reflect the assume_init nature of these methods in their names -- after all, there was a reason we named the method .assume_init().

I would propose that we rename these to:

  • assume_init_ref
  • assume_init_ref_mut (variants: assume_init_mut_ref and assume_init_mut -- I'm not sure exactly what the API convention was...)

(I've tried to pick names that are clear but also not super long at the same time.)

Documentation

Separately, I think fn get_ref/get_mut could use examples in the actual documentation (https://doc.rust-lang.org/nightly/std/mem/union.MaybeUninit.html#method.get_ref and https://doc.rust-lang.org/nightly/std/mem/union.MaybeUninit.html#method.get_mut).

Two aspects should I think be clarified about these methods (with examples):

  • You cannot, at least for now, use this to do field-by-field gradual initialization.
  • It's not OK to use this to get a slice to pass to Read::read.

These are both examples of "assuming it is initialized before it is" but examples are helpful.
I think it is also a good idea to add positive examples of where this would be used.

(I think this should happen before we stabilize as it tends not to happen for some time otherwise. It is also the quality we have for other methods in the MaybeUninit<T> docs.)

@SimonSapin

This comment has been minimized.

Copy link
Contributor

@SimonSapin SimonSapin commented Oct 19, 2019

Both good points, let’s formally file them:

@rfcbot concern naming
@rfcbot concern docs

@RalfJung

This comment has been minimized.

Copy link
Member

@RalfJung RalfJung commented Oct 19, 2019

After thinking about this a bit, I think I agree with stabilization (subject to the concerns raised by @Centril). In particular when it comes to docs, the big advantage of having these methods compared to letting people do &mut *m.as_mut_ptr() is that there are docs to look at (someone probably raised this above). So we should use these methods as a teaching opportunity for what the rules are around references and uninitialized data. In particular, we should clarify that all the UB rules for these methods equally apply when calling the raw ptr methods and creating a reference manually.

@danielhenrymantilla

This comment has been minimized.

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Oct 19, 2019

(And also .get_ref/mut references' lifetimes are tied to the borrow on the MaybeUninit, which is not the case with &[mut]* _.as[_mut]_ptr())

I can be the one drafting the docs if you want

@SimonSapin

This comment has been minimized.

Copy link
Contributor

@SimonSapin SimonSapin commented Oct 21, 2019

@danielhenrymantilla Please do! And submit a PR to get feedback on the wording.

@danielhenrymantilla

This comment has been minimized.

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Oct 30, 2019

Done (#65948), waiting on review: should I/we also update .as_ptr() and .as_mut_ptr() documentations ?

@scottmcm

This comment has been minimized.

Copy link
Member

@scottmcm scottmcm commented Oct 31, 2019

@rfcbot reviewed

I like the functionality, and I'm happy with whatever names people agree on for things here.

@RalfJung

This comment has been minimized.

Copy link
Member

@RalfJung RalfJung commented Nov 3, 2019

should I/we also update .as_ptr() and .as_mut_ptr() documentations ?
@scottmcm

What are you proposing to update?

@RalfJung

This comment has been minimized.

Copy link
Member

@RalfJung RalfJung commented Nov 3, 2019

In terms of naming, @Centril's assume_init_ref seems fine; I think the parallel name for mutable references then would be assume_init_mut.

pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 5, 2019
…t_ref_mut, r=RalfJung

Improve MaybeUninit::get_{ref,mut} documentation

As mentioned in rust-lang#63568 (comment), `MaybeUninit`'s `get_{ref,mut}` documentation is lacking, so this PR attempts to fix that.

That being said, and as @RalfJung mentions in that thread,

> In particular, we should clarify that all the UB rules for these methods equally apply when calling the raw ptr methods and creating a reference manually.

these other docs also need to be improved, which I can do in this PR ~~(hence the `[WIP]`)~~.

Finally, since all these documentations are related to clearly establishing when dealing with uninitialized memory which patterns are known to be sound and which patterns are currently UB (that is, until, if ever, the rules around references to unintialized integers get relaxed, this documentation will treat them as UB, and advise against such patterns (_e.g._, it is not possible to use uninitialized buffers with the `Read` API)), I think that adding even more examples to the main documentation of `MaybeUninit` inherent definition wouldn't hurt either.

___

  - [Rendered](http://dreamy-ritchie-99d637.netlify.com/core/mem/union.maybeuninit#method.get_ref)
danielhenrymantilla added a commit to danielhenrymantilla/rust that referenced this issue Nov 6, 2019
This is the last remaining concern blocking the stabilization of the by-ref accessors of `MaybeUninit` (rust-lang#63568).
This change of name not only does align with `.into_inner()` being called `.assume_init()`,
it also conveys the dangerous semantics of the method in a much clearer and more direct way,
reducing the change of misuse and thus of unsoundness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.