Add Iterator::intersperse

[camelid]

This is a rebase of #75784. I'm hoping to push this past the finish line!

cc @jonas-schievink

[rust-highfive]

r? @joshtriplett

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

[camelid]

Squashed.

[camelid]

r? @LukasKalbertodt (you're the original reviewer, feel free to re-assign)

[m-ou-se]

r? @m-ou-se

[lukaslueg]

Two cents: We could lift the I::Item: Clone bound by providing a method to the tune of Iterator::intersperse_with(seperator_gen: impl Fn() -> Self::Item). That way, we can do intersperse_with(|| Ok(1)), without requiring that Result<T, E> to be Clone, removing the Clone-bound from Intersperse. Only Iterator::intersperse has a bound then and becomes a shorthand which clones the given value over and over again internally.

---

Having thought about it again, I think we'd need a separate Iterator::intersperse_with() -> IntersperseWith, as Iterator::intersperse would have to need a return type akin to Intersperse<Self, impl FnMut -> Self::Item>, which we can't do in trait implementations as of now. Since intersperse.rs is small, the duplication wouldn't hurt. iter::Repeat and ::RepeatWith are precedent. Poke me for a PR if there is interest.

[CraftSpider]

What would the opinion be on making it support fn intersperse<T: Into<Self::Item>>(self, separator: T)? (Please tell me if this would better go in the tracking issue/zulip)

[m-ou-se]

The signature for this function is a good question. Since it only clones the separator, a reference to it would suffice. However, that'd make it a bit more complicated in usage, as it'd always be borrowing something, making it a bit tricky with the lifetime in some situations.

The most flexible signature would be that of intersperse_with (which I believe is added in itertools 0.10). If we also have that function, then intersperse itself is simply a more specific version of that function, just for one common case. So in that case, I'd be less worried about picking the perfect/most flexible signature for intersperse. With intersperse_with as the generic/flexible alternative, it makes sense for intersperse to just be the simplest version. It'd also avoid some breakage if it has the exact same signature as in itertools.

[lukaslueg]

I don't want to derail this, as there has already been quite some discussion before in #75784 and related. To reiterate, I just think the Clone-bound is quite harsh for the general case and especially iterators that do not iterate over immutable references. An intersperse_with would lift this restriction and also address #79479 (comment), as the closure can do the required conversion without complicating the signature of intersperse_with (akin Into). Afaics, an implementation of intersperse can't reuse the machinery of intersperse_with, as we'd have to generate a closure which does the cloning and becomes part of the return type, which we can't do in a trait. However, there is precedent in Repeat and RepeatWith for exactly this kind of situation.

To the tune of

Iterator::intersperse_with<G>(self, separator_gen: G) -> IntersperseWith<Self, G> where Self:Sized, G: FnMut() -> Self::Item
Iterator::intersperse(self, separator: Self::Item) -> Intersperse<Self> where Self:Sized, Self::Item: Clone

[camelid]

Since it only clones the separator, a reference to it would suffice. However, that'd make it a bit more complicated in usage, as it'd always be borrowing something, making it a bit tricky with the lifetime in some situations.

Could we use Borrow<Self::Item> instead?

[lukaslueg]

Borrow is more strict than AsRef and AsRef<Self::Item> probably suffices. However, I'd suggest not to have a generic parameter on intersperse in case we can have intersperse_with, because indirection can be done there, and intersperse's signature remains as simple and straightforward as possible.

[camelid]

I haven't added a function to libs before, so I'm fine with whatever is agreed upon as long as it's reasonable :)

[lukaslueg]

Imho the intersperse as it is now - with minimal signature - has merits; I didn't want to derail this PR, as extensive discussion seems to have taken place. I'd take a look at implementing intersperse_with as mentioned above to accommodate more elaborate use-cases (like Borrow/Into), unless @camelid wants to.

[lukaslueg]

@m-ou-se love poke

[m-ou-se]

Sorry for the delay. This looks good to me. r=me with the merge conflict resolved.

---

One comment: Should Intersperse implement FusedIterator if the underlying iterator implements it? (This can be added later, so doesn't need to be resolved now.)

[m-ou-se]

I'd take a look at implementing intersperse_with as mentioned above

Nice, I'd love to see a PR for that. :)

[camelid]

One comment: Should Intersperse implement FusedIterator if the underlying iterator implements it? (This can be added later, so doesn't need to be resolved now.)

That seems reasonable. Like you said, though, we can always add it later. I'll defer to you :)

[m-ou-se]

@bors r+

[bors]

📌 Commit 40bbb7f has been approved by m-ou-se

[pickfire]

Should there be a doc alias to join since it is similar?