Skip to content
Open
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 31 additions & 9 deletions src/uu/date/src/format_modifiers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,10 @@ struct ParsedSpec<'a> {
/// Flag characters from `[_0^#+-]`.
flags: &'a str,
/// Explicit width, if present. `None` means no width was specified.
/// A value that overflows `usize` is represented as `Some(usize::MAX)` so
/// Negative widths are used when `-` flag precedes the width.
/// A value that overflows `isize` is represented as `Some(isize::MAX)` so
/// the downstream allocation check surfaces it as `FieldWidthTooLarge`.
width: Option<usize>,
width: Option<isize>,
/// The specifier itself, including any leading colons (e.g. `Y`, `:z`, `::z`).
spec: &'a str,
/// Total byte length of the parsed sequence including the leading `%`.
Expand All @@ -104,13 +105,18 @@ fn parse_format_spec(s: &str) -> Option<ParsedSpec<'_>> {
}
let flags = &s[flags_start..pos];

// Check if '-' flag is present
let has_minus_flag = flags.contains('-');

// Width: zero or more ASCII digits.
let width_start = pos;
while pos < bytes.len() && bytes[pos].is_ascii_digit() {
pos += 1;
}
let width = if pos > width_start {
Some(s[width_start..pos].parse::<usize>().unwrap_or(usize::MAX))
let parsed: isize = s[width_start..pos].parse::<isize>().unwrap_or(isize::MAX);
// Make width negative if '-' flag is present
Some(if has_minus_flag { -parsed } else { parsed })
} else {
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse_format_spec currently negates the parsed width whenever the - flag is present (Some(if has_minus_flag { -parsed } else { parsed })). This changes the meaning of width for all specifiers (e.g. %-5d now yields a negative width) and causes downstream logic/tests that treat width as a magnitude to misbehave. Suggestion: keep width as a non-negative magnitude (or as usize) and rely on the existing no_pad/flag handling in apply_modifiers; if %N needs special behavior when - is present, encode that via flags/specifier-specific handling instead of sign-encoding width.

Copilot uses AI. Check for mistakes.
None
};
Expand Down Expand Up @@ -343,6 +349,11 @@ fn apply_modifiers(value: &str, parsed: &ParsedSpec<'_>) -> Result<String, Forma
let specifier = parsed.spec;
let mut result = value.to_string();

// Ensure nanoseconds are always zero-padded to 9 digits from the start
// if result.len() < 9 {
// result = format!("{:0>9}", result);
// }

Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is commented-out logic left in apply_modifiers (the // Ensure nanoseconds... block). This is effectively dead code in production and makes the intended behavior unclear. Either remove it, or implement it behind an actual condition with tests (especially if %N needs to be forced to 9 digits).

Suggested change
// Ensure nanoseconds are always zero-padded to 9 digits from the start
// if result.len() < 9 {
// result = format!("{:0>9}", result);
// }

Copilot uses AI. Check for mistakes.
// Determine default pad character based on specifier type
// Determine default pad character based on specifier type.
// Text specifiers (month names, etc.) and numeric specifiers like %e, %k, %l
Expand Down Expand Up @@ -411,14 +422,24 @@ fn apply_modifiers(value: &str, parsed: &ParsedSpec<'_>) -> Result<String, Forma

// If no_pad flag is active, suppress all padding and return
if no_pad {
if let Some(w) = width {
let abs_w = w.abs() as usize;
let end = abs_w.min(result.len());
let truncated = &result[..end];
// Return truncated nanoseconds without trimming trailing zeros (GNU behavior)
return Ok(truncated.to_string());
}
Comment on lines 411 to +420
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new no_pad handling truncates result whenever a width is present, and returns early without strip_default_padding. This breaks existing documented/tested behavior that %-5d ignores width and still strips default padding (see tests in this module). Additionally, &result[..end] truncates from the left and can panic on non-ASCII output (not guaranteed to be on a UTF-8 char boundary). This truncation logic should be limited to the %N specifier (if required), should preserve existing no-pad behavior for other specifiers, and should implement the intended direction (right-truncation per PR description) safely.

Suggested change
// If no_pad flag is active, suppress all padding and return
if no_pad {
if let Some(w) = width {
let abs_w = w.abs() as usize;
let end = abs_w.min(result.len());
let truncated = &result[..end];
// Return truncated nanoseconds without trimming trailing zeros (GNU behavior)
return Ok(truncated.to_string());
}
// If no_pad flag is active, suppress all padding and return.
// Width-based truncation only applies to %N; other specifiers keep the
// documented behavior of ignoring width and stripping default padding.
if no_pad {
if specifier == "N" {
if let Some(w) = width {
let abs_w = w.abs() as usize;
// Truncate from the right by keeping the leftmost characters,
// using char iteration to avoid slicing at a non-UTF-8 boundary.
let truncated: String = result.chars().take(abs_w).collect();
// Return truncated nanoseconds without trimming trailing zeros (GNU behavior)
return Ok(truncated);
}
}

Copilot uses AI. Check for mistakes.
return Ok(strip_default_padding(&result));
}

// Handle padding flag without explicit width: use default width.
// This applies when _ or 0 flag overrides the default padding character
// and no explicit width is specified (e.g., %_m, %0e).
let effective_width = match width {
Some(w) => w,
Some(w) => {
let abs_w = w.abs() as usize;
abs_w
}
None if underscore_flag || pad_char != default_pad => get_default_width(specifier),
None => 0,
};
Expand Down Expand Up @@ -525,7 +546,7 @@ mod tests {

/// Build a `ParsedSpec` for unit-testing `apply_modifiers` without a real
/// format string. `len` is set to 0 because these tests never use it.
fn spec<'a>(flags: &'a str, width: Option<usize>, spec: &'a str) -> ParsedSpec<'a> {
fn spec<'a>(flags: &'a str, width: Option<isize>, spec: &'a str) -> ParsedSpec<'a> {
ParsedSpec {
flags,
width,
Expand Down Expand Up @@ -844,11 +865,11 @@ mod tests {

#[test]
fn test_apply_modifiers_width_too_large() {
let err = apply_modifiers("x", &spec("", Some(usize::MAX), "c")).unwrap_err();
let err = apply_modifiers("x", &spec("", Some(isize::MAX), "c")).unwrap_err();
assert!(matches!(
err,
FormatError::FieldWidthTooLarge { width, specifier }
if width == usize::MAX && specifier == "c"
if width == isize::MAX && specifier == "c"
));
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assertions compare FormatError::FieldWidthTooLarge { width, .. } (where width is declared as a usize) against isize::MAX. As written this won’t compile. Either keep FormatError::FieldWidthTooLarge.width as usize and compare against an appropriate usize value (e.g., isize::MAX as usize if that’s now the sentinel), or change the error type consistently to isize and update all call sites/formatting accordingly.

Copilot uses AI. Check for mistakes.
}

Expand All @@ -865,7 +886,7 @@ mod tests {
assert!(matches!(
err,
FormatError::FieldWidthTooLarge { width, specifier }
if width == usize::MAX && specifier == "Y"
if width == isize::MAX && specifier == "Y"
));
Comment on lines 874 to 878
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue here: FormatError::FieldWidthTooLarge.width is a usize, but the match guard compares it to isize::MAX, which will not type-check. Please make the width sentinel/type consistent between parsing, error reporting, and tests.

Copilot uses AI. Check for mistakes.
}

Expand Down Expand Up @@ -939,7 +960,7 @@ mod tests {
#[test]
fn test_parse_format_spec() {
// (input, expected: Some((flags, width, spec, len)) or None)
type ParsedTuple = (&'static str, Option<usize>, &'static str, usize);
type ParsedTuple = (&'static str, Option<isize>, &'static str, usize);
let cases: &[(&str, Option<ParsedTuple>)] = &[
// ---- plain single-letter specifiers ----
("%Y", Some(("", None, "Y", 2))),
Expand Down Expand Up @@ -988,6 +1009,7 @@ mod tests {

for (input, expected) in cases {
let actual = parse_format_spec(input).map(|p| (p.flags, p.width, p.spec, p.len));

assert_eq!(actual, *expected, "input = {input:?}");
}
}
Expand Down
Loading