Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions PLAN.md
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,10 @@ Groups are the opposite case: `Command::get_groups`, `ArgGroup::get_args` and
errors. KDL, usage-lib, the typed derive/static metadata, generated Go, and the
clap bridge carry the policy. Repeatable, variadic, and count flags retain their
collecting behavior.
- [x] **Optional flag values.** `Option<Option<T>>` distinguishes an absent flag,
a bare flag, and an explicit value. The derive infers zero-or-one value arity,
usage-argv binds all three states, and emitted KDL/help use an optional
placeholder without requiring a synthetic `default_missing`.

**Help output**

Expand Down
116 changes: 104 additions & 12 deletions argv/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,12 @@ pub struct Flag<'a> {
/// attached form (`-i9229`, `-i=9229`) still binds: only the following word
/// is refused.
pub require_equals: bool,
/// Whether this value-taking flag may be present without a value.
///
/// A missing value emits the flag event with `value: None`; bindings such as
/// `Option<Option<T>>` can therefore distinguish an absent flag from a bare
/// flag and from a flag with an explicit value.
pub value_optional: bool,
/// Value used when the flag is present but no value is given.
///
/// clap's `default_missing_value` and the spec's `default_missing`. `--color`
Expand Down Expand Up @@ -379,6 +385,7 @@ impl Flag<'_> {
allow_negative_numbers: false,
value_terminator: ::core::option::Option::None,
require_equals: false,
value_optional: false,
default_missing: ::core::option::Option::None,
global: false,
};
Expand Down Expand Up @@ -1413,15 +1420,17 @@ impl<'t: 'v, 'a, 'v> Parser<'t, 'a, 'v> {

if let Some(flag) = self.find_long(name) {
let value = if flag.takes_value {
Some(match attached {
Some(v) => v,
match attached {
Some(v) => Some(v),
None => self.take_detached_value(flag)?,
})
}
Comment thread
cursor[bot] marked this conversation as resolved.
} else {
None
};
if flag.variadic {
self.start_collecting(flag, value.unwrap_or(b""))?;
if let Some(value) = value {
self.start_collecting(flag, value)?;
}
}
return Ok(Event::Flag {
flag,
Expand Down Expand Up @@ -1511,16 +1520,18 @@ impl<'t: 'v, 'a, 'v> Parser<'t, 'a, 'v> {
let value = if rest.is_empty() {
self.take_detached_value(flag)?
} else if rest[0] == b'=' {
&rest[1..]
Some(&rest[1..])
} else {
rest
Some(rest)
};
if flag.variadic {
self.start_collecting(flag, value)?;
if let Some(value) = value {
self.start_collecting(flag, value)?;
}
}
Ok(Event::Flag {
flag,
value: Some(value),
value,
negated: false,
})
}
Expand All @@ -1531,7 +1542,10 @@ impl<'t: 'v, 'a, 'v> Parser<'t, 'a, 'v> {
/// `--jobs --force` is far more likely a forgotten value than a deliberate
/// one, and the attached form is available for the deliberate case. Declared,
/// the next token is taken whatever it looks like, including `--`.
fn take_detached_value(&mut self, flag: &'t Flag<'t>) -> Result<&'v [u8], Error<'t, 'v>> {
fn take_detached_value(
&mut self,
flag: &'t Flag<'t>,
) -> Result<Option<&'v [u8]>, Error<'t, 'v>> {
if flag.require_equals {
return self.missing_or_default(flag);
}
Expand All @@ -1542,15 +1556,16 @@ impl<'t: 'v, 'a, 'v> Parser<'t, 'a, 'v> {
|| (flag.allow_negative_numbers && is_negative_number(bytes(next))) =>
{
self.pos += 1;
Ok(bytes(next))
Ok(Some(bytes(next)))
}
_ => self.missing_or_default(flag),
}
}

fn missing_or_default(&self, flag: &'t Flag<'t>) -> Result<&'v [u8], Error<'t, 'v>> {
fn missing_or_default(&self, flag: &'t Flag<'t>) -> Result<Option<&'v [u8]>, Error<'t, 'v>> {
match flag.default_missing {
Some(value) => Ok(value),
Some(value) => Ok(Some(value)),
None if flag.value_optional => Ok(None),
Comment thread
cursor[bot] marked this conversation as resolved.
None => Err(Error::MissingFlagValue { flag }),
}
}
Expand Down Expand Up @@ -3037,6 +3052,83 @@ mod tests {
);
}

#[test]
fn optional_flag_value_distinguishes_bare_and_explicit_forms() {
static BUMP: Flag = Flag {
key: 11,
name: "bump",
longs: &["bump"],
takes_value: true,
value_optional: true,
..Flag::BOOL
};
static OPTIONAL: Command = Command {
name: "ex",
flags: &[&BUMP],
..Command::EMPTY
};

assert_eq!(parse(&OPTIONAL, &argv([])).unwrap(), vec![]);
assert_eq!(
parse(&OPTIONAL, &argv(["--bump"])).unwrap(),
vec![Event::Flag {
flag: &BUMP,
value: None,
negated: false,
}]
);
assert_eq!(
parse(&OPTIONAL, &argv(["--bump=5"])).unwrap(),
vec![Event::Flag {
flag: &BUMP,
value: Some(b"5"),
negated: false,
}]
);

static INCLUDE: Flag = Flag {
key: 12,
name: "include",
longs: &["include"],
takes_value: true,
variadic: true,
value_optional: true,
..Flag::BOOL
};
static VERBOSE: Flag = Flag {
key: 13,
name: "verbose",
longs: &["verbose"],
..Flag::BOOL
};
static VARIADIC: Command = Command {
name: "ex",
flags: &[&INCLUDE, &VERBOSE],
args: &[&REST],
..Command::EMPTY
};
assert_eq!(
parse(&VARIADIC, &argv(["--include", "--verbose", "file"])).unwrap(),
vec![
Event::Flag {
flag: &INCLUDE,
value: None,
negated: false,
},
Event::Flag {
flag: &VERBOSE,
value: None,
negated: false,
},
Event::Arg {
arg: &REST,
value: b"file",
delimit: true,
},
]
);
}

#[test]
fn default_missing_with_require_equals_leaves_the_following_word() {
static INSPECT: Flag = Flag {
Expand Down
3 changes: 3 additions & 0 deletions argv/src/spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1641,6 +1641,9 @@ fn write_flag(out: &mut String, meta: &FlagMeta<'_>, depth: usize) -> core::fmt:
if meta.flag.require_equals {
out.push_str(" require_equals=#true");
}
if meta.flag.value_optional {
out.push_str(" value_optional=#true");
}
if let Some(missing) = meta.flag.default_missing {
write!(
out,
Expand Down
1 change: 1 addition & 0 deletions conformance/src/tables.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ fn build_flag(f: &SpecFlag) -> &'static Flag<'static> {
.and_then(|arg| arg.value_terminator.as_deref())
.map(|value| leak(value).as_bytes()),
require_equals: f.require_equals,
value_optional: f.value_optional,
default_missing: f.default_missing.as_deref().map(|s| leak(s).as_bytes()),
global: f.global,
}))
Expand Down
35 changes: 35 additions & 0 deletions conformance/tests/optional_flag_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,44 @@ fn the_reference_renders_it_the_same_way() {
fn the_emitted_spec_says_both_halves() {
let kdl = Ex::to_kdl();
assert!(kdl.contains(r#"arg "[BUMP]" required=#false"#), "{kdl}");
assert!(!kdl.contains("value_optional=#true"), "{kdl}");
assert!(kdl.contains("arg <PORT>"), "{kdl}");
}

#[test]
fn portable_tables_keep_help_optionality_separate_from_binding() {
let presentation: LibSpec = Ex::to_kdl().parse().unwrap();
let presentation = usage_conformance::tables::build_spec(&presentation);
let bare = [OsStr::new("--bump")];
let mut parser = usage_argv::Parser::new(presentation.root.cmd, &bare);
let presentation_error = loop {
match parser.next_event() {
Some(Ok(_)) => {}
Some(Err(_)) => break true,
None => break false,
}
};
assert!(presentation_error);

let executable: LibSpec = r#"
name "ex"
flag "--bump [BUMP]" value_optional=#true
"#
.parse()
.unwrap();
let executable = usage_conformance::tables::build_spec(&executable);
let mut parser = usage_argv::Parser::new(executable.root.cmd, &bare);
let mut events = Vec::new();
while let Some(event) = parser.next_event() {
events.push(event.unwrap());
}
assert_eq!(events.len(), 1);
assert!(matches!(
events[0],
usage_argv::Event::Flag { value: None, .. }
));
}

#[test]
fn it_still_takes_its_value() {
// Nothing about binding changed: the value is read where it is given.
Expand Down
38 changes: 38 additions & 0 deletions derive/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,11 @@ fn flag_table(i: usize, field: &Field) -> TokenStream {
None => quote!(::core::option::Option::None),
};
let require_equals = field.require_equals;
// `value_optional` can be a presentation-only declaration for clap/spec
// compatibility. Only a nested Option can represent a genuinely bare value
// in the typed result; `default_missing` turns the bare form into a value
// before binding.
let value_optional = field.optional_value_type || field.default_missing.is_some();
let default_missing = match field.default_missing.as_deref() {
Some(value) => quote!(::core::option::Option::Some(#value.as_bytes())),
None => quote!(::core::option::Option::None),
Expand All @@ -1089,6 +1094,7 @@ fn flag_table(i: usize, field: &Field) -> TokenStream {
allow_negative_numbers: #allow_negative_numbers,
value_terminator: #value_terminator,
require_equals: #require_equals,
value_optional: #value_optional,
Comment thread
jdx marked this conversation as resolved.
default_missing: #default_missing,
global: #global,
};
Expand Down Expand Up @@ -1819,6 +1825,9 @@ fn flag_arm(cli: &Cli, i: usize, field: &Field) -> TokenStream {
// Saturating, because a `u8` field given 256 occurrences would otherwise
// panic in debug and wrap to zero in release.
Shape::Count => quote!(partial.#ident = partial.#ident.saturating_add(1);),
Shape::Optional if field.optional_value_type => quote! {
partial.#ident = value.map(__usage_text);
},
Shape::Optional => quote! {
partial.#ident = ::std::option::Option::Some(__usage_value_text(value));
},
Expand Down Expand Up @@ -2810,6 +2819,7 @@ fn partial_defaults(cli: &Cli) -> TokenStream {
/// number, or a type of the adopter's own.
fn field_final(field: &Field) -> TokenStream {
let ident = &field.ident;
let given = format_ident!("__given_{}", ident);
let name = &field.name;
if matches!(field.kind, Kind::Skip) {
// clap's skip: not parsed, filled from Default when the struct is built.
Expand Down Expand Up @@ -2902,6 +2912,20 @@ fn field_final(field: &Field) -> TokenStream {
// A `match` rather than `.map`, and a loop rather than `.collect`, for the same
// reason the text path below uses them: the conversion can fail, and a `return`
// inside a closure would leave the error in the closure's own return type.
Shape::Optional if field.optional_value_type => {
let value = converted(quote!(__usage_value));
quote! {
#ident: match partial.#ident {
::std::option::Option::Some(__usage_value) => {
::std::option::Option::Some(::std::option::Option::Some(#value))
}
::std::option::Option::None if partial.#given => {
::std::option::Option::Some(::std::option::Option::None)
}
::std::option::Option::None => ::std::option::Option::None,
}
}
}
Shape::Optional => {
let value = converted(quote!(__usage_value));
quote! {
Expand Down Expand Up @@ -2991,6 +3015,20 @@ fn field_final(field: &Field) -> TokenStream {
let one = converted(quote!(partial.#ident));
quote!(#ident: #one)
}
Shape::Optional if field.optional_value_type => {
let one = converted(quote!(__usage_value));
quote! {
#ident: match partial.#ident {
::std::option::Option::Some(__usage_value) => {
::std::option::Option::Some(::std::option::Option::Some(#one))
}
::std::option::Option::None if partial.#given => {
::std::option::Option::Some(::std::option::Option::None)
}
::std::option::Option::None => ::std::option::Option::None,
}
}
}
Shape::Optional => {
let one = converted(quote!(__usage_value));
quote! {
Expand Down
Loading