Skip to content

u16 format precision change, breach of the Rust stability guarantee #140250

Closed as not planned
Closed as not planned
u16 format precision change, breach of the Rust stability guarantee#140250
Labels
A-fmtArea: `core::fmt`A-stabilityArea: `#[stable]`, `#[unstable]` etc.C-bugCategory: This is a bug.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Description

@ijackson
ijackson
opened on Apr 24, 2025

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:

Since then, we have also had more regressions reported:

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:

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

What is the process for that?

Activity

  1. ijackson
    added
    C-bugCategory: This is a bug.
    regression-untriagedUntriaged performance or correctness regression.
    on Apr 24, 2025
  2. rustbot
    added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    needs-triageThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triaging
    on Apr 24, 2025
  3. jieyouxu
    added
    T-libsRelevant to the library team, which will review and decide on the PR/issue.
    on Apr 24, 2025

  4. jieyouxu commented on Apr 24, 2025

    @jieyouxu
    jieyouxu
    on Apr 24, 2025
    Member

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

  5. rustbot
    added
    I-libs-nominatedNominated for discussion during a libs team meeting.
    on Apr 24, 2025
  6. jieyouxu
    added
    A-stabilityArea: `#[stable]`, `#[unstable]` etc.
    A-fmtArea: `core::fmt`
    and removed
    needs-triageThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triaging
    on Apr 24, 2025

  7. jieyouxu commented on Apr 24, 2025

    @jieyouxu
    jieyouxu
    on Apr 24, 2025
    Member

    Also FYI @m-ou-se

  8. m-ou-se
    added
    I-libs-api-nominated[DEPRECATED; DO NOT USE]
    and removed
    I-libs-nominatedNominated for discussion during a libs team meeting.
    on Apr 24, 2025

  9. m-ou-se commented on Apr 24, 2025

    @m-ou-se
    m-ou-se
    on Apr 24, 2025
    Member

    Nominated this for @rust-lang/libs-api

  10. m-ou-se commented on Apr 24, 2025

    @m-ou-se
    m-ou-se
    on Apr 24, 2025
    Member

    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.

  11. joshtriplett commented on Apr 29, 2025

    @joshtriplett
    joshtriplett
    on Apr 29, 2025
    Member

    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.

  12. joshtriplett
    closed this as not plannedon Apr 29, 2025
  13. jieyouxu
    removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Apr 29, 2025
  14. apiraino
    removed
    I-libs-api-nominated[DEPRECATED; DO NOT USE]
    regression-untriagedUntriaged performance or correctness regression.
    on Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-fmtArea: `core::fmt`A-stabilityArea: `#[stable]`, `#[unstable]` etc.C-bugCategory: This is a bug.T-libsRelevant to the library team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions