Skip to content

Add tests for dynamic width specifiers exceeding u16::MAX #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 21, 2025

Conversation

lopopolo
Copy link
Member

@lopopolo lopopolo commented Mar 20, 2025

Rust PR rust-lang/rust#136932 (part of rust-lang/rust#99012) limited format string width and precision to 16 bits, causing panics when dynamic padding exceeds u16::MAX.

These tests validate handling excessively large widths discovered via fuzzing in artichoke/strftime-ruby. They ensure correct, panic-free behavior consistent with CRuby's Time#strftime.

Additionally add tests for width specifiers which exceed INT_MAX to ensure they return the formatting string verbatim, which were among the cases discussed in the upstream PR.

This does not yet fix the panics when using padding widths > u16::MAX. Would love ideas on the best approach to take for that.

See:

Rust PR rust-lang/rust#136932 (part of rust-lang/rust#99012) limited
format string width and precision to 16 bits, causing panics when
dynamic padding exceeds `u16::MAX`.

These tests validate handling excessively large widths discovered via
fuzzing in artichoke/strftime-ruby. They ensure correct, panic-free
behavior consistent with CRuby's `Time#strftime`.

Additionally add tests for width specifiers which exceed `INT_MAX` to
ensure they return the formatting string verbatim, which were among the
cases discussed in the upstream PR.

See:

- Upstream report: rust-lang/rust#136932 (comment)
- Proposed fix: rust-lang/rust#136932 (comment)
@lopopolo lopopolo added A-formatter Area: strftime formatting. A-parser Area: strftime parser. labels Mar 20, 2025
@lopopolo lopopolo requested a review from x-hgg-x March 20, 2025 17:10
@lopopolo
Copy link
Member Author

the code coverage job is panicking because it uses nightly which has the new, panicking core::fmt::Argument::from_usize

Since the root cause was identified, the additional tests are more exhaustive and clearer to read
@x-hgg-x
Copy link
Collaborator

x-hgg-x commented Mar 21, 2025

There are 2 cases to solve when we have a width between u16::MAX and INTMAX:

  • For left padding, we can format with a width of u16::MAX then append the difference.
  • For right padding, we need to store the formatting result without a width in a temporary buffer, calculate and write the padding then write the formatted value.

I will try to make a PR implementing this.

@m-ou-se
Copy link
Contributor

m-ou-se commented Mar 21, 2025

Does this help? m-ou-se@width

(Just posted that on rust-lang/rust#136932 (comment))

@x-hgg-x
Copy link
Collaborator

x-hgg-x commented Mar 21, 2025

Does this help? m-ou-se@width

(Just posted that on rust-lang/rust#136932 (comment))

Yes, this is a good base.

@x-hgg-x
Copy link
Collaborator

x-hgg-x commented Mar 21, 2025

Published #175 based on this commit.

@lopopolo
Copy link
Member Author

Does this help? m-ou-se@width

(Just posted that on rust-lang/rust#136932 (comment))

thanks for the support @m-ou-se! and thanks so much for driving the perf improvements on the format machinery, the tracking issue is super chunky.

@lopopolo lopopolo force-pushed the dev/lopopolo-rust-fmt-args-u16 branch from 549a61e to 5330aec Compare March 21, 2025 22:29
@lopopolo
Copy link
Member Author

@x-hgg-x just did a slight amendment to add @m-ou-se as a co-authored-by on the commit from #175.

If this looks good to you, I'll prep and publish a release.

@x-hgg-x
Copy link
Collaborator

x-hgg-x commented Mar 21, 2025

There is still a compilation failure when running cargo +1.76 test --no-default-features.

`f64::abs` only recently became available in `no_std` crates:

- rust-lang/rust#131304
@lopopolo
Copy link
Member Author

There is still a compilation failure when running cargo +1.76 test --no-default-features.

fixed in 0be636c by bumping MSRV

@lopopolo
Copy link
Member Author

On branch trunk:
coveragePercent = 100.0
linesCovered = 1784
linesMissed = 0
linesTotal = 1784

On PR artichoke/strftime-ruby#172:
coveragePercent = 99.79
linesCovered = 1907
linesMissed = 4
linesTotal = 1911

code coverage job is failing because some of the more detailed assert!s that got added are formatted so the macro spans multiple lines. codecov doesn't like that and treats the assert messages as uncovered. Seems fine.

@lopopolo lopopolo merged commit d901bfa into trunk Mar 21, 2025
12 of 13 checks passed
@lopopolo lopopolo deleted the dev/lopopolo-rust-fmt-args-u16 branch March 21, 2025 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-formatter Area: strftime formatting. A-parser Area: strftime parser.
Development

Successfully merging this pull request may close these issues.

3 participants