Rollup merge of #152258 - asder8215:vecdeque_splice_151758, r=joboet

fixed VecDeque::splice() not filling the buffer correctly when resizing the buffer on start = end range

This PR fixes #151758. The issue came from `Splice::move_tail`, which as joboet pointed out:
> The issue is in move_tail, which resizes the buffer, but fails to account for the resulting hole.

The problem with reserving more space through `VecDeque`'s `buf.reserve()` is that it doesn't update `VecDeque`'s `head`, which means that this code in `move_tail`:
```rust
deque.wrap_copy(
    deque.to_physical_idx(tail_start),
    deque.to_physical_idx(new_tail_start),
    self.tail_len,
);
```
could move over the section of data that `tail_start..tail_start + self.tail_len` of the buffer is supposed to be held at to the incorrect destination since all `.to_physical_idx()` is doing is a wrapping add on the `VecDeque`'s head with the passed in `idx` value.

To avoid this I decided to use `VecDeque::reserve` to both allocate more space into the `VecDeque` if necessary and update head appropriately. However, `VecDeque::reserve` internally relies on the `VecDeque`'s `len` field. Earlier in `VecDeque::splice`, it modifies the `VecDeque`'s `len` constructing the drain via `Drain::new` (as it does a `mem::replace` on `deque.len` with the start bound of the passed in `range`). The `VecDeque`'s `len` can also be potentially modified in the earlier `Splice::fill()` call if it does any replacement towards elements within the passed in `range` value for `VecDeque::splice()`. I needed to temporarily restore the `VecDeque`'s `len` to its actual len, so that `VecDeque::reserve` can work properly. Afterward, you can bring back the `VecDeque`'s `len` to what its value was before and fill the gap appropriately with the rest of the `replace_with` content.

r? @joboet

Author: Zalathar

library/alloc/src/collections/vec_deque/splice.rs

@@ -138,8 +138,48 @@ impl<T, A: Allocator> Drain<'_, T, A> {
     /// self.deque must be valid.
     unsafe fn move_tail(&mut self, additional: usize) {
         let deque = unsafe { self.deque.as_mut() };
-        let tail_start = deque.len + self.drain_len;
-        deque.buf.reserve(tail_start + self.tail_len, additional);
+
+        // `Drain::new` modifies the deque's len (so does `Drain::fill` here)
+        // directly with the start bound of the range passed into
+        // `VecDeque::splice`. This causes a few different issue:
+        //     - Most notably, there will be a hole at the end of the
+        //       buffer when our buffer resizes in the case that our
+        //       data wraps around.
+        //     - We cannot use `VecDeque::reserve` directly because
+        //       how it reserves more space and updates the `VecDeque`'s
+        //       `head` field accordingly depends on the `VecDeque`'s
+        //       actual `len`.
+        //     - We cannot just directly modify `VecDeque`'s `len` and
+        //       and call `VecDeque::reserve` afterward because if
+        //       `VecDeque::reserve` panics on capacity overflow,
+        //       well now our `VecDeque`'s head does not get updated
+        //       and we still have a potential hole at the end of the
+        //       buffer.
+        // Therefore, we manually reserve additional space (if necessary)
+        // based on calculating the actual `len` of the `VecDeque` and adjust
+        // `VecDeque`'s len right *after* the panicking region of `VecDeque::reserve`
+        // (that is `RawVec` `reserve()` call)
+
+        let drain_start = deque.len;
+        let tail_start = drain_start + self.drain_len;
+
+        // Actual VecDeque's len = drain_start + tail_len + drain_len
+        let actual_len = drain_start + self.tail_len + self.drain_len;
+        let new_cap = actual_len.checked_add(additional).expect("capacity overflow");
+        let old_cap = deque.capacity();
+
+        if new_cap > old_cap {
+            deque.buf.reserve(actual_len, additional);
+            // If new_cap doesn't panic, we can safely set the `VecDeque` len to its
+            // actual len; this needs to be done in order to set deque.head correctly
+            // on `VecDeque::handle_capacity_increase`
+            deque.len = actual_len;
+            // SAFETY: this cannot panic since our internal buffer's new_cap should
+            // be bigger than the passed in old_cap
+            unsafe {
+                deque.handle_capacity_increase(old_cap);
+            }
+        }
 
         let new_tail_start = tail_start + additional;
         unsafe {
@@ -149,6 +189,9 @@ impl<T, A: Allocator> Drain<'_, T, A> {
                 self.tail_len,
             );
         }
+
+        // revert the `VecDeque` len to what it was before
+        deque.len = drain_start;
         self.drain_len += additional;
     }
 }

library/alloctests/tests/vec_deque.rs

@@ -2347,3 +2347,16 @@ fn test_splice_wrapping() {
 
     assert_eq!(Vec::from(vec), [7, 8, 9]);
 }
+
+#[test]
+fn test_splice_wrapping_and_resize() {
+    let mut vec = VecDeque::new();
+
+    for i in [1; 6] {
+        vec.push_front(i);
+    }
+
+    vec.splice(1..1, [2, 3, 4]);
+
+    assert_eq!(Vec::from(vec), [1, 2, 3, 4, 1, 1, 1, 1, 1])
+}