printf: Consistently handle negative widths/precision #7620

sargas · 2025-03-31T04:06:16Z

This pull request fixes the tests/printf/printf.sh tests. The main discrepancies between the uutil's implementation and GNU's appear to be the handling of negative widths or precisions when provided for %*.* - this was implemented for strings, but this PR brings other format specifications to also handle negative widths/precisions.

This PR also allows character constants with " instead of '. I had trouble finding any documentation about this; as far as I can tell GNU printf treats " and ' the same in the situations tested.
There is also a bug fix for interpolated values with %b to use \0XXX notation for octal bytes - this comes in for something like printf '\0001', which should be the null byte and an ASCII '1', vs printf '%b' '\0001', which is the single 0x01 byte because interpolated values use \0XXX formatted octal bytes.

github-actions · 2025-03-31T04:49:44Z

GNU testsuite comparison:

Congrats! The gnu test tests/printf/printf is no longer failing!

github-actions · 2025-03-31T06:18:27Z

GNU testsuite comparison:

Congrats! The gnu test tests/printf/printf is no longer failing!

drinkcat · 2025-03-31T07:48:42Z

src/uucore/src/lib/features/format/argument.rs

+        };
+        match next {
+            FormatArgument::SignedInt(n) if !n.is_negative() => *n,
+            FormatArgument::Unparsed(s) if !s.starts_with('-') => {


!s.starts_with('-') Can the string start with spaces? I think you should probably parse first...

Actually, you might be better off moving that logic to resolve_asterisk_precision, I feel it'd be easier to handle there.

Oh I see, you even want to handle negative values < i64::MIN... Still need to figure out how to handle leading white spaces though (and please add a test for that ,-P)

Well, GNU coreutils doesn't handle such large values, so maybe we don't have to either:

env printf "|%.*d|" " -7777777777777777777777777" "10" |printf: ‘ -7777777777777777777777777’: Numerical result out of range 10|

Interesting, I didn't realize GNU printf gave that error for significantly negative precision's - actually, it seems like I can remove get_i64_non_negative and still handle the case in the GNU tests with a precision of -2147483649.

Ignoring whitespace is unexpected for me, I don't think that's universal or documented in man printf? Either way, this is now tested.

Ignoring whitespace is unexpected for me, I don't think that's universal or documented in man printf? Either way, this is now tested.

Yeah, there are a lot of undocumented corner cases, especially when it comes to parsing/formatting. I'm still new here, but I think the idea is to be as compatible as possible with GNU coreutils (unless GNU behavior cannot really be justified as "correct"), that means reading the manual, but also feeding creative inputs to the GNU implementation and seeing how it behaves.

Thanks for fixing and testing that.

drinkcat · 2025-03-31T07:58:04Z

tests/by-util/test_printf.rs

+        .succeeds()
+        .stdout_only("|0|");
+
+    new_ucmd!()


Mind adding a comment here that explains that negative widths get aligned to the left? It's a bit tricky to see.

Sure, just pushed (also bumped the width to 3 to make it a little more visible

github-actions · 2025-04-02T06:15:20Z

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/printf/printf is no longer failing!

drinkcat · 2025-04-03T10:22:15Z

src/uucore/src/lib/features/format/spec.rs

@@ -349,7 +349,7 @@ impl Spec {
            Self::EscapedString => {
                let s = args.get_str();
                let mut parsed = Vec::new();
-                for c in parse_escape_only(s.as_bytes(), OctalParsing::default()) {
+                for c in parse_escape_only(s.as_bytes(), OctalParsing::ThreeDigits) {


Just to make sure, this doesn't break stuff like this right?

env printf "%b" "\\05llo" | hexdump -C 00000000 05 6c 6c 6f |.llo| env printf "%b" "\\5llo" | hexdump -C 00000000 05 6c 6c 6f |.llo|

It doesn't break it (this is with GNU coreutils 9.6 in /bin):

$ ./target/debug/printf '%b' "\\05llo"|od -c 0000000 005 l l o 0000004 $ ./target/debug/printf '%b' "\\5llo"|od -c 0000000 005 l l o 0000004 $ /bin/printf '%b' "\\05llo"|od -c 0000000 005 l l o 0000004 $ /bin/printf '%b' "\\5llo"|od -c 0000000 005 l l o 0000004

I added some tests to show that octal characters are still accepted at different lengths (1-3 numbers normally and 1-4 with leading zero for %b)

please avoid screenshot.
They are terrible for accessibility and search

Thank you for the reminder, I replaced the screenshot

drinkcat · 2025-04-03T10:24:18Z

I think this is mostly ok (just one small question on %b handling).

github-actions · 2025-04-03T12:22:17Z

GNU testsuite comparison:

Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/printf/printf is no longer failing!

drinkcat · 2025-04-03T13:07:23Z

LGTM thanks! I don't think the cargo clippy issues are your fault... (#7640)

sargas · 2025-04-03T13:55:23Z

Thanks for the reviews! Yeah, although I guess the clippy warnings are a good way to find out there's new rust version :)

github-actions · 2025-04-04T03:44:24Z

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/printf/printf is no longer failing!

sylvestre · 2025-04-04T07:56:06Z

Congrats! The gnu test tests/printf/printf is no longer failing!

impressive, bravo!

Copilot

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

sylvestre · 2025-04-04T07:57:13Z

src/uucore/src/lib/features/format/num_parser.rs

@@ -459,11 +459,11 @@ fn parse(

    // If nothing has been parsed, check if this is a special value, or declare the parsing unsuccessful
    if let Some((0, _)) = chars.peek() {
-        if integral_only {
-            return Err(ExtendedParserError::NotNumeric);
+        return if integral_only {


Sweet
this might be a new clippy warning
cc @samueltardieu ;)

Didn't redundant_else_block trigger on the original code (this covers if { …; return …; } else { … })?

Concerning the lint you're proposing, this could be a new lint indeed, see rust-lang/rust-clippy#14545 (I haven't found an existing proposal, maybe this is a duplicate).

My IDE actually warned about it, I didn't know it wasn't already in clippy: https://www.jetbrains.com/help/inspectopedia/RsLift.html#locating-this-inspection

sylvestre · 2025-04-04T07:58:35Z

src/uucore/src/lib/features/format/spec.rs

@@ -484,6 +490,21 @@ fn resolve_asterisk_maybe_negative<'a>(
    }
 }

+fn resolve_asterisk_precision<'a>(


would be nice to add a comment explaining what this function is doing
and write unit tests (if not too complex)

Sure thing, added doc and unit tests. Thanks for the review!

Also allows character constants with " instead of ', and for interpolated values with %b to use \0XXX notation for octal bytes

github-actions · 2025-04-05T07:38:36Z

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/printf/printf is no longer failing!

sargas force-pushed the pass-printf-test branch from 528d1a2 to f56ad61 Compare March 31, 2025 04:14

sargas force-pushed the pass-printf-test branch from f56ad61 to 43310fe Compare March 31, 2025 05:40

drinkcat reviewed Mar 31, 2025

View reviewed changes

sargas force-pushed the pass-printf-test branch from 43310fe to 5e0c64c Compare April 2, 2025 05:40

drinkcat reviewed Apr 3, 2025

View reviewed changes

sargas force-pushed the pass-printf-test branch from 3df883d to 19c2315 Compare April 3, 2025 11:47

sargas force-pushed the pass-printf-test branch from 19c2315 to 690ff8f Compare April 4, 2025 03:08

sylvestre requested a review from Copilot April 4, 2025 07:56

Copilot AI reviewed Apr 4, 2025

View reviewed changes

sylvestre reviewed Apr 4, 2025

View reviewed changes

sargas added 3 commits April 5, 2025 02:02

printf: Consistently handle negative widths/precision

95e5396

Also allows character constants with " instead of ', and for interpolated values with %b to use \0XXX notation for octal bytes

printf: add tests for different octal lengths

6061b67

uucore/format: add docs and tests for resolve_astrick_ methods

64ce76b

sargas force-pushed the pass-printf-test branch from 690ff8f to 64ce76b Compare April 5, 2025 07:03

sylvestre merged commit 3971bb3 into uutils:main Apr 5, 2025
67 of 68 checks passed

BrewTestBot mentioned this pull request May 24, 2025

uutils-coreutils 0.1.0 Homebrew/homebrew-core#224645

Merged

moonfruit mentioned this pull request May 26, 2025

uutils-selected 0.1.0 moonfruit/homebrew-tap#243

Closed

Uh oh!

printf: Consistently handle negative widths/precision #7620

printf: Consistently handle negative widths/precision #7620

Uh oh!

Conversation

sargas commented Mar 31, 2025

Uh oh!

github-actions bot commented Mar 31, 2025

Uh oh!

github-actions bot commented Mar 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sargas Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drinkcat commented Apr 3, 2025

Uh oh!

github-actions bot commented Apr 3, 2025

Uh oh!

drinkcat commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sargas commented Apr 3, 2025

Uh oh!

github-actions bot commented Apr 4, 2025

Uh oh!

sylvestre commented Apr 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 5, 2025

Uh oh!

Uh oh!

Uh oh!

sargas Apr 3, 2025 •

edited

Loading

drinkcat commented Apr 3, 2025 •

edited

Loading