Add try_remove to Vec

[ohsayan]

This provides a 'non-panicky' way of removing an element from a vector. When the index exists, we return it; Otherwise, None is returned.

Tracking issue: #77481

[rust-highfive]

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

[ohsayan]

@rustbot modify labels to +A-collections

[ohsayan]

@rustbot modify labels to +T-libs

[ohsayan]

I really like the complexity of this error

[timvermeulen]

If we're adding this, should we also add try_ variants of similar methods, such as try_swap_remove, try_insert, try_split_at? I worry that Vec's API will be in a bit of a weird state if one is added but not any others (a bit like Iterator::try_find).

[ohsayan]

If we're adding this, should we also add try_ variants of similar methods, such as try_swap_remove, try_insert, try_split_at? I worry that Vec's API will be in a bit of a weird state if one is added but not any others (a bit like Iterator::try_find).

I kinda agree. There should be non-panicky methods for methods that can panic, especially when a panic is expected. Now, if we have to enforce this try_ standard, then that's another story altogether.

[ohsayan]

@jyn514 Should I be adding the tests here?

[jyn514]

Sure, that seems like a good place. Maybe near the existing tests for swap_remove()?

[jyn514]

r=me on the implementation, but this should have someone from T-libs make sure this is a useful addition to the API that's on the roadmap.

[ohsayan]

r? @KodrAus

[jyn514]

It tells you if you scroll up:

error[E0658]: use of unstable library feature 'vec_try_remove'
   --> library/alloc/tests/vec.rs:527:20
    |
527 |     assert_eq!(vec.try_remove(0), Some(1));
    |                    ^^^^^^^^^^
    |
    = note: see issue #77481 <https://github.com/rust-lang/rust/issues/77481> for more information
    = help: add `#![feature(vec_try_remove)]` to the crate attributes to enable

[timvermeulen]

@ohsayan It needs to go in tests/lib.rs with the other #[feature(...)]s.

[scottmcm]

I feel like this is a convention change to Vec, and thus needs more than just a PR.

Because, as @timvermeulen mentioned, why is this just to remove? If we have try_remove, why not also try_drain and try_split_off and try_splice and try_swap_remove and try_insert? (Not to mention slice methods, like try_split_at or try_rotate_left or ... And should VecDeque get all these methods too?)

The preconditions for the indexes on all these are simple enough to check yourself, so I don't think it's worth the API bloat to have non-panicking versions of all of them, and I don't think that any of them are special enough to have such a version when the others don't.

That said, maybe it would make sense to have a fallible form of an API like #76885 that people could use to check bounds without panicking or hand-coding it? (For that conversation, maybe head over to #76393 (comment)?) Though I suppose v.get(x)?; can already serve as that just fine.

(Note also that it took rust-lang/rfcs#2116 to add try_reserve, and that's not even stable yet. That's not quite the same, because it's about the fallibility of the allocation, but that does mean that a hypothetical try_insert would have to get into questions about fallibility in the allocation too. Of course that particular consideration doesn't apply to try_swap_remove and such that don't need to allocate, but I think it's still interesting prior art to look at for try_* on Vec.)

[ohsayan]

@scottmcm Maybe we should have had all these methods returning some kind of Result<T, E> or Option<T> since their inception. For the exact same reasons of fallibility, we have get, try_find, try_fold, try_reserve, try_for_each and similar APIs.

Edit: The only way to make all these functions non-panicking is post 2.x.y, which doesn't seem plausible enough to me, just for the sake of having fallible variants.
Edit: Hence, the only way for us to avoid an unnecessary 2.x.y, would be to:

• Either have users manually doing bound-checking (that'd be a double check, since remove and insert themselves will do bound checks again, but again, this has the advantage of not bloating the API)
  Update: If users were to avoid double bound checks, this is the approach that they'd have to take:

  if idx < vec.len() {
      unsafe {
          // A custom impl of unchecked removal/insertion/...
      }
  }

  Edit: As correctly pointed out by @scottmcm in this comment (which I seem to have overlooked), LLVM will optimize the bound checks in some cases
• Or to have try_* variants

[ohsayan]

I'll be having some free time tomorrow; so I'll try and experiment with the possibilities of a convention change.

Edit: FWIW, should we have a dedicated issue for this?

[scottmcm]

Maybe we should have had all these methods returning some kind of Result<T, E> or Option since their inception.

This kind of thing comes up periodically, seemingly from a perspective of Result being always better than panic, but that's not really true. If 99% of the time people will just .unwrap() anyway, that's not an improvement -- and indexing seems to be the most common place where it's extremely common for people to have algorithmic invariants that mean they want to be able to just do v[i], not v.get(i).unwrap(). If the precondition is simple to explain and check ahead of time, I'm a big fan of just assert!ing it. This is especially true, to me, for "datastructure-y" things -- there's nothing the user can ever do with a ?ed error about "the index was out of range". It's a bug that needs to go to a programmer, and thus quite different from "I couldn't reach the internet" or whatever.

For the exact same reasons of fallibility, we have

I don't think those are all that useful as precedent here. As mentioned in my other post, try_reserve is about the allocator's fallibility (not about indexing) and both needed an RFC to happen -- which explicitly chose not to have try_push and friends -- and is still unstable. try_fold (and try_for_each) are actually about early-exiting of loops, and they and try_find are about calling closures that themselves return Try types, but of course there's no closures in try_remove. Yes, get is available as a fallible version of indexing. But it's uncommonly used and is only about indexing.

If users were to avoid double bound checks, this is the approach that they'd have to take:

Remember to give LLVM appropriate credit here. The number of checks that appear in the source code isn't necessarily the number of checks that happen at runtime; just the opposite. For example, https://rust.godbolt.org/z/rPbGah

pub fn looks_like_2_checks_actually_2_checks(x: &[i32]) -> i32 {
    x[0] + x[1]
}
pub fn looks_like_2_checks_actually_1_check(x: &[i32]) -> i32 {
    x[1] + x[0]
}
pub fn looks_like_3_checks_actually_1_check(x: &[i32]) -> i32 {
    assert!(x.len() >= 2);
    x[0] + x[1]
}

Similarly, something like this https://rust.godbolt.org/z/559fWe

fn my_remove<T>(v: &mut Vec<T>, i: usize) -> Option<T> {
    v.get(i)?;
    Some(v.remove(i))
}

Also optimizes out the panic check and code path from remove, because LLVM can see that it was already checked -- even though it was checked not-all-that-obviously through get.

---

Edit: I should clarify that I'm not saying that a try_remove method would be bad, necessarily. But it would be possible to have try_ versions of everything that panics, and the standard library generally doesn't. My argument here is more that while no individual method would be terrible, having all of them would be somewhat of a mess in the docs for minimal-at-best value. So given that checking the precondition manually works great and isn't hard, I don't see them pulling their weight.

[ohsayan]

From the discussion here — it seems like it'd be better if we left panic handling to the user

[KodrAus]

I agree with @scottmcm here that if we want to add fallible versions of these methods on Vec then we should go through the RFC process (at least a pre-RFC) to motivate them and see them in context.

[ohsayan]

As per the discussion here and with consensus from the community (and project members), a single PR will not be enough for providing fallible alternatives for insert/remove/... ops in Vec, VecDeque and related data structures. I will be creating an RFC for moving this discussion forward. Thank you @jyn514 @timvermeulen @scottmcm @KodrAus for sharing your inputs (and concerns).

[KodrAus]

Thanks for exploring this @ohsayan!

[optozorax]

@ohsayan any updates on RFC?

I highly prefer try_* methods, besauce Rust is The System Programming Language -- this means that it must have non-panicking methods for anything. If we want a core to be written on Rust, we don't want it to panic because of a remove methods.

having all of them would be somewhat of a mess in the docs

I think this is not the problem of a try_* methods, this problem of the docs. We should create docs features, that can handle that amount of methods in organized manner.

[ohsayan]

@optozorax Heya, I didn't get enough time to look back into this, but I'll try and create an RFC once I do

[adsick]

Hi, like many others (I believe) people I've also noticed the lack of non-panicking methods like .try_remove() or .try_insert()
It bothers me quite a little bit, so I would like to share a rather peculiar idea:
what if functions and methods could be generic over their error handling behavior?

please tell me if things like that already were discussed somewhere.

let's take for example simple Vec::get method, it returns Option<T>, but let's say it can be
a) panicking method, returning T
b) fallible method, returning Option (as it is)
c) unchecked unsafe method which returns T and is undefined in case of out of bound access.

let v = vec![1, 2, 3];

v.get(1); // Some(2)
v.get(1)!; // 2
unsafe{
v.get(1); // blazing fast 2
}
v.get(4); // None
v.get(4)!; // panic, note exclamation mark
unsafe{
v.get(4);
} // blazing fast UB

here a new operator is introduced, exclamation mark - it means we want to call a version of fn which is allowed to panic at runtime.
I know that this may be too much, but still.

how it would look like from perspective of method signature and definition - interesting question.

impl<T> Vec<T>{
fn get(&self, index: usize) -> Option<&T>
{
   if self.len() < usize{
       None
   } else {
   Some(&self[index])
   }
}
}

although this code may be trivially converted (by compiler) to

fn get(&self, index: usize)! -> &T
{
   &self[index]
}

(simply remove len check and "unwrap" desired value)
it may not be general, so we may have some form of overloading for this stuff:

//fallible method
fn get(&self, index: usize) -> Option<&T>
{
   if self.len() < usize{
       None
   } else {
   Some(&self[index])
   }
}

//panicking method
fn get(&self, index: usize)! -> &T
{
   &self[index]
}

unsafe fn get... something more advanced
}

I'm aware of this

p.s. probably it is a discussion for rust-lang/project-error-handling#49

[jyn514]

Posting comments on a closed PR is not a good way to get them reviewed. An ACP, RFC, or post on internals.rust-lang.org is a better way to get feedback.

[adsick]

@jyn514 thanks, didn't know that.