Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-derivation with multiple_occurrences #2195

Closed
fosskers opened this issue Oct 31, 2020 · 3 comments
Closed

Auto-derivation with multiple_occurrences #2195

fosskers opened this issue Oct 31, 2020 · 3 comments
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies
Milestone

Comments

@fosskers
Copy link

This issue is related to the following discussion: #2192 . The original issue is explained it detail there. I was asked to open an issue, since there may be a UI/UX change that can be made. Below is runnable code that demonstrates the problem.

Code and Behaviour

This first example shows the original form of the code.

use clap::Clap;

#[derive(Clap, Debug)]
struct Args {
    /// Some cleaning operation.
    #[clap(long, short)]
    clean: bool,
}

fn main() {
    let args = Args::parse();

    println!("{:#?}", args);
}

When we run this, we see that --clean is a flag. Either on or off.

> cargo run -- -h
clap-ma 

USAGE:
    clap-ma [FLAGS]

FLAGS:
    -c, --clean      Some cleaning operation
    -h, --help       Prints help information
    -V, --version    Prints version information

If we try to set --clean multiple times, we get an error as expected:

> cargo run -- -cc
error: The argument '--clean' was provided more than once, but cannot be used multiple times

USAGE:
    clap-ma [FLAGS]

For more information try --help

Now let's set multiple_occurrences = true:

use clap::Clap;

#[derive(Clap, Debug)]
struct Args {
    /// Some cleaning operation.
    #[clap(long, short, multiple_occurrences = true)]
    clean: bool,
}

fn main() {
    let args = Args::parse();

    println!("{:#?}", args);
}

This compiles fine, but help output doesn't change. -c can be passed more than once, but since the field is a single bool, nothing really changes. You might think that setting the field instead to u8 or Vec<bool> would then represent the count of the appearances of the flag, but doing so instead turns it into an arg-accepting "option":

> cargo run -- -h
clap-ma 

USAGE:
    clap-ma --clean <clean>...

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -c, --clean <clean>...    Some cleaning operation
colin@yumi ~/c/r/clap-ma (master)> cargo run -- -c
error: The argument '--clean <clean>...' requires a value but none was supplied

USAGE:
    clap-ma --clean <clean>...

For more information try --help

Steps to reproduce the issue

  1. Run the first example with cargo run -- -h
  2. See the expected output: --clean is a "flag".
  3. Activate multiple_occurrences.
  4. Notice that --help output remains the same.
  5. Change the field type to a non-bool.
  6. Notice that --clean becomes an arg-accepting "option".

Version

  • Rust: rustc 1.47.0 (18bf6b4f0 2020-10-07)
  • Clap: 3.0.0-beta.2

Actual Behavior Summary

See the above.

I am attempting to represent flags that can appear multiple times in my port of the Aura project to Rust, but I wish to do so via the auto-derived Clap trait, which is much cleaner than handwriting the App and Args.

Debug output

The following shows multiple_occurrences = true, but the field is a single bool:

Debug Output

> cargo run -- -cc
   Compiling clap v3.0.0-beta.2
   Compiling clap-ma v0.1.0 (/home/colin/code/rust/clap-ma)
    Finished dev [unoptimized + debuginfo] target(s) in 5.85s
     Running `target/debug/clap-ma -cc`
[            clap::build::app] 	App::_do_parse
[            clap::build::app] 	App::_build
[            clap::build::app] 	App::_propagate:clap-ma
[            clap::build::app] 	App::_derive_display_order:clap-ma
[            clap::build::app] 	App::_create_help_and_version
[            clap::build::app] 	App::_create_help_and_version: Building --help
[            clap::build::app] 	App::_create_help_and_version: Building --version
[clap::build::app::debug_asserts] 	App::_debug_asserts
[            clap::build::arg] 	Arg::_debug_asserts:clean
[            clap::build::arg] 	Arg::_debug_asserts:help
[            clap::build::arg] 	Arg::_debug_asserts:version
[         clap::parse::parser] 	Parser::get_matches_with
[         clap::parse::parser] 	Parser::_build
[         clap::parse::parser] 	Parser::_verify_positionals
[         clap::parse::parser] 	Parser::get_matches_with: Begin parsing '"-cc"' ([45, 99, 99])
[         clap::parse::parser] 	Parser::is_new_arg: "-cc":NotFound
[         clap::parse::parser] 	Parser::is_new_arg: arg_allows_tac=false
[         clap::parse::parser] 	Parser::is_new_arg: - found
[         clap::parse::parser] 	Parser::is_new_arg: starts_new_arg=true
[         clap::parse::parser] 	Parser::possible_subcommand: arg="-cc"
[         clap::parse::parser] 	Parser::get_matches_with: possible_sc=false, sc=None
[         clap::parse::parser] 	Parser::parse_short_arg: full_arg="-cc"
[         clap::parse::parser] 	Parser::parse_short_arg:iter:c
[         clap::parse::parser] 	Parser::parse_short_arg:iter:c: Found valid opt or flag
[         clap::parse::parser] 	Parser::check_for_help_and_version_char
[         clap::parse::parser] 	Parser::check_for_help_and_version_char: Checking if -c is help or version...
[         clap::parse::parser] 	Neither
[         clap::parse::parser] 	Parser::parse_flag
[    clap::parse::arg_matcher] 	ArgMatcher::inc_occurrence_of: arg=clean
[    clap::parse::arg_matcher] 	ArgMatcher::inc_occurrence_of: first instance
[            clap::build::app] 	App::groups_for_arg: id=clean
[         clap::parse::parser] 	Parser::parse_short_arg:iter:c
[         clap::parse::parser] 	Parser::parse_short_arg:iter:c: Found valid opt or flag
[         clap::parse::parser] 	Parser::check_for_help_and_version_char
[         clap::parse::parser] 	Parser::check_for_help_and_version_char: Checking if -c is help or version...
[         clap::parse::parser] 	Neither
[         clap::parse::parser] 	Parser::parse_flag
[    clap::parse::arg_matcher] 	ArgMatcher::inc_occurrence_of: arg=clean
[            clap::build::app] 	App::groups_for_arg: id=clean
[         clap::parse::parser] 	Parser::get_matches_with: After parse_short_arg Flag(clean)
[         clap::parse::parser] 	Parser::maybe_inc_pos_counter: arg = clean
[         clap::parse::parser] 	Parser::maybe_inc_pos_counter: is it positional?
[         clap::parse::parser] 	No
[            clap::build::app] 	App::groups_for_arg: id=clean
[         clap::parse::parser] 	Parser::remove_overrides
[         clap::parse::parser] 	Parser::remove_overrides:iter:clean
[      clap::parse::validator] 	Validator::validate
[         clap::parse::parser] 	Parser::add_defaults
[      clap::parse::validator] 	Validator::validate_conflicts
[      clap::parse::validator] 	Validator::validate_exclusive
[      clap::parse::validator] 	Validator::validate_exclusive:iter:clean
[      clap::parse::validator] 	Validator::gather_conflicts
[      clap::parse::validator] 	Validator::gather_conflicts:iter: id=clean
[            clap::build::app] 	App::groups_for_arg: id=clean
[      clap::parse::validator] 	Validator::validate_required: required=ChildGraph([])
[      clap::parse::validator] 	Validator::gather_requirements
[      clap::parse::validator] 	Validator::gather_requirements:iter:clean
[      clap::parse::validator] 	Validator::validate_required_unless
[      clap::parse::validator] 	Validator::validate_matched_args
[      clap::parse::validator] 	Validator::validate_matched_args:iter:clean: vals=[]
[      clap::parse::validator] 	Validator::validate_arg_num_vals
[      clap::parse::validator] 	Validator::validate_arg_values: arg="clean"
[      clap::parse::validator] 	Validator::validate_arg_requires:"clean"
[      clap::parse::validator] 	Validator::validate_arg_num_occurs: "clean"=2
[    clap::parse::arg_matcher] 	ArgMatcher::get_global_values: global_arg_vec=[]
Args {
    clean: true,
}

@fosskers fosskers added the C-bug Category: Updating dependencies label Oct 31, 2020
@pksunkara pksunkara added the A-derive Area: #[derive]` macro API label Oct 31, 2020
@pksunkara pksunkara added this to the 3.0 milestone Oct 31, 2020
@fosskers
Copy link
Author

fosskers commented Nov 20, 2020

I managed to satisfy my original requirements via the suggestion given here: #2192 (comment)

We can probably close this issue, since what I was looking for was effectively already possible.

@pksunkara
Copy link
Member

To resolve: Vec<bool> should not be treated as an arg taking values. Also, look into how Option<bool> performs.

@epage
Copy link
Member

epage commented Jul 20, 2021

I'd propose we "won't fix" this and instead point people to #[clap(..., parse(from_occurrences))]

Currently, we don't special case any compound bool types.

I'm trying to understand why/when someone would want one compared to from_occurrences:

  • Option<bool> is redundant: we already know whether the flag is present or not, based on bool
  • Vec<bool> will just be a variable number of true, only useful for the count.

As part of my holistic view for #1772 though I should call this out.

epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 1, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenver I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimeter requirement, can lead to a confusing
user experience but isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimeter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.

`Option<Vec<_>>` ends up matching `Vec<_>` which an raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can no when the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another option would have been to not infer a setting if someone sets a
handful of settings manually, which would have avoided the confusion in
Issue clap-rs#2599 but I see that being confusing (for someone who knows the
default, they will be expecting it to be additive; which flags?) and
brittle (as flags are added or changed, how do we ensure we keep this
up?)

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 0, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenver I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimeter requirement, can lead to a confusing
user experience but isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimeter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.

`Option<Vec<_>>` ends up matching `Vec<_>` which an raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can no when the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another option would have been to not infer a setting if someone sets a
handful of settings manually, which would have avoided the confusion in
Issue clap-rs#2599 but I see that being confusing (for someone who knows the
default, they will be expecting it to be additive; which flags?) and
brittle (as flags are added or changed, how do we ensure we keep this
up?)

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 0, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenever I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimiter requirement, can lead to a confusing
user experience and isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimiter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.

`Option<Vec<_>>` ends up matching `Vec<_>` which an raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can no when the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another option would have been to not infer a setting if someone sets a
handful of settings manually, which would have avoided the confusion in
Issue clap-rs#2599 but I see that being confusing (for someone who knows the
default, they will be expecting it to be additive; which flags?) and
brittle (as flags are added or changed, how do we ensure we keep this
up?)

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 0, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenever I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimiter requirement, can lead to a confusing
user experience and isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimiter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.  At least for me, I also rarely need a
required with multiple occurrences argument but more often need optional
with multiple occurrences.

`Option<Vec<_>>` ends up matching `Vec<_>` which an raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can no when the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another option would have been to not infer a setting if someone sets a
handful of settings manually, which would have avoided the confusion in
Issue clap-rs#2599 but I see that being confusing (for someone who knows the
default, they will be expecting it to be additive; which flags?) and
brittle (as flags are added or changed, how do we ensure we keep this
up?)

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 0, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenever I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimiter requirement, can lead to a confusing
user experience and isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimiter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.  At least for me, I also rarely need a
required with multiple occurrences argument but more often need optional
with multiple occurrences.

`Option<Vec<_>>` ends up matching `Vec<_>` which can raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can detect whether the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another design option would have been to not infer any special-type
settings if someone sets a handful of settings manually, which would
have avoided the confusion in Issue clap-rs#2599 but I see that being confusing
(for someone who knows the default, they will be expecting it to be
additive; which flags disable inferred settings?) and brittle (as flags
are added or changed, how do we ensure we keep this up?).

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717
- `Vec<Option<_>>` and many other potential combinations

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies
Projects
None yet
Development

No branches or pull requests

3 participants