u16 format precision change, breach of the Rust stability guarantee

[ijackson]

Hi. I have been following #136932, where format! is changed to panic on widths and precisions of more than 16 bits. (This is for performance reasons, as it makes FormattingOptions much smaller.)

When this was originally proposed, a crater run was performed. This showed two regressions:

• vasekp/stream-rust
• uutils/coreutils

Since then, we have also had more regressions reported:

• kitware ghostflow
• strftime-ruby
• object-store

This seems to show that this change is not merely a theoretical problem, but causes real regressions. The statement that it "affects almost no existing code" turns out to have been overly optimistic.

I appreciate the efforts to improve things, but it seems to me that this change:

• Breaches the Rust stability guarantee
• Is causing real regressions (5 reports, and it's been out in beta only, so far)
• Was FCP approved on the basis of an estimated impact which turns out to have been wrong.

I think the decison to merge #136932 should be reconsidered in the light of this new information.

What is the process for that?

[jieyouxu]

You can nominate this for library team (re-)discussion.
@rustbot label: I-libs-nominated

[jieyouxu]

Also FYI @m-ou-se

[m-ou-se]

Nominated this for @rust-lang/libs-api

[m-ou-se]

My (probably slightly biased) analysis:

Of those five cases listed above, three are only example strings in unit tests. In four all five cases, the author confirmed that they thought this change to Rust was acceptable.

Cases where this only broke an example string in a unit test:

• uutils/coreutils: The author agreed the change to rust is reasonable and was happy to change their test right away. I asked for their input before we made the change.
• kitware ghostflow: The author wrote: "It is test code, so not too much to shed tears over."
• object-store: Found in the most recent crater run. Author agreed we shouldn't roll the change back.

Other cases:

• vasekp/stream-rust: This actually uncovered a subtle bug and the author was happy to update their code. I asked for their input before we merged the change.
• strftime-ruby: This was found right after merging. I checked with the maintainers that the change was acceptable, and they actually thanked us for working on improving the performance of formatting in Rust.

All the broken tests had subtly non-portable code: on a 16-bit platform, the widths would be silently truncated resulting in wrong outputs and failed tests. The breaking change is fixing that bug, by only guaranteeing 16 bits which we know exists on all targets.

This all seems well within the range of acceptable to me. Especially because we changed a potential silent subtle bug (even if only on small platforms) to a clear error.

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting. We agreed with @m-ou-se's report on the issue, namely that all the affected crates were fine with this change. Based on this, we're planning to continue with this change.