Iterator::cloned: document side effect behavior

[llogiq]

During discussing work on clippy's upcoming iter_overeager_cloned lint, I found that the fact that the cloned implementation never optimizes any clone call away because the clone implementation could contain an observable side effect is underdocumented in the Iterator::cloned method.

This PR adds a note to rectify this omission.

[rust-highfive]

r? @joshtriplett

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

[camelid]

Is it really worth making this guarantee? I could imagine a future compiler optimization (or perhaps specialization) that would eliminate clone calls for non–side effecting Clone implementors, such as Vec.

[llogiq]

We will soon have a clippy lint for this, so no, I don't think we should sacrifice backwards compatibility for a possible perf gain.

[the8472]

Maybe the clippy lint is the issue then?

[the8472]

Also, there is no backwards compatibility issue since the API contract currently does not promise this. It only becomes an issue once the guarantee is added.

[llogiq]

There is no "promise", but it has been observable behavior since 1.0.0. So if we were to change that, we better have a good idea if any code depends on it.

[llogiq]

Also why would the clippy lint be an issue?

[the8472]

I assume the clippy lint is implying something about the side-effects since it was brought up here and since the lint is motivating this change? The lint itself wasn't linked, so I am just guessing.

There is no "promise", but it has been observable behavior since 1.0.0.

observable behavior != stability guarantee

This PR turns what someone might hypothetically rely on into a guarantee, that's a significant change that paints us into a corner.

[llogiq]

I'm not wedded to this PR – however I'd like to have the (non-)guarantees documented. I'd swap this out with "you should not rely on the items' Clone impl being called for all items observed by the iterator" in a heartbeat if that's what we decide.

Leaving the observable behavior undocumented makes it harder to say "you shouldn't have relied on it" later.

[the8472]

#90209 intends to optimize away some clones here.

[the8472]

Additionally this PR (saying that cloning always is a side-effect) is inconsistent with RFC 1521

[llogiq]

The PR says "cloning is regarded as a side effect, becaue Clone implementations may contain observable behavior". This doesn't mean "cloning is always regarded as a side effect". So how is this inconsistent with RFC 1521? The RFC just specifies that cloning is side effect free for Copy types.

[llogiq]

OK, so apparently @m-ou-se thinks that cloning should not matter here. I'll close this if #90209 lands.

[the8472]

So how is this inconsistent with RFC 1521?

It allows us to turn .clone() calls, even when they contain side-effects, into copies via specialization. The doc comment says

For example iter.cloned().last() will clone all elements even if only the last is actually returned.

The will clone part wouldn't be true in that case.

[llogiq]

Let's take a step back and see what the problem is and what options we have to deal with it:

We see a performance footgun, in that elements may be needlessly cloned when e.g. doing iter.cloned().last(). However, the clone calls could include observable behavior that in theory users may rely on.

So we can:

1. Use a clippy lint to steer users to cloning as late as possible. Document the perf footgun & reasoning (i.e. this PR). The perf footgun is still there, but users will mostly know about and avoid it.
2. Optimize out the clones in general. This may break some software that depends on it, but they shouldn't have so all right I guess? Given that iterator specialization is a tricky business, I'm also afraid that we might get edge cases where the optimization fails to apply. We should then also document the lack of clones under certain conditions.
3. Do nothing.

Is my analysis here correct?

[the8472]

You have to consider code that clippy doesn't see, e.g. iterators returned by a function and then consumed by another function, possibly in another crate. Or generic code which takes T: Clone but some T may be Copy too.
On the other hand it is correct that specialization can't solve everything because the possible combinations of iterator adapters can be difficult to cover with specialization traits and of course there are 3rd-party adapters which we can't specialize.
The approaches can be complementary.

And neither requires a documentation change. Just because a clippy lint is added we can still reserve the possibility of performing additional optimizations. And just because an optimization is added we do not necessarily need to document it. We have many optimizations in the standard library which are best-effort and not guaranteed. Some of them are observable if you look hard enough.

There's also the second-order problem that if we add explicit non-guarantees to Cloned, then does this mean the absence of non-guarantees on other adapters hints that they will never do any optimization? Obviously that's nonsense, but every time we make something explicit it shifts expectations a little. So it might be better to have such documentation about side-effects in a more central place, e.g. on the core::iter module saying that some adapters may perform side-effect eliding optimizations based on the overall iterator-shape alone, which is more aggressive than what the compiler is allowed to do.

in that elements may be needlessly cloned when e.g. doing iter.cloned().last()

At least the more general case cloned().foo().last() can optimize out the unnecessary clones without specialization if Foo requires a DoubleEndedIterator because then it could unconditionally implement last() via next_back(). Or some adapter could require ExactSizeIterator and then implement count() as len() and not clone any items at all. So even today we can't say too much about which clones are guaranteed to happen since adapters can't be reasoned about in isolation.

[llogiq]

That last point is a good one. I agree that the documentation should be centralized, but am not sure whether the iter module doc is the right place for it. We'll mention it in the clippy lint docs anyway.

So I'm closing this for now. Hope that #90209 is successful. 👍