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

&mut [MaybeUninit<T>]: Fill with value or with closure. #156

Closed
ajwock opened this issue Dec 30, 2022 · 9 comments
Closed

&mut [MaybeUninit<T>]: Fill with value or with closure. #156

ajwock opened this issue Dec 30, 2022 · 9 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@ajwock
Copy link

ajwock commented Dec 30, 2022

Proposal

Problem statement

This feature would provide more options for making use of slices of uninitialized memory without dropping to unsafe by initializing MaybeUninit slices with closures or a repeated values, akin to slice::fill and slice::fill_with.

Motivation, use-cases

Uninitialized memory is a unique case in terms of read/write safety- it's the only case I can think of where it is safe to write to that memory but unsafe to read from it. In particular, it is safe to treat uninitialized memory as initialized once it has been initialized exhaustively. After calling any method which is guaranteed to initialize all indicies of a MaybeUninit, it is safe to assume_init on that slice.

Over the past couple of years, safe apis for doing MaybeUninit -> T such as write, write_slice, and write_slice_cloned have been introduced. write_slice corresponds to slice::copy_from_slice, while write_slice_cloned corresponds to slice::clone_from_slice. Both are guaranteed to initialize all elements of the slice.

However, there are a couple of other methods which give the same all-initialized guarantee, but there is no corresponding [MaybeUninit] api that safely returns an initialized slice: slice::fill and slice::fill_with.

Another argument for this besides simple write-only slice method API compliance is that, unlike with write_slice/write_slice_cloned, one doesn't need to safely write to initialized memory just so they can safely initialize the uninit memory. The memory can have values or closure results written directly to it without just having to hope LLVM can figure out that the safe write was unnecessary.

Solution sketches

I propose this API:

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

Pretty much the equivalent to slice::fill, but it returns a slice of T at the end.

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

Like slice::fill_with.

Final Thoughts

RFCs suggest to mention potential drawbacks to your approach. One particular drawback that I can think of is MaybeUninit != Uninit, and the implication of this is that calling these methods on a partially initialized slice of MaybeUninit can cause destructors to leak. This is also true of the entire write API that already exists, and is debated on many issues (rust:80376, libs-team:122).

My opinion is that the solution to this is an Uninit api, which contains only the subset of MaybeUninit that guarantees that you either have truly uninitialized memory or you have to give up ownership of the Uninit in exchange for a T. Such an API would include the existing write api as well as these changes.

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@ajwock ajwock added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Dec 30, 2022
@the8472
Copy link
Member

the8472 commented Dec 30, 2022

There's an RFC for in-place box initialization, I think that should take precedence. rust-lang/rfcs#2884

@ajwock
Copy link
Author

ajwock commented Dec 30, 2022

There's an RFC for in-place box initialization, I think that should take precedence. rust-lang/rfcs#2884

Oh yeah, that's pretty cool stuff.

I can eliminate the box portion from this ACP. Though I still would find it useful to be able to initialize MaybeUninit<T> slices with any of the same methods that I can exhaustively initialize slices of T with.

@ajwock ajwock changed the title &mut [MaybeUninit<T>] / Box<[MaybeUninit<T>]>: Fill with value or with closure. &mut [MaybeUninit<T>]: Fill with value or with closure. Dec 30, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Oct 17, 2023

We discussed this in the libs-api meeting. This looks fine, without the write_ prefix.

We think it'd also be useful to have a fill_from(Iterator) function, which can terminate early, to only fill part of the available buffer (and then only return that part of the slice).

Feel free to go ahead and add fill, fill_with and fill_from as unstable methods. Thanks!

(Whether fill_from should also be added to regular slices is a different discussion, that requires its own ACP.)

@m-ou-se m-ou-se closed this as completed Oct 17, 2023
@programmerjake
Copy link
Member

if fill_from is added on slices and also on slices of MaybeUninit they will probably conflict...I think this should be addressed somehow before approving fill_from

@m-ou-se
Copy link
Member

m-ou-se commented Oct 17, 2023

@programmerjake The slice methods for MaybeUninit are added on MaybeUninit, not on [MaybeUninit]. (See the signature above.)

@jmillikin
Copy link

@ajwock If you haven't already started work on the implementation for this, would it be OK with you if I did so? I could use this functionality for implementing another ACP, and the implementation seems straightforward.

@jmillikin
Copy link

Tracking issue: rust-lang/rust#117428

Implementation PR: rust-lang/rust#117426 (note: API slightly adjusted)

@dtolnay dtolnay added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Nov 23, 2023
@ajwock
Copy link
Author

ajwock commented Feb 17, 2024

@jmillikin Hey, for some reason github wasn't sending me emails about the updates here. Sorry I didn't see the messaging around this here. I have my original implementations laying around from when I made the proposal. If you're not pushing your implementation PR forward still, would you mind if I submit a PR with mine?

@jmillikin
Copy link

jmillikin commented Feb 18, 2024

@ajwock Feel free! I don't have the time or interest to push PRs past upstream resistance right now, so it's all yours.

You're also welcome to reuse any part of my PR in case that would be helpful to you.

ajwock added a commit to ajwock/rust that referenced this issue Feb 18, 2024
ajwock added a commit to ajwock/rust that referenced this issue Feb 18, 2024
ajwock added a commit to ajwock/rust that referenced this issue Feb 19, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 5, 2024
ajwock added a commit to ajwock/rust that referenced this issue Mar 5, 2024
ajwock added a commit to ajwock/rust that referenced this issue Mar 5, 2024
ajwock added a commit to ajwock/rust that referenced this issue Mar 5, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Mar 7, 2024
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 9, 2024
Nadrieril added a commit to Nadrieril/rust that referenced this issue Mar 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 9, 2024
Rollup merge of rust-lang#121280 - ajwock:maybeuninit_fill, r=Amanieu

Implement MaybeUninit::fill{,_with,_from}

ACP: rust-lang/libs-team#156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

6 participants