Repository navigation
Stabilizing Iterator::intersperse breaks a large number of crates #88967
Description
Activity
Given #48552, rust-itertools/itertools#576 might solve this assuming it gets merged and released before 1.56 reaches stable.
Edit, wait... was the check for deprecated candidates in #48552 (comment) implemented? Doesn't look like it.
During yesterday's libs-api team meeting we discussed how we can stabilize intersperse without causing large amounts of breakage across the ecosystem. The plan we arrived upon is to first revert the stabilization and then plan on stabilizing the feature in a future release, @Mark-Simulacrum suggested 1.65.
Whenever we end up stabilizing it we need to have a plan for how we will avoid these same breakages. Suggestions so far include using build scripts or
#[cfg(accessible(...)]in itertools to detect the presence of intersperse instdand disable Itertools::intersperse when there would be a conflict.In a previous meeting we also discussed more long term solutions we could implement to avoid ambiguity breakages caused by introducing new methods like this. Suggestions in that meeting included:
- encoding the released std version in crates published to crates.io when uploading and ignore methods introduced in newer releases when building that published version
- annotating the edition in which new methods were added to de-prioritize them when ambiguity arises and emit a warning which indicates there is an ambiguity and that it will be promoted to a hard error in future editions.
Reacted by León Orell Valerian Liehr, Daniel Giger, Moja and Maximilian RoosWhenever we end up stabilizing it we need to have a plan for how we will avoid these same breakages.
I believe the accepted supertrait_item_shadowing RFC might provide a long-term solution to this kind of breakage.
Dropping from the 1.56 milestone -- no longer release sensitive.
How exactly is this method a breaking change? I looked through the comments here and in the tracking issue and couldn't find a minimal example showing the breakage of the API
Reacted by Caleb Stanford and Josh Junon@hamza1311 it causes ambiguous name resolution errors on files that already use the itertools intersperse method because now both the itertools and iterator traits are in scope and have a method with the same name, so the compiler goes from having one option to two options and produces an error because it can't pick arbitrarily from two options for which intersperse method to use.
The crater results mark linked in the top comment are where all the examples live. Here's one, scroll to the end to see the error
https://crater-reports.s3.amazonaws.com/beta-1.56-2/beta-2021-09-12/gh/Centril.consensusbot/log.txt
@yaahc wrote:
The plan we arrived upon is to first revert the stabilization and then plan on stabilizing the feature in a future release, @Mark-Simulacrum suggested 1.65.
Is this still the plan?
If
.intersperse()causes name resolution conflicts, the obvious thing to do is to rename. But why not also offer.join()which combines intersperse with collect (i.e., what users of this API actually want to do)?Renaming is a somewhat unsatisfying solution. That approach sets us up to have our own little smooshgate potentially every time a new method is added to the standard library. I am considering renaming every method in itertools to start with
it_, but that'll only be a partial solution. A considerable number of itertools users aren't on the latest release.RFC 2845 establishes a mechanism for reducing the breakage resulting from most of these situations, but requires implementation.
Reacted by Krish, cdbennett, Josh Junon and Tobias KernI don't see how it's analogous to smooshgate, because monkeypatching doesn't exist in Rust. itertools isn't using bad practice here.
There are plenty of reasonable alternative names like
.weaveor.interleave_item. How does choosing one of these set a bad precedent?Reacted by Jack Wrenn, Nicolas Abram, Dmitry Marakasov, Tobias Kern and lueIt is analogous in the sense that the respective standard library should not base its naming decisions on what downstream libs do. It's a violation of namespacing. That not fully qualified method resolution can lead to such conflicts is a known issue but it's better to resolve that rather than working around it.
Reacted by Caleb Stanford, Jon Gjengset, Marat Radchenko, Krish, Noa Bogart, cdbennett and Tobias KernI agree, but as a short-term solution renaming prevents this being blocked on what is a much bigger issue with Rust's namespacing. Otherwise it seems like this (and many similar stdlib improvements) are blocked for the near-term forseeable future, if I'm understanding the discussion correctly.
The RFC fixing the issue is already accepted and only needs to be implemented. Itertools provides that API for people that need it today, so there's no reason to rush.
Reacted by Jonas Platte, Bennet Bleßmann, Noa Bogart and cdbennett
The 1.56 crater run shows that there are at least 59 root crates (or repositories) broken by this change, and 260 crates.io crates along with 278 github repositories which depend on those.
See https://crater-reports.s3.amazonaws.com/beta-1.56-2/index.html > "regressed: root results" > E0034 for the root crates.
cc some threads with partial discussion:
Nominating for T-libs-api discussion as the FCP on #79524 seems to have been made without commentary about the expected level of breakage, so I suspect it was not discussed.