Revert "alloc: implement FromIterator for Box<str>"

[estebank]

This reverts commit c92c228 from #99969.

Given the regressions caused by #127343, I believe we need to seriously consider reverting the change. I'm opening this PR in order to have a focal point for the appropriate teams to have that conversation.

This revert can be temporary, while we find a less disruptive way for landing this change. If we were to merge this, it should be backported to 1.81, and I would argue it would warrant a 1.80.2 dot-release.

@rust-lang/libs-api @rust-lang/libs @rust-lang/compiler @rust-lang/lang @rust-lang/release

[rustbot]

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Urgau]

This was extensively discussed in yesterday T-libs-api meeting (with some member of T-release):

The point of reverting the change was discussed:

Mara: In terms of decisions: 1. do we want to roll this back? release a point release? I think no?

Josh: No.

Jubilee: No.

Unanimous decision: Not reverting.

[tgross35]

I think this needs to be seriously considered; we are weighing a trivial implementation against a PR nightmare that seems to be just getting started.

Are there really any downsides of reverting, outside of breaking a minuscule number of crates that may have started to use the feature? Giving Nix and the rest of the ecosystem a chance to fix things while we ease into this better with new lints seems exceptionally worthwhile, as well being a good faith show that Rust cares about its impacts on the broader community (I think many of us have had to argue that Rust is stable enough for industry use at some point, it makes a difference). I really haven't seen any argument against reverting for the time being outside of inertia.

[the8472]

PR nightmare

Isn't it mostly a few cargo updates, which are more of a routine thing?

Looking through the pingbacks in #127343 I even found FTB-Gamepedia/ftb-rs@18a730d which is called "Routine update" 😄

[estebank]

Isn't it mostly a few cargo updates, which are more of a routine thing?

@the8472 Besides the thousands of wasted hs doing this, this was us deciding for our users when they need to deal with a (usually transitive) dependency update. But sometimes people cannot update time because it is a transitive dependency for a crate they can't update for other reasons. I haven't spoken to a single Rust developer that wasn't impacted by this.

TBQH, Linus Torvalds "WE DO NOT BREAK USERSPACE" rant has been rattling in my head for the past week.

[tgross35]

PR nightmare

Isn't it mostly a few cargo updates, which are more of a routine thing?

I meant PR as public relations, not pull request. Mostly talking about the optics of us breaking nix, articles, other community concern, and the general developer expectation being broken.

Indeed the fix itself is easy.

Looking through the pingbacks in #127343 I even found FTB-Gamepedia/ftb-rs@18a730d which is called "Routine update" 😄

There is also a "Pin Rust version to 1.79.0", which is not what we want to encourage.

[cuviper]

I don't know why so many teams need to be nominated, but clearly this is an API review in the end.

r? libs-api

[m-ou-se]

they need to deal with a (usually transitive) dependency update.

time has been fixed in a minor release, so in nearly all cases this only needs updating time in the final Cargo.lock file. It doesn't need any of your dependencies to change their Cargo.toml or Cargo.lock file or anything like that.

The only cases where this resulted in serious breakage, is where people use a frozen upstream Cargo.lock file, such as in those Nix packages. They are already discussing doing that differenthly though. (See NixOS/nixpkgs#333702)

TBQH, Linus Torvalds "WE DO NOT BREAK USERSPACE" rant has been rattling in my head for the past week.

That is not and has never been our policy, though. We regularly make minor breaking changes. Just see how many other items are mentioned in the "Compatibility Notes" section in the release notes. (That this change didn't end up in the release notes and blog was a big issue. But that's something we already discussed between the libs-api and release teams, and should not happen again.)

So, following RFC 1105, it's not a question of 'is this a breaking change?' but rather 'is the breakage too big?'. It's clear we need a better way of answering that question. By various metrics, this didn't look like a bigger breaking change than some past breaking changes. Apparently those were not the right metrics. Also, it's now clear there are users (e.g. NixOs) where "just run cargo update" isn't that simple, which means we should look at these decisions differently in the future.

I believe we need to seriously consider reverting the change

Reverting a stable addition is a significant breaking change. A major breaking change, by RFC 1105. Anybody who started using the new FromIterator impls since the release of 1.80.0 would be broken by this revert, and the solution won't just be "cargo update".

[estebank]

I don't know why so many teams need to be nominated, but clearly this is an API review in the end.

libs-api/libs is clearly the one that has the last word on whether to revert this change, and as seen on the minutes and the reply above, that is not going to happen.

Mentioned compiler and lang because any work to mitigate this issue going forward is going to require design and implementation work to try and reduce the impact of this category of changes in general, and this change in particular.

Mentioned release because whatever we did was gonna impact them.

---

That is not and has never been our policy, though. We regularly make minor breaking changes.

I am aware. Yet I am concerned because 5500 affected crates is orders of magnitude beyond what I would have considered minor breaking changes.

Just see how many other items are mentioned in the "Compatibility Notes" section in the release notes.

Yes, from that list you can see that I approved #125596, which removed the proc_macro_back_compat hack, which affected 11 crates that haven't been touched in 3 years. The situation with time was not that. Yet the existence of that hack in the first place shows an earlier willingness to avoid breaking people, going as far as having something akin to quirks mode in the compiler, precisely to avoid breaking big parts of the ecosystem.

Reverting a stable addition is a significant breaking change. A major breaking change, by RFC 1105. Anybody who started using the new FromIterator impls since the release of 1.80.0 would be broken by this revert, and the solution won't just be "cargo update".

I'm not trying to force a revert. I'm trying every avenue to mitigate the impact of this and other changes like this one:

• the original change could have been delayed in the first place to give the ecosystem time to organically move to the new time version, I still fail to understand how a single release cycle was considered reasonable
• pushing on the recurring "we should do that" task of teaching cargo about breaking changes, either by implementing Cargo.toml linting, or a mechanism similar to RustSec to have it ready for the next time we do something like this
• I'm modifying the error telling people to update their time when encountering this error in particular (I insist this should have been part of 1.80 and I consider it a major issue we didn't have even this minimal level of support for our users, let alone not mentioning anything in the release notes)
• I'm writing a lint to detect problematic <T as Into::into(_) cases (similar to clippy::useless_conversion, which is unsuitable because the clippy version doesn't account for cfgd items at all) in order to verify how many other mines are waiting for us to step on
• we need to figure out a different mechanism than "pestering your dependencies' authors to update their dependencies" when the number of crates with unreachable authors will do nothing but continue to climb. The idea of "cargo provided patches" is one we need to seriously consider.

I am concerned that we are having a too cavalier attitude towards breaking changes, or that we are not all in the same page on what level of strife we're willing to put our users through. Inference issues in particular are painful, we can't freeze APIs in ember just because it might break one random crate. But I am no longer going to be telling people "you can always build your older projects with the latest rust version, so update away".

I have to say, I am disappointed at the timeline of this change (not giving users time to update their time, closing the regression ticket immediately as soon as there was a patch for time with no additional follow up to mitigate the impact) and I am further disappointed by the framing in the minutes. Statements implying that because "all the paperwork was in order" no error was made is rules-lawyering. Qualifying the reaction to the pain we inflicted on our users as "a mob" is missing the mark. Comparing this breakage with the breakage caused by the introduction of new lints, which do have a cap mechanism that causes them not to affect users' dependencies, only their own code, which they are able to modify. I felt compelled to open this PR and am working on others because there was radio silence on what the mitigation steps for this were and talking with others that also didn't know what was going on. The fact that I found out about the regression through social media because people were having issues. Seeing that it's been a month since the release came out, yet we still don't have a single post on blog.rust-lang.org to tell our users what the hell happened.

[m-ou-se]

the original change could have been delayed

Yes, if we could go back in time that's what I think should have happened. I agree mistakes were made.

"pestering your dependencies' authors to update their dependencies"

The time crate was patched in a minor version update. You do not need to pester your dependencies' authors at all. This is a matter that is fixed in your own Cargo.lock file. The Cargo.lock files of your dependencies are entirely irrelevant, regardless of how deep the time dependency is.

This problem only exists for those who use frozen Cargo.lock files from upstream that they don't maintain themselves, which is apparently how NixOs' packaging works.

Also note that even in the NixOs thread about this, it's made clear that they don't actually need upstream patches for unmaintained crates: NixOS/nixpkgs#332957 (comment) (And they are discussing having their own Cargo.lock: NixOS/nixpkgs#333702)

disappointed by the framing in the minutes

If I could type faster I might have been able to transcribe all the subtle nuances of what everyone said, but I'm afraid you'll have to do with my rough paraphrasing in the notes.

Statements implying that because "all the paperwork was in order" no error was made is rules-lawyering.

No, the point was that we apparently we need policy changes. There was a mistake, but we didn't make an error in following our policy, so apparently we need to change the policy. E.g. the way we do crater runs and how we communicate with the release team. We are making those changes.

Comparing this breakage with the breakage caused by the introduction of new lints,

We compared it to changes to the socket types, which were far more subtle and dangerous breaking changes: those changes can result in the affected crates having silent undefined behaviour, not just a compilation error.

Yet I am concerned because 5500 affected crates is orders of magnitude beyond what I would have considered minor breaking changes.

We have had other breakages with thousands of affected crates, where it was considered acceptable because all the root regressions had updates published that would be picked up on a simple cargo update. Those changes went through release just fine. And in this case, there was basically one root regression: the time crate.

I'm not saying that this time it was fine. It was clearly not. I'm saying that we clearly need a different metric than just 'total number of (indirectly) affected crates' or 'total number of root regressions'.

It'd be lovely if we had a better (documented) way in the future to deal with such 'minor but big' breakages, perhaps through a lang/compiler/cargo feature to warn people or mitigate the issue.

But I am no longer going to be telling people "you can always build your older projects with the latest rust version, so update away"

But you still can; that didn't change. Even in old Rust projects, you can still just "update away". Just make sure to not only run rustup update but also cargo update, as usual.

[Amanieu]

In the libs-api meeting we all agreed that this should have been reverted as soon as the extent the breakage became evident, before it hit stable. The question of what to do now that this has hit stable is more complicated since this revert is itself a breaking change which can in theory break stable code.

I have not personally been affected by this issue, so I can only rely on second-hand information to determine the impact that this has had, and more importantly that this can continue to have in the future. The impression that we got during the meeting was that most of the impact has already been mitigated by users upgrading the time crate and that there would be little value in making a second breaking change to revert the first one.

However I am 100% willing to support reverting this and making a patch release if this information is incorrect and there is still value in mitigating the breakage from this change. Obviously this should be checked to ensure that no crate has started relying on the new impls.

[jhpratt]

As the maintainer of time, I see no reason to revert at this point in time. Had the true scale been noticed in the manner it should have been before it hit stable, of course it should've been deferred a short while; I don't think anyone disagrees with that. But now that it's on stable, there is a stability guarantee provided that should not be walked back.

Personally, I think the biggest issue is that there wasn't sufficient time for downstream users to "naturally" upgrade their deps, Thankfully David submitted a PR with the extraneous line removed, but didn't give any context behind how it was found. That is, on its own, fine! It's not strictly relevant to me as the maintainer of the crate. But had I known the issues it could have caused, I'd have released it immediately rather than waiting to bundle it with other release-worthy things. This resulted in a delay from March 15 (when it was merged) to April 10 (when it was released). It is a near certainty that some of the users could have had the issue avoided had I been notified of the ramifications (by anyone, not David specifically).

[estebank]

Given the consensus that reverting at this point would be more trouble than desired (which I can understand), I'm closing this PR. We must take care not to allow a common error mode that I've noticed in the project (including with myself): whenever we encounter a novel failure mode, we brainstorm multiple mitigations so that it doesn't happen again, but then because there's no "owner" for designing/implementing/pushing them over the finish line, we end up not actually doing most of them, and then the problem happens a second time and we're caught with a wave of "yeah, I remember we discussed this". Let's try to avoid that for this error. I'm also noticing that there's been no official blogpost about this situation. Is there any desire in @rust-lang/libs-api to take the lead on doing so?

While closing this PR, I also want to cross-link to rust-lang/cargo#14452, which proposes a hot-patching mechanism in cargo itself, and https://internals.rust-lang.org/t/brainstorming-how-to-help-old-code-locked-to-old-time-build-on-new-toolchains/21438, which is a new thread to look for more alternative solutions.