From 22273eeb1c2ebd57b55daf6ec5e2af68702e20d4 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Sat, 1 Aug 2026 23:33:40 +0900 Subject: [PATCH] fix(parse): enforce double_dash="required" for positional args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parser never looked at `SpecDoubleDashChoices::Required`. Seeing a `--` only disabled flag parsing, so an arg declared arg "[file]" double_dash="required" was filled whether or not the separator was given, and an arg sitting behind a greedy variadic was never filled at all — even after a `--`. Reported against mise in jdx/mise#5759, where `mise run echo test` and `mise run echo -- test` both succeed. Both halves of clap's `Arg::last(true)` are now implemented, which is what `double_dash="required"` is generated from: - A word offered to such an arg before an explicit `--` is rejected with `ArgRequiresDoubleDash` and not assigned. A variadic arg reports this once rather than once per word, and an arg that is both `required` and `double_dash="required"` no longer also collects `MissingArg`. - An explicit `--` moves the positional cursor onto the arg that requires it, past earlier args including a greedy variadic. Routing can leave an earlier arg empty, so "next arg" and "how many args are filled" no longer coincide. The cursor becomes an explicit index and the two loops that used `skip(out.args.len())` to find unfilled args — the `MissingArg` check and the default/env application in `Parser::parse` — now ask `out.args` by key instead of counting. A `--` that `double_dash="preserve"` keeps as a value stays a value: it does not count as a separator and does not unlock a required arg. `restart_token` resets the separator, so each invocation after it needs its own `--`. `ParseOutput` gains `next_arg` and `double_dash_seen` so completions follow the same cursor as the parser. Every path that completes a positional — the one at the cursor, the first arg after a `restart_token`, and a default subcommand's — goes through one helper, so the rule cannot be honoured in one and forgotten in another. Past a `--` the completion also stops offering flags, since the parser reads everything after it as a positional value. --- cli/src/cli/complete_word.rs | 109 ++++-- cli/tests/complete_word.rs | 96 +++++ docs/spec/reference/arg.md | 40 +- .../double-dash-default-subcommand.usage.kdl | 14 + examples/double-dash.usage.kdl | 37 ++ lib/src/error.rs | 3 + lib/src/lib.rs | 2 +- lib/src/parse.rs | 364 +++++++++++++++++- lib/tests/parse.rs | 111 ++++++ 9 files changed, 729 insertions(+), 47 deletions(-) create mode 100644 examples/double-dash-default-subcommand.usage.kdl create mode 100644 examples/double-dash.usage.kdl 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/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/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 84c96c14..43f2fe32 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