printf: Fix extra padding (#6548)

Kev1n8 · web-flow · commit a77848fb830e · 2024-07-13T10:19:01.000+02:00
diff --git a/src/uucore/src/lib/features/format/num_format.rs b/src/uucore/src/lib/features/format/num_format.rs
@@ -5,6 +5,7 @@
 
 //! Utilities for formatting numbers in various formats
 
+use std::cmp::min;
 use std::io::Write;
 
 use super::{
@@ -77,22 +78,16 @@ pub struct SignedInt {
 impl Formatter for SignedInt {
     type Input = i64;
 
-    fn fmt(&self, mut writer: impl Write, x: Self::Input) -> std::io::Result<()> {
-        if x >= 0 {
-            match self.positive_sign {
-                PositiveSign::None => Ok(()),
-                PositiveSign::Plus => write!(writer, "+"),
-                PositiveSign::Space => write!(writer, " "),
-            }?;
-        }
+    fn fmt(&self, writer: impl Write, x: Self::Input) -> std::io::Result<()> {
+        let s = if self.precision > 0 {
+            format!("{:0>width$}", x.abs(), width = self.precision)
+        } else {
+            x.abs().to_string()
+        };
 
-        let s = format!("{:0width$}", x, width = self.precision);
+        let sign_indicator = get_sign_indicator(self.positive_sign, &x);
 
-        match self.alignment {
-            NumberAlignment::Left => write!(writer, "{s:<width$}", width = self.width),
-            NumberAlignment::RightSpace => write!(writer, "{s:>width$}", width = self.width),
-            NumberAlignment::RightZero => write!(writer, "{s:0>width$}", width = self.width),
-        }
+        write_output(writer, sign_indicator, s, self.width, self.alignment)
     }
 
     fn try_from_spec(s: Spec) -> Result<Self, FormatError> {
@@ -244,16 +239,8 @@ impl Default for Float {
 impl Formatter for Float {
     type Input = f64;
 
-    fn fmt(&self, mut writer: impl Write, x: Self::Input) -> std::io::Result<()> {
-        if x.is_sign_positive() {
-            match self.positive_sign {
-                PositiveSign::None => Ok(()),
-                PositiveSign::Plus => write!(writer, "+"),
-                PositiveSign::Space => write!(writer, " "),
-            }?;
-        }
-
-        let s = if x.is_finite() {
+    fn fmt(&self, writer: impl Write, x: Self::Input) -> std::io::Result<()> {
+        let mut s = if x.is_finite() {
             match self.variant {
                 FloatVariant::Decimal => {
                     format_float_decimal(x, self.precision, self.force_decimal)
@@ -272,11 +259,13 @@ impl Formatter for Float {
             format_float_non_finite(x, self.case)
         };
 
-        match self.alignment {
-            NumberAlignment::Left => write!(writer, "{s:<width$}", width = self.width),
-            NumberAlignment::RightSpace => write!(writer, "{s:>width$}", width = self.width),
-            NumberAlignment::RightZero => write!(writer, "{s:0>width$}", width = self.width),
-        }
+        // The format function will parse `x` together with its sign char,
+        // which should be placed in `sign_indicator`. So drop it here
+        s = if x < 0. { s[1..].to_string() } else { s };
+
+        let sign_indicator = get_sign_indicator(self.positive_sign, &x);
+
+        write_output(writer, sign_indicator, s, self.width, self.alignment)
     }
 
     fn try_from_spec(s: Spec) -> Result<Self, FormatError>
@@ -326,6 +315,18 @@ impl Formatter for Float {
     }
 }
 
+fn get_sign_indicator<T: PartialOrd + Default>(sign: PositiveSign, x: &T) -> String {
+    if *x >= T::default() {
+        match sign {
+            PositiveSign::None => String::new(),
+            PositiveSign::Plus => String::from("+"),
+            PositiveSign::Space => String::from(" "),
+        }
+    } else {
+        String::from("-")
+    }
+}
+
 fn format_float_non_finite(f: f64, case: Case) -> String {
     debug_assert!(!f.is_finite());
     let mut s = format!("{f}");
@@ -501,6 +502,47 @@ fn strip_fractional_zeroes_and_dot(s: &mut String) {
     }
 }
 
+fn write_output(
+    mut writer: impl Write,
+    sign_indicator: String,
+    mut s: String,
+    width: usize,
+    alignment: NumberAlignment,
+) -> std::io::Result<()> {
+    // Take length of `sign_indicator`, which could be 0 or 1, into consideration when padding
+    // by storing remaining_width indicating the actual width needed.
+    // Using min() because self.width could be 0, 0usize - 1usize should be avoided
+    let remaining_width = width - min(width, sign_indicator.len());
+    match alignment {
+        NumberAlignment::Left => write!(
+            writer,
+            "{sign_indicator}{s:<width$}",
+            width = remaining_width
+        ),
+        NumberAlignment::RightSpace => {
+            let is_sign = sign_indicator.starts_with('-') || sign_indicator.starts_with('+'); // When sign_indicator is in ['-', '+']
+            if is_sign && remaining_width > 0 {
+                // Make sure sign_indicator is just next to number, e.g. "% +5.1f" 1 ==> $ +1.0
+                s = sign_indicator + s.as_str();
+                write!(writer, "{s:>width$}", width = remaining_width + 1) // Since we now add sign_indicator and s together, plus 1
+            } else {
+                write!(
+                    writer,
+                    "{sign_indicator}{s:>width$}",
+                    width = remaining_width
+                )
+            }
+        }
+        NumberAlignment::RightZero => {
+            write!(
+                writer,
+                "{sign_indicator}{s:0>width$}",
+                width = remaining_width
+            )
+        }
+    }
+}
+
 #[cfg(test)]
 mod test {
     use crate::format::num_format::{Case, ForceDecimal};
diff --git a/src/uucore/src/lib/features/format/spec.rs b/src/uucore/src/lib/features/format/spec.rs
@@ -252,7 +252,11 @@ impl Spec {
                 } else {
                     Case::Lowercase
                 },
-                alignment,
+                alignment: if flags.zero && !flags.minus {
+                    NumberAlignment::RightZero // float should always try to zero pad despite the precision
+                } else {
+                    alignment
+                },
                 positive_sign,
             },
             _ => return Err(&start[..index]),
diff --git a/tests/by-util/test_printf.rs b/tests/by-util/test_printf.rs
@@ -792,3 +792,127 @@ fn float_invalid_precision_fails() {
         .fails()
         .stderr_is("printf: invalid precision: '2147483648'\n");
 }
+
+// The following padding-tests test for the cases in which flags in ['0', ' '] are given.
+// For integer, only try to pad when no precision is given, while
+// for float, always try to pad
+#[test]
+fn space_padding_with_space_test() {
+    //  Check if printf gives an extra space in the beginning
+    new_ucmd!()
+        .args(&["% 3d", "1"])
+        .succeeds()
+        .stdout_only("  1");
+}
+
+#[test]
+fn zero_padding_with_space_test() {
+    new_ucmd!()
+        .args(&["% 03d", "1"])
+        .succeeds()
+        .stdout_only(" 01");
+}
+
+#[test]
+fn zero_padding_with_plus_test() {
+    new_ucmd!()
+        .args(&["%+04d", "1"])
+        .succeeds()
+        .stdout_only("+001");
+}
+
+#[test]
+fn negative_zero_padding_test() {
+    new_ucmd!()
+        .args(&["%03d", "-1"])
+        .succeeds()
+        .stdout_only("-01");
+}
+
+#[test]
+fn negative_zero_padding_with_space_test() {
+    new_ucmd!()
+        .args(&["% 03d", "-1"])
+        .succeeds()
+        .stdout_only("-01");
+}
+
+#[test]
+fn float_with_zero_precision_should_pad() {
+    new_ucmd!()
+        .args(&["%03.0f", "-1"])
+        .succeeds()
+        .stdout_only("-01");
+}
+
+#[test]
+fn precision_check() {
+    new_ucmd!()
+        .args(&["%.3d", "1"])
+        .succeeds()
+        .stdout_only("001");
+}
+
+#[test]
+fn space_padding_with_precision() {
+    new_ucmd!()
+        .args(&["%4.3d", "1"])
+        .succeeds()
+        .stdout_only(" 001");
+}
+
+#[test]
+fn float_zero_padding_with_precision() {
+    new_ucmd!()
+        .args(&["%04.1f", "1"])
+        .succeeds()
+        .stdout_only("01.0");
+}
+
+#[test]
+fn float_space_padding_with_precision() {
+    new_ucmd!()
+        .args(&["%4.1f", "1"])
+        .succeeds()
+        .stdout_only(" 1.0");
+}
+
+#[test]
+fn negative_float_zero_padding_with_precision() {
+    new_ucmd!()
+        .args(&["%05.1f", "-1"])
+        .succeeds()
+        .stdout_only("-01.0");
+}
+
+#[test]
+fn float_default_precision_space_padding() {
+    new_ucmd!()
+        .args(&["%10f", "1"])
+        .succeeds()
+        .stdout_only("  1.000000");
+}
+
+#[test]
+fn float_default_precision_zero_padding() {
+    new_ucmd!()
+        .args(&["%010f", "1"])
+        .succeeds()
+        .stdout_only("001.000000");
+}
+
+#[test]
+fn flag_position_space_padding() {
+    new_ucmd!()
+        .args(&["% +3.1d", "1"])
+        .succeeds()
+        .stdout_only(" +1");
+}
+
+#[test]
+fn float_flag_position_space_padding() {
+    new_ucmd!()
+        .args(&["% +5.1f", "1"])
+        .succeeds()
+        .stdout_only(" +1.0");
+}
