diff --git a/PLAN.md b/PLAN.md index 0e1c70358..88f4dfe05 100644 --- a/PLAN.md +++ b/PLAN.md @@ -665,13 +665,25 @@ feature list is not an exhaustive audit. or a flattened Args wrapper; their targeted diagnostic explains the named `#[usage(flatten)]` rewrite instead of reporting a generic unsupported shape. Facade tests exercise unit roots and nested unit Args commands. -- [ ] **Relationships through flatten and positional IDs.** Positional +- [x] **Post-binding relationships through flatten and positional IDs.** Positional relationships are already a general gap above. The fleet exposed the second half: a field cannot name a flag contributed by a flattened Args type because validation runs against the declaring struct before the command is assembled. aube lost statically declared relationships and hk - and fnox needed runtime conflict checks. Validate selectors against the - composed command and carry stable IDs for both flags and positionals. + and fnox needed runtime conflict checks. Derived `CommandArgs` now expose + selector-stable presence and value lookup across flattened boundaries, so + parent conflicts, requirements, conditional requirements/defaults, and + required-if/unless rules resolve against the composed command without a + dynamic command graph or allocation. Flags use every accepted spelling and + positionals use their stable field identity. Binding-time `overrides` remains + a separate gap below and is rejected when it names a flattened field rather + than compiling into a no-op. +- [ ] **Binding-time overrides across flatten boundaries.** `overrides` is + last-token-wins and therefore cannot be implemented by the post-binding + presence/value lookup used for conflicts and requirements. Add a composed + displacement hook that can reset an opaque flattened partial, preserve the + losing field's default/env suppression, and handle either token order before + allowing a parent selector to name a flattened target. - [ ] **Flattened help topology.** clap's `next_help_heading` and flattened flag groups preserve meaningful sections in aube's long help. usage flattens the fields but discards that struct-level heading, so a migration can preserve @@ -1170,9 +1182,13 @@ repeated here. subcommand variant selects the shared `CommandArgs` table independently. The same facade regression covers the fnox full-command shape; mise's flattened `ConfigLs` reuse remains covered by the fleet shadow. -- [ ] **Relationships across a flatten boundary.** A flag in the parent - conflicting with a flag in the flattened group has no spelling; hk and - aube both hit it and enforce post-bind by hand. +- [x] **Post-binding relationships across a flatten boundary.** A parent flag can name a flag + or positional contributed by a flattened group. The nested partial answers + selector lookup through `CommandArgs`, preserving the same argv/env/default + presence semantics as relationships declared inside one struct. hk and aube + can remove their post-bind conflict checks when repinned to this stack tip. + Binding-time `overrides` is tracked separately and rejected across the + boundary until composed displacement exists. - [x] **Checked-in specs vs release automation.** `SpecView::omit_version()` removes the runtime version from a cold emitted metadata view without changing the base derived spec or built-in `--version` behavior. tak uses diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 91b9a3c20..871293753 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -2112,6 +2112,17 @@ pub const fn concat_bindings( joined } +/// Presence information exposed across a flattened [`CommandArgs`] boundary. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ArgumentState { + /// The canonical name used in diagnostics. + pub name: &'static str, + /// Whether argv or an environment fallback supplied the argument. + pub given: bool, + /// Whether the argument is satisfied, including an unconditional default. + pub satisfied: bool, +} + pub trait CommandArgs: Sized { /// Values collected so far. Partly-filled by construction, since a parse can /// stop early. @@ -2182,6 +2193,23 @@ pub trait CommandArgs: Sized { None } + /// Find an argument by any selector it accepts. + /// + /// Parents use this to enforce a relationship declared beside a flattened + /// argument group. The default keeps hand-written implementations source compatible. + fn argument_state(partial: &Self::Partial, selector: &str) -> Option { + let _ = (partial, selector); + None + } + + /// Whether a selected argument has an explicitly supplied value. + /// + /// This is the value-aware half needed by conditional defaults and requirements. + fn argument_matches(partial: &Self::Partial, selector: &str, value: &[u8]) -> Option { + let _ = (partial, selector, value); + None + } + /// Fill fields in this command from their declared defaults. /// /// Kept separate from [`CommandArgs::check`] so a parent can preserve defaults in a diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index f887e2c4a..30eadc3de 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -17,7 +17,8 @@ use quote::{format_ident, quote}; use crate::crate_name::{crate_name, FoundCrate}; use crate::model::{ - rendered_path, Cli, ConditionalDefault, DoubleDash, Field, Kind, Shape, Subcommands, ValueEnum, + rendered_path, to_kebab, Cli, ConditionalDefault, DoubleDash, Field, Kind, Shape, Subcommands, + ValueEnum, }; /// Construct the user's command type after its generated partial has been checked. @@ -218,6 +219,7 @@ pub fn emit(cli: &Cli) -> TokenStream { let sub_build = parts.as_ref().map(|p| p.build.clone()).unwrap_or_default(); let partial = partial_struct(cli); + let argument_lookup = argument_lookup_functions(cli); let defaults = partial_defaults(cli); // A root resolves settings when it binds one itself, or when it says so — which is how a CLI // whose bound flags all live in a flattened group asks for the entry points, since it cannot @@ -478,6 +480,7 @@ pub fn emit(cli: &Cli) -> TokenStream { #partial #apply + #argument_lookup /// Everything decided after the last token. /// @@ -1922,6 +1925,167 @@ fn presence_methods(cli: &Cli) -> TokenStream { #selected_exclusive ::std::option::Option::None } + + fn argument_state( + partial: &Self::Partial, + selector: &str, + ) -> ::std::option::Option { + argument_state(partial, selector) + } + + fn argument_matches( + partial: &Self::Partial, + selector: &str, + value: &[u8], + ) -> ::std::option::Option { + argument_matches(partial, selector, value) + } + } +} + +fn field_selectors(field: &Field) -> Vec { + match &field.kind { + Kind::Flag { + longs, + shorts, + negate, + .. + } => longs + .iter() + .map(|long| format!("--{long}")) + .chain(shorts.iter().map(|short| format!("-{short}"))) + .chain(negate.iter().map(|long| format!("--{long}"))) + .collect(), + Kind::Arg { .. } => { + let inferred = to_kebab(&field.ident.to_string()); + if inferred == field.name { + vec![field.name.clone()] + } else { + vec![field.name.clone(), inferred] + } + } + _ => Vec::new(), + } +} + +/// Selector lookup composed across flattened argument groups. +/// +/// A parent cannot inspect another derive expansion's fields, so the flattened type answers +/// through `CommandArgs`. Keeping the lookup beside the partial avoids building a dynamic +/// command graph or allocating on the successful parse path. +fn argument_lookup_functions(cli: &Cli) -> TokenStream { + let state_arms = cli.fields.iter().filter_map(|field| { + if matches!( + field.kind, + Kind::Flatten { .. } | Kind::Subcommand { .. } | Kind::Skip + ) { + return None; + } + let selectors = field_selectors(field); + let ident = &field.ident; + let given = format_ident!("__given_{}", ident); + let name = &field.name; + // Look through conditional defaults as well as unconditional ones. This lookup + // runs after `apply_defaults` during relationship validation, so a predicate + // that filled the field is still observable even though defaults deliberately + // do not set `__given_*`. + let defaulted = !field.default.is_empty(); + let conditionally_defaulted = + default_if_would_apply(cli, field).unwrap_or_else(|| quote!(false)); + Some(quote! { + #(#selectors)|* => return ::std::option::Option::Some( + usage_argv::spec::ArgumentState { + name: #name, + given: partial.#given, + satisfied: partial.#given || #defaulted || (#conditionally_defaulted), + }, + ), + }) + }); + let state_flattened = cli.fields.iter().filter_map(|field| { + let Kind::Flatten { ty } = &field.kind else { + return None; + }; + let ident = &field.ident; + Some(quote! { + if let ::std::option::Option::Some(state) = + <#ty as usage_argv::spec::CommandArgs>::argument_state( + &partial.#ident, + selector, + ) + { + return ::std::option::Option::Some(state); + } + }) + }); + let match_arms = cli.fields.iter().filter_map(|field| { + if matches!( + field.kind, + Kind::Flatten { .. } | Kind::Subcommand { .. } | Kind::Skip + ) { + return None; + } + let selectors = field_selectors(field); + let ident = &field.ident; + let given = format_ident!("__given_{}", ident); + let matches = match field.shape { + Shape::Optional => quote!(partial.#ident.as_deref().is_some_and(|v| v == value)), + Shape::Required => quote!(partial.#ident.as_slice() == value), + Shape::Many => quote!(partial.#ident.iter().any(|v| v.as_slice() == value)), + Shape::Bool => quote!( + (value == b"true" && partial.#ident) || (value == b"false" && !partial.#ident) + ), + Shape::Count => quote!( + (value == b"true" && partial.#ident > 0) + || (value == b"false" && partial.#ident == 0) + ), + }; + Some(quote!(#(#selectors)|* => return ::std::option::Option::Some(partial.#given && #matches),)) + }); + let match_flattened = cli.fields.iter().filter_map(|field| { + let Kind::Flatten { ty } = &field.kind else { + return None; + }; + let ident = &field.ident; + Some(quote! { + if let ::std::option::Option::Some(matches) = + <#ty as usage_argv::spec::CommandArgs>::argument_matches( + &partial.#ident, + selector, + value, + ) + { + return ::std::option::Option::Some(matches); + } + }) + }); + quote! { + #[allow(dead_code)] + pub fn argument_state( + partial: &Partial, + selector: &str, + ) -> ::std::option::Option { + match selector { + #(#state_arms)* + _ => {} + } + #(#state_flattened)* + ::std::option::Option::None + } + + #[allow(dead_code)] + pub fn argument_matches( + partial: &Partial, + selector: &str, + value: &[u8], + ) -> ::std::option::Option { + match selector { + #(#match_arms)* + _ => {} + } + #(#match_flattened)* + ::std::option::Option::None + } } } @@ -2650,7 +2814,14 @@ fn assign_literal(field: &Field, first: &str, all: &[String]) -> TokenStream { fn default_if_predicate(cli: &Cli, condition: &ConditionalDefault) -> TokenStream { let Some(other) = cli.field_for_selector(&condition.selector) else { - return quote!(false); + let selector = &condition.selector; + return match &condition.when { + None => quote!(argument_state(partial, #selector).is_some_and(|state| state.given)), + Some(when) => quote!( + argument_matches(partial, #selector, #when.as_bytes()) + == ::std::option::Option::Some(true) + ), + }; }; let other_given = format_ident!("__given_{}", other.ident); let ident = &other.ident; @@ -3030,6 +3201,7 @@ pub fn emit_args(cli: &Cli) -> TokenStream { .map(|value| option_expr(Some(value))) .unwrap_or_else(|| option_str(cli.long_about.as_deref())); let partial = partial_struct(cli); + let argument_lookup = argument_lookup_functions(cli); let defaults = partial_defaults(cli); let apply = apply_fn(cli); let post = post_binding(cli); @@ -3134,6 +3306,7 @@ pub fn emit_args(cli: &Cli) -> TokenStream { #partial #apply + #argument_lookup pub fn start() -> Partial { #defaults @@ -4511,21 +4684,36 @@ fn post_binding(cli: &Cli) -> TokenStream { let conflict_checks = cli.fields.iter().flat_map(move |f| { let given = format_ident!("__given_{}", f.ident); let name = &f.name; - f.conflicts.iter().filter_map(move |selector| { - // Resolved in the model, which rejects a selector naming nothing. - let other = cli.field_for_selector(selector)?; - let other_given = format_ident!("__given_{}", other.ident); - let other_name = &other.name; - Some(quote! { - if partial.#given && partial.#other_given { - return ::std::result::Result::Err( - usage_argv::Error::ConflictingFlags { - name: #name, - other: #other_name, - }, - ); + f.conflicts.iter().map(move |selector| { + if let Some(other) = cli.field_for_selector(selector) { + let other_given = format_ident!("__given_{}", other.ident); + let other_name = &other.name; + quote! { + if partial.#given && partial.#other_given { + return ::std::result::Result::Err( + usage_argv::Error::ConflictingFlags { + name: #name, + other: #other_name, + }, + ); + } } - }) + } else { + quote! { + if partial.#given { + if let ::std::option::Option::Some(other) = argument_state(partial, #selector) { + if other.given { + return ::std::result::Result::Err( + usage_argv::Error::ConflictingFlags { + name: #name, + other: other.name, + }, + ); + } + } + } + } + } }) }); @@ -4541,15 +4729,28 @@ fn post_binding(cli: &Cli) -> TokenStream { // A target with a default is skipped outright, since it can never be missing. let requirement_checks = cli.fields.iter().flat_map(move |f| { let given = format_ident!("__given_{}", f.ident); - f.requires.iter().filter_map(move |selector| { - // Resolved in the model, which rejects a selector naming nothing. - let other = cli.field_for_selector(selector)?; + f.requires.iter().map(move |selector| { + let Some(other) = cli.field_for_selector(selector) else { + return quote! { + if partial.#given { + match argument_state(partial, #selector) { + ::std::option::Option::Some(other) if other.satisfied => {} + ::std::option::Option::Some(other) => { + return ::std::result::Result::Err( + usage_argv::Error::MissingRequired { name: other.name }, + ); + } + ::std::option::Option::None => {} + } + } + }; + }; // A flag with a default always has a value, so the requirement it satisfies // can never fail — the same reason plain required-ness skips such a field. // Decided at compile time, so the check is not merely always-true at run // time, it is not there. if !other.default.is_empty() { - return None; + return quote!(); } let other_given = format_ident!("__given_{}", other.ident); let other_name = &other.name; @@ -4558,7 +4759,7 @@ fn post_binding(cli: &Cli) -> TokenStream { usage_argv::Error::MissingRequired { name: #other_name }, ); }; - Some(match default_if_would_apply(cli, other) { + match default_if_would_apply(cli, other) { Some(pred) => quote! { if partial.#given && !partial.#other_given && !(#pred) { #missing @@ -4569,7 +4770,7 @@ fn post_binding(cli: &Cli) -> TokenStream { #missing } }, - }) + } }) }); @@ -4579,10 +4780,48 @@ fn post_binding(cli: &Cli) -> TokenStream { let conditional_requirement_checks = cli.fields.iter().flat_map(move |f| { let given = format_ident!("__given_{}", f.ident); let ident = &f.ident; - f.requires_if.iter().filter_map(move |condition| { - let other = cli.field_for_selector(&condition.requires)?; + f.requires_if.iter().map(move |condition| { + let external = cli.field_for_selector(&condition.requires).is_none(); + let other = cli.field_for_selector(&condition.requires); + if external { + let selector = &condition.requires; + let value = &condition.value; + let matches = match f.shape { + Shape::Optional => { + quote!(partial.#ident.as_deref().is_some_and(|v| v == #value.as_bytes())) + } + Shape::Required => quote!(partial.#ident.as_slice() == #value.as_bytes()), + Shape::Many => { + quote!(partial.#ident.iter().any(|v| v.as_slice() == #value.as_bytes())) + } + Shape::Bool => match value.as_str() { + "true" => quote!(partial.#ident), + "false" => quote!(!partial.#ident), + _ => quote!(false), + }, + Shape::Count => match value.as_str() { + "true" => quote!(partial.#ident > 0), + "false" => quote!(partial.#ident == 0), + _ => quote!(false), + }, + }; + return quote! { + if partial.#given && #matches { + match argument_state(partial, #selector) { + ::std::option::Option::Some(other) if other.satisfied => {} + ::std::option::Option::Some(other) => { + return ::std::result::Result::Err( + usage_argv::Error::MissingRequired { name: other.name }, + ); + } + ::std::option::Option::None => {} + } + } + }; + } + let other = other.expect("the local relationship was resolved above"); if !other.default.is_empty() { - return None; + return quote!(); } let other_given = format_ident!("__given_{}", other.ident); let other_name = &other.name; @@ -4610,13 +4849,13 @@ fn post_binding(cli: &Cli) -> TokenStream { Some(pred) => quote!(&& !(#pred)), None => quote!(), }; - Some(quote! { + quote! { if partial.#given && #matches && !partial.#other_given #unless_default_if { return ::std::result::Result::Err( usage_argv::Error::MissingRequired { name: #other_name }, ); } - }) + } }) }); @@ -4846,9 +5085,15 @@ fn post_binding(cli: &Cli) -> TokenStream { let given = format_ident!("__given_{}", f.ident); let name = &f.name; let selector_given = |selector: &String| { - let other = cli.field_for_selector(selector)?; - let other_given = format_ident!("__given_{}", other.ident); - Some(quote!(partial.#other_given)) + Some(match cli.field_for_selector(selector) { + Some(other) => { + let other_given = format_ident!("__given_{}", other.ident); + quote!(partial.#other_given) + } + None => quote!( + argument_state(partial, #selector).is_some_and(|state| state.given) + ), + }) }; let if_given: Vec<_> = f.required_if.iter().filter_map(selector_given).collect(); let unless_given: Vec<_> = f diff --git a/derive/src/model.rs b/derive/src/model.rs index 1b0ceada9..75de5186d 100644 --- a/derive/src/model.rs +++ b/derive/src/model.rs @@ -1175,6 +1175,10 @@ impl Cli { // is the advantage of declaring them in code: a spec written by hand can only // find a typo'd selector at parse time, or never, since a selector naming // nothing quietly holds no relationship at all. + let has_flatten = self + .fields + .iter() + .any(|field| matches!(field.kind, Kind::Flatten { .. })); for field in &self.fields { for (option, selectors) in [ ("overrides", &field.overrides), @@ -1185,6 +1189,25 @@ impl Cli { ] { for selector in selectors { let Some(target) = self.field_for_selector(selector) else { + // Post-binding relationships can ask an opaque flattened partial + // about presence and values. `overrides` is different: last-one-wins + // has to displace a value while tokens are being bound, and the + // parent cannot mutate an unknown field in another derive expansion. + // Reject it explicitly instead of accepting a declaration that does + // nothing at runtime. + if has_flatten && option != "overrides" { + continue; + } + if has_flatten && option == "overrides" { + return Err(syn::Error::new( + field.span, + format!( + "`overrides = \"{selector}\"` cannot cross a flatten \ + boundary; declare the override inside the flattened Args \ + type" + ), + )); + } return Err(syn::Error::new( field.span, format!( @@ -1205,6 +1228,9 @@ impl Cli { for condition in &field.requires_if { let selector = &condition.requires; let Some(target) = self.field_for_selector(selector) else { + if has_flatten { + continue; + } return Err(syn::Error::new( field.span, format!( @@ -1223,6 +1249,9 @@ impl Cli { for condition in &field.default_if { let selector = &condition.selector; let Some(target) = self.field_for_selector(selector) else { + if has_flatten { + continue; + } return Err(syn::Error::new( field.span, format!( @@ -3199,7 +3228,7 @@ fn shout(form: &str) -> String { form.to_uppercase().replace('-', "_") } -fn to_kebab(s: &str) -> String { +pub(crate) fn to_kebab(s: &str) -> String { let mut out = String::with_capacity(s.len() + 4); for (i, ch) in s.chars().enumerate() { if ch == '_' { @@ -5666,6 +5695,24 @@ mod tests { ); } + #[test] + fn overrides_does_not_silently_cross_a_flatten_boundary() { + let err = rejection( + r#" + struct Ex { + #[usage(long, overrides = "--nested")] + force: bool, + #[usage(flatten)] + shared: Shared, + } + "#, + ); + assert!( + err.contains("cannot cross a flatten boundary"), + "unhelpful message: {err}" + ); + } + #[test] fn required_unless_needs_somewhere_to_put_absent() { // A bare `String` is always filled, so the exception could never take effect: diff --git a/usage-rs/tests/facade.rs b/usage-rs/tests/facade.rs index a1112944c..a00d28336 100644 --- a/usage-rs/tests/facade.rs +++ b/usage-rs/tests/facade.rs @@ -80,6 +80,35 @@ struct ClapSpellings { path: Option, } +#[derive(usage::Args)] +struct FlattenedRelationshipTargets { + #[usage(long)] + frozen: bool, + #[usage(long, default_if("--preset", "true"))] + key: bool, + #[usage(long)] + json: bool, + #[usage(long)] + preset: bool, +} + +#[derive(Cli)] +#[usage(bin = "flattened-relationships")] +struct FlattenedRelationships { + #[usage(long, conflicts = "--frozen")] + fix: bool, + #[usage(long, requires = "--key")] + signed: bool, + #[usage(long, requires_if("json", "--key"))] + mode: Option, + #[usage(long, required_if = "--json")] + schema: Option, + #[usage(long, default_if("--json", "auto"))] + output: Option, + #[usage(flatten)] + shared: FlattenedRelationshipTargets, +} + #[derive(ValueEnum)] #[usage(ignore_case)] enum Shell { @@ -572,6 +601,45 @@ fn clap_field_ids_and_aliases_need_no_rewrite() { ); } +#[test] +fn relationships_resolve_targets_inside_flattened_args() { + assert!( + FlattenedRelationships::parse_from(&[OsStr::new("--fix"), OsStr::new("--frozen"),]) + .is_err() + ); + assert!(FlattenedRelationships::parse_from(&[OsStr::new("--signed")]).is_err()); + let defaulted = + FlattenedRelationships::parse_from(&[OsStr::new("--signed"), OsStr::new("--preset")]) + .expect("a flattened conditional default should satisfy the parent requirement"); + assert!(defaulted.signed); + assert!(defaulted.shared.key); + assert!( + FlattenedRelationships::parse_from(&[OsStr::new("--mode"), OsStr::new("json"),]).is_err() + ); + assert!(FlattenedRelationships::parse_from(&[OsStr::new("--json")]).is_err()); + + let parsed = FlattenedRelationships::parse_from(&[ + OsStr::new("--json"), + OsStr::new("--schema"), + OsStr::new("schema.json"), + ]) + .expect("the flattened condition should satisfy the schema relationship"); + assert!(!parsed.fix); + assert!(!parsed.signed); + assert!(parsed.mode.is_none()); + assert_eq!(parsed.schema.as_deref(), Some("schema.json")); + assert_eq!(parsed.output.as_deref(), Some("auto")); + assert!(parsed.shared.json); + assert!(!parsed.shared.frozen); + assert!(!parsed.shared.key); + assert!(!parsed.shared.preset); + + let kdl = FlattenedRelationships::to_kdl(); + assert!(kdl.contains("conflicts=\"--frozen\""), "{kdl}"); + assert!(kdl.contains("requires=\"--key\""), "{kdl}"); + assert!(kdl.contains("required_if=\"--json\""), "{kdl}"); +} + #[test] fn emitted_parser_settings_are_portable_spec_metadata() { let Err(_) = StrictEx::parse_from(&[OsStr::new("--wat")]) else {