Restore original implementation of Vec::retain
This PR reverts #48065, which aimed to optimize `Vec::retain` by making use of `Vec::drain_filter`. Unfortunately at that time, `drain_filter` was unsound.
The soundness hole in `Vec::drain_filter` was fixed in #61224 by guaranteeing that cleanup logic runs via a nested `Drop`, even in the event of a panic. Implementing this nested drop affects codegen (apparently?) and results in slower code.
Fixes#65970
This PR reverts #48065, which aimed to optimize `Vec::retain` by
making use of `Vec::drain_filter`. Unfortunately at that time,
`drain_filter` was unsound.
The soundness hole in `Vec::drain_filter` was fixed in #61224 by
guaranteeing that cleanup logic runs via a nested `Drop`, even in
the event of a panic. Implementing this nested drop affects codegen
(apparently?) and results in slower code.
Fixes#65970
impl TrustedLen for vec::Drain
The iterator methods just forward to `slice::Iter`, which is `TrustedLen`.
This can probably be applied to other `Drain` structs as well.
The previous ordering of the sentences kept switching between the return
value and the value of `self` after execution, making it hard to follow.
Additionally, as rendered in the browser, the period in "`Self`. `self`"
was difficult to make out as being a sentence separator and not one code
block.
Make the semantics of Vec::truncate(N) consistent with slices.
This commit simplifies the implementation of `Vec::truncate(N)` and
makes its semantics identical to dropping the `[vec.len() - N..]`
sub-slice tail of the vector, which is the same behavior as dropping a
vector containing the same sub-slice.
This changes two unspecified aspects of `Vec::truncate` behavior:
* the drop order, from back-to-front to front-to-back,
* the behavior of `Vec::truncate` on panics: if dropping one element of
the tail panics, currently, `Vec::truncate` panics, but with this PR all other
elements are still dropped, and if dropping a second element of the tail
panics, with this PR, the program aborts.
Programs can trivially observe both changes. For example
([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7bef575b83b06e82b3e3529e4edbcac7)):
```rust
fn main() {
struct Bomb(usize);
impl Drop for Bomb {
fn drop(&mut self) {
panic!(format!("{}", self.0));
}
}
let mut v = vec![Bomb(0), Bomb(1)];
std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
v.truncate(0);
}));
assert_eq!(v.len(), 1);
std::mem::forget(v);
}
```
panics printing `1` today and succeeds. With this change, it panics
printing `0` first (due to the drop order change), and then aborts
with a double-panic printing `1`, just like dropping the
`[Bomb(0), Bomb(1)]` slice does, or dropping
`vec![Bomb(0), Bomb(1)]` does.
This needs to go through a crater run.
r? @SimonSapin
https://github.com/rust-lang/rust/pull/49496 introduced specialization based on:
```
unsafe impl<T: ?Sized> IsZero for *mut T {
fn is_zero(&self) -> bool {
(*self).is_null()
}
}
```
… to call `RawVec::with_capacity_zeroed` for creating `Vec<*mut T>`,
which is incorrect for fat pointers
since `<*mut T>::is_null` only looks at the data component.
That is, a fat pointer can be “null” without being made entirely of zero bits.
This commit fixes it by removing the `?Sized` bound on this impl
(and the corresponding `*const T` one).
This regresses `vec![x; n]` with `x` a null raw slice of length zero,
but that seems exceptionally uncommon.
(Vtable pointers are never null, so raw trait objects would not take
the fast path anyway.
An alternative to keep the `?Sized` bound
(or even generalize to `impl<U: Copy> IsZero for U`)
would be to cast to `&[u8]` of length `size_of::<U>()`,
but the optimizer seems not to be able to propagate alignment information
and sticks with comparing one byte at a time:
https://rust.godbolt.org/z/xQFkwL
----
Without the library change, the new test fails as follows:
```
---- vec::vec_macro_repeating_null_raw_fat_pointer stdout ----
[src/liballoc/tests/vec.rs:1301] ptr_metadata(raw_dyn) = 0x00005596ef95f9a8
[src/liballoc/tests/vec.rs:1306] ptr_metadata(vec[0]) = 0x0000000000000000
thread 'vec::vec_macro_repeating_null_raw_fat_pointer' panicked at 'assertion failed: vec[0] == null_raw_dyn', src/liballoc/tests/vec.rs:1307:5
```
This commit simplifies the implementation of `Vec::truncate(N)` and
makes its semantics identical to dropping the `[vec.len() - N..]`
sub-slice tail of the vector, which is the same behavior as dropping a
vector containing the same sub-slice.
This changes two unspecified aspects of `Vec::truncate` behavior:
* the drop order, from back-to-front to front-to-back,
* the behavior of `Vec::truncate` on panics: if dropping one element of
the tail panics, currently, `Vec::truncate` panics, but with this PR all other
elements are still dropped, and if dropping a second element of the tail
panics, with this PR, the program aborts.
Programs can trivially observe both changes. For example
([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7bef575b83b06e82b3e3529e4edbcac7)):
```rust
fn main() {
struct Bomb(usize);
impl Drop for Bomb {
fn drop(&mut self) {
panic!(format!("{}", self.0));
}
}
let mut v = vec![Bomb(0), Bomb(1)];
std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
v.truncate(0);
}));
assert_eq!(v.len(), 1);
std::mem::forget(v);
}
```
panics printing `1` today and succeeds. With this change, it panics
printing `0` first (due to the drop order change), and then aborts
with a double-panic printing `1`, just like dropping the
`[Bomb(0), Bomb(1)]` slice does, or dropping
`vec![Bomb(0), Bomb(1)]` does.
This allows lints and other diagnostics to refer to items
by a unique ID instead of relying on whacky path
resolution schemes that may break when items are
relocated.
Rename .cap() methods to .capacity()
As mentioned in #60316, there are a few `.cap()` methods, which seem out-of-place because such methods are called `.capacity()` in the rest of the code.
This PR renames them to `.capacity()` but leaves `RawVec::cap()` in there for backwards compatibility.
I didn't try to mark the old version as "deprecated", because I guess this would cause too much noise.
Prevent Vec::drain_filter from double dropping on panic
Fixes: #60977
The changes in this PR prevent leaking and double-panicking in addition to double-drop.
Tracking issue: #43244
Add implementations of last in terms of next_back on a bunch of DoubleEndedIterators
Provided a `DoubleEndedIterator` has finite length, `Iterator::last` is equivalent to `DoubleEndedIterator::next_back`. But searching forwards through the iterator when it's unnecessary is obviously not good for performance. I ran into this on one of the collection iterators.
I tried adding appropriate overloads for a bunch of the iterator adapters like filter, map, etc, but I ran into a lot of type inference failures after doing so.
The other interesting case is what to do with `Repeat`. Do we consider it part of the contract that `Iterator::last` will loop forever on it? The docs do say that the iterator will be evaluated until it returns None. This is also relevant for the adapters, it's trivially easy to observe whether a `Map` adapter invoked its closure a zillion times or just once for the last element.
It's natural for `retain` to work in order from beginning to end, but
this wasn't actually documented to be the case. If we actually promise
this, then the caller can do useful things like track the index of each
element being tested, as [discussed in the forum][1]. This is now
documented for `Vec`, `VecDeque`, and `String`.
[1]: https://users.rust-lang.org/t/vec-retain-by-index/27697
`HashMap` and `HashSet` also have `retain`, and the `hashbrown`
implementation does happen to use a plain `iter()` order too, but it's
not certain that this should always be the case for these types.
Matthew Kraai