Implement Iterator::last for vec::IntoIter

[thaliaarchi]

Avoid iterating everything when we have random access to the last element.

[rustbot]

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[thaliaarchi]

I'm unsatisfied that this suppresses the Clippy diagnostic for vec::IntoIter, as I think it is still good advice. It would be somewhat imprecise for manual last implementations, though, in general.

[samueltardieu]

I'm unsatisfied that this suppresses the Clippy diagnostic for vec::IntoIter, as I think it is still good advice. It would be somewhat imprecise for manual last implementations, though, in general.

This removes the whole purpose of the test which is to check whether this would modify the drop order of the collection's elements. Could you maybe replace the let v = v.into_iter(); by let v = &mut v.into_iter() as &mut dyn DoubleEndedIterator<Item = S>;? This should make the lint trigger, and the drop order be checked.

[thaliaarchi]

I think making a custom DoubleEndedIterator just for the test would be more clear. Really, I just blessed the tests to get CI to pass for the sake of discussion.

Do you think it's fine that this case is no longer linted for vec::IntoIter<T>? On one hand, the forced drop is usually less desirable, but may be exactly what someone wants. Maybe the lint could be selected reenabled for this type and others similar? I could go either way.

[thaliaarchi]

I'm confused why you edited my comment. Perhaps you were grabbing the Markdown contents for a reply and edited it instead? I've reverted the edit and reproduce your comment below:

@samueltardieu commented 2025-04-14T14:02:22Z:

Do you think it's fine that this case is no longer linted for vec::IntoIter<T>? On one hand, the forced drop is usually less desirable, but may be exactly what someone wants. Maybe the lint could be selected reenabled for this type and others similar? I could go either way.

Independently from the Clippy lint, I wonder if your last() implementation conforms to the methods semantics, as described in the documentation of Iterator::last():

Consumes the iterator, returning the last element.

This method will evaluate the iterator until it returns None. While doing so, it keeps track of the current element. After None is returned, last() will then return the last element it saw.

For me, it implies that elements are dropped in order. So I'd say that if your change is accepted (and the documentation for Iterator::last() is updated), then it is fine for Clippy not to lint about Vec::IntoIter<_>::last(), as it would have nothing useful to suggest and to warn about.

[thaliaarchi]

I've now made a little wrapper iterator for the test to still exercise that same case, since it wasn't really intending to test the implementation details of vec::IntoIter. And I agree now that vec::IntoIter shouldn't trigger the lint anymore after this change.

[samueltardieu]

I'm confused why you edited my comment. Perhaps you were grabbing the Markdown contents for a reply and edited it instead?

Ooops, this is was not only unintentional but also unnoticed. Thanks for fixing this.

[bors]

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

[workingjubilee]

The semantics of Iterator::last are

Consumes the iterator, returning the last element.

Note the "consumes" part.

If I understand this change correctly, on some Vecs (depending on exact size) I can now call last() twice and the second call does not offer None? There's more to the description there that seems to make it very clear the second call should offer None, I think.

[workingjubilee]

I am not convinced that this implementation is not reneging on Iterator::last's contract. I am willing to hear reasoning otherwise, but I am pretty sure my reading of the docs here is correct.

I think it can probably be fixed to have this obviously-faster implementation while still upholding the equivalent of that contract? But I am not sure. I am not going to think that far right now.

This either

• needs to be closed
• needs to be worked on
• needs to include an explanation of why jubilee doesn't know what she is saying

I will leave it to your judgement to decide which path is viable.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[thaliaarchi]

I think the detail you're missing is that last takes the iterator by self, not &mut self. That's the “consumes” part. Other places in std use next_back inside of last, so it seems likely to be fine here.

[workingjubilee]

squint Yep, that's probably why Jubilee doesn't know what she is talking about. Let me take a closer look. I have a question about a particular case involving iterators that have side effects upon iteration that I should just write the test for...

[thaliaarchi]

Oh, by the way, if #139847 merges first, I'll add this bit here, though it's entirely inconsequential. It was actually how I discovered this.

--- a/library/std/src/sys/args/common.rs
+++ b/library/std/src/sys/args/common.rs
@@ -51,4 +51,4 @@ impl Iterator for Args {
     #[inline]
-    fn last(mut self) -> Option<OsString> {
-        self.iter.next_back()
+    fn last(self) -> Option<OsString> {
+        self.iter.last()
     }

[thaliaarchi]

The drop order of Vec and of vec::IntoIter are the same, since they both use ptr::drop_in_place on their slices (though neither documents this). ptr::drop_in_place drops the elements of a slice from front-to-back, which matches the front-to-back iteration of last. Since Iterator::last takes the iterator by self, the iterator is dropped before the last element is returned and the drop order is always the same.

This is the current status quo:

struct D(u32);
impl Drop for D {
    fn drop(&mut self) {
        println!("drop {}", self.0);
    }
}

println!("Vec::drop");
drop(vec![D(1), D(2), D(3)]);
println!("vec::IntoIter::last");
drop(vec![D(4), D(5), D(6)].into_iter().last());

Vec::drop
drop 1
drop 2
drop 3
vec::IntoIter::last
drop 4
drop 5
drop 6

[workingjubilee]

still cogitating but first

@rustbot ready

[thaliaarchi]

I've applied the patch I mentioned, now that #139847 has merged.

[workingjubilee]

I... feel very skeptical of the standard library's use of specialization, now, as it contributed greatly to my initial uncertainty. I feel like it weakens the ability to do generic reasoning.

However, I am, for now, reasonably confident that it does not affect this PR.

@bors r+

[bors]

📌 Commit cbdd713 has been approved by workingjubilee

It is now in the queue for this repository.