diagnostics: point inside quoted arguments, share the locating

src/uu/expr/locales/en-US.ftl

@@ -70,7 +70,6 @@ expr-error-too-big-range-quantifier-index = Regular expression too big
 expr-error-match-utf8 = match does not support invalid UTF-8 encoding in { $arg }
 
 # Diagnostic labels, used when errors are rendered with a source snippet
-expr-diag-label-unexpected-argument = the expression was already complete here
 expr-diag-help-missing-argument = every operator needs a value on both sides
 expr-diag-help-unexpected-argument = the shell may have expanded an operator; quote it as '{"*"}' or escape it as {"\\*"}
 expr-diag-help-non-integer-argument = arithmetic operators need integers; use = or {"!"}= to compare strings instead

src/uu/expr/locales/fr-FR.ftl

@@ -70,7 +70,6 @@ expr-error-too-big-range-quantifier-index = Expression régulière trop grande
 expr-error-match-utf8 = match ne supporte pas l'encodage UTF-8 invalide dans { $arg }
 
 # Étiquettes de diagnostic, utilisées quand les erreurs sont rendues avec un extrait
-expr-diag-label-unexpected-argument = l'expression était déjà complète ici
 expr-diag-help-missing-argument = chaque opérateur a besoin d'une valeur de chaque côté
 expr-diag-help-unexpected-argument = le shell a peut-être développé un opérateur ; protégez-le avec '{"*"}' ou échappez-le avec {"\\*"}
 expr-diag-help-non-integer-argument = les opérateurs arithmétiques attendent des entiers ; utilisez = ou {"!"}= pour comparer des chaînes

src/uu/expr/src/diagnostics.rs

@@ -57,7 +57,7 @@ fn locate(snapshot: &Snapshot, err: &ExprError, at: &FailurePoint) -> Option<Loc
         ExprError::UnexpectedArgument(_) => (
             // The parser stopped on the argument it did not expect.
             stopped_at,
-            Some("expr-diag-label-unexpected-argument"),
+            Some("diagnostics-label-expression-complete"),
             Some("expr-diag-help-unexpected-argument"),
         ),
         ExprError::MissingArgument(_) => {

src/uu/test/locales/en-US.ftl

@@ -164,7 +164,6 @@ test-error-invalid-integer = invalid integer { $value }
 test-error-unary-operator-expected = { $operator }: unary operator expected
 
 # Diagnostic labels, used when errors are rendered with a source snippet
-test-diag-label-extra-argument = the expression was already complete here
 test-diag-label-unary-operator-expected = this needs an expression on both sides
 test-diag-help-integer-op = -eq, -ne, -lt, -le, -gt and -ge compare integers; use =, {"!"}=, {"<"} or {">"} to compare strings
 test-diag-help-integer-op-mnemonics = -eq equal, -ne not equal, -lt less than, -le less than or equal, -gt greater than, -ge greater than or equal

src/uu/test/locales/fr-FR.ftl

@@ -164,7 +164,6 @@ test-error-invalid-integer = entier invalide { $value }
 test-error-unary-operator-expected = { $operator } : opérateur unaire attendu
 
 # Étiquettes de diagnostic, utilisées quand les erreurs sont rendues avec un extrait
-test-diag-label-extra-argument = l'expression était déjà complète ici
 test-diag-label-unary-operator-expected = nécessite une expression de chaque côté
 test-diag-help-integer-op = -eq, -ne, -lt, -le, -gt et -ge comparent des entiers ; utilisez =, {"!"}=, {"<"} ou {">"} pour comparer des chaînes
 test-diag-help-integer-op-mnemonics = -eq égal, -ne différent, -lt inférieur, -le inférieur ou égal, -gt supérieur, -ge supérieur ou égal

src/uu/test/src/diagnostics.rs

@@ -35,7 +35,7 @@ pub fn render(args: &[OsString], err: &ParseError) -> bool {
     let (label, help) = match &err.kind {
         ParseErrorKind::Expected(_) => (None, None),
         ParseErrorKind::ExtraArgument(_) => (
-            Some(translate!("test-diag-label-extra-argument")),
+            Some(translate!("diagnostics-label-expression-complete")),
             Some(translate!("test-diag-help-extra-argument")),
         ),
         ParseErrorKind::MissingArgument(_) => {

src/uucore/locales/en-US.ftl

@@ -111,6 +111,9 @@ format-error-invalid-universal-character = invalid universal character name \{ $
 # The word ariadne heads the advice line of a caret report with
 diagnostics-help-label = Help
 
+# Diagnostic label shared by the utilities whose arguments are an expression
+diagnostics-label-expression-complete = the expression was already complete here
+
 # Checksum errors (cksum, md5sum, sha*sum, b2sum)
 checksum-error-raw-multiple-files = the --raw option is not supported with multiple files
 checksum-error-check-only-flag = the --{ $flag } option is meaningful only when verifying checksums

src/uucore/locales/fr-FR.ftl

@@ -105,6 +105,9 @@ format-error-invalid-universal-character = nom de caractère universel invalide
 # Le mot en tête de la ligne de conseil d'un rapport avec caret
 diagnostics-help-label = Aide{" "}
 
+# Étiquette de diagnostic partagée par les utilitaires dont les arguments forment une expression
+diagnostics-label-expression-complete = l'expression était déjà complète ici
+
 # Erreurs de somme de contrôle (cksum, md5sum, sha*sum, b2sum)
 checksum-error-raw-multiple-files = l'option --raw n'est pas prise en charge avec plusieurs fichiers
 checksum-error-check-only-flag = l'option --{ $flag } n'a de sens que lors de la vérification de sommes de contrôle

src/uucore/src/lib/features.rs

@@ -24,6 +24,9 @@ pub mod diagnostics;
 #[cfg(not(feature = "diagnostics"))]
 #[path = "features/diagnostics_stub.rs"]
 pub mod diagnostics;
+// The part of the diagnostics that is not a no-op without the feature: both
+// `diagnostics` above re-export it rather than each carrying a copy.
+mod diagnostics_boundary;
 #[cfg(feature = "encoding")]
 pub mod encoding;
 #[cfg(feature = "extendedbigdecimal")]

src/uucore/src/lib/features/diagnostics.rs

@@ -93,16 +93,15 @@ pub fn operands(args: &[OsString]) -> Option<Vec<OsString>> {
     capture(args.get(1..).unwrap_or_default())
 }
 
+pub use crate::features::diagnostics_boundary::{char_span, floor_boundary};
+
 /// An argument list rendered as a single line, with the position of every
 /// argument inside it.
 pub struct Snapshot {
     /// The arguments joined by spaces, quoted where needed.
     text: String,
     /// Byte range of each argument inside `text`.
     spans: Vec<Range<usize>>,
-    /// Whether each argument was written out as-is, so that an offset inside it
-    /// also holds inside `text`.
-    verbatim: Vec<bool>,
     /// The arguments themselves, for [`Snapshot::index_of`].
     args: Vec<OsString>,
     /// Index of the first operand: 1 when argument 0 is the program name.
@@ -163,7 +162,6 @@ impl Snapshot {
         Self {
             text: String::new(),
             spans: Vec::with_capacity(len),
-            verbatim: Vec::with_capacity(len),
             args: Vec::with_capacity(len),
             first_operand: 0,
         }
@@ -178,20 +176,17 @@ impl Snapshot {
         // argument itself: quoting changes the length (a non-UTF-8 argument is
         // shown as `$'fo\x80o'`) and the caret has to line up with what is
         // actually printed.
-        let verbatim = match arg.to_str() {
+        match arg.to_str() {
             // Operators such as `=` or `!=` are shell-special, but quoting them
             // here would only obscure the expression.
             Some(s) if !s.is_empty() && !s.chars().any(char::is_whitespace) => {
                 self.text.push_str(s);
-                true
             }
             _ => {
                 let _ = write!(self.text, "{}", arg.maybe_quote());
-                false
             }
-        };
+        }
         self.spans.push(start..self.text.len());
-        self.verbatim.push(verbatim);
         self.args.push(arg);
     }
 
@@ -424,21 +419,79 @@ impl Snapshot {
         self.report(span, message, label, help)
     }
 
+    /// Write a report pointing at `range` inside `operand`, where `operand` is
+    /// the value of an option.
+    ///
+    /// The pairing of [`Snapshot::index_of_value`] with
+    /// [`Snapshot::render_inside_at`] is what almost every option value needs —
+    /// a `chmod` mode, a `sort` key, a `cut` range, a `head` size — so it is
+    /// written once here rather than in each utility.
+    ///
+    /// # Arguments
+    ///
+    /// * `operand` - The value at fault, as the parser received it.
+    /// * `short` - The option's short name (`'k'` for `-k`), if it has one.
+    /// * `long` - The option's long name (`"key"` for `--key`), if it has one.
+    /// * `range` - Byte range inside `operand` to point at. An empty range
+    ///   marks the character it starts at.
+    /// * `message` - The error message, already localized.
+    /// * `label` - Text placed under the caret, already localized, or `None`
+    ///   for a bare underline.
+    /// * `help` - An optional line of advice, already localized.
+    ///
+    /// # Returns
+    ///
+    /// `false` when no argument carries `operand` as that option's value, or
+    /// when nothing could be rendered, in which case the caller should fall
+    /// back to the plain one-line message.
+    // One more parameter than clippy likes, but every one of them is already
+    // part of `render_inside_at`; folding them into a struct would only move
+    // the list to the call site.
+    #[allow(clippy::too_many_arguments)]
+    pub fn render_option_value(
+        &self,
+        operand: &str,
+        short: Option<char>,
+        long: Option<&str>,
+        range: Range<usize>,
+        message: &str,
+        label: Option<&str>,
+        help: Option<&str>,
+    ) -> bool {
+        let Some(index) = self.index_of_value(operand, short, long) else {
+            return false;
+        };
+        self.render_inside_at(index, operand, range, message, label, help)
+    }
+
     /// Byte range covered by `range` — an offset inside `operand` — within the
     /// argument at `index`.
     fn locate_at(&self, index: usize, operand: &str, range: Range<usize>) -> Option<Range<usize>> {
         let arg = self.args.get(index)?;
         let whole = self.spans[index].clone();
-        if !self.verbatim[index] || !arg.as_encoded_bytes().ends_with(operand.as_bytes()) {
+        // An empty operand has nothing of its own to point at, and would be
+        // found at the end of the argument rather than where it was written.
+        // The argument as echoed — a bare pair of quotes — is what was typed.
+        if operand.is_empty() || !arg.as_encoded_bytes().ends_with(operand.as_bytes()) {
             return Some(whole);
         }
-        Some(self.locate_tail(whole, operand, range))
+        // Where the operand's own text sits in what was printed for this
+        // argument. An argument holding a space is echoed quoted, so the
+        // operand no longer ends the span it is drawn in — but the quoting
+        // only wraps it, and an offset inside it still holds wherever its
+        // bytes turned up. An argument that had to be escaped to be printed
+        // — a non-UTF-8 one, or one quoting cannot wrap — does not contain
+        // them contiguously, and falls back to a plain underline.
+        let Some(offset) = self.text[whole.clone()].rfind(operand) else {
+            return Some(whole);
+        };
+        Some(self.locate_operand(whole.start + offset, operand, range))
     }
 
     /// Byte range covered by `range` — an offset inside `operand` — where
-    /// `operand` is the tail of the argument spanning `whole`.
-    fn locate_tail(&self, whole: Range<usize>, operand: &str, range: Range<usize>) -> Range<usize> {
-        let base = whole.end - operand.len();
+    /// `operand` is drawn starting at `base`.
+    fn locate_operand(&self, base: usize, operand: &str, range: Range<usize>) -> Range<usize> {
+        let whole = base..base + operand.len();
         // Offsets come from someone else's parser, so they are only trusted as
         // far as the text agrees with them.
         let start = self.floor_boundary(base + range.start.min(operand.len()));
@@ -447,20 +500,18 @@ impl Snapshot {
             return start..end;
         }
         // An empty range means something is missing rather than wrong; give the
-        // caret the character it stopped at, or the whole argument if the
-        // operand ran out.
-        match self.text[start..].chars().next() {
-            Some(c) if start < whole.end => start..start + c.len_utf8(),
-            _ => whole,
+        // caret the character it stopped at, or the whole operand if it ran
+        // out.
+        let stopped = char_span(&self.text, start);
+        if start < whole.end && !stopped.is_empty() {
+            return stopped;
         }
+        whole
     }
 
     /// `offset`, moved back to the nearest character boundary.
     fn floor_boundary(&self, offset: usize) -> usize {
-        (0..=offset)
-            .rev()
-            .find(|&i| self.text.is_char_boundary(i))
-            .unwrap_or(0)
+        floor_boundary(&self.text, offset)
     }
 
     /// Translate the word ariadne heads the advice line with.
@@ -723,17 +774,21 @@ mod tests {
     }
 
     #[test]
-    fn a_range_at_the_end_of_an_operand_falls_back_to_the_argument() {
+    fn a_range_at_the_end_of_an_operand_falls_back_to_the_operand() {
         let snap = snapshot(&["-k1,"]);
-        assert_eq!(snap.locate_at(0, "1,", 2..2), Some(0..4));
+        // Nothing left to point at, so the caret takes the operand — not the
+        // `-k` in front of it, which is not what went wrong.
+        assert_eq!(snap.locate_at(0, "1,", 2..2), Some(2..4));
+        assert_eq!(&snap.text[2..4], "1,");
     }
 
     #[test]
-    fn a_quoted_operand_falls_back_to_the_whole_argument() {
+    fn offsets_hold_inside_an_operand_the_report_had_to_quote() {
         let snap = snapshot(&["a b", "-k1"]);
-        // Offsets inside `a b` would land on the quotes that were added.
-        assert_eq!(snap.locate_at(0, "a b", 1..2), Some(0..5));
-        assert_eq!(&snap.text[snap.locate_at(0, "a b", 1..2).unwrap()], "'a b'");
+        // The quoting only wraps the operand, so an offset into it still lands
+        // on the same bytes.
+        let span = snap.locate_at(0, "a b", 1..2).unwrap();
+        assert_eq!(&snap.text[span], " ");
     }
 
     #[test]

src/uucore/src/lib/features/diagnostics_boundary.rs

@@ -0,0 +1,81 @@
+// This file is part of the uutils coreutils package.
+//
+// For the full copyright and license information, please view the LICENSE
+// file that was distributed with this source code.
+
+//! The character-boundary arithmetic behind the caret diagnostics.
+//!
+//! Both [`crate::diagnostics`] and its no-op stand-in re-export these, and both
+//! do so for the same reason: a caller may floor an offset before it knows
+//! whether anything will be drawn, so the arithmetic has to be real even when
+//! the rendering is compiled out. Keeping it here means the two cannot drift.
+
+use std::ops::Range;
+
+/// `offset`, moved back to the nearest character boundary of `text`.
+///
+/// Offsets handed to the diagnostics are counted in bytes by someone else's
+/// parser, which may have been walking an `OsStr` rather than text; one can
+/// land inside a multi-byte character. Clamped to the end of `text`.
+///
+/// # Arguments
+///
+/// * `text` - The string the offset counts into.
+/// * `offset` - A byte offset, trusted only as far as `text` agrees with it.
+pub fn floor_boundary(text: &str, offset: usize) -> usize {
+    let offset = offset.min(text.len());
+    (0..=offset)
+        .rev()
+        .find(|&i| text.is_char_boundary(i))
+        .unwrap_or(0)
+}
+
+/// The range covering the character `offset` falls in.
+///
+/// Useful for the errors that blame a single character, so that a caret marks
+/// the whole of it rather than its first byte.
+///
+/// # Arguments
+///
+/// * `text` - The string the offset counts into.
+/// * `offset` - A byte offset, trusted only as far as `text` agrees with it.
+///
+/// # Returns
+///
+/// An empty range at the end of `text`, which the renderer reads as "nothing
+/// left to point at".
+pub fn char_span(text: &str, offset: usize) -> Range<usize> {
+    let start = floor_boundary(text, offset);
+    match text[start..].chars().next() {
+        Some(c) => start..start + c.len_utf8(),
+        None => start..start,
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{char_span, floor_boundary};
+
+    #[test]
+    fn floors_into_a_multibyte_character() {
+        // "é" is two bytes, so offset 1 is inside it.
+        assert_eq!(floor_boundary("é", 1), 0);
+        assert_eq!(floor_boundary("aé", 2), 1);
+    }
+
+    #[test]
+    fn clamps_past_the_end() {
+        assert_eq!(floor_boundary("ab", 9), 2);
+    }
+
+    #[test]
+    fn spans_the_whole_character() {
+        assert_eq!(char_span("aé", 1), 1..3);
+        assert_eq!(char_span("aé", 2), 1..3);
+    }
+
+    #[test]
+    fn spans_nothing_at_the_end() {
+        assert_eq!(char_span("ab", 2), 2..2);
+    }
+}

src/uucore/src/lib/features/diagnostics_stub.rs

@@ -29,6 +29,12 @@ pub fn operands(_args: &[OsString]) -> Option<Vec<OsString>> {
     None
 }
 
+// Boundary arithmetic is real even when rendering is not: a caller may floor
+// an offset before it knows whether anything will be drawn. It is the one part
+// of this module that is not a no-op, so it is shared with the real one rather
+// than restated here.
+pub use crate::features::diagnostics_boundary::{char_span, floor_boundary};
+
 /// A snapshot of nothing: it finds nothing and renders nothing.
 ///
 /// Deliberately derives nothing the real `Snapshot` does not, so that what a
@@ -91,4 +97,18 @@ impl Snapshot {
     ) -> bool {
         false
     }
+
+    #[allow(clippy::too_many_arguments)]
+    pub fn render_option_value(
+        &self,
+        _operand: &str,
+        _short: Option<char>,
+        _long: Option<&str>,
+        _range: Range<usize>,
+        _message: &str,
+        _label: Option<&str>,
+        _help: Option<&str>,
+    ) -> bool {
+        false
+    }
 }