numfmt: handle --invalid one field at a time

src/uu/numfmt/src/format.rs

@@ -6,13 +6,15 @@
 // spell-checker:ignore powf seps replacen
 
 use std::fmt::Write as _;
+use std::io::Write as _;
 
 use uucore::display::Quotable;
 use uucore::i18n::decimal::{locale_decimal_separator, locale_grouping_separator};
-use uucore::translate;
+use uucore::{show, translate};
 
+use crate::errors::NumfmtError;
 use crate::numeric::ParsedNumber;
-use crate::options::{NumfmtOptions, RoundMethod, TransformOptions};
+use crate::options::{InvalidModes, NumfmtOptions, RoundMethod, TransformOptions};
 use crate::units::{
     DisplayableSuffix, RawSuffix, Result, Suffix, Unit, iec_bases_f64, si_bases_f64,
 };
@@ -935,13 +937,34 @@ fn split_bytes<'a>(input: &'a [u8], delim: &'a [u8]) -> impl Iterator<Item = &'a
     })
 }
 
+/// Handles a field that could not be converted the way `--invalid` asks:
+/// `abort` fails the line, the other modes report the field (or not, for
+/// `ignore`) so that the caller can write it unchanged and go on.
+fn report_invalid_field(
+    options: &NumfmtOptions,
+    message: String,
+) -> std::result::Result<(), WriteError> {
+    match options.invalid {
+        InvalidModes::Abort => return Err(WriteError::Invalid(message)),
+        InvalidModes::Fail => show!(NumfmtError::FormattingError(message)),
+        InvalidModes::Warn => {
+            let _ = writeln!(std::io::stderr(), "numfmt: {message}");
+        }
+        InvalidModes::Ignore => {}
+    }
+    Ok(())
+}
+
+/// Writes `input` with each selected field converted, and returns whether a
+/// field could not be converted.
 pub fn write_formatted_with_delimiter<W: std::io::Write + ?Sized>(
     writer: &mut W,
     input: &[u8],
     options: &NumfmtOptions,
     eol: Option<u8>,
-) -> std::result::Result<(), WriteError> {
+) -> std::result::Result<bool, WriteError> {
     let delimiter = options.delimiter.as_deref().unwrap();
+    let mut saw_invalid = false;
 
     for (n, field) in (1..).zip(split_bytes(input, delimiter)) {
         let field_selected = uucore::ranges::contain(&options.fields, n);
@@ -953,11 +976,17 @@ pub fn write_formatted_with_delimiter<W: std::io::Write + ?Sized>(
 
         if field_selected {
             // Field must be valid UTF-8 for numeric conversion
-            let field_str = std::str::from_utf8(field)
-                .map_err(|_| translate!("numfmt-error-invalid-number", "input" => escape_line(field).quote()))?
-                .trim_start();
-            let formatted = format_string(field_str, options, None)?;
-            writer.write_all(formatted.as_bytes())?;
+            let formatted = std::str::from_utf8(field)
+                .map_err(|_| translate!("numfmt-error-invalid-number", "input" => escape_line(field).quote()))
+                .and_then(|field_str| format_string(field_str.trim_start(), options, None));
+            match formatted {
+                Ok(formatted) => writer.write_all(formatted.as_bytes())?,
+                Err(message) => {
+                    report_invalid_field(options, message)?;
+                    saw_invalid = true;
+                    writer.write_all(field)?;
+                }
+            }
         } else {
             // add unselected field without conversion
             writer.write_all(field)?;
@@ -968,7 +997,7 @@ pub fn write_formatted_with_delimiter<W: std::io::Write + ?Sized>(
         writer.write_all(&[eol])?;
     }
 
-    Ok(())
+    Ok(saw_invalid)
 }
 
 /// Writes the space that stands for the first of the `blanks` between two
@@ -981,12 +1010,15 @@ fn write_separator<'a, W: std::io::Write + ?Sized>(
     Ok(&blanks[blanks.chars().next().map_or(0, char::len_utf8)..])
 }
 
+/// Writes `s` with each selected field converted, and returns whether a field
+/// could not be converted.
 pub fn write_formatted_with_whitespace<W: std::io::Write + ?Sized>(
     writer: &mut W,
     s: &str,
     options: &NumfmtOptions,
     eol: Option<u8>,
-) -> std::result::Result<(), WriteError> {
+) -> std::result::Result<bool, WriteError> {
+    let mut saw_invalid = false;
     for (n, (prefix, field)) in (1..).zip(WhitespaceSplitter {
         s: Some(s),
         options,
@@ -1009,8 +1041,16 @@ pub fn write_formatted_with_whitespace<W: std::io::Write + ?Sized>(
                 None
             };
 
-            let formatted = format_string(field, options, implicit_padding)?;
-            writer.write_all(formatted.as_bytes())?;
+            match format_string(field, options, implicit_padding) {
+                Ok(formatted) => writer.write_all(formatted.as_bytes())?,
+                Err(message) => {
+                    // An invalid field is written as it is, like an unselected one.
+                    report_invalid_field(options, message)?;
+                    saw_invalid = true;
+                    writer.write_all(prefix.as_bytes())?;
+                    writer.write_all(field.as_bytes())?;
+                }
+            }
         } else {
             // the first blank between fields is written as a space
             let prefix = if n > 1 {
@@ -1028,7 +1068,7 @@ pub fn write_formatted_with_whitespace<W: std::io::Write + ?Sized>(
         writer.write_all(&[eol])?;
     }
 
-    Ok(())
+    Ok(saw_invalid)
 }
 
 /// Writes a line that could not be converted, with the blanks between its

src/uu/numfmt/src/numfmt.rs

@@ -54,19 +54,15 @@ fn format_and_write(
         None => input_line,
     };
 
-    // In non-abort modes we buffer the formatted output so that on error we
-    // can emit the original line instead.
-    let buffer_output = !matches!(options.invalid, InvalidModes::Abort);
-    let mut buf = Vec::new();
-    let dest: &mut dyn Write = if buffer_output { &mut buf } else { writer };
-
+    // A field that cannot be converted is reported and written unchanged by
+    // the writers below, unless --invalid=abort, where it fails the line.
     let result = if options.delimiter.is_some() {
-        write_formatted_with_delimiter(dest, line, options, eol)
+        write_formatted_with_delimiter(writer, line, options, eol)
     } else {
         match std::str::from_utf8(line) {
             // Scientific notation is rejected per field when the number is
             // parsed, so a token in a field that is not converted is left alone.
-            Ok(s) => write_formatted_with_whitespace(dest, s, options, eol),
+            Ok(s) => write_formatted_with_whitespace(writer, s, options, eol),
             Err(_) => Err(translate!(
                 "numfmt-error-invalid-number",
                 "input" => escape_line(line).quote()
@@ -76,50 +72,42 @@ fn format_and_write(
     };
 
     // A failure to write is fatal, whatever --invalid says.
-    let result = match result {
-        Ok(()) => Ok(()),
+    let msg = match result {
+        Ok(saw_invalid) => return Ok(saw_invalid),
         Err(WriteError::Io(error)) => {
             return Err::<bool, _>(error).map_err_context(|| translate!("numfmt-error-write"));
         }
-        Err(WriteError::Invalid(message)) => Err(message),
+        Err(WriteError::Invalid(message)) => message,
     };
 
-    if let Err(msg) = result {
-        match options.invalid {
-            InvalidModes::Abort => {
-                return Err(Box::new(NumfmtError::FormattingError(msg)));
-            }
-            InvalidModes::Fail => {
-                show!(NumfmtError::FormattingError(msg));
-            }
-            InvalidModes::Warn => {
-                let _ = writeln!(stderr(), "numfmt: {msg}");
-            }
-            InvalidModes::Ignore => {}
+    // Only --invalid=abort, or a line that is not valid UTF-8, gets here.
+    match options.invalid {
+        InvalidModes::Abort => {
+            return Err(Box::new(NumfmtError::FormattingError(msg)));
         }
-        // On error, echo the original line, with the blanks between fields
-        // written as usual unless an explicit delimiter is set.
-        match std::str::from_utf8(input_line) {
-            Ok(s) if options.delimiter.is_none() => {
-                write_unconverted_with_whitespace(writer, s, options)
-            }
-            _ => writer.write_all(input_line),
+        InvalidModes::Fail => {
+            show!(NumfmtError::FormattingError(msg));
         }
-        .map_err_context(|| translate!("numfmt-error-write"))?;
-        if let Some(eol) = eol {
-            writer
-                .write_all(&[eol])
-                .map_err_context(|| translate!("numfmt-error-write"))?;
+        InvalidModes::Warn => {
+            let _ = writeln!(stderr(), "numfmt: {msg}");
         }
-        return Ok(true);
+        InvalidModes::Ignore => {}
     }
-
-    if buffer_output {
+    // Echo the original line, with the blanks between fields written as
+    // usual unless an explicit delimiter is set.
+    match std::str::from_utf8(input_line) {
+        Ok(s) if options.delimiter.is_none() => {
+            write_unconverted_with_whitespace(writer, s, options)
+        }
+        _ => writer.write_all(input_line),
+    }
+    .map_err_context(|| translate!("numfmt-error-write"))?;
+    if let Some(eol) = eol {
         writer
-            .write_all(&buf)
+            .write_all(&[eol])
             .map_err_context(|| translate!("numfmt-error-write"))?;
     }
-    Ok(false)
+    Ok(true)
 }
 
 /// Run `body` with stdout buffered the way stdio buffers it: a block at a time

tests/by-util/test_numfmt.rs

@@ -1156,6 +1156,31 @@ fn test_invalid_fail_with_fields_does_not_duplicate_output() {
         .stderr_is("numfmt: invalid number: 'Foo'\n");
 }
 
+#[test]
+fn test_invalid_field_does_not_stop_the_line() {
+    // Each invalid field is reported and written as it is, and the fields
+    // around it are still converted.
+    new_ucmd!()
+        .args(&["--invalid=fail", "--to=si", "--field=-"])
+        .pipe_in("x 1000 y 2000\n")
+        .fails_with_code(2)
+        .stdout_is("x 1.0k y 2.0k\n")
+        .stderr_is("numfmt: invalid number: 'x'\nnumfmt: invalid number: 'y'\n");
+
+    new_ucmd!()
+        .args(&["--invalid=warn", "--to=si", "--field=-", "-d:"])
+        .pipe_in("x:1000:y\n")
+        .succeeds()
+        .stdout_is("x:1.0k:y\n")
+        .stderr_is("numfmt: invalid number: 'x'\nnumfmt: invalid number: 'y'\n");
+
+    new_ucmd!()
+        .args(&["--invalid=ignore", "--to=si", "--field=-"])
+        .pipe_in("   x    1000\n")
+        .succeeds()
+        .stdout_only("   x    1.0k\n");
+}
+
 #[test]
 fn test_abort_with_fields_preserves_partial_output() {
     new_ucmd!()