Weaken guarantee around advancing underlying iterators in zip

[the8472]

The current guarantee (introduced in #52279) is too strong as it prevents adapters from exploiting knowledge about the iterator length and using counted loops for example because they would stop calling next() before it ever returned None. Additionally several nested zip iterators already fail to uphold this.

This does not yet remove any of the specialization code that tries (and sometimes fails) to uphold the guarantee for next()
because removing it would also affect next_back() in more surprising ways.

The intent is to be able to remove for example this branch

rust/library/core/src/iter/adapters/zip.rs

Lines 234 to 243 in 36bcf40

	} else if A::MAY_HAVE_SIDE_EFFECT && self.index < self.a_len {
	let i = self.index;
	self.index += 1;
	self.len += 1;
	// match the base implementation's potential side effects
	// SAFETY: we just checked that `i` < `self.a.len()`
	unsafe {
	self.a.__iterator_get_unchecked(i);
	}
	None

or this test

rust/library/core/tests/iter/adapters/zip.rs

Lines 177 to 188 in 36bcf40

	#[test]
	fn test_zip_nested_sideffectful() {
	let mut xs = [0; 6];
	let ys = [0; 4];
	{
	// test that it has the side effect nested inside enumerate
	let it = xs.iter_mut().map(|x| *x = 1).enumerate().zip(&ys);
	it.count();
	}
	assert_eq!(&xs, &[1, 1, 1, 1, 1, 0]);
	}

Solves #82303 by declaring it a non-issue.

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[the8472]

@rustbot label T-libs

[the8472]

#83796 can be used for a crater run and serves as an example what we might get rid off.

[m-ou-se]

no more elements to return

What would this mean for non-fused iterators? What if both iterators return None at some point, but both start giving Some(..) again some time later? Are you sayingg that the Zip iterator might just stop forever at that point (fusing the iterator), or are you just talking about iterators that we know will give None forever because of our specializations?

[the8472]

A hypothetical implementation could query size_hint() of one or both iterators on every call to next() and short-circuit when the upper bound of either is 0. That should be safe and not require any specialization. Whether that could be considered as fusing the iterator would depend on how the non-fused one calculates its size hint.

It's intentionally kept vague to leave more flexibility for optimizations while at the same time telling the users that the underlying iterators' next() might not be called. Previously it only said that the 2nd iterator's might not be called, now it may call neither.

[Dylan-DPC-zz]

marking this as blocked till #83796 crater run is complete and triaged.

[the8472]

@rustbot label -S-waiting-on-crater +S-waiting-on-review

The crater run in #83796 completed, I went through the results in #83796 (comment) and wrote my conclusion in #83796 (comment)

Possible routes that can be taken from here:

A)

1. Accept this PR, which only updates documentation to declare Better documentation for Iter::zip behavior #52279 a non-issue. This would only legalize the current state of TrustedRandomAccess specializations eliding the side-effects of calling zip.next() after a None has been returned. This is a fairly niche side-effect and the crater run didn't turn up anything relying on that even though it actually tested a more aggressive optimization. I.e. this only affects forward iteration.
2. (optional) Further simplifcation of Zip. In the specialization we could also elide the length-trimming side-effects during backwards iteration when the iterators are not of the same length. This would be a somewhat bigger change, but still around the totally unspecified case of backwards iteration in Zip. The crater run also tested that and didn't find any issues. It would be a change in behavior but not an API break since the API simply didn't specify anything in that regard.
3. (optional and tangential) Further specializations for the purpose of optimization on the default iterator implementations. This would extend the changes in 1. to all iterators (where applicable)

B)

1. reject this PR and instead remove most specializations from Zip. This would simplify its implementation and only leave the default implementation which behaves according to the documented behavior. This would also end all the problems surrounding zip. The downside is of course that it would also revert to the performance characteristics of the default implementation
2. introduce ZipExact ([ER] Iterator::zip_exact #85342) as replacement that is easier to optimize because we can design its contract so that it is easier to optimize and avoid the specialization pitfalls that we have learned from zip.

C)

Middle ground between A) and B): We could remove the TrustedRandomAccess specialization for next() and next_back() and introduce them only for fold and for_each instead. This way internal iteration would still be optimized while external iteration would take the default implementation.

D)

Somehow extend TrustedRandomAccess to fix this case too? @steffahn hinted at some ideas in #85873 (comment) something a bit more concrete would help to fill in this option

[steffahn]

Yeah, I’m working on an approach that’ll need two or three additional methods for TrustedRandomAccess. Which probably means they’ll technically become methods of Iterator the same way that get_unchecked() does… Basically a method to be called before iteration from the back starts, and a method that can be called to emulate the side effects of next() after the iterator is depleted. I haven’t finalized the safety conditions around those, also performance impact will probably be a relevant question.

So the new methods would be just for triggering side effects. Maybe extra const …: bool flags would also help to avoid any (runtime) performance impact in case of no such side effects being present.

Such a proposal would also come with a list of recommendations/requirements around using these methods (not safety requirements) that you’ll need to follow if you want to not mess with an iterator’s order of side effects.

Two or three, because a third method to be called before iteration from the front starts could be added for symmetry and e.g. to make Rev implement TrustedRandomAccess – in case you’re wondering how that works: I’d also disallow the usage of next_back() after get_unchecked. But I haven’t thought this through in detail yet, does this also need a forth method corresponding to next_back on an empty iterator?

I’d expect to be able to provide more details within 2 weeks.

[the8472]

Interesting. Another idea I want to toss in, it might help to pass the nominal direction of iteration with each call to get_unchecked. This may not be relevant for current issues but it could help with implementing TRA for StepBy.

Which probably means they’ll technically become methods of Iterator the same way that get_unchecked() does…

While technically this is fine it seems aesthetically unpleasing. I wonder if we can somehow bundle this stuff into a helper trait/struct/associated type to separate it more cleanly. Or maybe we should use multiple impl blocks and collect them into a separate file. I think it'll make future improvements easier.

[the8472]

Updated, please check if the change is now sufficiently small.

It should still solve #82303 by saying that the 1st iterator is advanced zero or one times without touching the guarantee about the 2nd iterator.

[steffahn]

If the zipped iterator has no more elements to return[,] then each further attempt to advance it will […] not call next on the second iterator.

This is not correct. This only applies to the case where the first iterator has no more elements. If the first iterator is non-empty but the second one is empty, of course next will (or at least can) be called on the second iterator.

[the8472]

Right, how about?

    /// If the zipped iterator has no more elements to return then each further attempt to advance
    /// it will first try to advance the first iterator at most one time and if it still yielded an item
    /// try to advance the second iterator at most one time.

[dtolnay]

@bors r+ rollup

[bors]

📌 Commit 2ff677d has been approved by dtolnay