Deprecate Itertools::{intersperse, intersperse_with}

[tranzystorekk]

These probably need to be deprecated once Rust 1.56.0 has been released (rust-lang/rust#88548)

[fandahao17]

I guess they probably need to be removed because the conflict with the standard library version?

The following code errors on rustc 1.57.0-nightly (b69fe5726 2021-09-10).

use itertools::Itertools;

fn main() {
    itertools::assert_equal((0..3).intersperse(8), vec![0, 8, 1, 8, 2]);
}

The error is:

error[E0034]: multiple applicable items in scope
 --> src/main.rs:4:36
  |
4 |     itertools::assert_equal((0..3).intersperse(8), vec![0, 8, 1, 8, 2]);
  |                                    ^^^^^^^^^^^ multiple `intersperse` found
  |
  = note: candidate #1 is defined in an impl of the trait `Iterator` for the type `std::ops::Range<A>`
  = note: candidate #2 is defined in an impl of the trait `Itertools` for the type `T`
help: disambiguate the associated function for candidate #1
  |
4 |     itertools::assert_equal(Iterator::intersperse((0..3), 8), vec![0, 8, 1, 8, 2]);
  |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
help: disambiguate the associated function for candidate #2
  |
4 |     itertools::assert_equal(Itertools::intersperse((0..3), 8), vec![0, 8, 1, 8, 2]);
  |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[phimuemue]

Ideally I'd want to cause as little trouble as possible for users. So I guess in an ideal world, Itertools would check if std already offers intersperse - and if so, does not provide intersperse itself anymore. If this kind of problem is likely to occur more often in the future, this may be a good strategy in general. Otherwise not sure if we want to go that route, though. (Iirc there's one pull request containing this kind of logic, so we possibly would not have to do all the work from the beginning.)

Other ideas:

• Deprecate intersperse now, suggesting the free function as a workaround, and publish a new version. Once Rust 1.57 is available, remove intersperse.
• Just live with intersperse in Itertools. (Not my favorite.)
• Once Rust 1.57 is out, simply publish a new version without intersperse. (Not my favorite.)

@jswrenn, you usually have a good grasp about what is best here. Any thoughts? By the way, hope you're fine!

[VeaaC]

The easiest for user could be to check the Rust version (e.g. in a build script), and use conditional compilation (cfg) to remove intersperse

[jswrenn]

Historically, I believe what we've done is to remove the offending method upon the stabilization of the stdlib method, sometimes preceeded by a period of deprecation.

Looking at our next milestone, I don't see any breaking changes. So let's mark these functions as deprecated and out a semver-compatible release soon. We'll release 0.11.0 to coincide with the stabilization of Rust 1.57.

(And sorry I've been AWOL! I haven't been fine, but things are steadily getting better. Thank you for all your continued work on this crate.)

[SoniEx2]

... Just version-dependently #[cfg] it out, maybe with a build script? Assuming the Itertools one and the std one are API-compatible?

[jswrenn]

The stabilization of Iterator::{intersperse, intersperse_with} was postponed. There's some discussion on Zulip about how to proceed, including consideration of language-level solutions. The action we'll have to take here will depend on how Iterator::{intersperse, intersperse_with} is stabilized. So, we'll revisit this PR after those discussions progress a bit.

[bluss]

I agree that the build script/rustc version solution sounds like a good part way solution. Unfortunately it's quite part way since it doesn't fix the problem for those that are pinning an old version of itertools, use a lockfile or use a non-current version like 0.9.*.

If rust friends on Zulip and elsewhere decide that cfg(accessible) is the best way, then that would mean the build script/rust version solution is the second best way, though.

[tranzystorekk]

I'm considering closing this PR since there doesn't seem to be any plans of restabilizing Iterator::intersperse in the near future

[jhpratt]

The reason Iterator::intersperse hasn't had any progress towards re-stabilization appears to be because this PR hasn't been merged.

[tranzystorekk]

The reason Iterator::intersperse hasn't had any progress towards re-stabilization appears to be because this PR hasn't been merged.

Was this mentioned on zulip or some github discussion?

[tgross35]

The reason Iterator::intersperse hasn't had any progress towards re-stabilization appears to be because this PR hasn't been merged.

Was this mentioned on zulip or some github discussion?

I think this comes from rust-lang/rust#88967 (comment), suggesting that if rustc prioritized non-deprecated methods during resolution¹ and if this deprecation PR merges, then stabilization would be able to happen. I don't think "itertools is blocking stabilization of these methods" is at all accurate. More like, "if itertools has some way to yield to std's implementations when available, and (critically) this has time to soak into the ecosystem, then std can go ahead with stabilization of this particular thing without extra work".

"Extra work" meaning this rust-lang/rust#89151, which also resolves this issue in a much better way - but has no implementation.

If deciding in favor of moving something like this PR forward - could a better solution be to check for std::iter::Iterator::intersperse (such as with autocfg) and config the implementation based on that? Or perhaps mark it deprecated ony if that cfg is true? This would prevent some noise for anyone in gap of using the itertools implementation of this method, but std not having it stable yet.

Footnotes

1. based on the comments at the link, it isn't even clear whether this works, or would even be desired with https://github.com/rust-lang/rust/issues/89151 ↩

[jswrenn]

I've been considering our options lately (see further discussion on #702), and wrote out some of the history of the issue in a document: https://hackmd.io/@jswrenn/HyKLk_1Cn

I'm going to try to implement RFC2845. If that doesn't work out, Itertools will rename all of its methods to be prefixed with it_. Either way, this recurring name conflict issue will be resolved.

[tgross35]

@jswrenn was there an update to anything, or just closing out this dead issue?

[jswrenn]

Just cleaning house. We're unlikely to deprecate these methods until they've actually been stabilized (and even then, we might just push a new major release that just deletes these methods), and they're unlikely to be stabilized until RFC2845 is implemented.

Given the long timeline on this, it's safe to close this PR and revisit things once the situation changes.

[tgross35]

Agreed, thanks for clarifying!

[jadebenn]

I think it may be worth to revisit this given that the only reason the Rust feature has not stabilized is because the itertools methods cause a name collision. There's a small movement to explore an alternative name right now, which might avoid the issue if it gets anywhere, but it's worth pointing out that waiting for the official implementation to stabilize before depreciating is essentially putting the cart before the horse: The name conflict is why it has not stabilized.

[tranzystorekk]

is there a reason to rush this? as long as we provide intersperse it seems like a good solution for anyone needing this functionality

[jswrenn]

I'm in agreement with @tranzystorekk. A primary goal of itertools is to provide convenience for users, and removing intersperse and intersperse_with is contrary to that goal; it will be disruptive. We might be able to work around some of those disruptions with cleverly-timed releases and build-time rustc version detection, but that won't immediately pave the way to the stabilization of these methods. A majority of our dependents are not on the latest version train of itertools; we have a very long tail of users who are slow to update. If the library team stabilizes intersperse and intersperse_with, those users will still be instant-broken.

These sorts of conflicts between Itertools and Iterator have occured as long as Itertools has existed. The accepted RFC2845 provides a permanent, sustainable mechanism for resolving these conflicts. I'd very much like to see it implemented.

[jadebenn]

It's definitely the superior solution, but it doesn't look like there's been much interest.

[Philippe-Cholet]

@jswrenn
I fully read RFC 2845 and saw the associated rust issue where there is a concern about the prelude.

The trait Iterator is always in scope (because in the prelude, as it should).
So when we do use itertools::Itertools;, both iterator traits are (always) in scope. The problem is that the RFC says

However, when both subtrait and supertrait are brought into scope, the ambiguity remains.

Therefore because of the prelude, if the RFC is implemented as is, then one would not shadow the other but we would get a compiler error (resolvable by explicitely qualifying the method).

So the rust issue mentions implicit import (prelude) vs explicit import.

[jswrenn]

The RFC has some oddities in it and would almost certainly not be implemented as written. It is definitely intended to address this use case. Off the top of my head, I'd recommend a modification that doesn't alter resolution based on how implicit or explicit the import is, but rather solely on the super/sub trait relationship. The exact details should be worked out between the implementor and the lang team.