diff --git a/Cargo.lock b/Cargo.lock index b25e70fa..7f6a0b44 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -102,6 +102,16 @@ dependencies = [ "once_cell", ] +[[package]] +name = "ariadne" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8454c8a44ce2cb9cc7e7fae67fc6128465b343b92c6631e94beca3c8d1524ea5" +dependencies = [ + "unicode-width", + "yansi", +] + [[package]] name = "assert_cmd" version = "2.2.2" @@ -1569,6 +1579,7 @@ version = "0.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f24a5910ebbf2c0baf486a9125705f8e026a8f7741a3ea8cf5eca3c9257c9bdf" dependencies = [ + "ariadne", "bstr", "clap", "dns-lookup", diff --git a/Cargo.toml b/Cargo.toml index dd4992b7..05a6ab4d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,7 @@ faccess = "0.2.4" nix = { version = "0.31", features = ["fs", "user"] } onig = { version = "6.5", default-features = false } regex = "1.12" -uucore = { version = "0.12.0", features = ["entries", "fs", "fsext", "mode"] } +uucore = { version = "0.12.0", features = ["diagnostics", "entries", "fs", "fsext", "mode"] } walkdir = "2.5" itertools = "0.15.0" thiserror = "2.0.12" diff --git a/README.md b/README.md index a5747a81..495cccf0 100644 --- a/README.md +++ b/README.md @@ -9,6 +9,9 @@ Rust implementation of [GNU findutils](https://www.gnu.org/software/findutils/): `xargs`, `find`, `locate` and `updatedb`. The goal is to be a full drop-in replacement of the original commands. +At a terminal, `find` underlines the argument at fault in expression errors; see +[Extensions](docs/src/extensions.md#rich-expression-diagnostics). + ## Run the GNU testsuite on rust/findutils: ``` diff --git a/docs/src/extensions.md b/docs/src/extensions.md index fcc2f08d..d07e81a7 100644 --- a/docs/src/extensions.md +++ b/docs/src/extensions.md @@ -44,3 +44,229 @@ default. This is most useful when you want reproducible output: comparing two trees, generating a manifest, or writing a test whose expected output is a fixed list of lines. + +## Rich expression diagnostics + +`find` expressions get long, and a lone error line does not say *where* in the +expression the problem is. Down a pipe — a script, a test suite — that lone +line is all you get, because that is what such callers parse: + +```console +$ find /srv/www -type f -a \( -name '*.php' -o -nmae '*.inc' \) -print 2>&1 | cat +find: unknown predicate `-nmae' +``` + +At a terminal, where the reader is a person, `find` underlines the argument at +fault in place, on the command line you actually typed: + +```console +$ find /srv/www -type f -a \( -name '*.php' -o -name '*.phtml' -o -nmae '*.inc' \) -print +find: unknown predicate `-nmae' + ╭─[ find:1:60 ] + │ + 1 │ find /srv/www -type f -a ( -name *.php -o -name *.phtml -o -nmae *.inc ) -print + │ ──┬── + │ ╰──── not a known predicate + │ + │ Help: did you mean `-name'? +───╯ +``` + +The first line is the message `find` has always printed, so anything that matched +it before still matches. Everything below it is added context. + +### What gets a diagnostic + +Errors in the *structure* of the expression, and in the predicates themselves. +The unknown-predicate case is shown above; the rest follow, all with +`UUTILS_DIAG=always` set in the environment. + +A predicate left without its argument: + +```console +$ find /home -xdev -type f -size +1G -a -mtime -7 -printf +find: missing argument to `-printf' + ╭─[ find:1:49 ] + │ + 1 │ find /home -xdev -type f -size +1G -a -mtime -7 -printf + │ ───┬─── + │ ╰───── this predicate needs an argument +───╯ +``` + +An operator with nothing *after* it, typically a half-finished edit: + +```console +$ find /var/spool -type f -a \( -user postfix -o -group mail \) -a -mtime +7 -o +find: expected an expression after '-o' + ╭─[ find:1:74 ] + │ + 1 │ find /var/spool -type f -a ( -user postfix -o -group mail ) -a -mtime +7 -o + │ ─┬ + │ ╰── nothing follows this operator +───╯ +``` + +The same slip inside a group, where it is the `)` that cuts the operator off: + +```console +$ find /var/log -type f \( -name '*.gz' -o -name '*.old' -o \) -print +find: expected an expression between '-o' and ')' + ╭─[ find:1:53 ] + │ + 1 │ find /var/log -type f ( -name *.gz -o -name *.old -o ) -print + │ ─┬ + │ ╰── nothing between this operator and the ')' +───╯ +``` + +An operator with nothing *before* it. Note this underlines the leading token, +where the previous example underlined the trailing one — two mistakes that read +identically in the plain message now look different: + +```console +$ find . -o -type f -name '*.tmp' -print +find: invalid expression; you have used a binary operator '-o' with nothing before it. + ╭─[ find:1:8 ] + │ + 1 │ find . -o -type f -name *.tmp -print + │ ─┬ + │ ╰── no expression before this operator +───╯ +``` + +A `(` that is never closed. There are three `(` on this line; the diagnostic +picks the unbalanced one rather than pointing at the end of the command: + +```console +$ find . \( -type d -a \( -name .git -o -name target \) -prune \) -o \( -type f -print +find: invalid expression; I was expecting to find a ')' somewhere but did not see one. + ╭─[ find:1:64 ] + │ + 1 │ find . ( -type d -a ( -name .git -o -name target ) -prune ) -o ( -type f -print + │ ┬ + │ ╰── this parenthesis is never closed +───╯ +``` + +An empty group: + +```console +$ find . -type f \( \) -o -name '*.bak' -print +find: invalid expression; empty parentheses are not allowed. + ╭─[ find:1:16 ] + │ + 1 │ find . -type f ( ) -o -name *.bak -print + │ ┬ + │ ╰── nothing between these parentheses +───╯ +``` + +And a `)` with no opener: + +```console +$ find /etc \( -name '*.conf' -a -newer /etc/fstab \) \) -o -name '*.cfg' -print +find: you have too many ')' + ╭─[ find:1:49 ] + │ + 1 │ find /etc ( -name *.conf -a -newer /etc/fstab ) ) -o -name *.cfg -print + │ ┬ + │ ╰── no matching '(' before this +───╯ +``` + +Errors in the *value* of an argument — a bad `-size` suffix, `-perm` mode, +`-type` list, `-printf` format or `-newerXY` date — keep the plain single-line +message for now. + +### Comparison with GNU + +GNU `find` reports the same errors, but only ever as a single line. On a typo +buried in a group, that is all you get: + +```console +$ find /srv/www -type f -a \( -name '*.php' -o -name '*.phtml' -o -nmae '*.inc' \) -print +find: unknown predicate `-nmae' +``` + +```console +$ UUTILS_DIAG=always find /srv/www -type f -a \( -name '*.php' -o -name '*.phtml' -o -nmae '*.inc' \) -print +find: unknown predicate `-nmae' + ╭─[ find:1:60 ] + │ + 1 │ find /srv/www -type f -a ( -name *.php -o -name *.phtml -o -nmae *.inc ) -print + │ ──┬── + │ ╰──── not a known predicate + │ + │ Help: did you mean `-name'? +───╯ +``` + +The difference is starkest where the message names no argument at all. GNU tells +you a `)` is missing, but not which `(` is unbalanced — on a line with three of +them, that is the whole question: + +```console +$ find . \( -type d -a \( -name .git -o -name target \) -prune \) -o \( -type f -print +find: invalid expression; I was expecting to find a ')' somewhere but did not see one. +``` + +```console +$ UUTILS_DIAG=always find . \( -type d -a \( -name .git -o -name target \) -prune \) -o \( -type f -print +find: invalid expression; I was expecting to find a ')' somewhere but did not see one. + ╭─[ find:1:64 ] + │ + 1 │ find . ( -type d -a ( -name .git -o -name target ) -prune ) -o ( -type f -print + │ ┬ + │ ╰── this parenthesis is never closed +───╯ +``` + +In both cases the first line is identical to what GNU 4.10.0 prints, and the exit +code is `1` either way. The report only ever *adds* the block below. + +`UUTILS_DIAG` overrides the terminal check in either direction: set it to +`never` to always get the single line, or to `always` to get the report even +when standard error is redirected. Values are matched case-insensitively; +anything else — including an empty value — counts as unset and leaves the +terminal check in charge. It is the same variable, with the same values, that +the uutils coreutils read. + +Every message is byte-for-byte identical to GNU's under `LC_ALL=C`: + +```text +unknown predicate `-nmae' +missing argument to `-printf' +expected an expression after '-o' +expected an expression between '-o' and ')' +invalid expression; you have used a binary operator '-o' with nothing before it. +invalid expression; empty parentheses are not allowed. +invalid expression; I was expecting to find a ')' somewhere but did not see one. +you have too many ')' +``` + +### Notes + +- **The rendered line is a reconstruction, not an echo.** It shows the arguments + `find` was handed, after the shell had its say: `\(` and `'*.php'` arrive as + `(` and `*.php` and are shown that way. Only an argument holding whitespace, or + an empty one, is quoted, so that the underline still lines up with a word. That + is what makes the underline trustworthy even when the shell rewrote what you + typed. +- **Suggestions are edit-distance based**, with a threshold that scales with the + length of the name (one edit up to 3 characters, two up to 7, three beyond), + and a second rule that an edit may never rewrite half of what was typed. + Transpositions and dropped letters are caught — `-nam`, `-pritn`, `-exce`, + `-mindpeth` — while genuinely unrelated input gets no suggestion rather than a + misleading one. The half rule is what keeps a misplaced `-H` or `-P` from + being "corrected" to an unrelated two-character predicate it happens to sit + one edit away from. +- **Colour follows [`NO_COLOR`](https://no-color.org)** and is used only when + standard error is a terminal. A report forced on with `UUTILS_DIAG=always` + still goes wherever standard error points, so it stays plain when that is a + file. +- **On Windows the report heading reads `find.exe:`**, where the plain line + reads `find:`: the heading comes from uucore, which keeps the `.exe`. +- **Nothing changes for a non-terminal standard error**, which is how the GNU + and bfs compatibility testsuites — and every script — run `find`. Standard + error stays byte-for-byte what it was. diff --git a/src/find/error.rs b/src/find/error.rs new file mode 100644 index 00000000..33ea08a7 --- /dev/null +++ b/src/find/error.rs @@ -0,0 +1,227 @@ +// This file is part of the uutils findutils package. +// +// For the full copyright and license information, please view the LICENSE +// file that was distributed with this source code. + +//! Expression parse errors that remember which argument they refer to. +//! `Display` is the plain GNU message. + +use std::error::Error; + +use uucore::diagnostics::Snapshot; + +/// A command-line expression error that can point at the argument that caused it. +#[derive(Debug, thiserror::Error)] +#[error("{message}")] +pub struct ParseError { + message: String, + /// Index of the offending argument, relative to the argument slice this + /// error was created in. [`ParseError::shift`] moves it along at each + /// parsing-layer boundary until it indexes the process's full argv. + arg_index: Option, + label: Option, + help: Option, +} + +impl ParseError { + pub fn new(message: impl Into) -> Self { + Self { + message: message.into(), + arg_index: None, + label: None, + help: None, + } + } + + /// Records which argument the error refers to. + pub fn at(mut self, arg_index: usize) -> Self { + self.arg_index = Some(arg_index); + self + } + + /// Sets the text shown under the underlined argument. + pub fn with_label(mut self, label: impl Into) -> Self { + self.label = Some(label.into()); + self + } + + /// Sets a suggestion shown below the report. + pub fn with_help(mut self, help: impl Into) -> Self { + self.help = Some(help.into()); + self + } + + pub fn arg_index(&self) -> Option { + self.arg_index + } + + pub fn help(&self) -> Option<&str> { + self.help.as_deref() + } + + /// Adds `by` to the argument index of a `ParseError`, for callers that + /// parsed a sub-slice of argv. Other errors pass through untouched. + pub fn shift(err: Box, by: usize) -> Box { + match err.downcast::() { + Ok(mut parse_error) => { + if let Some(index) = parse_error.arg_index { + parse_error.arg_index = Some(index + by); + } + parse_error + } + Err(other) => other, + } + } + + /// Draws the error under the offending argument on stderr, headed by the + /// plain message line. Returns `false`, having printed nothing, when the + /// error names no argument. + pub fn render(&self, argv: &[&str]) -> bool { + let Some(arg_index) = self.arg_index else { + return false; + }; + // uucore translates the "Help:" label from its own embedded strings; + // without a localizer it prints the raw message id instead. Done here + // so that only a drawn report pays for it. + let _ = uucore::locale::setup_localization("find"); + Snapshot::with_program(argv).render( + arg_index, + &self.message, + self.label.as_deref(), + self.help.as_deref(), + ) + } +} + +/// Returns the entry of `candidates` closest to `input`, if one is close enough +/// to be a plausible typo. +pub fn closest_match<'a>( + input: &str, + candidates: impl IntoIterator, +) -> Option<&'a str> { + // Allow one edit for short predicates and more for longer ones, but never + // so many that unrelated names start matching. Two edits is the useful + // minimum for anything but the shortest names, since a swapped pair of + // letters already costs that much. + let input_length = input.chars().count(); + let max_distance = match input_length { + 0..=3 => 1, + 4..=7 => 2, + _ => 3, + }; + + candidates + .into_iter() + .map(|candidate| (edit_distance(input, candidate), candidate)) + // A suggestion is only worth making if more of what was typed survives + // it than not. Without this, every two-character candidate sits one + // edit from every other, and `-H` -- a real option a user is likely to + // misplace after the path operand -- would be "corrected" to whichever + // short predicate happens to come first in the candidate list. + .filter(|(distance, _)| *distance <= max_distance && distance * 2 < input_length) + .min_by_key(|(distance, candidate)| (*distance, candidate.len())) + .map(|(_, candidate)| candidate) +} + +/// Levenshtein distance between two strings, counting characters rather than +/// bytes. +fn edit_distance(left: &str, right: &str) -> usize { + let right_chars: Vec = right.chars().collect(); + // Distances from the empty prefix of `left` to each prefix of `right`. + let mut previous_row: Vec = (0..=right_chars.len()).collect(); + let mut current_row = vec![0; right_chars.len() + 1]; + + for (left_index, left_char) in left.chars().enumerate() { + current_row[0] = left_index + 1; + for (right_index, &right_char) in right_chars.iter().enumerate() { + let substitution_cost = usize::from(left_char != right_char); + current_row[right_index + 1] = (current_row[right_index] + 1) + .min(previous_row[right_index + 1] + 1) + .min(previous_row[right_index] + substitution_cost); + } + std::mem::swap(&mut previous_row, &mut current_row); + } + + previous_row[right_chars.len()] +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn display_is_the_bare_message() { + let error = ParseError::new("unknown predicate `-zap'") + .at(3) + .with_label("not a known predicate") + .with_help("did you mean `-zip'?"); + assert_eq!(error.to_string(), "unknown predicate `-zap'"); + } + + #[test] + fn render_without_an_index_draws_nothing() { + // Nothing to point at: the caller has to print the plain line. + let error = ParseError::new("something went wrong"); + assert!(!error.render(&["find", "/srv"])); + } + + #[test] + fn shift_moves_the_index_and_composes() { + let error: Box = Box::new(ParseError::new("oops").at(2)); + let error = ParseError::shift(ParseError::shift(error, 3), 1); + let shifted = error.downcast_ref::().unwrap(); + assert_eq!(shifted.arg_index(), Some(6)); + } + + #[test] + fn shift_leaves_indexless_and_foreign_errors_alone() { + let indexless: Box = Box::new(ParseError::new("oops")); + let indexless = ParseError::shift(indexless, 4); + assert_eq!( + indexless.downcast_ref::().unwrap().arg_index(), + None + ); + + let foreign: Box = From::from("plain message"); + let foreign = ParseError::shift(foreign, 4); + assert_eq!(foreign.to_string(), "plain message"); + assert!(foreign.downcast_ref::().is_none()); + } + + #[test] + fn edit_distance_counts_single_edits() { + assert_eq!(edit_distance("", ""), 0); + assert_eq!(edit_distance("-name", "-name"), 0); + assert_eq!(edit_distance("-nmae", "-name"), 2); + assert_eq!(edit_distance("-nam", "-name"), 1); + assert_eq!(edit_distance("", "-name"), 5); + assert_eq!(edit_distance("-name", ""), 5); + } + + #[test] + fn closest_match_finds_plausible_typos() { + let candidates = ["-name", "-newer", "-nogroup", "-print"]; + assert_eq!(closest_match("-nmae", candidates), Some("-name")); + assert_eq!(closest_match("-printt", candidates), Some("-print")); + assert_eq!(closest_match("-nogruop", candidates), Some("-nogroup")); + } + + #[test] + fn closest_match_rejects_short_input() { + // `-H` is one edit from any other two-character name, so a suggestion + // here would be a coin toss dressed up as advice. + let candidates = ["-a", "-o", "-ls", "-name"]; + assert_eq!(closest_match("-H", candidates), None); + assert_eq!(closest_match("-P", candidates), None); + // Three characters leave a majority intact after a single edit, so a + // suggestion is fair game again. + assert_eq!(closest_match("-lz", candidates), Some("-ls")); + } + + #[test] + fn closest_match_rejects_distant_input() { + let candidates = ["-name", "-newer", "-print"]; + assert_eq!(closest_match("-zzzzzzzzzz", candidates), None); + assert_eq!(closest_match("-xyz", candidates), None); + } +} diff --git a/src/find/matchers/logical_matchers.rs b/src/find/matchers/logical_matchers.rs index a892e12c..35af99b3 100644 --- a/src/find/matchers/logical_matchers.rs +++ b/src/find/matchers/logical_matchers.rs @@ -252,16 +252,16 @@ impl ListMatcherBuilder { self.submatchers.last_mut().unwrap().new_or_condition(arg) } - pub fn check_new_and_condition(&mut self) -> Result<(), Box> { + pub fn check_new_and_condition(&mut self, arg: &str) -> Result<(), Box> { { let child_or_matcher = &self.submatchers.last().unwrap(); let grandchild_and_matcher = &child_or_matcher.submatchers.last().unwrap(); if grandchild_and_matcher.submatchers.is_empty() { - return Err(From::from( - "invalid expression; you have used a binary operator '-a' \ - with nothing before it.", - )); + return Err(From::from(format!( + "invalid expression; you have used a binary operator \ + '{arg}' with nothing before it." + ))); } } Ok(()) diff --git a/src/find/matchers/mod.rs b/src/find/matchers/mod.rs index a33d47c8..5a86af66 100644 --- a/src/find/matchers/mod.rs +++ b/src/find/matchers/mod.rs @@ -70,6 +70,7 @@ use std::{ time::SystemTime, }; +use super::error::{self, ParseError}; use super::{Config, Dependencies}; pub use entry::{FileType, WalkEntry, WalkError}; @@ -286,7 +287,7 @@ pub fn build_top_level_matcher( args: &[&str], config: &mut Config, ) -> Result, Box> { - let (_, top_level_matcher) = (build_matcher_tree(args, config, 0, false))?; + let (_, top_level_matcher) = (build_matcher_tree(args, config, 0, None))?; // if the matcher doesn't have any side-effects, then we default to printing if !top_level_matcher.has_side_effects() { @@ -456,15 +457,208 @@ fn get_or_create_file(path: &str) -> Result> { Ok(file) } +/// What a predicate or operator on the command line resolves to. The parser +/// matches on this, so it has to handle every entry of [`PREDICATES`]. +#[derive(Clone, Copy)] +enum Predicate { + Print, + Print0, + Printf, + Fprint, + Fprintf, + Fprint0, + Ls, + Fls, + True, + False, + Lname, + Name, + Path, + Readable, + RegexType, + Regex, + Iregex, + Type, + Xtype, + Fstype, + Delete, + Newer, + Time(FileTimeType), + Min(FileTimeType), + Size, + Empty, + Exec, + Prompt, + Inum, + Links, + SameFile, + User, + NoUser, + Uid, + Group, + NoGroup, + Gid, + Executable, + Perm, + Prune, + Quit, + Writable, + Not, + And, + Or, + Comma, + OpenParen, + CloseParen, + Follow, + DayStart, + NoLeaf, + Depth, + Mount, + Sorted, + MaxDepth, + MinDepth, + Help, + Version, + Files0From, + /// Not in [`PREDICATES`]: a `-newerXY` form, or an unknown predicate. + Other, +} + +/// Every name the parser accepts, which also serves as the list of corrections +/// for a mistyped one. A variant missing here is never constructed, and the +/// compiler warns about it. +const PREDICATES: &[(&str, Predicate)] = &[ + ("-print", Predicate::Print), + ("-print0", Predicate::Print0), + ("-printf", Predicate::Printf), + ("-fprint", Predicate::Fprint), + ("-fprintf", Predicate::Fprintf), + ("-fprint0", Predicate::Fprint0), + ("-ls", Predicate::Ls), + ("-fls", Predicate::Fls), + ("-true", Predicate::True), + ("-false", Predicate::False), + ("-lname", Predicate::Lname), + ("-ilname", Predicate::Lname), + ("-name", Predicate::Name), + ("-iname", Predicate::Name), + ("-path", Predicate::Path), + ("-ipath", Predicate::Path), + ("-wholename", Predicate::Path), + ("-iwholename", Predicate::Path), + ("-readable", Predicate::Readable), + ("-regextype", Predicate::RegexType), + ("-regex", Predicate::Regex), + ("-iregex", Predicate::Iregex), + ("-type", Predicate::Type), + ("-xtype", Predicate::Xtype), + ("-fstype", Predicate::Fstype), + ("-delete", Predicate::Delete), + ("-newer", Predicate::Newer), + ("-atime", Predicate::Time(FileTimeType::Accessed)), + ("-ctime", Predicate::Time(FileTimeType::Changed)), + ("-mtime", Predicate::Time(FileTimeType::Modified)), + ("-amin", Predicate::Min(FileTimeType::Accessed)), + ("-cmin", Predicate::Min(FileTimeType::Changed)), + ("-mmin", Predicate::Min(FileTimeType::Modified)), + ("-size", Predicate::Size), + ("-empty", Predicate::Empty), + ("-exec", Predicate::Exec), + ("-execdir", Predicate::Exec), + ("-ok", Predicate::Prompt), + ("-okdir", Predicate::Prompt), + ("-inum", Predicate::Inum), + ("-links", Predicate::Links), + ("-samefile", Predicate::SameFile), + ("-user", Predicate::User), + ("-nouser", Predicate::NoUser), + ("-uid", Predicate::Uid), + ("-group", Predicate::Group), + ("-nogroup", Predicate::NoGroup), + ("-gid", Predicate::Gid), + ("-executable", Predicate::Executable), + ("-perm", Predicate::Perm), + ("-prune", Predicate::Prune), + ("-quit", Predicate::Quit), + ("-writable", Predicate::Writable), + ("-not", Predicate::Not), + ("!", Predicate::Not), + ("-and", Predicate::And), + ("-a", Predicate::And), + ("-or", Predicate::Or), + ("-o", Predicate::Or), + (",", Predicate::Comma), + ("(", Predicate::OpenParen), + (")", Predicate::CloseParen), + ("-follow", Predicate::Follow), + ("-daystart", Predicate::DayStart), + ("-noleaf", Predicate::NoLeaf), + ("-d", Predicate::Depth), + ("-depth", Predicate::Depth), + ("-mount", Predicate::Mount), + ("-xdev", Predicate::Mount), + ("-sorted", Predicate::Sorted), + ("-maxdepth", Predicate::MaxDepth), + ("-mindepth", Predicate::MinDepth), + ("-help", Predicate::Help), + ("--help", Predicate::Help), + ("-version", Predicate::Version), + ("--version", Predicate::Version), + ("-files0-from", Predicate::Files0From), +]; + +fn lookup_predicate(arg: &str) -> Predicate { + PREDICATES + .iter() + .find(|(name, _)| *name == arg) + .map_or(Predicate::Other, |&(_, predicate)| predicate) +} + +/// Turns one of the `logical_matchers` "binary operator with nothing before it" +/// errors into a diagnostic pointing at the operator. +fn binary_operator_error(err: &dyn Error, arg_index: usize) -> ParseError { + ParseError::new(err.to_string()) + .at(arg_index) + .with_label("no expression before this operator") +} + +/// The error for a predicate at `args[index]` that is missing its argument. +fn missing_argument_error(args: &[&str], index: usize) -> ParseError { + ParseError::new(format!("missing argument to `{}'", args[index])) + .at(index) + .with_label("this predicate needs an argument") +} + +/// The error for an operator at `args[index]` that has nothing to apply to, +/// either because the expression ends there or because a ')' closes it. +fn missing_operand_error(args: &[&str], index: usize) -> ParseError { + let operator = args[index]; + if args.get(index + 1) == Some(&")") { + ParseError::new(format!( + "expected an expression between '{operator}' and ')'" + )) + .at(index) + .with_label("nothing between this operator and the ')'") + } else { + ParseError::new(format!("expected an expression after '{operator}'")) + .at(index) + .with_label("nothing follows this operator") + } +} + /// The main "translate command-line args into a matcher" function. Will call /// itself recursively if it encounters an opening bracket. A successful return /// consists of a tuple containing the new index into the args array to use (if /// called recursively) and the resulting matcher. +/// +/// `open_bracket` is the index of the '(' this call is parsing the contents of, +/// or `None` at the top level. It doubles as the "a ')' is expected" flag and +/// lets an unclosed bracket be pointed at in the error. fn build_matcher_tree( args: &[&str], config: &mut Config, arg_index: usize, - mut expecting_bracket: bool, + mut open_bracket: Option, ) -> Result<(usize, Box), Box> { let mut top_level_matcher = ListMatcherBuilder::new(); @@ -477,28 +671,28 @@ fn build_matcher_tree( let mut i = arg_index; let mut invert_next_matcher = false; while i < args.len() { - let possible_submatcher = match args[i] { - "-print" => Some(Printer::new(PrintDelimiter::Newline, None).into_box()), - "-print0" => Some(Printer::new(PrintDelimiter::Null, None).into_box()), - "-printf" => { + let possible_submatcher = match lookup_predicate(args[i]) { + Predicate::Print => Some(Printer::new(PrintDelimiter::Newline, None).into_box()), + Predicate::Print0 => Some(Printer::new(PrintDelimiter::Null, None).into_box()), + Predicate::Printf => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(Printf::new(args[i], None)?.into_box()) } - "-fprint" => { + Predicate::Fprint => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; let file = get_or_create_file(args[i])?; Some(Printer::new(PrintDelimiter::Newline, Some(file)).into_box()) } - "-fprintf" => { + Predicate::Fprintf => { if i + 2 >= args.len() { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } // Action: -fprintf file format @@ -510,130 +704,116 @@ fn build_matcher_tree( i += 1; Some(Printf::new(args[i], Some((file, output_path)))?.into_box()) } - "-fprint0" => { + Predicate::Fprint0 => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; let file = get_or_create_file(args[i])?; Some(Printer::new(PrintDelimiter::Null, Some(file)).into_box()) } - "-ls" => Some(Ls::new(None).into_box()), - "-fls" => { + Predicate::Ls => Some(Ls::new(None).into_box()), + Predicate::Fls => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; let file = get_or_create_file(args[i])?; Some(Ls::new(Some(file)).into_box()) } - "-true" => Some(TrueMatcher.into_box()), - "-false" => Some(FalseMatcher.into_box()), - "-lname" | "-ilname" => { + Predicate::True => Some(TrueMatcher.into_box()), + Predicate::False => Some(FalseMatcher.into_box()), + Predicate::Lname => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(LinkNameMatcher::new(args[i], args[i - 1].starts_with("-i")).into_box()) } - "-name" | "-iname" => { + Predicate::Name => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(NameMatcher::new(args[i], args[i - 1].starts_with("-i")).into_box()) } - "-path" | "-ipath" | "-wholename" | "-iwholename" => { + Predicate::Path => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(PathMatcher::new(args[i], args[i - 1].starts_with("-i")).into_box()) } - "-readable" => Some(AccessMatcher::Readable.into_box()), - "-regextype" => { + Predicate::Readable => Some(AccessMatcher::Readable.into_box()), + Predicate::RegexType => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; regex_type = regex::RegexType::from_str(args[i])?; Some(TrueMatcher.into_box()) } - "-regex" => { + Predicate::Regex => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(RegexMatcher::new(regex_type, args[i], false)?.into_box()) } - "-iregex" => { + Predicate::Iregex => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(RegexMatcher::new(regex_type, args[i], true)?.into_box()) } - "-type" => { + Predicate::Type => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(TypeMatcher::new(args[i])?.into_box()) } - "-xtype" => { + Predicate::Xtype => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(XtypeMatcher::new(args[i])?.into_box()) } - "-fstype" => { + Predicate::Fstype => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(FileSystemMatcher::new(args[i].to_string()).into_box()) } - "-delete" => { + Predicate::Delete => { // -delete implicitly requires -depth config.depth_first = true; Some(DeleteMatcher::new().into_box()) } - "-newer" => { + Predicate::Newer => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(NewerMatcher::new(args[i], config.follow)?.into_box()) } - "-mtime" | "-atime" | "-ctime" => { + Predicate::Time(file_time_type) => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } - let file_time_type = match args[i] { - "-atime" => FileTimeType::Accessed, - "-ctime" => FileTimeType::Changed, - "-mtime" => FileTimeType::Modified, - // This shouldn't be possible. We've already checked the value - // is one of those three values. - _ => unreachable!("Encountered unexpected value {}", args[i]), - }; let days = convert_arg_to_comparable_value(args[i], args[i + 1])?; i += 1; Some(FileTimeMatcher::new(file_time_type, days, config.today_start).into_box()) } - "-amin" | "-cmin" | "-mmin" => { + Predicate::Min(file_time_type) => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } - let file_time_type = match args[i] { - "-amin" => FileTimeType::Accessed, - "-cmin" => FileTimeType::Changed, - "-mmin" => FileTimeType::Modified, - _ => unreachable!("Encountered unexpected value {}", args[i]), - }; let minutes = convert_arg_to_comparable_value(args[i], args[i + 1])?; i += 1; Some( @@ -641,17 +821,17 @@ fn build_matcher_tree( .into_box(), ) } - "-size" => { + Predicate::Size => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } let (size, unit) = convert_arg_to_comparable_value_and_suffix(args[i], args[i + 1])?; i += 1; Some(SizeMatcher::new(size, &unit)?.into_box()) } - "-empty" => Some(EmptyMatcher::new().into_box()), - "-exec" | "-execdir" => { + Predicate::Empty => Some(EmptyMatcher::new().into_box()), + Predicate::Exec => { let mut arg_index = i + 1; while arg_index < args.len() && args[arg_index] != ";" @@ -667,7 +847,7 @@ fn build_matcher_tree( if arg_index < i + required_arg || arg_index == args.len() { // at the minimum we need the executable and the ';' // or the executable and the '{} +' - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } let expression = args[i]; if expression == "-execdir" { @@ -700,7 +880,7 @@ fn build_matcher_tree( _ => unreachable!("Encountered unexpected value {}", args[arg_index]), } } - "-ok" | "-okdir" => { + Predicate::Prompt => { // -ok is like -exec ... ; but prompts before each invocation. // Only ';' is accepted: POSIX does not define -ok ... + and // GNU find rejects it (batch mode makes no sense with prompts). @@ -710,7 +890,7 @@ fn build_matcher_tree( } if arg_index < i + 2 || arg_index == args.len() { // Need at least the executable and the terminating ';'. - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } let expression = args[i]; if expression == "-okdir" { @@ -730,36 +910,36 @@ fn build_matcher_tree( ) } #[cfg(unix)] - "-inum" => { + Predicate::Inum => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } let inum = convert_arg_to_comparable_value(args[i], args[i + 1])?; i += 1; Some(InodeMatcher::new(inum).into_box()) } #[cfg(not(unix))] - "-inum" => { + Predicate::Inum => { return Err(From::from( "Inode numbers are not available on this platform", )); } #[cfg(unix)] - "-links" => { + Predicate::Links => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } let inum = convert_arg_to_comparable_value(args[i], args[i + 1])?; i += 1; Some(LinksMatcher::new(inum).into_box()) } #[cfg(not(unix))] - "-links" => { + Predicate::Links => { return Err(From::from("Link counts are not available on this platform")); } - "-samefile" => { + Predicate::SameFile => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; let path = args[i]; @@ -767,9 +947,9 @@ fn build_matcher_tree( .map_err(|e| format!("{path}: {e}"))?; Some(matcher.into_box()) } - "-user" => { + Predicate::User => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } let user = args[i + 1]; @@ -786,19 +966,19 @@ fn build_matcher_tree( })?; Some(matcher.into_box()) } - "-nouser" => Some(NoUserMatcher {}.into_box()), - "-uid" => { + Predicate::NoUser => Some(NoUserMatcher {}.into_box()), + Predicate::Uid => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } // check if the argument is a number let uid = convert_arg_to_comparable_value(args[i], args[i + 1])?; i += 1; Some(UserMatcher::from_comparable(uid).into_box()) } - "-group" => { + Predicate::Group => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } let group = args[i + 1]; @@ -817,89 +997,88 @@ fn build_matcher_tree( })?; Some(matcher.into_box()) } - "-nogroup" => Some(NoGroupMatcher {}.into_box()), - "-gid" => { + Predicate::NoGroup => Some(NoGroupMatcher {}.into_box()), + Predicate::Gid => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } // check if the argument is a number let gid = convert_arg_to_comparable_value(args[i], args[i + 1])?; i += 1; Some(GroupMatcher::from_comparable(gid).into_box()) } - "-executable" => Some(AccessMatcher::Executable.into_box()), - "-perm" => { + Predicate::Executable => Some(AccessMatcher::Executable.into_box()), + Predicate::Perm => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } i += 1; Some(PermMatcher::new(args[i])?.into_box()) } - "-prune" => Some(PruneMatcher::new().into_box()), - "-quit" => Some(QuitMatcher.into_box()), - "-writable" => Some(AccessMatcher::Writable.into_box()), - "-not" | "!" => { + Predicate::Prune => Some(PruneMatcher::new().into_box()), + Predicate::Quit => Some(QuitMatcher.into_box()), + Predicate::Writable => Some(AccessMatcher::Writable.into_box()), + Predicate::Not => { if !are_more_expressions(args, i) { - return Err(From::from(format!( - "expected an expression after {}", - args[i] - ))); + return Err(missing_operand_error(args, i).into()); } invert_next_matcher = !invert_next_matcher; None } - "-and" | "-a" => { + Predicate::And => { if !are_more_expressions(args, i) { - return Err(From::from(format!( - "expected an expression after {}", - args[i] - ))); + return Err(missing_operand_error(args, i).into()); } - top_level_matcher.check_new_and_condition()?; + top_level_matcher + .check_new_and_condition(args[i]) + .map_err(|e| binary_operator_error(e.as_ref(), i))?; None } - "-or" | "-o" => { + Predicate::Or => { if !are_more_expressions(args, i) { - return Err(From::from(format!( - "expected an expression after {}", - args[i] - ))); + return Err(missing_operand_error(args, i).into()); } - top_level_matcher.new_or_condition(args[i])?; + top_level_matcher + .new_or_condition(args[i]) + .map_err(|e| binary_operator_error(e.as_ref(), i))?; None } - "," => { + Predicate::Comma => { if !are_more_expressions(args, i) { - return Err(From::from(format!( - "expected an expression after {}", - args[i] - ))); + return Err(missing_operand_error(args, i).into()); } - top_level_matcher.new_list_condition()?; + top_level_matcher + .new_list_condition() + .map_err(|e| binary_operator_error(e.as_ref(), i))?; None } - "(" => { - let (new_arg_index, sub_matcher) = build_matcher_tree(args, config, i + 1, true)?; + Predicate::OpenParen => { + let (new_arg_index, sub_matcher) = + build_matcher_tree(args, config, i + 1, Some(i))?; i = new_arg_index; Some(sub_matcher) } - ")" => { - if !expecting_bracket { - return Err(From::from( - "invalid expression: expected expression before closing parentheses ')'.", - )); - } + Predicate::CloseParen => { + let Some(open_bracket_index) = open_bracket else { + return Err(ParseError::new("you have too many ')'") + .at(i) + .with_label("no matching '(' before this") + .into()); + }; let bracket = args[i - 1]; if bracket == "(" { - return Err(From::from( + return Err(ParseError::new( "invalid expression; empty parentheses are not allowed.", - )); + ) + .at(open_bracket_index) + .with_label("nothing between these parentheses") + .into()); } return Ok((i, top_level_matcher.build())); } - "-follow" => { + Predicate::Follow => { // This option affects multiple matchers. // 1. It will use noleaf by default. (but -noleaf No change of behavior) // Unless -L or -H is specified: @@ -914,105 +1093,113 @@ fn build_matcher_tree( config.no_leaf_dirs = true; Some(TrueMatcher.into_box()) } - "-daystart" => { + Predicate::DayStart => { config.today_start = true; Some(TrueMatcher.into_box()) } - "-noleaf" => { + Predicate::NoLeaf => { // No change of behavior config.no_leaf_dirs = true; Some(TrueMatcher.into_box()) } - "-d" | "-depth" => { + Predicate::Depth => { // TODO add warning if it appears after actual testing criterion config.depth_first = true; Some(TrueMatcher.into_box()) } - "-mount" | "-xdev" => { + Predicate::Mount => { // TODO add warning if it appears after actual testing criterion config.same_file_system = true; Some(TrueMatcher.into_box()) } - "-sorted" => { + Predicate::Sorted => { // TODO add warning if it appears after actual testing criterion config.sorted_output = true; Some(TrueMatcher.into_box()) } - "-maxdepth" => { + Predicate::MaxDepth => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } config.max_depth = convert_arg_to_number(args[i], args[i + 1])?; i += 1; Some(TrueMatcher.into_box()) } - "-mindepth" => { + Predicate::MinDepth => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } config.min_depth = convert_arg_to_number(args[i], args[i + 1])?; i += 1; Some(TrueMatcher.into_box()) } - "-help" | "--help" => { + Predicate::Help => { config.help_requested = true; None } - "-version" | "--version" => { + Predicate::Version => { config.version_requested = true; None } - "-files0-from" => { + Predicate::Files0From => { if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); + return Err(missing_argument_error(args, i).into()); } let _ = config.files0_argument.insert(args[i + 1].to_string()); i += 1; Some(TrueMatcher.into_box()) } - _ => { - match parse_str_to_newer_args(args[i]) { - Some((x_option, y_option)) => { - if i >= args.len() - 1 { - return Err(From::from(format!("missing argument to {}", args[i]))); - } - #[cfg(target_os = "linux")] - if x_option == "B" { - return Err(From::from("This system does not provide a way to find the birth time of a file.")); - } - if y_option == "t" { - let time = args[i + 1]; - let newer_time_type = NewerOptionType::from_str(x_option.as_str()); - // Convert args to unix timestamps. (expressed in numeric types) - let Some(comparable_time) = parse_date_str_to_timestamps(time) else { - return Err(From::from(format!( - "I cannot figure out how to interpret ‘{}’ as a date or time", - args[i + 1] - ))); - }; - i += 1; - Some(NewerTimeMatcher::new(newer_time_type, comparable_time).into_box()) - } else { - let file_path = args[i + 1]; - i += 1; - Some( - NewerOptionMatcher::new(&x_option, &y_option, file_path)? - .into_box(), - ) - } - } - // Match GNU find wording for unknown predicates. - None => { - return Err(From::from(format!("unknown predicate `{}'", args[i]))); + Predicate::Other => { + // Match GNU find wording for unknown predicates. + let Some((x_option, y_option)) = parse_str_to_newer_args(args[i]) else { + let mut err = ParseError::new(format!("unknown predicate `{}'", args[i])) + .at(i) + .with_label("not a known predicate"); + if let Some(suggestion) = error::closest_match( + args[i], + PREDICATES + .iter() + .map(|(name, _)| *name) + // Handled as `-newerXY` but worth suggesting. + .chain(["-anewer", "-cnewer"]), + ) { + err = err.with_help(format!("did you mean `{suggestion}'?")); } + return Err(err.into()); + }; + if i >= args.len() - 1 { + return Err(missing_argument_error(args, i).into()); + } + #[cfg(target_os = "linux")] + if x_option == "B" { + return Err(From::from( + "This system does not provide a way to find the birth time of a file.", + )); + } + if y_option == "t" { + let time = args[i + 1]; + let newer_time_type = NewerOptionType::from_str(x_option.as_str()); + // Convert args to unix timestamps. (expressed in numeric types) + let Some(comparable_time) = parse_date_str_to_timestamps(time) else { + return Err(From::from(format!( + "I cannot figure out how to interpret ‘{}’ as a date or time", + args[i + 1] + ))); + }; + i += 1; + Some(NewerTimeMatcher::new(newer_time_type, comparable_time).into_box()) + } else { + let file_path = args[i + 1]; + i += 1; + Some(NewerOptionMatcher::new(&x_option, &y_option, file_path)?.into_box()) } } }; i += 1; if config.help_requested || config.version_requested { // Ignore anything, even invalid expressions, after -help/-version - expecting_bracket = false; + open_bracket = None; break; } if let Some(submatcher) = possible_submatcher { @@ -1024,11 +1211,14 @@ fn build_matcher_tree( } } } - if expecting_bracket { - return Err(From::from( + if let Some(open_bracket_index) = open_bracket { + return Err(ParseError::new( "invalid expression; I was expecting to find a ')' somewhere but \ did not see one.", - )); + ) + .at(open_bracket_index) + .with_label("this parenthesis is never closed") + .into()); } Ok((i, top_level_matcher.build())) } @@ -1158,8 +1348,7 @@ mod tests { let mut config = Config::default(); if let Err(e) = build_top_level_matcher(&[arg], &mut config) { - assert!(e.to_string().contains("missing argument to")); - assert!(e.to_string().contains(arg)); + assert_eq!(e.to_string(), format!("missing argument to `{arg}'")); } else { panic!("parsing argument lists that end in -not should fail"); } @@ -1194,12 +1383,21 @@ mod tests { #[test] fn build_top_level_matcher_and_without_expr1() { - let mut config = Config::default(); + // The operator is named as it was spelled, not normalised to `-a`. + for arg in ["-and", "-a"] { + let mut config = Config::default(); - if let Err(e) = build_top_level_matcher(&["-a", "-true"], &mut config) { - assert!(e.to_string().contains("you have used a binary operator")); - } else { - panic!("parsing argument list that begins with -a should fail"); + if let Err(e) = build_top_level_matcher(&[arg, "-true"], &mut config) { + assert_eq!( + e.to_string(), + format!( + "invalid expression; you have used a binary operator \ + '{arg}' with nothing before it." + ) + ); + } else { + panic!("parsing argument list that begins with {arg} should fail"); + } } } @@ -1354,14 +1552,37 @@ mod tests { &["-type", "f", "(", "-name", "*.txt", ")", ")"], &mut config, ) { - assert!(e - .to_string() - .contains("expected expression before closing parentheses ')'")); + assert_eq!(e.to_string(), "you have too many ')'"); } else { panic!("parsing argument list with too many closing brackets should fail"); } } + #[test] + fn build_top_level_matcher_operator_without_operand() { + // An operator left dangling at the end names only itself; one cut off + // by a ')' names both, since the ')' is what ended the expression. + let cases: [(&[&str], &str); 4] = [ + (&["-false", ","], "expected an expression after ','"), + (&["-not"], "expected an expression after '-not'"), + ( + &["(", "-empty", "-or", ")"], + "expected an expression between '-or' and ')'", + ), + ( + &["(", "!", ")", "-print"], + "expected an expression between '!' and ')'", + ), + ]; + for (args, expected) in cases { + let mut config = Config::default(); + match build_top_level_matcher(args, &mut config) { + Err(e) => assert_eq!(e.to_string(), expected), + Ok(_) => panic!("{args:?} should fail to parse"), + } + } + } + #[test] fn build_top_level_matcher_can_use_bracket_as_arg() { let mut config = Config::default(); @@ -1899,6 +2120,51 @@ mod tests { } } + /// Parses `args`, expecting a failure, and returns the index of the + /// argument the resulting diagnostic points at. + fn failing_arg_index(args: &[&str]) -> Option { + let mut config = Config::default(); + let err = build_top_level_matcher(args, &mut config) + .err() + .expect("expression should have been rejected"); + err.downcast_ref::() + .expect("expression errors should carry an argument index") + .arg_index() + } + + #[test] + fn expression_errors_point_at_the_offending_argument() { + // The predicate that is missing its argument, not the end of the list. + assert_eq!(failing_arg_index(&["-true", "-a", "-name"]), Some(2)); + // The mistyped predicate. + assert_eq!(failing_arg_index(&["-true", "-o", "-nmae", "x"]), Some(2)); + // The operator with nothing after it. + assert_eq!(failing_arg_index(&["-true", "-o"]), Some(1)); + // The operator with nothing before it. + assert_eq!(failing_arg_index(&["-o", "-true"]), Some(0)); + assert_eq!(failing_arg_index(&[",", "-true"]), Some(0)); + // The '(' that is never closed, rather than the end of the list. + assert_eq!(failing_arg_index(&["-true", "(", "-false"]), Some(1)); + // The '(' of the empty pair, rather than its ')'. + assert_eq!(failing_arg_index(&["-true", "(", ")"]), Some(1)); + // The ')' that has no opener. + assert_eq!(failing_arg_index(&["-true", ")"]), Some(1)); + } + + #[test] + fn unknown_predicates_suggest_a_close_match() { + let mut config = Config::default(); + let err = build_top_level_matcher(&["-nmae", "x"], &mut config) + .err() + .expect("typo should have been rejected"); + // The suggestion travels as help text, leaving the message GNU-compatible. + assert_eq!(err.to_string(), "unknown predicate `-nmae'"); + assert_eq!( + err.downcast_ref::().unwrap().help(), + Some("did you mean `-name'?") + ); + } + #[test] fn build_top_level_matcher_option_logical() { let mut config = Config::default(); diff --git a/src/find/mod.rs b/src/find/mod.rs index cbc49f7c..b3c1982f 100644 --- a/src/find/mod.rs +++ b/src/find/mod.rs @@ -4,8 +4,10 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. +pub mod error; pub mod matchers; +use error::ParseError; use matchers::{Follow, WalkEntry}; use std::cell::RefCell; use std::error::Error; @@ -239,7 +241,11 @@ fn parse_args(args: &[&str]) -> Result> { if i == paths_start { paths.push(".".to_string()); } - let matcher = matchers::build_top_level_matcher(&args[i..], &mut config)?; + // The matcher builder only sees the expression part of the command line, so + // any argument index it reports has to be moved back to where that part + // started. + let matcher = matchers::build_top_level_matcher(&args[i..], &mut config) + .map_err(|e| ParseError::shift(e, i))?; let mut files0_paths = None; if let Some(name) = &config.files0_argument { if paths.len() == 1 && paths[0] == "." { @@ -518,7 +524,17 @@ pub fn find_main(args: &[&str], deps: &dyn Dependencies) -> i32 { match do_find(&args[1..], deps) { Ok(ret) => ret, Err(e) => { - writeln!(&mut stderr(), "{}: {e}", program_name()).unwrap(); + // `do_find` was handed argv without the program name. + let e = ParseError::shift(e, 1); + // Only at a terminal or with `UUTILS_DIAG=always`, so captured + // output (the GNU and bfs suites) keeps the single GNU line. + let rendered = uucore::diagnostics::enabled() + && e.downcast_ref::() + .is_some_and(|parse_error| parse_error.render(args)); + // The report heads itself with the same line. + if !rendered { + writeln!(&mut stderr(), "{}: {e}", program_name()).unwrap(); + } 1 } } diff --git a/tests/test_find.rs b/tests/test_find.rs index 2a19a3ed..95df061f 100644 --- a/tests/test_find.rs +++ b/tests/test_find.rs @@ -24,6 +24,13 @@ use common::test_helpers::fix_up_slashes; mod common; +/// How `find` names itself in errors. +const PROGRAM: &str = "find"; + +/// How uucore heads an expression report: argv[0]'s base name, which keeps the +/// `.exe` on Windows. +const REPORT_PROGRAM: &str = if cfg!(windows) { "find.exe" } else { "find" }; + /// Returns a UCommand for `find` with the working directory set to the /// repository root, so that tests using relative `test_data/` paths work. fn ucmd() -> uutests::util::UCommand { @@ -179,6 +186,67 @@ fn mindepth_exceeds_maxdepth_outputs_nothing() { .no_stdout(); } +#[test] +fn expression_diagnostics_stay_out_of_the_way_of_scripts() { + // A test harness captures stderr, so it is never a terminal and the error + // must stay the single GNU-compatible line -- which is what the GNU and + // bfs compatibility suites assert on. `never` holds even at a terminal; + // any value other than `always` reads as unset rather than as "on". + for value in [None, Some(""), Some("never"), Some("NEVER"), Some("1")] { + let mut command = ucmd(); + command.args(&["-true", "-o", "-nmae", "x"]); + if let Some(value) = value { + command.env("UUTILS_DIAG", value); + } + let output = command.fails().no_stdout().stderr_str().to_owned(); + assert_eq!( + output.trim_end(), + format!("{PROGRAM}: unknown predicate `-nmae'") + ); + } +} + +#[test] +fn expression_diagnostics_underline_the_bad_argument() { + // Asking for a report has to produce one, including at a redirected + // stderr like this test's. + for value in ["always", "ALWAYS"] { + let output = ucmd() + .args(&["-true", "-o", "-nmae", "x"]) + .env("UUTILS_DIAG", value) + .fails() + .no_stdout() + .stderr_str() + .to_owned(); + + // The plain message still leads, so nothing that matched before stops + // matching. + assert!(output.starts_with(&format!("{REPORT_PROGRAM}: unknown predicate `-nmae'\n"))); + assert!(output.contains("-true -o -nmae x")); + assert!(output.contains("not a known predicate")); + assert!(output.contains("did you mean `-name'?")); + // Piped output must not be coloured. + assert!(!output.contains('\u{1b}')); + } +} + +#[test] +fn expression_diagnostics_do_not_guess_at_two_letter_arguments() { + // `-H` is a real option, valid only before the paths; put it after them and + // it reaches the expression parser. Every two-character predicate is one + // edit away from it, so any suggestion here would be arbitrary. + let output = ucmd() + .args(&[".", "-H"]) + .env("UUTILS_DIAG", "always") + .fails() + .no_stdout() + .stderr_str() + .to_owned(); + + assert!(output.starts_with(&format!("{REPORT_PROGRAM}: unknown predicate `-H'\n"))); + assert!(!output.contains("did you mean")); +} + #[test] fn multiple_matcher_failure() { ucmd() @@ -434,7 +502,7 @@ fn files0_basic() { ucmd() .arg("-files0-from") .fails() - .stderr_contains("missing argument to -files0-from") + .stderr_contains("missing argument to `-files0-from'") .no_stdout(); } @@ -779,7 +847,7 @@ fn find_printf_width_too_large() { "%70000s\\n", ]) .fails() - .stderr_contains("find: Format width too large"); + .stderr_contains(format!("{PROGRAM}: Format width too large")); ucmd() .args(&[ "./test_data/simple", @@ -789,7 +857,7 @@ fn find_printf_width_too_large() { "%99999999999999999999s\\n", ]) .fails() - .stderr_contains("find: Invalid format width"); + .stderr_contains(format!("{PROGRAM}: Invalid format width")); } #[test] @@ -807,7 +875,7 @@ fn find_printf_multibyte_char_after_directive() { ucmd() .args(&["./test_data/simple", "-maxdepth", "0", "-printf", "%A€"]) .fails() - .stderr_contains("find: Invalid time specifier"); + .stderr_contains(format!("{PROGRAM}: Invalid time specifier")); } #[cfg(unix)] @@ -1107,18 +1175,21 @@ fn find_newer_xy() { .no_stderr(); } - ucmd().args(&[".", arg, "invalid"]).fails().stderr_only( - "find: I cannot figure out how to interpret ‘invalid’ as a date or time\n", - ); + ucmd() + .args(&[".", arg, "invalid"]) + .fails() + .stderr_only(format!( + "{PROGRAM}: I cannot figure out how to interpret ‘invalid’ as a date or time\n" + )); } #[cfg(target_os = "linux")] ucmd() .args(&[".", "-newerBt", "jan 01, 2000"]) .fails() - .stderr_only( - "find: This system does not provide a way to find the birth time of a file.\n", - ); + .stderr_only(format!( + "{PROGRAM}: This system does not provide a way to find the birth time of a file.\n" + )); } #[test] @@ -1140,9 +1211,9 @@ fn find_age_range() { ucmd() .args(&["test_data/simple", arg, time_string]) .fails() - .stderr_contains( - "find: Expected a decimal integer (with optional + or - prefix) argument to", - ) + .stderr_contains(format!( + "{PROGRAM}: Expected a decimal integer (with optional + or - prefix) argument to" + )) .no_stdout(); } } @@ -1340,12 +1411,12 @@ fn find_fprintf_missing_arguments() { ucmd() .args(&["-fprintf"]) .fails() - .stderr_contains("missing argument to -fprintf"); + .stderr_contains("missing argument to `-fprintf'"); ucmd() .args(&["-fprintf", "/tmp/find_fprintf_out"]) .fails() - .stderr_contains("missing argument to -fprintf"); + .stderr_contains("missing argument to `-fprintf'"); } #[test] @@ -1542,7 +1613,7 @@ fn find_ok_missing_semicolon() { .args(&["test_data/simple", "-ok", "echo", "{}"]) .pipe_in("") .fails() - .stderr_contains("missing argument to -ok") + .stderr_contains("missing argument to `-ok'") .no_stdout(); }