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 maybe_uninit_fill #117428

Open
3 tasks
jmillikin opened this issue Oct 31, 2023 · 14 comments
Open
3 tasks

Tracking Issue for maybe_uninit_fill #117428

jmillikin opened this issue Oct 31, 2023 · 14 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@jmillikin
Copy link
Contributor

jmillikin commented Oct 31, 2023

Feature gate: #![feature(maybe_uninit_fill)]

This is a tracking issue for ACP rust-lang/libs-team#156

Public API

// core::mem

impl<T> MaybeUninit<T> {
    pub fn fill<'a>(this: &'a mut [MaybeUninit<T>], value: T) -> &'a mut [T]
        where T: Clone

    pub fn fill_with<'a, F>(this: &'a mut [MaybeUninit<T>], f: F) -> &'a mut [T]
        where F: FnMut() -> T;

    pub fn fill_from<'a, I>(this: &'a mut [MaybeUninit<T>], iter: I) -> (&'a mut [T], &'a mut [MaybeUninit<T>])
        where I: Iterator<Item = T>;
}

Steps / History

Unresolved Questions

  • None yet.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@jmillikin jmillikin added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 31, 2023
@python-ast-person
Copy link

Why do the functions return a shared reference instead of a mutable reference?

@jmillikin
Copy link
Contributor Author

Why do the functions return a shared reference instead of a mutable reference?

#80376

If a &mut [T] is returned for T: Clone, there's no indication to downstream code that writing values to that slice will cause them to leak.

@python-ast-person
Copy link

Are there plans to add mutable variants for the functions then?

@jmillikin
Copy link
Contributor Author

jmillikin commented Nov 15, 2023

I'm not planning to implement a <T: Clone>(&mut [MaybeUninit<T>], T) -> &mut [T] function as part of this feature, though I think it would be fine for someone else to do so if they have a view on what the correct Drop semantics should be.

Alternatively, maybe a -> &mut [ManuallyDrop<T>] would work for your use case? That would be safe to add since it makes the special-ness clear.

@python-ast-person
Copy link

My use-case would an API along the lines of fn make_box<T:?Sized>(filler:impl FnOnce(&mut MaybeUninit<T>)->&mut T,size:<T as Pointee>::Metadata)->Box<T>, except with some traits to work around the lack of unsized MaybeUninit and a stable Pointee trait yet.

@jmillikin
Copy link
Contributor Author

jmillikin commented Nov 24, 2023

@python-ast-person If I understand you correctly, then you'd use the proposed fill_cloned to implement in-place construction of a Box<[T]> like this (playground link):

// presumed to exist as part of unsized MaybeUninit
impl<T> MaybeUninit<T> {
    fn transpose_slice<'a>(this: &'a mut MaybeUninit<[T]>) -> &'a mut [MaybeUninit<T>] {
        unimplemented!()
    }
}

fn make_box<T: ?Sized>(
    size: <T as Pointee>::Metadata,
    filler: impl FnOnce(&mut MaybeUninit<T>) -> &mut T,
) -> Box<T> {
    unimplemented!()
}

fn main() {
    let boxed = make_box::<[String]>(10, |u: &mut MaybeUninit<[String]>| {
        let a = String::from("a");
        let transposed = MaybeUninit::transpose_slice(u);
        MaybeUninit::fill_cloned(transposed, &a);
        unsafe { MaybeUninit::slice_assume_init_mut(transposed) }
    });
    println!("boxed: {boxed:?}");
}

Having MaybeUninit::fill_cloned() return a &mut [T] would provide only slightly cleaner code (one less line).

If you want construction of Box<[T]> to be possible without any unsafe at all, then you might want a helper function that takes ownership of its input, which should be Drop-safe. This can't be part of MaybeUninit directly because Box isn't available in the core crate, but it should be straightforward to implement if you already have unsized MaybeUninit in your codebase.

// internally does fill_cloned + slice_assume_init_mut() as above
fn fill_box_cloned<T: Clone>(this: Box<MaybeUninit<[T]>>,  value: &T) -> Box<[T]>;

@ajwock
Copy link
Contributor

ajwock commented Feb 17, 2024

Hey, a couple of opinions on the API here.

One, I suggest that fill_from returns -> (&'a mut [T], &'a mut [MaybeUninit]) where the second slice is the uninitialized remainder. This produces a more useful API while harmless to anyone who doesn't want that remainder- they can just throw it away.

Another thing: I disagree that having fill_cloned should return an immutable reference. The &mut [T] returned from fill_cloned will already cause the destructors to be leaked regardless of mutability, or regardless of whether new values are written into the mutable slice. I don't know of any other method in std which takes a mutable reference and returns an immutable reference to the safe object and, in my opinion, should only be used where necessary to prevent true safety issues.

A box version was in my original proposal. I removed it due to a suggestion referring to rust-lang/rfcs#2884. Perhaps these two things should coexist, however.

@ajwock
Copy link
Contributor

ajwock commented Feb 18, 2024

jmillikin has allowed me to take over implementation: #121280

@ajwock
Copy link
Contributor

ajwock commented Feb 20, 2024

rust-lang/libs-team#338

This idea is my response to the (legitimate) criticism of these APIs that they could lead to destructor leaks.

@cynecx
Copy link
Contributor

cynecx commented Feb 23, 2024

It would've been nice to also have the "same" interface for boxed slices:

impl<T, A: Allocator> Box<[MaybeUninit<T>], A> {
    pub fn fill(boxed: Box<[MaybeUninit<T>], A>, value: T) -> Box<[T], A>
    where
        T: Clone;
    
    // EDIT: Not needed because it is covered by above.
    // pub fn fill_cloned(boxed: Box<[MaybeUninit<T>], A>, value: &T) -> Box<[T], A>
    // where
    //     T: Clone;

    pub fn fill_with<F>(boxed: Box<[MaybeUninit<T>], A>, f: F) -> Box<[T], A>
    where
        F: FnMut() -> T;

    pub fn fill_from<I>(boxed: Box<[MaybeUninit<T>], A>, iter: I) -> Box<[T], A>
    where
        I: Iterator<Item = T>;
}

Does this need a new ACP or would it be possible to also consider this under maybe_uninit_fill?

@cynecx
Copy link
Contributor

cynecx commented Feb 23, 2024

It might be useful for fill_with's F to accept an index:

pub fn fill_with<'a, F>(this: &'a mut [MaybeUninit<T>], f: F) -> &'a [T]
where
    F: FnMut(usize) -> T; // <-- accepts an index (current element that is being written to)

Alternatively, you could just store and mutate a counter inside the passed in closure, but this might be more convenient...

@ajwock
Copy link
Contributor

ajwock commented Feb 27, 2024

Having the same interface for Box needs a new ACP. I agree that it would be useful in some cirucumstances. Also, if you are going to make such an ACP, note that fill_cloned is not necessary due to specialization. See the implementation of the slice’s fill.

re: the function accepting an index. This is a different API from the slice’s fill_with method. Ideally MaybeUninit slice APIs mirror the slice APIs, especially if occupying the same name. Loading your closure with index state is trivial, and any use case that doesn’t want the index will have to throw it away.

@cynecx
Copy link
Contributor

cynecx commented Feb 27, 2024

Also, if you are going to make such an ACP, note that fill_cloned is not necessary due to specialization. See the implementation of the slice’s fill.

Hmm, those are quite "different" APIs. Slice's fill and co doesn't allow a fully safe way to initialize a boxed slice.

impl<T, A: Allocator> Box<[MaybeUninit<T>], A> {
    pub fn fill_cloned(boxed: Box<[MaybeUninit<T>], A>, value: &T) -> Box<[T], A>
    where
        T: Clone;
}

// vs

impl<T> [T] {
    pub fn fill(&mut self, value: T)
    where
        T: Clone,
}

Not sure if that's the one you mentioned, but I've looked through several places and I can't seem to find a similar api:

EDIT (To reduce additional noise here):

I think the confusion here is simply the outdated OP issue description. I was under the impression that the API design written above was the most recent one but as ajwock mentioned below, the preference is to use specialization to avoid having both fill and fill_cloned which makes total sense. It would be great if someone could update the issue description though, just to help future readers :)

@ajwock
Copy link
Contributor

ajwock commented Feb 27, 2024

What I mean is that you don’t need a separate function for Clone and Copy fills. Specialization can be used to optimize the method appropriately based on whether it is Copy. This is true regardless of initializing a slice or boxed slice.

Thus, users don’t need to make an effort to call the copy or clone version- it’d be the same API for both.

Edit: to add context, my understanding of why we have slice::copy_from_slice and slice::clone_from_slice (and thus: MaybeUninit::{copy,clone}_from_slice) rather than just the latter is that specialization became useable and acceptable for std usage after slice::{clone,copy}_from_slice was stabilized. Meaning those APIs couldn’t be merged into one anymore. Wheras, slice::fill is specialized and does not have a fill_cloned variant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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

4 participants