diff --git a/benches/gate/tests/differential.rs b/benches/gate/tests/differential.rs index 3f5b1f55d..e69955a51 100644 --- a/benches/gate/tests/differential.rs +++ b/benches/gate/tests/differential.rs @@ -264,21 +264,6 @@ fn explained(o: Outcome) -> Option<&'static str> { clap: Question, } => Some("a question, which usage-lib answers by accepting rather than by erroring"), - // A global flag given at two levels: `mise -y config ls -y`. clap lets a global be - // repeated deeper down and the inner occurrence win; usage-argv sees the same flag - // twice and calls it a duplicate; usage-lib accepts. Found by the generator once it - // learned to put tokens *before* the command path, which is what makes this shape - // reachable at all. - // - // usage-argv is the strict one here, and the only one of the three, so this is - // usage-argv's to settle rather than something the fuzzer should paper over — - // recorded, with `a_repeated_global_is_usage_argvs_alone` below as the witness. - Outcome { - argv: Conflict, - lib: true, - clap: Accept, - } => Some("usage-argv calls a global given at two levels a duplicate; clap allows it"), - // usage-argv and clap both refuse; usage-lib accepts. Three of the four classes the // first run found are this shape: a missing subcommand, a flag whose value is missing, // and a repeated flag that may not repeat. usage-lib accepts all three. @@ -492,29 +477,39 @@ fn a_bare_word_diverges_because_of_the_mount_not_the_parser() { } #[test] -fn a_repeated_global_is_usage_argvs_alone() { - // Found by the generator once `head` let it put tokens before the command path. mise's - // `-y` is `global=#true`, and clap lets a global be given again on the subcommand — the - // inner occurrence simply wins. usage-argv sees one flag bound twice and refuses. - // - // Pinned rather than fixed: usage-argv is the only strict one of the three, and mise - // ships clap today, so `mise -y config ls -y` is a line that works now. Which way that - // should go is usage-argv's to settle; this records what is true meanwhile. +fn a_global_may_be_given_once_per_level() { + // Found by the generator once `head` let it put tokens before the command path, and fixed + // in the commit this test came with. `-y` is `global=#true`; clap lets it be given again on + // a subcommand — the inner occurrence wins — and refuses a repeat at one level. usage-argv + // now draws the same line, which matters because `mise -y install -y` works today. let spec = spec(); - let repeated: Vec = ["-y", "config", "ls", "-y"] - .iter() - .map(|s| s.to_string()) - .collect(); - let o = run(&spec, &repeated); - assert_eq!(o.argv, Verdict::Conflict, "{o:?}"); - assert_eq!(o.accepted(), (false, true, true), "{o:?}"); - assert!(explained(o).is_some(), "{o:?}"); - - // At one level, though, clap refuses too — so the divergence really is about globals - // crossing a command boundary, not about repetition. - let twice: Vec = ["-y", "-y"].iter().map(|s| s.to_string()).collect(); - let o = run(&spec, &twice); - assert_eq!((o.argv, o.clap), (Verdict::Conflict, Verdict::Conflict)); + let go = |words: &[&str]| { + run( + &spec, + &words.iter().map(|s| s.to_string()).collect::>(), + ) + }; + + // Across a boundary: allowed, at any depth, and more than once. + for words in [ + &["-y", "config", "ls", "-y"][..], + &["-y", "config", "-y", "ls"][..], + ] { + let o = go(words); + assert_eq!(o.accepted(), (true, true, true), "{words:?} {o:?}"); + } + + // Twice at one level: refused, and refused by clap too — whether that level is the root or + // a command three words in. This is the half a blanket exemption for globals would lose. + for words in [ + &["-y", "-y"][..], + &["config", "ls", "-y", "-y"][..], + &["-y", "config", "ls", "-y", "-y"][..], + ] { + let o = go(words); + assert_eq!(o.argv, Verdict::Conflict, "{words:?} {o:?}"); + assert_eq!(o.clap, Verdict::Conflict, "{words:?} {o:?}"); + } } #[test] diff --git a/conformance/tests/global_given_twice.rs b/conformance/tests/global_given_twice.rs new file mode 100644 index 000000000..56a11f1c2 --- /dev/null +++ b/conformance/tests/global_given_twice.rs @@ -0,0 +1,213 @@ +//! A `global` flag may be given once per command, not once per line. +//! +//! It is in scope for every descendant, so `ex -y sub -y` names it twice and means it once — +//! the inner occurrence wins. clap works that way, and mise ships clap, so `mise -y install -y` +//! is a line that works today; usage-argv refused it as a duplicate. +//! +//! Repeating it at *one* level is still an error, which is why the check is per level rather +//! than simply off for globals. clap draws the same line, and both halves are measured here. +//! +//! The rule holds however the global is declared: plainly, with a negation, or inside a +//! flattened group — each of which tracks its duplicates differently, so each is measured. + +use std::ffi::OsStr; + +use usage_argv::Error; +use usage_derive::{Args, Cli, Subcommands}; + +/// Do the thing +#[derive(Args)] +struct Sub { + /// Its own flag + #[usage(long)] + thing: bool, + #[usage(subcommand)] + command: Option, +} + +/// Deeper still +#[derive(Args)] +struct Leaf { + #[usage(long)] + leafy: bool, +} + +#[derive(Subcommands)] +enum Deeper { + /// A third level + Leaf(Leaf), +} + +#[derive(Subcommands)] +enum Commands { + /// A second level + Sub(Sub), +} + +/// What every level shares, spliced in rather than declared again +#[derive(Args)] +struct Shared { + /// Global, and living in a flattened group — whose partial is its own, so the + /// level markers have to be cleared through it rather than around it + #[usage(long, global)] + force: bool, + /// Global with a negation, in the flattened group too + #[usage(long = "tint", negate = "--no-tint", global)] + tint: bool, +} + +#[derive(Cli)] +#[usage(bin = "ex")] +struct Ex { + /// Say yes to everything + #[usage(long, short = 'y', global)] + yes: bool, + /// Global and negatable, so both spellings are in play at every level + #[usage(long = "colour", negate = "--no-colour", global)] + colour: bool, + /// Not global, for contrast + #[usage(long, short = 'q')] + quiet: bool, + #[usage(flatten)] + shared: Shared, + #[usage(subcommand)] + command: Option, +} + +fn parse(words: &[&str]) -> Result { + let owned: Vec<&OsStr> = words.iter().map(|s| OsStr::new(*s)).collect(); + Ex::parse_from(&owned).map_err(|e| format!("{e:?}")) +} + +/// The failure a line produced, or `None` if it parsed. `Ex` derives no `Debug`, which a +/// generated struct has no reason to, so this reports the error rather than the value. +fn failure(words: &[&str]) -> Option { + parse(words).err() +} + +#[test] +fn across_a_command_boundary_the_inner_occurrence_wins() { + let parsed = parse(&["-y", "sub", "-y"]).expect("a global may be repeated deeper"); + assert!(parsed.yes); + + // Three levels, given at each: still one flag, meant once. + let parsed = parse(&["-y", "sub", "-y", "leaf", "-y"]).expect("at every level"); + assert!(parsed.yes); + + // And the long form, which is the same flag by another name. + let parsed = parse(&["--yes", "sub", "--yes"]).expect("long form too"); + assert!(parsed.yes); +} + +#[test] +fn twice_at_one_level_is_still_a_duplicate() { + // The half a blanket exemption for globals would lose. clap refuses these too. + for words in [ + &["-y", "-y"][..], + &["sub", "-y", "-y"][..], + &["-y", "sub", "-y", "-y"][..], + &["--yes", "--yes"][..], + ] { + let err = failure(words) + .unwrap_or_else(|| panic!("{words:?} repeats at one level and should be refused")); + assert!(err.contains("DuplicateFlag"), "{words:?}: {err}"); + } +} + +#[test] +fn a_flag_that_is_not_global_is_unaffected() { + // `--quiet` belongs to the root alone, so there is no second level at which to give it and + // nothing about this rule should reach it. + let err = failure(&["-q", "-q"]).expect("a non-global repeat is a duplicate"); + assert!(err.contains("DuplicateFlag"), "{err}"); + + let parsed = parse(&["-q", "sub"]).expect("once is fine"); + assert!(parsed.quiet); +} + +#[test] +fn the_levels_bind_their_own_flags_too() { + // Not only about `-y`: the words at each level still land where they belong. This is also + // what keeps the fixture's fields read — an unread field in a generated CLI is a warning the + // adopter cannot silence. + let parsed = parse(&["-y", "sub", "--thing", "-y", "leaf", "--leafy"]).expect("parses"); + assert!(parsed.yes); + let Some(Commands::Sub(sub)) = parsed.command else { + panic!("routed to sub") + }; + assert!(sub.thing); + let Some(Deeper::Leaf(leaf)) = sub.command else { + panic!("routed to leaf") + }; + assert!(leaf.leafy); +} + +#[test] +fn a_negatable_global_crosses_a_boundary_like_a_plain_one() { + // Same polarity at two levels: allowed, and the inner occurrence wins. clap accepts + // `--colour sub --colour` when the flag is global whether or not it can be negated, so + // the negate arm asks the duplicate question per level exactly as the plain arm does. + let parsed = parse(&["--colour", "sub", "--colour"]).expect("repeated deeper"); + assert!(parsed.colour); + + // Opposite polarity across the boundary: the inner occurrence wins here too. + let parsed = parse(&["--colour", "sub", "--no-colour"]).expect("negated deeper"); + assert!(!parsed.colour); +} + +#[test] +fn a_negatable_global_still_refuses_one_spelling_twice_at_one_level() { + for words in [ + &["--colour", "--colour"][..], + &["--no-colour", "--no-colour"][..], + &["--colour", "sub", "--no-colour", "--no-colour"][..], + ] { + let err = failure(words).unwrap_or_else(|| { + panic!("{words:?} repeats one spelling at one level and should be refused") + }); + assert!(err.contains("DuplicateFlag"), "{words:?}: {err}"); + } + + // Alternating spellings is an override rather than a repeat, at one level as ever. + let parsed = parse(&["--colour", "--no-colour"]).expect("the last spelling wins"); + assert!(!parsed.colour); +} + +#[test] +fn a_global_in_a_flattened_group_crosses_a_boundary_too() { + // The group's flags sit in this command's table, but their duplicate state lives in the + // group's own partial — so the level has to end there too, not only in the command that + // did the flattening. + let parsed = parse(&["--force", "sub", "--force"]).expect("a flattened global, deeper"); + assert!(parsed.shared.force); + + // With a negation, in the flattened group, across the same boundary. + let parsed = parse(&["--tint", "sub", "--tint"]).expect("negatable and flattened"); + assert!(parsed.shared.tint); + let parsed = parse(&["--tint", "sub", "--no-tint"]).expect("the inner polarity wins"); + assert!(!parsed.shared.tint); +} + +#[test] +fn a_flattened_global_still_refuses_a_repeat_at_one_level() { + for words in [ + &["--force", "--force"][..], + &["sub", "--force", "--force"][..], + &["--force", "sub", "--force", "--force"][..], + &["--tint", "sub", "--no-tint", "--no-tint"][..], + ] { + let err = failure(words) + .unwrap_or_else(|| panic!("{words:?} repeats at one level and should be refused")); + assert!(err.contains("DuplicateFlag"), "{words:?}: {err}"); + } +} + +#[test] +fn the_error_still_names_the_flag() { + let owned: Vec<&OsStr> = ["-y", "-y"].iter().map(|s| OsStr::new(*s)).collect(); + match Ex::parse_from(&owned) { + Err(Error::DuplicateFlag { name }) => assert_eq!(name, "yes"), + Err(other) => panic!("expected a duplicate naming `yes`, got {other:?}"), + Ok(_) => panic!("expected a duplicate naming `yes`, and it parsed"), + } +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index fe4ad65a4..e0d014140 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -1202,15 +1202,33 @@ fn flag_arm(cli: &Cli, i: usize, field: &Field) -> TokenStream { let duplicated = format_ident!("__duplicated_{}", ident); if has_negate(field) { let negated = format_ident!("__negated_{}", ident); + // For a global the question is asked per level, exactly as in the arm + // below: clap accepts `--colour sub --colour` when `--colour` is global + // and negatable just as when it is plain. + let (guard, mark) = if duplicates_per_level(field) { + let here = format_ident!("__here_{}", ident); + (quote!(partial.#here), quote!(partial.#here = true;)) + } else { + (quote!(partial.#given), TokenStream::new()) + }; quote! { - if partial.#given { + if #guard { // The positive and negative spellings override one another: the // last of `--color --no-color` wins just like an explicit // `overrides` pair. Repeating the same spelling is still an error. partial.#duplicated = partial.#negated == negated; } + #mark partial.#negated = negated; } + } else if duplicates_per_level(field) { + let here = format_ident!("__here_{}", ident); + quote! { + if partial.#here { + partial.#duplicated = true; + } + partial.#here = true; + } } else { quote! { if partial.#given { @@ -1258,6 +1276,29 @@ fn flag_arm(cli: &Cli, i: usize, field: &Field) -> TokenStream { /// /// Counts and collections repeat by definition, and `var` explicitly opts a value-taking flag /// into repetition. Every other flag matches clap's default of one occurrence. +/// Whether a repeat of this flag is only a duplicate *within one command*. +/// +/// A `global` flag is in scope for every descendant, and clap lets it be given again on a +/// subcommand — the inner occurrence simply wins, which is what makes `mise -y install -y` a +/// line that works today. Repeating it at *one* level is still an error there, so the check +/// cannot simply be dropped: it has to be per level, which is what `__here_` records. +fn duplicates_per_level(field: &Field) -> bool { + rejects_duplicate(field) && matches!(field.kind, Kind::Flag { global: true, .. }) +} + +/// Clearing those markers, to run when a command word is read: descending starts a new level. +fn reset_per_level(cli: &Cli) -> TokenStream { + let resets = cli + .fields + .iter() + .filter(|f| duplicates_per_level(f)) + .map(|f| { + let here = format_ident!("__here_{}", f.ident); + quote!(partial.#here = false;) + }); + quote!(#(#resets)*) +} + fn rejects_duplicate(field: &Field) -> bool { matches!(field.kind, Kind::Flag { .. }) && !matches!(field.shape, Shape::Count | Shape::Many) @@ -1409,11 +1450,17 @@ fn partial_struct(cli: &Cli) -> TokenStream { let duplicated = format_ident!("__duplicated_{}", ident); quote!(pub #duplicated: bool,) }); + // Given at *this* level, for a global — see `duplicates_per_level`. Only for those + // fields, so nothing else carries a `bool` no code reads. + let here = duplicates_per_level(f).then(|| { + let here = format_ident!("__here_{}", ident); + quote!(pub #here: bool,) + }); let negated = has_negate(f).then(|| { let negated = format_ident!("__negated_{}", ident); quote!(pub #negated: bool,) }); - Some(quote!(pub #ident: #ty, pub #given: bool, #overridden #duplicated #negated)) + Some(quote!(pub #ident: #ty, pub #given: bool, #overridden #duplicated #here #negated)) }); // No derived `Default`: `start` is what produces a fresh partial, because a @@ -1737,6 +1784,10 @@ fn partial_defaults(cli: &Cli) -> TokenStream { let duplicated = format_ident!("__duplicated_{}", ident); quote!(#duplicated: false,) }); + let here = duplicates_per_level(f).then(|| { + let here = format_ident!("__here_{}", ident); + quote!(#here: false,) + }); let negated = has_negate(f).then(|| { let negated = format_ident!("__negated_{}", ident); quote!(#negated: false,) @@ -1746,6 +1797,7 @@ fn partial_defaults(cli: &Cli) -> TokenStream { #given: false, #overridden #duplicated + #here #negated }) }); @@ -2037,6 +2089,7 @@ fn reset_to_default(field: &Field) -> TokenStream { /// Take one event and say whether it belonged to this command. fn apply_fn(cli: &Cli) -> TokenStream { let route = subcommand_parts(cli).map(|p| p.route).unwrap_or_default(); + let per_level_resets = reset_per_level(cli); // A flattened struct's flags are in this command's table, but its *keys* were minted in // its own expansion — so they cannot be matched here. Its `apply` recognises them, and // says whether it took the event. @@ -2094,8 +2147,15 @@ fn apply_fn(cli: &Cli) -> TokenStream { } } // Descending is the caller's business: it is what decides which - // command's fields the following events belong to. - Event::Command(_) => false, + // command's fields the following events belong to. But any command + // word — not only a child of ours — begins a level at which this + // struct's globals may be given again, and this arm is the one + // place every partial sees the word, whether it belongs to a root, + // a subcommand, or a flattened group. + Event::Command(_) => { + #per_level_resets + false + } } } }