Stabilize iter_advance_by

[theemathas]

Closes #77404 (tracking issue)

See also zulip discussion: #t-libs > Status of `Iterator::advance_by`?

Implementation history

• Implementation (Add Iterator::advance_by and DoubleEndedIterator::advance_back_by #76909)
• Add tracking issue (Add tracking issue of iter_advance_by feature #77405)
• Implement for Chain (Implement advance_by, advance_back_by for iter::Chain #77594)
• implement for slice::{Iter, IterMut} (Implement advance_by, advance_back_by for slice::{Iter, IterMut} #87387, #[inline] slice::Iter::advance_by #87736)
• implement for vec::IntoIter, ops::Range, iter::{Cycle, Skip, Take, Copied, Flatten} (implement advance_(back_)_by on more iterators #87091)
• Change advance(_back)_by to return the remainder instead of the number of processed elements (Change advance(_back)_by to return the remainder instead of the number of processed elements #92284)

API being stabilized

trait Iterator {
    // irrelevant items omitted

    fn advance_by(&mut self, n: usize) -> Result<(), NonZero<usize>> { .... }
}

trait DoubleEndedIterator {
    // irrelevant items omitted

    fn advance_back_by(&mut self, n: usize) -> Result<(), NonZero<usize>>) { .... }
}

If the advancement causes an attempt to iterate past the end of the iterator, the method returns an Err with the number of steps that are remaining by the time the iterator has hit the end.

Calling .advance_by(0) may meaningfully mutate the iterator. As the documentation says:

Calling advance_by(0) can do meaningful work, for example Flatten can advance its outer iterator until it finds an inner iterator that is not empty, which then often allows it to return a more accurate size_hint() than in its initial state.

Experience Report

The feature is quite a popular unstable feature, as can be seen from the github search results, which finds 2k files.

The tracking issue also has many emoji reactions (currently at 27 hearts), which indicates a desire for this API.

Potential issues / questions

• Should this have a where Self: Sized bound to reduce the vtable size? (The only other methods in the vtable are next, size_hint, and nth.) Tracking Issue for feature(iter_advance_by) #77404 (comment)
  • I think this is fine, given the tradeoff of potentially getting better performance on methods that forward to advance_by.
• Should code using generic iterators call nth or advance_by for the best performance? Calling .advance_by might skip the need to actually generate the element, while calling nth would work better with iterator implementations that implement nth (since it was stabilized first), but not advance_by. Tracking Issue for feature(iter_advance_by) #77404 (comment)
  • There doesn't seem to be an ideal solution, but I don't think we should let that block stabilization.
• Should advance_by be guaranteed to perform side-effects of producing the skipped-over elements? Tracking Issue for feature(iter_advance_by) #77404 (comment)
  • I think that it should guarantee performing side-effects, unless next forwards to a method that is conventionally pure, such as Clone.
• Should the method return Ok(&mut self) in order to enable method chaining? #t-libs > Status of `Iterator::advance_by`? @ 💬
  • I think we should use the current return type. Changing it makes the method dyn-incompatible, which hampers optimization of methods on dyn Iterator that forward to it. I think the ability to chain is of questionable benefit anyway.

[rustbot]

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

[rustbot]

r? @Darksonn

rustbot has assigned @Darksonn.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from 6 candidates

[theemathas]

cc @ChrisDenton

[ChrisDenton]

@rfcbot merge libs

[rust-rfcbot]

@ChrisDenton has proposed to merge this. The next step is review by the rest of the tagged team members:

• @ChrisDenton
• @Mark-Simulacrum
• @cuviper
• @dtolnay
• @joshtriplett
• @nia-e
• @the8472
• @thomcc

Concerns:

• nth-advance-by (Stabilize iter_advance_by #163328 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/libs-ping: FCP proposed for libs, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[the8472]

Should advance_by be guaranteed to perform side-effects of producing the skipped-over elements? #77404 (comment)

I think that it should guarantee performing side-effects, unless next forwards to a method that is conventionally pure, such as Clone.

Currently we're not even doing that for Cloned (but I'd like it do). But then what is pure, we lack effects so we'd have to approximate it. are Fn()s "conventionally pure", since it has no mutable capture? Or a ZST Fn() which can't have interior mutability either?

Personally I want to be about as aggressive about eliding effects as I can get away with and introducing a new method whose semantics nobody relies on is a good opportunity for that. Other iterator adapters can then build on this capability (if not the std adapters due to backcompat, then at least itertools).

[theemathas]

@the8472 We currently skip over cloning in the implementation for iter::Repeat.

rust/library/core/src/iter/sources/repeat.rs

Lines 91 to 96 in ecd8f1d

	#[inline]
	fn advance_by(&mut self, n: usize) -> Result<(), NonZero<usize>> {
	// Advancing an infinite iterator of a single element is a no-op.
	let _ = n;
	Ok(())
	}

[Mark-Simulacrum]

Should code using generic iterators call nth or advance_by for the best performance?

It looks like currently advance_by doesn't attempt to forward to nth to allow migrating here. Is there some discussion of why that isn't feasible? The tracking issue says this can't be be done because "advance_by returns the number of items you can't implement in terms of .nth() which was the previous recommended way to skip elements." -- do we think the Err variant of advance_by is useful enough that we should keep it? What is the intended purpose of the skipped length?

I think this isn't blocking necessarily, but I think the stabilization report should include more thought on this than ("There doesn't seem to be an ideal solution, but I don't think we should let that block stabilization."). What (else) did we consider?

@rfcbot concern nth-advance-by

[the8472]

-- do we think the Err variant of advance_by is useful enough that we should keep it?

The Err variant is necessary to implement it efficiently for composite iterators like Chain.

[Darksonn]

@rustbot label +S-waiting-on-t-libs -S-waiting-on-review

[programmerjake]

not something that blocks stabilization, but I noticed a missing impl of advance_back_by for Fuse