Merge pull request #172 from artichoke/dev/lopopolo-rust-fmt-args-u16

Add tests for dynamic width specifiers exceeding `u16::MAX`

Author: lopopolo

.github/workflows/ci.yaml

@@ -73,7 +73,7 @@ jobs:
       - name: Install Rust toolchain
         uses: artichoke/setup-rust/[email protected]
         with:
-          toolchain: "1.76.0"
+          toolchain: "1.84.0"
 
       - name: Compile
         run: cargo build --verbose

Cargo.toml

@@ -1,18 +1,24 @@
 [package]
 name = "strftime-ruby"
 # remember to set `html_root_url` in `src/lib.rs`.
-version = "1.1.0"
+version = "1.2.0"
 authors = ["Ryan Lopopolo <[email protected]>", "x-hgg-x"]
 license = "MIT"
 edition = "2021"
-rust-version = "1.76.0"
+rust-version = "1.84.0"
 readme = "README.md"
 repository = "https://github.com/artichoke/strftime-ruby"
 documentation = "https://docs.rs/strftime-ruby"
 homepage = "https://github.com/artichoke/strftime-ruby"
 description = "Ruby `Time#strftime` parser and formatter"
 keywords = ["ruby", "strftime", "time"]
-categories = ["date-and-time", "no-std", "no-std::no-alloc", "parser-implementations", "value-formatting"]
+categories = [
+    "date-and-time",
+    "no-std",
+    "no-std::no-alloc",
+    "parser-implementations",
+    "value-formatting",
+]
 include = ["/src/**/*", "/tests/**/*", "/LICENSE", "/README.md"]
 
 [lib]

README.md

@@ -37,7 +37,7 @@ Add this to your `Cargo.toml`:
 
 ```toml
 [dependencies]
-strftime-ruby = "1.1.0"
+strftime-ruby = "1.2.0"
 ```
 
 ## Crate features
@@ -64,7 +64,7 @@ All features are enabled by default.
 
 ### Minimum Supported Rust Version
 
-This crate requires at least Rust 1.76.0. This version can be bumped in minor
+This crate requires at least Rust 1.84.0. This version can be bumped in minor
 releases.
 
 ## License

clippy.toml

@@ -0,0 +1,4 @@
+doc-valid-idents = [
+  "CRuby",
+  ".."
+]

src/format/mod.rs

@@ -297,10 +297,10 @@ impl Piece {
         if self.flags.contains(Flag::LeftPadding) {
             write!(f, "{value}")
         } else if self.padding == Padding::Spaces {
-            let width = self.width.unwrap_or(default_width);
+            let width = self.pad_width(f, b' ', default_width)?;
             write!(f, "{value: >width$}")
         } else {
-            let width = self.width.unwrap_or(default_width);
+            let width = self.pad_width(f, b'0', default_width)?;
             write!(f, "{value:0width$}")
         }
     }
@@ -315,16 +315,24 @@ impl Piece {
         if self.flags.contains(Flag::LeftPadding) {
             write!(f, "{value}")
         } else if self.padding == Padding::Zeros {
-            let width = self.width.unwrap_or(default_width);
+            let width = self.pad_width(f, b'0', default_width)?;
             write!(f, "{value:0width$}")
         } else {
-            let width = self.width.unwrap_or(default_width);
+            let width = self.pad_width(f, b' ', default_width)?;
             write!(f, "{value: >width$}")
         }
     }
 
+    /// Returns the width to use for the padding.
+    ///
+    /// Prints any excessive padding directly.
+    fn pad_width(&self, f: &mut SizeLimiter<'_>, pad: u8, default: usize) -> Result<usize, Error> {
+        let width = self.width.unwrap_or(default);
+        f.pad(pad, width.saturating_sub(u16::MAX.into()))?;
+        Ok(width.min(u16::MAX.into()))
+    }
+
     /// Format nanoseconds with the specified precision.
-    #[allow(clippy::uninlined_format_args)] // for readability and symmetry between if branches
     fn format_nanoseconds(
         &self,
         f: &mut SizeLimiter<'_>,
@@ -335,38 +343,37 @@ impl Piece {
 
         if width <= 9 {
             let value = nanoseconds / 10u32.pow(9 - width as u32);
-            write!(f, "{value:0n$}", n = width)
+            write!(f, "{value:0width$}")
         } else {
-            write!(f, "{nanoseconds:09}{:0n$}", 0, n = width - 9)
+            write!(f, "{nanoseconds:09}")?;
+            f.pad(b'0', width - 9)
         }
     }
 
     /// Format a string value.
     fn format_string(&self, f: &mut SizeLimiter<'_>, s: &str) -> Result<(), Error> {
-        match self.width {
-            None => write!(f, "{s}"),
-            Some(width) => {
-                if self.flags.contains(Flag::LeftPadding) {
-                    write!(f, "{s}")
-                } else if self.padding == Padding::Zeros {
-                    write!(f, "{s:0>width$}")
-                } else {
-                    write!(f, "{s: >width$}")
-                }
-            }
+        if !self.flags.contains(Flag::LeftPadding) {
+            self.write_padding(f, s.len())?;
         }
+
+        write!(f, "{s}")
     }
 
     /// Write padding separately.
     fn write_padding(&self, f: &mut SizeLimiter<'_>, min_width: usize) -> Result<(), Error> {
-        if let Some(width) = self.width {
-            let n = width.saturating_sub(min_width);
+        let Some(width) = self.width else {
+            return Ok(());
+        };
+
+        let n = width.saturating_sub(min_width);
+
+        let pad = match self.padding {
+            Padding::Zeros => b'0',
+            _ => b' ',
+        };
+
+        f.pad(pad, n)?;
 
-            match self.padding {
-                Padding::Zeros => write!(f, "{:0>n$}", "")?,
-                _ => write!(f, "{: >n$}", "")?,
-            };
-        }
         Ok(())
     }
 
@@ -390,14 +397,34 @@ impl Piece {
         UtcOffset::new(hour, minute, second)
     }
 
-    /// Compute hour padding for the `%z` specifier.
-    fn hour_padding(&self, min_width: usize) -> usize {
-        const MIN_PADDING: usize = "+hh".len();
+    /// Write the hour sign.
+    fn write_hour_sign(f: &mut SizeLimiter<'_>, hour: f64) -> Result<(), Error> {
+        if hour.is_sign_negative() {
+            write!(f, "-")?;
+        } else {
+            write!(f, "+")?;
+        }
+
+        Ok(())
+    }
 
-        match self.width {
-            Some(width) => width.saturating_sub(min_width) + MIN_PADDING,
-            None => MIN_PADDING,
+    /// Write the hour with padding for the `%z` specifier.
+    fn write_offset_hour(&self, f: &mut SizeLimiter<'_>, hour: f64, w: usize) -> Result<(), Error> {
+        let mut pad = self.width.unwrap_or(0).saturating_sub(w);
+
+        if hour < 10.0 {
+            pad += 1;
+        }
+
+        if self.padding == Padding::Spaces {
+            f.pad(b' ', pad)?;
+            Self::write_hour_sign(f, hour)?;
+        } else {
+            Self::write_hour_sign(f, hour)?;
+            f.pad(b'0', pad)?;
         }
+
+        write!(f, "{:.0}", hour.abs())
     }
 
     /// Write the time zone UTC offset as `"+hh"`.
@@ -406,13 +433,7 @@ impl Piece {
         f: &mut SizeLimiter<'_>,
         utc_offset: &UtcOffset,
     ) -> Result<(), Error> {
-        let hour = utc_offset.hour;
-        let n = self.hour_padding("+hh".len());
-
-        match self.padding {
-            Padding::Spaces => write!(f, "{hour: >+n$.0}"),
-            _ => write!(f, "{hour:+0n$.0}"),
-        }
+        self.write_offset_hour(f, utc_offset.hour, "+hh".len())
     }
 
     /// Write the time zone UTC offset as `"+hhmm"`.
@@ -421,13 +442,10 @@ impl Piece {
         f: &mut SizeLimiter<'_>,
         utc_offset: &UtcOffset,
     ) -> Result<(), Error> {
-        let UtcOffset { hour, minute, .. } = utc_offset;
-        let n = self.hour_padding("+hhmm".len());
+        let UtcOffset { hour, minute, .. } = *utc_offset;
 
-        match self.padding {
-            Padding::Spaces => write!(f, "{hour: >+n$.0}{minute:02}"),
-            _ => write!(f, "{hour:+0n$.0}{minute:02}"),
-        }
+        self.write_offset_hour(f, hour, "+hhmm".len())?;
+        write!(f, "{minute:02}")
     }
 
     /// Write the time zone UTC offset as `"+hh:mm"`.
@@ -436,13 +454,10 @@ impl Piece {
         f: &mut SizeLimiter<'_>,
         utc_offset: &UtcOffset,
     ) -> Result<(), Error> {
-        let UtcOffset { hour, minute, .. } = utc_offset;
-        let n = self.hour_padding("+hh:mm".len());
+        let UtcOffset { hour, minute, .. } = *utc_offset;
 
-        match self.padding {
-            Padding::Spaces => write!(f, "{hour: >+n$.0}:{minute:02}"),
-            _ => write!(f, "{hour:+0n$.0}:{minute:02}"),
-        }
+        self.write_offset_hour(f, hour, "+hh:mm".len())?;
+        write!(f, ":{minute:02}")
     }
 
     /// Write the time zone UTC offset as `"+hh:mm:ss"`.
@@ -455,14 +470,10 @@ impl Piece {
             hour,
             minute,
             second,
-        } = utc_offset;
-
-        let n = self.hour_padding("+hh:mm:ss".len());
+        } = *utc_offset;
 
-        match self.padding {
-            Padding::Spaces => write!(f, "{hour: >+n$.0}:{minute:02}:{second:02}"),
-            _ => write!(f, "{hour:+0n$.0}:{minute:02}:{second:02}"),
-        }
+        self.write_offset_hour(f, hour, "+hh:mm:ss".len())?;
+        write!(f, ":{minute:02}:{second:02}")
     }
 
     /// Format time using the formatting directive.

src/format/utils.rs

@@ -81,11 +81,31 @@ impl<'a> SizeLimiter<'a> {
             count: 0,
         }
     }
+
+    /// Pad with the specified character.
+    pub(crate) fn pad(&mut self, c: u8, n: usize) -> Result<(), Error> {
+        if self.count.saturating_add(n) > self.size_limit {
+            return Err(Error::FormattedStringTooLarge);
+        }
+
+        let buffer = [c; 1024];
+        let mut remaining = n;
+
+        while remaining > 0 {
+            let size = remaining.min(1024);
+            self.inner.write_all(&buffer[..size])?;
+            remaining -= size;
+        }
+
+        self.count += n;
+
+        Ok(())
+    }
 }
 
 impl Write for SizeLimiter<'_> {
     fn write(&mut self, buf: &[u8]) -> Result<usize, Error> {
-        if self.count + buf.len() > self.size_limit {
+        if self.count.saturating_add(buf.len()) > self.size_limit {
             return Err(Error::FormattedStringTooLarge);
         }
 

src/tests/format.rs

@@ -1,57 +1,7 @@
-#![allow(clippy::should_panic_without_expect)]
-
 use crate::format::TimeFormatter;
-use crate::{Error, Time};
-
-include!("../mock.rs.in");
-
-fn get_format_err(time: &MockTime<'_>, format: &str) -> Error {
-    TimeFormatter::new(time, format)
-        .fmt(&mut &mut [0u8; 100][..])
-        .unwrap_err()
-}
-
-fn check_format(time: &MockTime<'_>, format: &str, expected: &str) {
-    const SIZE: usize = 100;
-    let mut buf = [0u8; SIZE];
-    let mut cursor = &mut buf[..];
-
-    TimeFormatter::new(time, format).fmt(&mut cursor).unwrap();
-    let written = SIZE - cursor.len();
-    let data = core::str::from_utf8(&buf[..written]).unwrap();
-
-    assert_eq!(data, expected);
-}
-
-fn check_all(times: &[MockTime<'_>], format: &str, all_expected: &[&str]) {
-    assert_eq!(times.len(), all_expected.len());
-    for (time, expected) in times.iter().zip(all_expected) {
-        check_format(time, format, expected);
-    }
-}
-
-#[test]
-#[should_panic]
-#[rustfmt::skip]
-fn test_check_format_panics_on_error() {
-    let time = MockTime { year: 1111,  ..Default::default() };
-
-    check_format(&time, "'%Y'", "'1112'");
-}
+use crate::Error;
 
-#[test]
-#[should_panic]
-#[rustfmt::skip]
-fn test_check_all_panics_on_error() {
-    let times = [
-        MockTime { year: -1111, ..Default::default() },
-        MockTime { year: -11,   ..Default::default() },
-        MockTime { year: 1,     ..Default::default() },
-        MockTime { year: 1111,  ..Default::default() },
-    ];
-
-    check_all(&times, "'%Y'", &["'-1111'", "'-0011'", "'0001'",  "'1112'"]);
-}
+use super::{check_all, check_format, get_format_err, MockTime};
 
 #[test]
 #[rustfmt::skip]
@@ -875,7 +825,7 @@ fn test_format_large_width() {
     check_format(&time, "%-100000000m", "1");
     check_format(&time, "%2147483648m", "%2147483648m");
 
-    let err = get_format_err(&time, "%2147483647m");
+    let err = get_format_err(&time, "%1000m");
     assert!(matches!(err, Error::WriteZero));
 }