vec: add try_* methods and a try_vec! macro to make Vec usable in without infallible allocation methods #95051

dpaoliello · 2022-03-17T19:44:03Z

As a part of the work for #86942 the no_global_oom_handling cfg was added in #84266, however enabling that cfg makes it difficult to use Vec: the vec! macro is missing and there is no way to insert or push new elements, nor to create a Vec from an iterator (without resorting to unsafe APIs to manually expand and write to the underlying buffer).

This change adds try_ equivalent methods for all methods in Vec that are disabled by no_global_oom_handling as well as a try_vec! as the equivalent of vec!.

Performance and implementation notes: A goal of this change was to make sure to NOT regress the performance of the existing infallible methods - a naive approach would be to move the actual implementation into the fallible method, then call it from the infallible method and unwrap(), but this would add extra compare+branch instructions into the infallible methods. It would also be possible to simply duplicate the code for the fallible version - I did opt for this in a few cases, but where the code was larger and more complex this would lead to a maintenance nightmare. Instead, I moved the implementation into an *_impl method that was then specialized based on a new VecError trait and returned Result<_, VecError>, this trait also provided a pair of methods for raising errors. Never (!) was used as the infallible version of this trait (and always panics) and TryReserveError as the fallible version (and returns the correct error object). All these VecError method were marked with #[inline], so after inlining the compiler could see for the infallible version always returns Ok() on success (and panics on error) thus the ? operator or unwrap() call was a no-op, and so the same non-branching instructions as before are generated.

I also added try_from_iter and try_expand methods for completeness, even though their infallible equivalents are trait implementations.

List of Vec APIs added:

try_vec!

impl<T> Vec<T> {
    pub fn try_with_capacity(capacity: usize) -> Result<Self, TryReserveError>;
    pub fn try_from_iter<I: IntoIterator<Item = T>>(iter: I) -> Result<Vec<T>, TryReserveError>;
}

impl<T, A: Allocator> Vec<T, A> {
    pub fn try_append(&mut self, other: &mut Self) -> Result<(), TryReserveError>;
    pub fn try_extend<I: IntoIterator<Item = T>>(&mut self, iter: I, ) -> Result<(), TryReserveError>;
    pub fn try_extend_from_slice(&mut self, other: &[T]) -> Result<(), TryReserveError>;
    pub fn try_extend_from_within<R>(&mut self, src: R) -> Result<(), TryReserveError> where R: RangeBounds<usize>; // NOTE: still panics if given an invalid range
    pub fn try_insert(&mut self, index: usize, element: T) -> Result<(), TryReserveError>; // NOTE: still panics if given an invalid index
    pub fn try_into_boxed_slice(self) -> Result<Box<[T], A>, TryReserveError>;
    pub fn try_push(&mut self, value: T) -> Result<(), TryReserveError>;
    pub fn try_resize(&mut self, new_len: usize, value: T) -> Result<(), TryReserveError>;
    pub fn try_resize_with<F>(&mut self, new_len: usize, f: F) -> Result<(), TryReserveError> where F: FnMut() -> T;
    pub fn try_shrink_to(&mut self, min_capacity: usize) -> Result<(), TryReserveError>;
    pub fn try_shrink_to_fit(&mut self) -> Result<(), TryReserveError>;
    pub fn try_split_off(&mut self, at: usize) -> Result<Self, TryReserveError> where A: Clone; // NOTE: still panics if given an invalid index
    pub fn try_with_capacity_in(capacity: usize, alloc: A) -> Result<Self, TryReserveError>;
}

#[doc(hidden)]
pub fn try_from_elem<T: Clone>(elem: T, n: usize) -> Result<Vec<T>, TryReserveError>;

#[doc(hidden)]
pub fn try_from_elem_in<T: Clone, A: Allocator>(elem: T, n: usize, alloc: A) -> Result<Vec<T, A>, TryReserveError>;

Other helper APIs added, than can be made pub(crate) if folks prefer:

impl TryReserveError {
    pub fn alloc_error(layout: Layout) -> Self;
}

impl<T> Result<T, !> {
    pub fn unwrap_infallible(self) -> T;
}

rust-highfive · 2022-03-17T19:44:06Z

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

library/alloc/src/macros.rs

dpaoliello · 2022-03-17T19:48:42Z

library/alloc/src/vec/mod.rs

+    /// # Ok::<(), alloc::collections::TryReserveError>(())
+    /// ```
+    #[inline]
+    #[unstable(feature = "more_fallible_allocation_methods", issue = "86942")]


What's the policy for cases where an API depends on two unstable features (e.g., in this case more_fallible_allocation_methods and allocator_api)?

Not sure! I've wondered that myself :).

dpaoliello · 2022-03-17T19:52:51Z

FYI @Ericson2314 @Xuanwo

Ericson2314 · 2022-03-17T21:35:46Z

Great job trying to tackle the dedup-without-penality problem!

An additional technical I would like to see tried is returning Result<Self, !> in the infallible case. Right now, life is easy because we are just doing flat containers. But I originally also did infallible linked lists and other more complicated things, where they are multiple possible failure points.

I think the Result<Self, !> will scale better, allowing dedup in that case too. The idea would be to have Result<Self, Operations::Error>, not the associated error type. And the most code just does try! everwhere, but in the Error = ! case, that try becomes a no-op.

This might seem overkill on this PR, but if folks are willing to give it a try I think it's better to do so sooner rather than later.

dpaoliello · 2022-03-17T23:21:41Z

Great job trying to tackle the dedup-without-penality problem!

An additional technical I would like to see tried is returning Result<Self, !> in the infallible case. Right now, life is easy because we are just doing flat containers. But I originally also did infallible linked lists and other more complicated things, where they are multiple possible failure points.

I think the Result<Self, !> will scale better, allowing dedup in that case too. The idea would be to have Result<Self, Operations::Error>, not the associated error type. And the most code just does try! everwhere, but in the Error = ! case, that try becomes a no-op.

This might seem overkill on this PR, but if folks are willing to give it a try I think it's better to do so sooner rather than later.

I was worried that using ! as the error type would run into some of the issues noted in #35121 (I don't fully understand what exactly those issues are, or how to detect them).

I've tried switching the code to use ! locally and it seems to be working - if you think it is a good idea, then I'm happy to push it to this change.

Ericson2314 · 2022-03-18T02:14:11Z

@dpaoliello I think the issues in that thread are mainly worrying about arbitrary code. This is a mere implementation detail in alloc, and so we don't need to be so nervous.

Maybe keep a branch for the old version in case someone disagrees with me, but please do push! I would be very curious to see what you've got :).

dpaoliello · 2022-03-18T16:57:25Z

@dpaoliello I think the issues in that thread are mainly worrying about arbitrary code. This is a mere implementation detail in alloc, and so we don't need to be so nervous.

Maybe keep a branch for the old version in case someone disagrees with me, but please do push! I would be very curious to see what you've got :).

Ok, pushed! It's a trivial change, unless I've misunderstood what you were suggesting...

library/alloc/src/vec/mod.rs

library/alloc/src/vec/source_iter_marker.rs

library/alloc/src/vec/mod.rs

Ericson2314 · 2022-03-21T19:45:09Z

This is looking really nice! Really wonderful to see more red appear over time to balance out that green :).

bors · 2022-03-24T04:46:11Z

☔ The latest upstream changes (presumably #87667) made this pull request unmergeable. Please resolve the merge conflicts.

dpaoliello · 2022-05-23T22:35:22Z

@Ericson2314 @ojeda @m-ou-se Ok, RFC created: rust-lang/rfcs#3271

I tried to think of as many alternatives as possible. I also wrote down the starting of an idea to deal with "function variations" in the Future Possibilities section, but it would require a lot more language design and discussion.

dpaoliello · 2022-06-03T17:37:27Z

@m-ou-se I'd like to be able to get these functions merged in to unblock folks that need them: if you're worried about cluttering Vec, then I'm more than happy to mark them as #[doc(hidden)] for now.

the8472 · 2022-06-28T17:15:24Z

Also, the try_ prefix often doesn't refer to allocation failures. I've heard ideas about a Vec::try_push that only pushes if there's capacity left (without ever trying to reallocate), for example.

In #89123 I chose a name for that function that doesn't start with try_ to avoid a name collision since that prefix already is used for fallible allocation in try_reserve.

i509VCB · 2022-06-29T20:23:42Z

For the try_ functions which add some T or an iterator of T to the Vec, do we need a way to get the value(s) back if allocation fails?

Using the OS kernel example, if pushing to an vec fails in kernel, you may need to take a moment to clean up some garbage/kill some processes and then try again.

Ericson2314 · 2022-06-29T22:52:21Z

@i509VCB I think the right way to do that is a more "emplace"-type mechanism by which

One ensures the capacity is large enough, and on success gets back a thing like a lock guard
One uses the thing like a lock guard to push the item without possibility of failure.

We can do this either way, but the benefits are a lot easier to understand with Result returning (i.e., this RFC) because the types (`Result needed for step 1 but not step 2) make explicit what is going on.

The `vec!` macro has 3 rules, but two are not usable under `no_global_oom_handling` builds of the standard library (even with a zero size): ```rust let _ = vec![42]; // Error: requires `exchange_malloc` lang_item. let _ = vec![42; 0]; // Error: cannot find function `from_elem`. ``` Thus those two rules should not be available to begin with. The remaining one, with an empty matcher, is just a shorthand for `new()` and may not make as much sense to have alone, since the idea behind `vec!` is to enable `Vec`s to be defined with the same syntax as array expressions. Furthermore, the documentation can be confusing since it shows the other rules. Thus perhaps it is better and simpler to disable `vec!` entirely under `no_global_oom_handling` environments, and let users call `new()` instead: ```rust let _: Vec<i32> = vec![]; let _: Vec<i32> = Vec::new(); ``` Notwithstanding this, a `try_vec!` macro would be useful, such as the one introduced in rust-lang/rust#95051. If the shorthand for `new()` is deemed worth keeping on its own, then it may be interesting to have a separate `vec!` macro with a single rule and different, simpler documentation. Signed-off-by: Miguel Ojeda <[email protected]>

…Simulacrum `alloc`: make `vec!` unavailable under `no_global_oom_handling` `alloc`: make `vec!` unavailable under `no_global_oom_handling` The `vec!` macro has 3 rules, but two are not usable under `no_global_oom_handling` builds of the standard library (even with a zero size): ```rust let _ = vec![42]; // Error: requires `exchange_malloc` lang_item. let _ = vec![42; 0]; // Error: cannot find function `from_elem`. ``` Thus those two rules should not be available to begin with. The remaining one, with an empty matcher, is just a shorthand for `new()` and may not make as much sense to have alone, since the idea behind `vec!` is to enable `Vec`s to be defined with the same syntax as array expressions. Furthermore, the documentation can be confusing since it shows the other rules. Thus perhaps it is better and simpler to disable `vec!` entirely under `no_global_oom_handling` environments, and let users call `new()` instead: ```rust let _: Vec<i32> = vec![]; let _: Vec<i32> = Vec::new(); ``` Notwithstanding this, a `try_vec!` macro would be useful, such as the one introduced in rust-lang/rust#95051. If the shorthand for `new()` is deemed worth keeping on its own, then it may be interesting to have a separate `vec!` macro with a single rule and different, simpler documentation. Signed-off-by: Miguel Ojeda <[email protected]>

JohnCSimon · 2022-10-08T23:03:09Z

ping from triage:
@dpaoliello
Returning to author to address comments + merge conflicts

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@rustbot author

…hout infallible allocation methods As a part of the work for rust-lang#86942 the `no_global_oom_handling` cfg was added in rust-lang#84266, however enabling that cfg makes it difficult to use `Vec`: the `vec!` macro is missing and there is no way to insert or push new elements, nor to create a `Vec` from an iterator (without resorting to `unsafe` APIs to manually expand and write to the underlying buffer). This change adds `try_` equivalent methods for all methods in `Vec` that are disabled by `no_global_oom_handling` as well as a `try_vec!` as the equivalent of `vec!`. Performance and implementation notes: A goal of this change was to make sure to NOT regress the performance of the existing infallible methods - a naive approach would be to move the actual implementation into the fallible method, then call it from the infallible method and `unwrap()`, but this would add extra compare+branch instructions into the infallible methods. It would also be possible to simply duplicate the code for the fallible version - I did opt for this in a few cases, but where the code was larger and more complex this would lead to a maintenance nightmare. Instead, I moved the implementation into an `*_impl` method that was then specialized based on a new `VecError` trait and returned `Result<_, VecError>`, this trait also provided a pair of methods for raising errors. Never (`!`) was used as the infallible version of this trait (and always panics) and `TryReserveError` as the fallible version (and returns the correct error object). All these `VecError` method were marked with `#[inline]`, so after inlining the compiler could see for the infallible version always returns `Ok()` on success (and panics on error) thus the `?` operator or `unwrap()` call was a no-op, and so the same non-branching instructions as before are generated. I also added `try_from_iter` and `try_expand` methods for completeness, even though their infallible equivalents are trait implementations.

rustbot · 2022-10-10T21:16:13Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

dpaoliello · 2022-10-10T21:17:39Z

@rustbot label +T-libs-api -T-libs

dpaoliello · 2022-10-10T21:19:33Z

@rustbot ready

the8472 · 2022-10-19T18:47:39Z

While most of the APIs at least appear somewhat reasonable (but not ideal or low-abstraction-orthogonal-building-blocks)

these two seem more dubious than the others:

    pub fn try_from_iter<I: IntoIterator<Item = T>>(iter: I) -> Result<Vec<T>, TryReserveError>;
    pub fn try_extend<I: IntoIterator<Item = T>>(&mut self, iter: I, ) -> Result<(), TryReserveError>;

They basically implement inherent, fallible versions of the FromIterator and Extend impls. They also lead to a lot of trait impls getting modified. But neither of those interfaces are well-suited for fallible operations because they consume an iterator, take any number of items from it and then may suddenly fail and drop those items - and in the former case a partially allocated vec - on the floor. That is fine in the infallible case where the unwind would usually lead to those getting dropped anyway. But in a case where one needs to recover from errors that can be a bad idea.

Fallible iterator-absorbing methods may need separate implementations that allocate ahead of time (at the risk of allocating a wee bit too much) and hand you back the pieces when stuff breaks.

Removing those would also shrink the diff.

Ericson2314 · 2022-11-01T15:49:24Z

If it makes this more acceptable to @m-ou-se and the libs team, I am not opposed to splitting those two out to be dealt with later. @dpaoliello what do you think?

bors · 2022-11-27T04:15:24Z

☔ The latest upstream changes (presumably #104818) made this pull request unmergeable. Please resolve the merge conflicts.

dpaoliello · 2023-03-09T03:27:46Z

Closing this for now.
In the short term, I've open sourced the FallibleVec trait we've been using at Microsoft: https://crates.io/crates/fallible_vec
In the long term, hopefully Keyword Generics will provide a solution for this: https://blog.rust-lang.org/inside-rust/2022/07/27/keyword-generics.html

Ericson2314 · 2023-05-20T15:08:02Z

library/alloc/src/vec/mod.rs

 mod spec_extend;

+pub(crate) trait VecError {


@workingjubilee this is mechanism that allows the eager bail out (just like today) or error throwing.

rust-highfive assigned joshtriplett Mar 17, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2022

dpaoliello commented Mar 17, 2022

View reviewed changes

library/alloc/src/macros.rs Show resolved Hide resolved

dpaoliello commented Mar 17, 2022

View reviewed changes