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 Vec::spare_capacity_mut #75017

Closed
2 tasks
Amanieu opened this issue Aug 1, 2020 · 15 comments · Fixed by #93016
Closed
2 tasks

Tracking Issue for Vec::spare_capacity_mut #75017

Amanieu opened this issue Aug 1, 2020 · 15 comments · Fixed by #93016
Labels
A-collections Area: std::collections. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Amanieu
Copy link
Member

Amanieu commented Aug 1, 2020

The feature gate for the issue is #![feature(vec_spare_capacity)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

@Amanieu Amanieu added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Aug 1, 2020
@jonas-schievink jonas-schievink added B-unstable Feature: Implemented in the nightly compiler and unstable. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-collections Area: std::collections. labels Aug 1, 2020
@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 Aug 6, 2020
@danielhenrymantilla
Copy link
Contributor

FWIW, here is a full-featured API provided by an external crate: https://docs.rs/uninit/0.3.0/uninit/extension_traits/trait.VecCapacity.html#foreign-impls

@djc
Copy link
Contributor

djc commented Jan 20, 2021

I wonder how this API relates to ReadBuf. It would be great if there was some safe API that wrapped around the use of ReadBuf to access a Vec's spare capacity, then updated len based on the ReadBufs filled.

@the8472
Copy link
Member

the8472 commented Jan 31, 2021

FWIW, here is a full-featured API provided by an external crate: https://docs.rs/uninit/0.3.0/uninit/extension_traits/trait.VecCapacity.html#foreign-impls

Maybe we should follow that example and generalize spare_capacity_mut() to return (&mut [T], &mut [MaybeUninit<T>])?

@Shnatsel
Copy link
Member

Shnatsel commented Feb 2, 2021

Maybe we should follow that example and generalize spare_capacity_mut() to return (&mut [T], &mut [MaybeUninit<T>])?

#79015 has implemented that, albeit as a private function. Sounds like a good idea to me.

@the8472
Copy link
Member

the8472 commented Feb 9, 2021

Since #81944 exists now I think vec_spare_capacity is redundant.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Feb 10, 2021
Make Vec::split_at_spare_mut public

This PR introduces a new method to the public API, under
`vec_split_at_spare` feature gate:

```rust
impl<T, A: Allocator> impl Vec<T, A> {
    pub fn split_at_spare_mut(&mut self) -> (&mut [T], &mut [MaybeUninit<T>]);
}
```

The method returns 2 slices, one slice references the content of the vector,
and the other references the remaining spare capacity.

The method was previously implemented while adding `Vec::extend_from_within` in rust-lang#79015,
and used to implement `Vec::spare_capacity_mut` (as the later is just a
subset of former one).

See also previous [discussion in `Vec::spare_capacity_mut` tracking issue](rust-lang#75017 (comment)).

## Unresolved questions

- [ ] Should we consider changing the name? `split_at_spare_mut` doesn't seem like an intuitive name
- [ ] Should we deprecate `Vec::spare_capacity_mut`? Any usecase of `Vec::spare_capacity_mut` can be replaced with `Vec::split_at_spare_mut` (but not vise-versa)

r? `@KodrAus`
@WaffleLapkin
Copy link
Member

@the8472

Since #81944 exists now I think vec_spare_capacity is redundant.

That's not actually true due to pointer invalidation, see #81944 (comment) and #82564

@saethlin
Copy link
Member

saethlin commented Jan 4, 2022

I think it would be better to have this feature sooner rather than later. I'm starting to notice instances where people are doing things that are probably or definitely UB because they want this functionality. So I think this API falls squarely into the standard library's mission to provide pre-built and correct implementations of things that are subtle or difficult to get right.

rayon

    /// Reserve space for `len` more elements in the vector,
    /// and return a slice to the uninitialized tail of the vector
    fn reserve_get_tail_slice(vec: &mut Vec<T>, len: usize) -> &mut [MaybeUninit<T>] {
        // Reserve the new space.
        vec.reserve(len);

        // TODO: use `Vec::spare_capacity_mut` instead
        // SAFETY: `MaybeUninit<T>` is guaranteed to have the same layout
        // as `T`, and we already made sure to have the additional space.
        let start = vec.len();
        let tail_ptr = vec[start..].as_mut_ptr() as *mut MaybeUninit<T>;
        unsafe { slice::from_raw_parts_mut(tail_ptr, len) }
    }

protobuf

/// Slice from `vec[vec.len()..vec.capacity()]`
pub unsafe fn remaining_capacity_as_slice_mut<A>(vec: &mut Vec<A>) -> &mut [A] {
    let range = vec.len()..vec.capacity();
    vec.get_unchecked_mut(range)
}

(originally posted on #81944 but per Ralf's comments there, it is probably better to have this method for the above use cases)

@Amanieu
Copy link
Member Author

Amanieu commented Jan 4, 2022

I agree. I've been using this in my code and found it to be very useful. Even though split_at_spare_mut is more general, I've never had a need for it and I feel that spare_capacity_mut is useful on its own.

The API being stabilized is:

impl Vec<T> {
    pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>];
}

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jan 4, 2022

Team member @Amanieu 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 Jan 4, 2022
@danielhenrymantilla
Copy link
Contributor

@saethlin while waiting for this feature to be stabilized, you should suggest that rayon and protobuf use one of the convenience methods in: https://docs.rs/uninit/0.4.0/uninit/extension_traits/trait.VecCapacity.html#method.split_at_extra_cap

@rfcbot rfcbot added 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 Jan 4, 2022
@rfcbot
Copy link

rfcbot commented Jan 4, 2022

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

@saethlin
Copy link
Member

saethlin commented Jan 4, 2022

@danielhenrymantilla Ralf mentioned this on #81944, mostly rewording with my own opinion: I would prefer not to recommend that function, because even if you just want access to the spare region, under SB, these interfaces invalidate all pointers to the initialized region. I think that's too much of a footgun for codebases where people are already writing code that is rejected by Miri.

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Jan 4, 2022

I wonder how many people do keep outstanding pointers to the initialized part while toying with the extra part 🤔 ::uninit's implementation is already better than their 0-elems slices that get unsafe-ly converted into non-empty slices 😱 (and currently better than the stdlib one since it doesn't involve a 12-week delay for stable Rust).

Anyhow, I've just released a new version (0.5.0) with a dedicated .spare_cap() adapter: https://docs.rs/uninit/0.5.0-rc1/uninit/extension_traits/trait.VecCapacity.html#method.spare_cap, for extra good measure.

@saethlin
Copy link
Member

saethlin commented Jan 4, 2022

I wonder how many people do keep outstanding pointers to the initialized part while toying with the extra part

To be clear, my concern is that the splitting interfaces are adding a landmine for the future. If I'm sending someone a PR I'll make sure it passes Miri at the time, but that's a hard condition to maintain. Especially because the two aforementioned codebases have dependencies that do int-ptr casts, an operation that Miri just doesn't understand at the moment, so it would be hard to advocate Miri in CI.

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

rfcbot commented Jan 14, 2022

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 18, 2022
@bors bors closed this as completed in e012b9a Jan 18, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: std::collections. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.