diff --git a/Cargo.lock b/Cargo.lock index 2b869c8c..37d9f18b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1930,7 +1930,7 @@ checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" [[package]] name = "usage-cli" -version = "4.1.0" +version = "5.0.0" dependencies = [ "assert_cmd", "clap", @@ -1961,7 +1961,7 @@ dependencies = [ [[package]] name = "usage-lib" -version = "4.1.0" +version = "5.0.0" dependencies = [ "clap", "criterion", diff --git a/Cargo.toml b/Cargo.toml index e479d49c..22ad4833 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,7 +16,7 @@ license = "MIT" [workspace.dependencies] clap_usage = { path = "./clap_usage", version = "4.0.0" } usage-cli = { path = "./cli" } -usage-lib = { path = "./lib", version = "4.1.0", features = ["clap"] } +usage-lib = { path = "./lib", version = "5.0.0", features = ["clap"] } [workspace.metadata.release] allow-branch = ["main"] diff --git a/cli/Cargo.toml b/cli/Cargo.toml index ebb7697a..f9b08540 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "usage-cli" edition = "2021" -version = "4.1.0" +version = "5.0.0" description = "CLI for working with usage-based CLIs" license = { workspace = true } authors = { workspace = true } diff --git a/cli/src/cli/complete_word.rs b/cli/src/cli/complete_word.rs index f1c1cbf9..e8f8a068 100644 --- a/cli/src/cli/complete_word.rs +++ b/cli/src/cli/complete_word.rs @@ -12,8 +12,7 @@ use std::sync::LazyLock; use xx::process::check_status; use xx::{regex, XXError, XXResult}; -use usage::parse::{ParseOutput, ParseValue}; -use usage::{Spec, SpecArg, SpecCommand, SpecComplete, SpecFlag}; +use usage::{Spec, SpecArg, SpecCommand, SpecComplete, SpecDoubleDashChoices, SpecFlag}; use crate::cli::generate; @@ -120,13 +119,16 @@ impl CompleteWord { // Not `available_flags`: inside a mounted command, the mounting CLI's flags stay // recognized for parsing but are not accepted there, so they must not be offered. let flags = parsed.completion_flags(); - let mut choices = if ctoken == "-" { + // An explicit `--` stops the parser reading flags, so past one there is no such thing + // as a flag to complete — a dash-prefixed word is a positional value. + let flags_possible = !parsed.double_dash_seen; + let mut choices = if flags_possible && ctoken == "-" { let shorts = self.complete_short_flag_names(&flags, ""); let longs = self.complete_long_flag_names(&flags, ""); shorts.into_iter().chain(longs).collect::>() - } else if ctoken.starts_with("--") { + } else if flags_possible && ctoken.starts_with("--") { self.complete_long_flag_names(&flags, &ctoken) - } else if ctoken.starts_with('-') { + } else if flags_possible && ctoken.starts_with('-') { self.complete_short_flag_names(&flags, &ctoken) } else if after_restart_token { // After a restart_token, complete from the first arg of the current command @@ -134,8 +136,16 @@ impl CompleteWord { // but before flag_awaiting_value (since restart clears pending flag values) let mut choices = vec![]; if let Some(arg) = parsed.cmd.args.first() { - has_explicit_choices = arg.choices.is_some(); - choices.extend(self.complete_arg(&ctx, spec, &parsed.cmd, arg, &ctoken)?); + let (found, constrained) = self.complete_positional( + &ctx, + spec, + &parsed.cmd, + arg, + &ctoken, + parsed.double_dash_seen, + )?; + has_explicit_choices = constrained; + choices.extend(found); } choices } else if let Some(flag) = parsed.flag_awaiting_value.first() { @@ -144,9 +154,17 @@ impl CompleteWord { self.complete_arg(&ctx, spec, &parsed.cmd, arg, &ctoken)? } else { let mut choices = vec![]; - if let Some(arg) = next_arg_for_completion(&parsed) { - has_explicit_choices = arg.choices.is_some(); - choices.extend(self.complete_arg(&ctx, spec, &parsed.cmd, arg, &ctoken)?); + if let Some(arg) = parsed.next_arg.as_deref() { + let (found, constrained) = self.complete_positional( + &ctx, + spec, + &parsed.cmd, + arg, + &ctoken, + parsed.double_dash_seen, + )?; + has_explicit_choices = constrained; + choices.extend(found); } if !parsed.cmd.subcommands.is_empty() { choices.extend(self.complete_subcommands(&parsed.cmd, &ctoken)); @@ -155,23 +173,37 @@ impl CompleteWord { if parsed.cmd.name == spec.cmd.name { if let Some(default_name) = &spec.default_subcommand { if let Some(default_cmd) = spec.cmd.find_subcommand(default_name) { - // Include completions from default subcommand's first arg + // Include completions from default subcommand's first arg. + // + // The `constrained` half is dropped on purpose: unlike the two call + // sites above, this arg belongs to a *different* command and is only + // a guess that the user means to elide the subcommand name. Letting + // its choices set `has_explicit_choices` would suppress the root + // command's own file fallback whenever the token failed to match + // them — see `complete_word_default_subcommand_choices_do_not_block_ + // root_file_fallback`. The `double_dash="required"` rule does apply, + // which is why this goes through the helper at all. if let Some(arg) = default_cmd.args.first() { - choices.extend(self.complete_arg( + let (found, _) = self.complete_positional( &ctx, spec, default_cmd, arg, &ctoken, - )?); + parsed.double_dash_seen, + )?; + choices.extend(found); } } } } choices }; - // Fallback to file completions if nothing is known about this argument and it's not a flag - if choices.is_empty() && !ctoken.starts_with('-') && !has_explicit_choices { + // Fallback to file completions if nothing is known about this argument and it's not a + // flag. Past a `--` a dash-prefixed word is not a flag but a value, so a path like + // `-input` still gets completed there. + let looks_like_a_flag = flags_possible && ctoken.starts_with('-'); + if choices.is_empty() && !looks_like_a_flag && !has_explicit_choices { let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); let files = self.complete_path(&cwd, &ctoken, |_| true); choices = files.into_iter().map(|n| (n, String::new())).collect(); @@ -259,6 +291,36 @@ impl CompleteWord { names.into_iter().map(|n| (n, String::new())).collect() } + /// Completions for a positional argument, under the rule the parser enforces for + /// `double_dash="required"`: nothing reaches such an argument until an explicit `--` has + /// been typed, so before that the separator is the only useful candidate. + /// + /// Every path that completes a positional goes through here — the one at the parser's + /// cursor, the first argument after a `restart_token`, and the default subcommand's — so + /// the rule cannot be honoured in one and forgotten in another. + /// + /// The second return value says whether the argument constrains what may go there, which + /// is what suppresses the file-path fallback. + fn complete_positional( + &self, + ctx: &tera::Context, + spec: &Spec, + cmd: &SpecCommand, + arg: &SpecArg, + ctoken: &str, + double_dash_seen: bool, + ) -> miette::Result<(Vec<(String, String)>, bool)> { + if arg.double_dash == SpecDoubleDashChoices::Required && !double_dash_seen { + // No filename is valid here either, so the fallback stays off. + let separator = ctoken.is_empty().then(|| ("--".to_string(), String::new())); + return Ok((separator.into_iter().collect(), true)); + } + Ok(( + self.complete_arg(ctx, spec, cmd, arg, ctoken)?, + arg.choices.is_some(), + )) + } + fn complete_arg( &self, ctx: &tera::Context, @@ -375,23 +437,6 @@ impl CompleteWord { } } -fn next_arg_for_completion(parsed: &ParseOutput) -> Option<&SpecArg> { - parsed.cmd.args.iter().find(|arg| { - let parsed_value = parsed - .args - .iter() - .find_map(|(parsed_arg, value)| (parsed_arg.name == arg.name).then_some(value)); - - match parsed_value { - Some(ParseValue::MultiString(values)) if arg.var => { - values.len() < arg.var_max.unwrap_or(usize::MAX) - } - Some(_) => false, - None => true, - } - }) -} - /// Wrap a completion value in single quotes if any character would otherwise /// be interpreted by the shell. The result is meant to be inserted by /// `compadd -Q` verbatim, so the user sees consistent single-quote quoting diff --git a/cli/tests/complete_word.rs b/cli/tests/complete_word.rs index 172cdb70..4cc87518 100644 --- a/cli/tests/complete_word.rs +++ b/cli/tests/complete_word.rs @@ -1,6 +1,7 @@ use assert_cmd::assert::Assert; use assert_cmd::cargo; use std::env; +use std::fs; use std::process::Command; use assert_cmd::prelude::*; @@ -25,6 +26,101 @@ fn complete_word_variadic_arg_reuses_completer() { .stdout("foo\nbar\n"); } +#[test] +fn complete_word_double_dash_required_offers_the_separator() { + // The parser rejects anything given to a `double_dash="required"` arg before `--`, so + // there is exactly one useful thing to complete there. + assert_cmd("double-dash.usage.kdl", &["--", "separator", ""]).stdout("--\n"); + // Once the separator is in, the arg completes normally. + assert_cmd("double-dash.usage.kdl", &["--", "separator", "--", ""]).stdout("alpha\nbeta\n"); +} + +#[test] +fn complete_word_double_dash_stops_flag_completion() { + // Before the separator, a dash-prefixed token completes to flags as usual... + assert_cmd("double-dash.usage.kdl", &["--", "separator", "--v"]).stdout(contains("--verbose")); + // ...and after it there are no flags left to complete: the parser reads everything past + // `--` as a positional value, so offering `--verbose` would suggest something it would + // hand to verbatim. + assert_cmd("double-dash.usage.kdl", &["--", "separator", "--", "--v"]).stdout(""); +} + +#[test] +fn complete_word_double_dash_keeps_file_fallback_for_dash_prefixed_values() { + // A dash-prefixed word means a flag before the separator and a value after it, so path + // completion has to be withheld in the first case and offered in the second. Needs a file + // whose name starts with `-` to tell the two apart, hence the scratch directory. + let dir = std::env::temp_dir().join(format!("usage_dash_file_{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + fs::write(dir.join("-dashed.txt"), "").unwrap(); + + let spec = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .unwrap() + .join("examples") + .join("double-dash.usage.kdl"); + let run = |args: &[&str]| { + let mut c = Command::new(cargo::cargo_bin!("usage")); + c.current_dir(&dir) + .args(["cw", "--shell", "fish", "-f"]) + .arg(&spec) + .arg("mycli") + .args(args); + String::from_utf8(c.output().unwrap().stdout).unwrap() + }; + + // The leading `--` is clap's own escape, so the words start after it. + assert!( + !run(&["--", "paths", "-d"]).contains("-dashed.txt"), + "before `--`, `-d` is a flag prefix" + ); + assert!( + run(&["--", "paths", "--", "-d"]).contains("-dashed.txt"), + "after `--`, `-d` is the start of a value" + ); + + fs::remove_dir_all(&dir).unwrap(); +} + +#[test] +fn complete_word_double_dash_applies_after_a_restart_token() { + // A restart_token starts a fresh invocation, so the separator has to be typed again. + assert_cmd( + "double-dash.usage.kdl", + &["--", "restarted", "--", "alpha", ":::", ""], + ) + .stdout("--\n"); + assert_cmd( + "double-dash.usage.kdl", + &["--", "restarted", "--", "alpha", ":::", "--", ""], + ) + .stdout("alpha\nbeta\n"); +} + +#[test] +fn complete_word_double_dash_applies_to_a_default_subcommand() { + // Root-level completion reaches the default subcommand's first arg by its own path. + assert_cmd("double-dash-default-subcommand.usage.kdl", &["--", ""]) + .stdout(contains("--\n")) + .stdout(contains("alpha").not()); + assert_cmd( + "double-dash-default-subcommand.usage.kdl", + &["--", "--", ""], + ) + .stdout(contains("alpha")) + .stdout(contains("beta")); +} + +#[test] +fn complete_word_double_dash_routes_past_greedy_variadic() { + // Before the separator the greedy variadic is still the target... + assert_cmd("double-dash.usage.kdl", &["--", "routed", ""]).stdout("one\ntwo\n"); + assert_cmd("double-dash.usage.kdl", &["--", "routed", "one", ""]).stdout("one\ntwo\n"); + // ...and after it the completion follows the parser onto the arg that required it. + assert_cmd("double-dash.usage.kdl", &["--", "routed", "one", "--", ""]).stdout("alpha\nbeta\n"); +} + #[test] fn complete_word_variadic_arg_respects_var_max() { assert_cmd("variadic-completion.usage.kdl", &["--", "bounded", ""]).stdout("foo\nbar\n"); diff --git a/cli/usage.usage.kdl b/cli/usage.usage.kdl index 7346ee54..ee0feb78 100644 --- a/cli/usage.usage.kdl +++ b/cli/usage.usage.kdl @@ -2,7 +2,7 @@ min_usage_version "4.0" name usage-cli bin usage -version "4.1.0" +version "5.0.0" about "CLI for working with usage-based CLIs" usage "Usage: usage-cli [OPTIONS] [COMPLETIONS] " flag --usage-spec help="Outputs a `usage.kdl` spec for this CLI itself" diff --git a/docs/cli/reference/commands.json b/docs/cli/reference/commands.json index 41f3b5c0..1664acf5 100644 --- a/docs/cli/reference/commands.json +++ b/docs/cli/reference/commands.json @@ -1136,7 +1136,7 @@ "config": { "props": {} }, - "version": "4.1.0", + "version": "5.0.0", "usage": "Usage: usage-cli [OPTIONS] [COMPLETIONS] ", "complete": {}, "source_code_link_template": "https://github.com/jdx/usage/blob/main/cli/src/cli/{{path}}.rs", diff --git a/docs/cli/reference/index.md b/docs/cli/reference/index.md index 2a35421f..afcd6107 100644 --- a/docs/cli/reference/index.md +++ b/docs/cli/reference/index.md @@ -4,7 +4,7 @@ **Usage**: `usage [--usage-spec] [COMPLETIONS] ` -**Version**: 4.1.0 +**Version**: 5.0.0 - **Usage**: `usage [--usage-spec] [COMPLETIONS] ` diff --git a/docs/spec/reference/arg.md b/docs/spec/reference/arg.md index 9853efcc..3603cee7 100644 --- a/docs/spec/reference/arg.md +++ b/docs/spec/reference/arg.md @@ -50,8 +50,42 @@ arg "" { arg "" long_help="longer help for --help (as oppoosed to -h)" // double-dash behavior -arg "" double_dash="required" // arg must be passed after a double dash (e.g. mycli -- file.txt) -arg "" double_dash="optional" // arg may be passed after a double dash (e.g. mycli -- file.txt or mycli file.txt) -arg "..." double_dash="automatic" // once arg is passed, behave as if a double dash was passed (e.g. mycli file.txt --filewithdash) +arg "" double_dash="required" // arg only accepts values after a double dash; `mycli file.txt` is an error, `mycli -- file.txt` is not +arg "<-- file>" // shorthand for double_dash="required" (also `arg "[-- file]"`, `arg "<-- files>..."`) +arg "" double_dash="optional" // arg may be passed after a double dash (e.g. mycli -- file.txt or mycli file.txt) — the default +arg "..." double_dash="automatic" // once arg is passed, behave as if a double dash was passed (e.g. mycli file.txt --filewithdash) — not yet enforced by the parser arg "..." double_dash="preserve" // preserve double dashes as args (e.g. mycli arg1 -- arg2 -- arg3) ``` + +## Double-Dash Behavior + +`double_dash="required"` is enforced while parsing, so the three points below are +what a caller of your CLI actually sees. + +**The arg is unreachable until a `--` is typed.** A word offered to it beforehand is +rejected with `Argument can only be set after a "--" separator`, and the value +is not assigned. A variadic arg reports this once, not once per word, and an arg that +is both `required` and `double_dash="required"` reports only this — not also +`Missing required arg`. + +**Everything after the `--` is routed to that arg**, jumping past earlier args — +including a greedy variadic that would otherwise swallow the rest. This matches +clap's [`Arg::last(true)`](https://docs.rs/clap/latest/clap/struct.Arg.html#method.last), +which is what specs generated from clap map to `double_dash="required"`. + +```kdl +arg "[tool]..." +arg "[-- command]..." +// mycli node@20 -- node app.js => tool=[node@20], command=[node, app.js] +// mycli -- ls => tool=[], command=[ls] +``` + +**Flag parsing stops at the `--`**, as usual, so `mycli -- --verbose` gives the arg the +literal string `--verbose` rather than setting a `--verbose` flag. + +Two interactions are worth calling out: + +- A `--` that `double_dash="preserve"` keeps as a value is a _value_, not a separator. + It does not unlock a `double_dash="required"` arg — one token cannot be both. +- A command's `restart_token` starts a fresh invocation, which resets the separator. + Each invocation after the token needs its own `--`. diff --git a/examples/double-dash-default-subcommand.usage.kdl b/examples/double-dash-default-subcommand.usage.kdl new file mode 100644 index 00000000..eeaae25d --- /dev/null +++ b/examples/double-dash-default-subcommand.usage.kdl @@ -0,0 +1,14 @@ +// A `default_subcommand` whose first arg requires `--`. +// +// Root-level completion also offers that arg, on a separate code path from the one the +// parser's cursor uses, so the `double_dash="required"` rule has to hold there too. +// Kept apart from `double-dash.usage.kdl` because `default_subcommand` is spec-level and +// would change what every command in that fixture completes to. + +default_subcommand "run" + +cmd "run" { + arg "[-- target]" required=#false { + choices "alpha" "beta" + } +} diff --git a/examples/double-dash.usage.kdl b/examples/double-dash.usage.kdl new file mode 100644 index 00000000..7db58928 --- /dev/null +++ b/examples/double-dash.usage.kdl @@ -0,0 +1,37 @@ +// `double_dash="required"` args, spelled with the `[-- name]` shorthand. +// +// Choices rather than a `complete` runner so the completions are resolved in-process +// and the fixture works on every platform. + +// A greedy variadic sits in front of the arg that requires `--`, so reaching the +// latter depends on the separator moving the parser's cursor past the former. +cmd routed { + arg "[args]…" var=#true required=#false { + choices "one" "two" + } + arg "[-- last]…" var=#true required=#false { + choices "alpha" "beta" + } +} + +// Nothing can be given to until a `--` shows up. The name avoids `file`/`dir`, +// which are completed as paths regardless of their choices. +cmd separator { + flag --verbose help="Not a flag once `--` has been typed" + arg "[-- target]" required=#false { + choices "alpha" "beta" + } +} + +// No choices, so completion falls back to paths — including for a dash-prefixed word, +// which past the separator is a value rather than a flag. +cmd paths { + arg "[-- target]" required=#false +} + +// A `restart_token` begins a fresh invocation, which needs its own `--`. +cmd restarted restart_token=":::" { + arg "[-- target]" required=#false { + choices "alpha" "beta" + } +} diff --git a/lib/Cargo.toml b/lib/Cargo.toml index 01759992..cd2b2f51 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "usage-lib" edition = "2021" -version = "4.1.0" +version = "5.0.0" rust-version = "1.80.0" include = [ "/Cargo.toml", diff --git a/lib/src/error.rs b/lib/src/error.rs index d117d9bd..336695ef 100644 --- a/lib/src/error.rs +++ b/lib/src/error.rs @@ -26,6 +26,9 @@ pub enum UsageErr { #[error("Missing required arg: <{0}>")] MissingArg(String), + #[error("Argument <{0}> can only be set after a `--` separator")] + ArgRequiresDoubleDash(String), + #[error("{0}")] Help(String), diff --git a/lib/src/lib.rs b/lib/src/lib.rs index f8bbf275..6360eb87 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -3,7 +3,7 @@ extern crate insta; extern crate log; pub use crate::parse::{available_flags, parse, Parser}; -pub use crate::spec::arg::SpecArg; +pub use crate::spec::arg::{SpecArg, SpecDoubleDashChoices}; pub use crate::spec::builder::{SpecArgBuilder, SpecCommandBuilder, SpecFlagBuilder}; pub use crate::spec::choices::SpecChoices; pub use crate::spec::cmd::SpecCommand; diff --git a/lib/src/parse.rs b/lib/src/parse.rs index 84df8960..996ee6d6 100644 --- a/lib/src/parse.rs +++ b/lib/src/parse.rs @@ -252,6 +252,19 @@ pub struct ParseOutput { pub available_flags: BTreeMap>, pub flag_awaiting_value: Vec>, pub errors: Vec, + /// The positional argument the next word would have filled, i.e. where the parser's + /// cursor stopped. `None` once every argument is satisfied. + /// + /// Completions need exactly this: the parser already accounts for `var_max`, for + /// `restart_token` rewinds, and for the jump an explicit `--` performs onto a + /// `double_dash="required"` argument, so re-deriving it from `args` would disagree. + pub next_arg: Option>, + /// Whether an explicit `--` was consumed *as a separator*. + /// + /// A `--` that `double_dash="preserve"` keeps as a value does not count: it is a value + /// of the variadic argument collecting it, not a separator, so it does not unlock a + /// `double_dash="required"` argument. + pub double_dash_seen: bool, } impl ParseOutput { @@ -350,7 +363,13 @@ impl<'a> Parser<'a> { }; // Apply env vars and defaults for args - for arg in out.cmd.args.iter().skip(out.args.len()) { + // + // Not `skip(out.args.len())`: an explicit `--` can jump the parser's cursor past an arg + // that stayed empty, leaving a gap that makes the fill count a wrong starting offset. + for arg in out.cmd.args.iter() { + if out.args.contains_key(arg) { + continue; + } if let Some(env_var) = arg.env.as_ref() { if let Some(env_value) = get_env(env_var) { validate_choice_value( @@ -541,6 +560,8 @@ fn parse_partial_with_env( available_flags: gather_flags(&spec.cmd), flag_awaiting_value: vec![], errors: vec![], + next_arg: None, + double_dash_seen: false, }; // Phase 1: Scan for subcommands and collect global flags @@ -662,9 +683,20 @@ fn parse_partial_with_env( // // Now that we've identified all subcommands and executed their mounts, // we can parse the remaining arguments, flags, and their values. - let mut next_arg = out.cmd.args.first(); + // The cursor into `out.cmd.args`, kept as an index rather than a reference because an + // explicit `--` may jump it *past* arguments that stay empty (see the `w == "--"` arm). + // With such a gap `out.args.len()` no longer equals the cursor, so anything asking "is this + // argument filled?" has to consult `out.args` by key instead of counting. + let mut next_arg_idx: usize = 0; let mut enable_flags = true; let mut grouped_flag = false; + // Whether an explicit `--` has been consumed *as a separator* (as opposed to being kept as a + // value by `double_dash="preserve"`). Args declared `double_dash="required"` only accept + // words that come after it — see `report_double_dash_violation`. + let mut seen_double_dash = false; + // Args already reported as filled without the `--` they require, so a variadic one does not + // report the same violation for every word it collects. + let mut double_dash_violations: HashSet = HashSet::new(); while !input.is_empty() { let mut w = input.pop_front().unwrap(); @@ -676,12 +708,15 @@ fn parse_partial_with_env( // e.g., `mise run lint ::: test ::: check` with restart_token=":::" if let Some(ref restart_token) = out.cmd.restart_token { if w == *restart_token { - // Reset argument parsing state for a fresh command invocation + // Reset argument parsing state for a fresh command invocation, keeping the + // flags. `double_dash_violations` is deliberately *not* cleared: `out.errors` + // is not cleared here either, so clearing it would let one arg report the same + // violation once per invocation. out.args.clear(); - next_arg = out.cmd.args.first(); + next_arg_idx = 0; out.flag_awaiting_value.clear(); // Clear any pending flag values enable_flags = true; // Reset -- separator effect - // Keep flags and continue parsing + seen_double_dash = false; // The next invocation needs its own `--` continue; } } @@ -692,14 +727,40 @@ fn parse_partial_with_env( // Only preserve the double dash token if we're collecting values for a variadic arg // in double_dash == `preserve` mode - let should_preserve = next_arg + let should_preserve = out + .cmd + .args + .get(next_arg_idx) .map(|arg| arg.var && arg.double_dash == SpecDoubleDashChoices::Preserve) .unwrap_or(false); if should_preserve { - // Fall through to arg parsing + // Fall through to arg parsing. This `--` is a *value*, not a separator, so it + // neither counts as one nor unlocks a `double_dash="required"` arg. } else { // Default behavior, skip the token + seen_double_dash = true; + + // Everything after an explicit `--` belongs to the arg that requires one, so + // jump the cursor there — past any earlier arg, including a greedy variadic + // that would otherwise swallow the rest. This mirrors clap's `Arg::last(true)`, + // which is what `double_dash="required"` is generated from. Specs without such + // an arg find nothing and keep the cursor where it was. + let target = out.cmd.args.iter().position(|arg| { + arg.double_dash == SpecDoubleDashChoices::Required + && !out.args.contains_key(arg) + }); + if let Some(target) = target { + // Forward only. An unfilled required arg declared *before* the cursor is + // left where it is rather than rewound to — words already assigned to + // later args would have to be taken back for that to mean anything, and + // the arg keeps its `MissingArg`. `double_dash="required"` mirrors clap's + // `Arg::last(true)`, which is the final positional, so a spec that puts + // one ahead of others is already outside what this models. + if target > next_arg_idx { + next_arg_idx = target; + } + } continue; } } @@ -720,6 +781,7 @@ fn parse_partial_with_env( custom_env, )?; if should_return { + record_cursor(&mut out, next_arg_idx, seen_double_dash); return Ok(out); } continue; @@ -757,6 +819,7 @@ fn parse_partial_with_env( if is_help_arg(spec, &w) { out.errors .push(render_help_err(spec, &out.cmd, w.len() > 2)); + record_cursor(&mut out, next_arg_idx, seen_double_dash); return Ok(out); } } @@ -793,6 +856,7 @@ fn parse_partial_with_env( if is_help_arg(spec, &w) { out.errors .push(render_help_err(spec, &out.cmd, w.len() > 2)); + record_cursor(&mut out, next_arg_idx, seen_double_dash); return Ok(out); } if grouped_flag { @@ -812,12 +876,23 @@ fn parse_partial_with_env( custom_env, )?; if should_return { + record_cursor(&mut out, next_arg_idx, seen_double_dash); return Ok(out); } continue; } - if let Some(arg) = next_arg { + if let Some(arg) = out.cmd.args.get(next_arg_idx) { + // Before anything else: an arg that requires `--` accepts nothing until one has been + // seen. Checking ahead of `validate_choices` keeps a discarded word from also being + // reported as an invalid choice, and from reaching that function's help escape. + if arg.double_dash == SpecDoubleDashChoices::Required && !seen_double_dash { + report_double_dash_violation(arg, &mut out.errors, &mut double_dash_violations); + // Drop the word without filling the arg or advancing the cursor: every later + // word hits the same arg and is rejected the same way, so the parse still ends + // in an error rather than in `unexpected word`. + continue; + } if validate_choices( spec, &out.cmd, @@ -827,6 +902,7 @@ fn parse_partial_with_env( arg.choices.as_ref(), custom_env, )? { + record_cursor(&mut out, next_arg_idx, seen_double_dash); return Ok(out); } if arg.var { @@ -838,24 +914,36 @@ fn parse_partial_with_env( .unwrap(); arr.push(w); if arr.len() >= arg.var_max.unwrap_or(usize::MAX) { - next_arg = out.cmd.args.get(out.args.len()); + next_arg_idx += 1; } } else { out.args .insert(Arc::new(arg.clone()), ParseValue::String(w)); - next_arg = out.cmd.args.get(out.args.len()); + next_arg_idx += 1; } continue; } if is_help_arg(spec, &w) { out.errors .push(render_help_err(spec, &out.cmd, w.len() > 2)); + record_cursor(&mut out, next_arg_idx, seen_double_dash); return Ok(out); } bail!("unexpected word: {w}"); } - for arg in out.cmd.args.iter().skip(out.args.len()) { + record_cursor(&mut out, next_arg_idx, seen_double_dash); + + // Not `skip(out.args.len())`: a `--` may have jumped the cursor past an arg that stayed + // empty, so position and fill count can disagree. Ask `out.args` which args it holds. + for arg in out.cmd.args.iter() { + if out.args.contains_key(arg) { + continue; + } + // Already reported as needing a `--`; one mistake should not yield two messages. + if double_dash_violations.contains(&arg.name) { + continue; + } if arg.required && arg.default.is_empty() { // Check if there's an env var available (custom env map takes precedence) let has_env = arg.env.as_ref().is_some_and(|e| { @@ -1088,6 +1176,30 @@ fn validate_choice_values( Ok(()) } +/// Publish where Phase 2 left its positional cursor, so callers that do not re-run the parse — +/// completions, above all — agree with it. Called on every exit from the loop, including the +/// early ones that render help, where the cursor is still the useful answer. +fn record_cursor(out: &mut ParseOutput, next_arg_idx: usize, seen_double_dash: bool) { + out.next_arg = out.cmd.args.get(next_arg_idx).cloned().map(Arc::new); + out.double_dash_seen = seen_double_dash; +} + +/// Record that `arg` was handed a word before the `--` it requires. +/// +/// A variadic arg would otherwise report the same mistake once per word it was offered, so the +/// message is emitted only the first time each arg is seen. The set is also what suppresses the +/// `MissingArg` that a `required` + `double_dash="required"` arg would otherwise collect at the +/// end of the parse. +fn report_double_dash_violation( + arg: &SpecArg, + errors: &mut Vec, + violations: &mut HashSet, +) { + if violations.insert(arg.name.clone()) { + errors.push(UsageErr::ArgRequiresDoubleDash(arg.name.clone())); + } +} + fn is_help_arg(spec: &Spec, w: &str) -> bool { spec.disable_help != Some(true) && (w == "--help" @@ -3129,6 +3241,236 @@ cmd "run" { assert_eq!(extra_value.to_string(), "-- arg1 -- --foo"); } + fn spec_with_args(args: impl IntoIterator) -> Spec { + let cmd = SpecCommand::builder().name("test").args(args).build(); + Spec { + name: "test".to_string(), + bin: "test".to_string(), + cmd, + ..Default::default() + } + } + + fn arg_value(parsed: &ParseOutput, name: &str) -> String { + let arg = parsed + .args + .keys() + .find(|a| a.name == name) + .unwrap_or_else(|| panic!("expected arg {name} to be parsed")); + parsed.args.get(arg).unwrap().to_string() + } + + fn required_arg(name: &str) -> SpecArg { + SpecArg::builder() + .name(name) + .var(true) + .required(false) + .double_dash(SpecDoubleDashChoices::Required) + .build() + } + + #[test] + fn test_double_dash_required_reports_error_once_for_variadic() { + // A variadic arg is offered every remaining word, but the mistake is one mistake. + let spec = spec_with_args([required_arg("files")]); + + let parsed = parse_partial(&spec, &input(&["test", "a", "b", "c"])).unwrap(); + + assert!(parsed.args.is_empty()); + assert_eq!(parsed.errors.len(), 1); + assert!( + matches!(&parsed.errors[0], UsageErr::ArgRequiresDoubleDash(name) if name == "files") + ); + } + + #[test] + fn test_double_dash_required_suppresses_missing_arg() { + // The arg is never filled, so the end-of-parse check would also call it missing. + let spec = spec_with_args([SpecArg::builder() + .name("file") + .required(true) + .double_dash(SpecDoubleDashChoices::Required) + .build()]); + + let parsed = parse_partial(&spec, &input(&["test", "x"])).unwrap(); + + assert_eq!(parsed.errors.len(), 1); + assert!(matches!( + &parsed.errors[0], + UsageErr::ArgRequiresDoubleDash(_) + )); + // The cursor stays put, so a completion keeps offering the same arg. + assert_eq!( + parsed.next_arg.as_ref().map(|a| a.name.as_str()), + Some("file") + ); + assert!(!parsed.double_dash_seen); + } + + #[test] + fn test_double_dash_routes_to_required_arg() { + // Everything after `--` belongs to the arg that requires it, even though the greedy + // variadic before it would otherwise swallow the rest (clap's `Arg::last(true)`). + let spec = spec_with_args([ + SpecArg::builder() + .name("tool") + .var(true) + .required(false) + .build(), + required_arg("command"), + ]); + + let parsed = parse(&spec, &input(&["test", "node@20", "--", "node", "app.js"])).unwrap(); + + assert_eq!(arg_value(&parsed, "tool"), "node@20"); + assert_eq!(arg_value(&parsed, "command"), "node app.js"); + assert!(parsed.double_dash_seen); + } + + #[test] + fn test_double_dash_routes_with_gap_reports_missing_arg() { + // Jumping the cursor leaves `tool` empty even though `command` is filled, so the + // "is it filled?" check cannot be a count of how many args were filled. + let spec = spec_with_args([ + SpecArg::builder() + .name("tool") + .var(true) + .required(true) + .build(), + required_arg("command"), + ]); + + let parsed = parse_partial(&spec, &input(&["test", "--", "ls"])).unwrap(); + + assert_eq!(arg_value(&parsed, "command"), "ls"); + assert!(parsed.args.keys().all(|a| a.name != "tool")); + assert!(parsed + .errors + .iter() + .any(|e| matches!(e, UsageErr::MissingArg(name) if name == "tool"))); + } + + #[test] + fn test_double_dash_gap_applies_defaults() { + // Same gap, seen from `Parser::parse`: the skipped arg still gets its default. + let spec = spec_with_args([ + SpecArg::builder() + .name("tool") + .var(true) + .required(false) + .default_value("node@20") + .build(), + required_arg("command"), + ]); + + let parsed = parse(&spec, &input(&["test", "--", "ls"])).unwrap(); + + assert_eq!(arg_value(&parsed, "command"), "ls"); + assert_eq!(arg_value(&parsed, "tool"), "node@20"); + } + + fn spec_with_restart_token_and_required_arg() -> Spec { + let run_cmd = SpecCommand::builder() + .name("run") + .arg(SpecArg::builder().name("task").build()) + .arg(required_arg("run_args")) + .restart_token(":::".to_string()) + .build(); + let mut cmd = SpecCommand::builder().name("test").build(); + cmd.subcommands.insert("run".to_string(), run_cmd); + Spec { + name: "test".to_string(), + bin: "test".to_string(), + cmd, + ..Default::default() + } + } + + #[test] + fn test_double_dash_required_restart_token_resets_separator() { + // The `--` before `:::` belongs to the previous invocation only. + let spec = spec_with_restart_token_and_required_arg(); + + let parsed = parse_partial( + &spec, + &input(&["test", "run", "task1", "--", "a", ":::", "task2", "b"]), + ) + .unwrap(); + + assert_eq!(arg_value(&parsed, "task"), "task2"); + assert!(parsed.args.keys().all(|a| a.name != "run_args")); + // Reported once even though the arg was violated after already succeeding once. + assert_eq!( + parsed + .errors + .iter() + .filter(|e| matches!(e, UsageErr::ArgRequiresDoubleDash(_))) + .count(), + 1 + ); + } + + #[test] + fn test_double_dash_required_restart_token_accepts_new_separator() { + let spec = spec_with_restart_token_and_required_arg(); + + let parsed = parse( + &spec, + &input(&["test", "run", "task1", "--", "a", ":::", "task2", "--", "c"]), + ) + .unwrap(); + + assert_eq!(arg_value(&parsed, "task"), "task2"); + assert_eq!(arg_value(&parsed, "run_args"), "c"); + } + + #[test] + fn test_double_dash_preserve_is_not_a_separator() { + // A `--` that `preserve` keeps is a *value* of that arg, so it must not unlock the + // arg that requires a separator. Deliberate: one token cannot be both. + let spec = spec_with_args([ + SpecArg::builder() + .name("kept") + .var(true) + .var_max(1) + .required(false) + .double_dash(SpecDoubleDashChoices::Preserve) + .build(), + required_arg("rest"), + ]); + + let parsed = parse_partial(&spec, &input(&["test", "--", "x"])).unwrap(); + + assert_eq!(arg_value(&parsed, "kept"), "--"); + assert!(parsed.args.keys().all(|a| a.name != "rest")); + assert!(!parsed.double_dash_seen); + assert_eq!(parsed.errors.len(), 1); + } + + #[test] + fn test_double_dash_required_does_not_bail_in_parse_partial() { + // Completions parse half-typed command lines; they must still get a result. + let spec = spec_with_args([required_arg("file")]); + + assert!(parse_partial(&spec, &input(&["test", "x"])).is_ok()); + assert!(parse(&spec, &input(&["test", "x"])).is_err()); + } + + #[test] + fn test_double_dash_without_required_arg_does_not_move_cursor() { + // Specs with no `double_dash="required"` arg are untouched by the jump. + let spec = spec_with_args([ + SpecArg::builder().name("first").required(false).build(), + SpecArg::builder().name("second").required(false).build(), + ]); + + let parsed = parse(&spec, &input(&["test", "--", "a", "b"])).unwrap(); + + assert_eq!(arg_value(&parsed, "first"), "a"); + assert_eq!(arg_value(&parsed, "second"), "b"); + assert!(parsed.next_arg.is_none()); + } + #[test] fn test_parser_with_custom_env_for_required_arg() { let spec = spec_with_arg( diff --git a/lib/tests/parse.rs b/lib/tests/parse.rs index 31264941..62a04aa8 100644 --- a/lib/tests/parse.rs +++ b/lib/tests/parse.rs @@ -299,6 +299,117 @@ variadic_flag_choices_err: args="--level debug --level invalid --level error", expected=r#"Invalid choice for option level: invalid, expected one of debug, info, warn, error"#, +double_dash_required_missing_separator: + spec=r#"arg "[file]" double_dash="required""#, + args="test.txt", + expected=r#"Argument can only be set after a `--` separator"#, + +double_dash_required_with_separator: + spec=r#"arg "[file]" double_dash="required""#, + args="-- test.txt", + expected=r#"{"usage_file": "test.txt"}"#, + +double_dash_required_absent_ok: + spec=r#"arg "[file]" double_dash="required""#, + args="", + expected=r#"{}"#, + +// `[-- file]` is shorthand for double_dash="required" +double_dash_required_shorthand: + spec=r#"arg "[-- file]""#, + args="test.txt", + expected=r#"Argument can only be set after a `--` separator"#, + +double_dash_required_shorthand_with_separator: + spec=r#"arg "[-- file]""#, + args="-- test.txt", + expected=r#"{"usage_file": "test.txt"}"#, + +// One mistake, one message: the arg is never filled, but it must not also be +// reported as missing. +double_dash_required_and_required_single_error: + spec=r#"arg "<-- file>""#, + args="test.txt", + expected=r#"Argument can only be set after a `--` separator"#, + +double_dash_required_and_required_missing: + spec=r#"arg "<-- file>""#, + args="", + expected=r#"Missing required arg: "#, + +// A variadic arg is offered every remaining word; it reports the violation once. +double_dash_required_var_reports_once: + spec=r#"arg "[-- files]...""#, + args="a b c", + expected=r#"Argument can only be set after a `--` separator"#, + +double_dash_required_var_with_separator: + spec=r#"arg "[-- files]...""#, + args="-- a b c", + expected=r#"{"usage_files": "a b c"}"#, + +double_dash_required_after_positional: + spec=r#" + arg "" + arg "[-- run]..." + "#, + args="mytask -- echo hi", + expected=r#"{"usage_run": "echo hi", "usage_task": "mytask"}"#, + +double_dash_required_after_positional_no_separator: + spec=r#" + arg "" + arg "[-- run]..." + "#, + args="mytask echo hi", + expected=r#"Argument can only be set after a `--` separator"#, + +// Flag parsing stays off after the separator, so the arg gets the literal word. +double_dash_required_captures_flags_after_separator: + spec=r#" + flag "--verbose" + arg "[-- file]" + "#, + args="-- --verbose", + expected=r#"{"usage_file": "--verbose"}"#, + +// The choices check must not fire for a word the double-dash rule already rejected. +double_dash_required_choices_not_validated: + spec=r#"arg "[-- shell]" { + choices "bash" "zsh" +}"#, + args="tcsh", + expected=r#"Argument can only be set after a `--` separator"#, + +// An explicit `--` routes the rest to the arg that requires it, even past a +// greedy variadic that would otherwise swallow everything. +double_dash_routes_past_greedy_var: + spec=r#" + arg "[args]..." + arg "[-- last]..." + "#, + args="a -- b", + expected=r#"{"usage_args": "a", "usage_last": "b"}"#, + +double_dash_routes_with_empty_leading_var: + spec=r#" + arg "[args]..." + arg "[-- last]..." + "#, + args="-- b", + expected=r#"{"usage_last": "b"}"#, + +// double_dash="optional" (the default) is unaffected in either direction. +double_dash_optional_without_separator: + spec=r#"arg "[file]""#, + args="test.txt", + expected=r#"{"usage_file": "test.txt"}"#, + +double_dash_optional_with_separator: + spec=r#"arg "[file]""#, + args="-- test.txt", + expected=r#"{"usage_file": "test.txt"}"#, + //shell_escape_arg: // spec=r#" // arg "" shell_escape=#true