Skip to content

clap_derive CommandFactory::command_for_update implementations set required to true by default instead of true #3924

New issue
New issue
Closed
Closed
clap_derive CommandFactory::command_for_update implementations set required to true by default instead of true#3924
Labels
C-bugCategory: bug
@AndreasBackx

Description

@AndreasBackx
AndreasBackx
opened on Jul 13, 2022

Please complete the following tasks

Rust Version

rustc 1.62.0 (a8314ef7d 2022-06-27)

Clap Version

3.2.10

Minimal reproducible code

use std::env;

use anyhow::Result;
use clap::Parser;
use clap::Subcommand;

#[derive(Parser)]
struct Cli {
    #[clap(subcommand)]
    command: Cmd,
}

#[derive(Subcommand)]
enum Cmd {
    Foo {
        #[clap(short, long)]
        bar: f32,
    },
}

fn main() -> Result<()> {
    eprintln!("--------------------------------------------------------------------------------");
    eprintln!("Below is the output when using command_for_update instead of command to get the");
    eprintln!("command from a CommandFactory in derive.");
    eprintln!("--------------------------------------------------------------------------------");
    eprintln!();

    match parse_mut() {
        Ok(_) => {
            println!("Everything good!");
        }
        Err(error) => {
            // Just printing instead of exiting to show differences.
            error.print()?;
        }
    }

    eprintln!();
    eprintln!("--------------------------------------------------------------------------------");
    eprintln!(
        "Below is when using command, the default when using Parser::parse."
    );
    eprintln!("--------------------------------------------------------------------------------");
    eprintln!();

    Cli::parse();
    Ok(())
}

fn parse_mut() -> std::result::Result<Cli, clap::Error> {
    let mut cmd = <Cli as clap::CommandFactory>::command_for_update();
    // We're ignoring the error here because it doesn't apply for our test case.
    let mut matches = cmd
        .try_get_matches_from_mut(&mut env::args_os())
        .map_err(|e| e.format(&mut cmd))?;
    <Cli as clap::FromArgMatches>::from_arg_matches_mut(&mut matches)
        .map_err(|e| e.format(&mut cmd))
}

Steps to reproduce the bug with the above code

cargo run -- foo

Actual Behaviour

--------------------------------------------------------------------------------
Below is the output when using command_for_update instead of command to get the
command from a CommandFactory in derive.
--------------------------------------------------------------------------------

error: The following required argument was not provided: bar

USAGE:
    clap_command_for_update_bug [SUBCOMMAND]

For more information try --help

--------------------------------------------------------------------------------
Below is when using command, the default when using Parser::parse.
--------------------------------------------------------------------------------

error: The following required arguments were not provided:
    --bar <BAR>

USAGE:
    clap_command_for_update_bug foo --bar <BAR>

clap_derive sets required to false in the generated code:

  • clap/clap_derive/src/derives/args.rs

    Lines 427 to 441 in ffd24af

    Ty::Other => {
    let required = attrs.find_default_method().is_none() && !override_required;
    // `ArgAction::takes_values` is assuming `ArgAction::default_value` will be
    // set though that won't always be true but this should be good enough,
    // otherwise we'll report an "arg required" error when unwrapping.
    let action_value = action.args();
    quote_spanned! { ty.span()=>
    .takes_value(true)
    .value_name(#value_name)
    .required(#required && #action_value.takes_values())
    #possible_values
    #validator
    #value_parser
    #action
    }

The error does eventually get caught in the from_arg_matches_mut it seems, though then there's some context missing.

Expected Behaviour

I'm not exactly "expecting" this, though I would love to know why it works like this internally.

--------------------------------------------------------------------------------
Below is the output when using command_for_update instead of command to get the
command from a CommandFactory in derive.
--------------------------------------------------------------------------------

error: The following required arguments were not provided:
    --bar <BAR>

USAGE:
    clap_command_for_update_bug foo --bar <BAR>

--------------------------------------------------------------------------------
Below is when using command, the default when using Parser::parse.
--------------------------------------------------------------------------------

error: The following required arguments were not provided:
    --bar <BAR>

USAGE:
    clap_command_for_update_bug foo --bar <BAR>

Additional Context

We were using CommandFactory::command_for_update because we have a wrapper macro that adds some defaults. We didn't think twice about the difference between command and command_for_update, the mind kinda was blank and assumed "mutability" from its name. We fixed it by using command instead, though it would be neat if there was some more documentation on how this works and why there are two differences that seemingly return the same, though their implementation differs from what I see.

So I'm leaning towards this not being a bug. Though am reporting it nonetheless hoping to learn more about the internals and maybe we could add more documentation around these functions?

Debug Output

--------------------------------------------------------------------------------
Below is the output when using command_for_update instead of command to get the
command from a CommandFactory in derive.
--------------------------------------------------------------------------------

[      clap::builder::command]  Command::_do_parse
[      clap::builder::command]  Command::_build: name="clap_command_for_update_bug"
[      clap::builder::command]  Command::_propagate:clap_command_for_update_bug
[      clap::builder::command]  Command::_check_help_and_version: clap_command_for_update_bug
[      clap::builder::command]  Command::_check_help_and_version: Removing generated version
[      clap::builder::command]  Command::_check_help_and_version: Building help subcommand
[      clap::builder::command]  Command::_propagate_global_args:clap_command_for_update_bug
[      clap::builder::command]  Command::_propagate removing foo's help
[      clap::builder::command]  Command::_propagate pushing help to foo
[      clap::builder::command]  Command::_propagate removing help's help
[      clap::builder::command]  Command::_propagate pushing help to help
[      clap::builder::command]  Command::_derive_display_order:clap_command_for_update_bug
[      clap::builder::command]  Command::_derive_display_order:foo
[      clap::builder::command]  Command::_derive_display_order:help
[clap::builder::debug_asserts]  Command::_debug_asserts
[clap::builder::debug_asserts]  Arg::_debug_asserts:help
[clap::builder::debug_asserts]  Command::_verify_positionals
[        clap::parser::parser]  Parser::get_matches_with
[        clap::parser::parser]  Parser::get_matches_with: Begin parsing 'RawOsStr("foo")' ([102, 111, 111])
[        clap::parser::parser]  Parser::get_matches_with: Positional counter...1
[        clap::parser::parser]  Parser::get_matches_with: Low index multiples...false
[        clap::parser::parser]  Parser::possible_subcommand: arg=Ok("foo")
[        clap::parser::parser]  Parser::get_matches_with: sc=Some("foo")
[        clap::parser::parser]  Parser::parse_subcommand
[         clap::output::usage]  Usage::get_required_usage_from: incls=[], matcher=false, incl_last=true
[         clap::output::usage]  Usage::get_required_usage_from: unrolled_reqs={}
[         clap::output::usage]  Usage::get_required_usage_from: ret_val={}
[      clap::builder::command]  Command::_build_subcommand Setting bin_name of foo to "clap_command_for_update_bug foo"
[      clap::builder::command]  Command::_build_subcommand Setting display_name of foo to "clap_command_for_update_bug-foo"
[      clap::builder::command]  Command::_build: name="foo"
[      clap::builder::command]  Command::_propagate:foo
[      clap::builder::command]  Command::_check_help_and_version: foo
[      clap::builder::command]  Command::_check_help_and_version: Removing generated version
[      clap::builder::command]  Command::_propagate_global_args:foo
[      clap::builder::command]  Command::_derive_display_order:foo
[clap::builder::debug_asserts]  Command::_debug_asserts
[clap::builder::debug_asserts]  Arg::_debug_asserts:bar
[clap::builder::debug_asserts]  Arg::_debug_asserts:help
[clap::builder::debug_asserts]  Command::_verify_positionals
[        clap::parser::parser]  Parser::parse_subcommand: About to parse sc=foo
[        clap::parser::parser]  Parser::get_matches_with
[        clap::parser::parser]  Parser::add_defaults
[        clap::parser::parser]  Parser::add_defaults:iter:bar:
[        clap::parser::parser]  Parser::add_default_value:iter:bar: doesn't have default missing vals
[        clap::parser::parser]  Parser::add_default_value: doesn't have conditional defaults
[        clap::parser::parser]  Parser::add_default_value:iter:bar: doesn't have default vals
[        clap::parser::parser]  Parser::add_defaults:iter:help:
[        clap::parser::parser]  Parser::add_default_value:iter:help: doesn't have default missing vals
[        clap::parser::parser]  Parser::add_default_value: doesn't have conditional defaults
[        clap::parser::parser]  Parser::add_default_value:iter:help: doesn't have default vals
[     clap::parser::validator]  Validator::validate
[     clap::parser::validator]  Validator::validate_conflicts
[     clap::parser::validator]  Validator::validate_exclusive
[     clap::parser::validator]  Validator::validate_required: required=ChildGraph([])
[     clap::parser::validator]  Validator::gather_requires
[     clap::parser::validator]  Validator::validate_required: is_exclusive_present=false
[     clap::parser::validator]  Validator::validate_required_unless
[     clap::parser::validator]  Validator::validate_matched_args
[        clap::parser::parser]  Parser::add_defaults
[        clap::parser::parser]  Parser::add_defaults:iter:help:
[        clap::parser::parser]  Parser::add_default_value:iter:help: doesn't have default missing vals
[        clap::parser::parser]  Parser::add_default_value: doesn't have conditional defaults
[        clap::parser::parser]  Parser::add_default_value:iter:help: doesn't have default vals
[     clap::parser::validator]  Validator::validate
[     clap::parser::validator]  Validator::validate_conflicts
[     clap::parser::validator]  Validator::validate_exclusive
[     clap::parser::validator]  Validator::validate_required: required=ChildGraph([])
[     clap::parser::validator]  Validator::gather_requires
[     clap::parser::validator]  Validator::validate_required: is_exclusive_present=false
[     clap::parser::validator]  Validator::validate_required_unless
[     clap::parser::validator]  Validator::validate_matched_args
[   clap::parser::arg_matcher]  ArgMatcher::get_global_values: global_arg_vec=[help, help]
[      clap::builder::command]  Command::_build: name="clap_command_for_update_bug"
[      clap::builder::command]  Command::_build: already built
[      clap::builder::command]  Command::_build: name="clap_command_for_update_bug"
[      clap::builder::command]  Command::_build: already built
[         clap::output::usage]  Usage::create_usage_with_title
[         clap::output::usage]  Usage::create_usage_no_title
[         clap::output::usage]  Usage::create_help_usage; incl_reqs=true
[         clap::output::usage]  Usage::get_required_usage_from: incls=[], matcher=false, incl_last=false
[         clap::output::usage]  Usage::get_required_usage_from: unrolled_reqs={}
[         clap::output::usage]  Usage::get_required_usage_from: ret_val={}
[         clap::output::usage]  Usage::needs_options_tag
[         clap::output::usage]  Usage::needs_options_tag:iter: f=help
[         clap::output::usage]  Usage::needs_options_tag:iter Option is built-in
[         clap::output::usage]  Usage::needs_options_tag: [OPTIONS] not required
[         clap::output::usage]  Usage::create_help_usage: usage=clap_command_for_update_bug [SUBCOMMAND]
[      clap::builder::command]  Command::color: Color setting...
[      clap::builder::command]  Auto
[      clap::builder::command]  Command::color: Color setting...
[      clap::builder::command]  Auto
error: The following required argument was not provided: bar

USAGE:
    clap_command_for_update_bug [SUBCOMMAND]

For more information try --help

--------------------------------------------------------------------------------
Below is when using command, the default when using Parser::parse.
--------------------------------------------------------------------------------

[      clap::builder::command]  Command::_do_parse
[      clap::builder::command]  Command::_build: name="clap_command_for_update_bug"
[      clap::builder::command]  Command::_propagate:clap_command_for_update_bug
[      clap::builder::command]  Command::_check_help_and_version: clap_command_for_update_bug
[      clap::builder::command]  Command::_check_help_and_version: Removing generated version
[      clap::builder::command]  Command::_check_help_and_version: Building help subcommand
[      clap::builder::command]  Command::_propagate_global_args:clap_command_for_update_bug
[      clap::builder::command]  Command::_propagate removing foo's help
[      clap::builder::command]  Command::_propagate pushing help to foo
[      clap::builder::command]  Command::_propagate removing help's help
[      clap::builder::command]  Command::_propagate pushing help to help
[      clap::builder::command]  Command::_derive_display_order:clap_command_for_update_bug
[      clap::builder::command]  Command::_derive_display_order:foo
[      clap::builder::command]  Command::_derive_display_order:help
[clap::builder::debug_asserts]  Command::_debug_asserts
[clap::builder::debug_asserts]  Arg::_debug_asserts:help
[clap::builder::debug_asserts]  Command::_verify_positionals
[        clap::parser::parser]  Parser::get_matches_with
[        clap::parser::parser]  Parser::get_matches_with: Begin parsing 'RawOsStr("foo")' ([102, 111, 111])
[        clap::parser::parser]  Parser::get_matches_with: Positional counter...1
[        clap::parser::parser]  Parser::get_matches_with: Low index multiples...false
[        clap::parser::parser]  Parser::possible_subcommand: arg=Ok("foo")
[        clap::parser::parser]  Parser::get_matches_with: sc=Some("foo")
[        clap::parser::parser]  Parser::parse_subcommand
[         clap::output::usage]  Usage::get_required_usage_from: incls=[], matcher=false, incl_last=true
[         clap::output::usage]  Usage::get_required_usage_from: unrolled_reqs={}
[         clap::output::usage]  Usage::get_required_usage_from: ret_val={}
[      clap::builder::command]  Command::_build_subcommand Setting bin_name of foo to "clap_command_for_update_bug foo"
[      clap::builder::command]  Command::_build_subcommand Setting display_name of foo to "clap_command_for_update_bug-foo"
[      clap::builder::command]  Command::_build: name="foo"
[      clap::builder::command]  Command::_propagate:foo
[      clap::builder::command]  Command::_check_help_and_version: foo
[      clap::builder::command]  Command::_check_help_and_version: Removing generated version
[      clap::builder::command]  Command::_propagate_global_args:foo
[      clap::builder::command]  Command::_derive_display_order:foo
[clap::builder::debug_asserts]  Command::_debug_asserts
[clap::builder::debug_asserts]  Arg::_debug_asserts:bar
[clap::builder::debug_asserts]  Arg::_debug_asserts:help
[clap::builder::debug_asserts]  Command::_verify_positionals
[        clap::parser::parser]  Parser::parse_subcommand: About to parse sc=foo
[        clap::parser::parser]  Parser::get_matches_with
[        clap::parser::parser]  Parser::add_defaults
[        clap::parser::parser]  Parser::add_defaults:iter:bar:
[        clap::parser::parser]  Parser::add_default_value:iter:bar: doesn't have default missing vals
[        clap::parser::parser]  Parser::add_default_value: doesn't have conditional defaults
[        clap::parser::parser]  Parser::add_default_value:iter:bar: doesn't have default vals
[        clap::parser::parser]  Parser::add_defaults:iter:help:
[        clap::parser::parser]  Parser::add_default_value:iter:help: doesn't have default missing vals
[        clap::parser::parser]  Parser::add_default_value: doesn't have conditional defaults
[        clap::parser::parser]  Parser::add_default_value:iter:help: doesn't have default vals
[     clap::parser::validator]  Validator::validate
[     clap::parser::validator]  Validator::validate_conflicts
[     clap::parser::validator]  Validator::validate_exclusive
[     clap::parser::validator]  Validator::validate_required: required=ChildGraph([Child { id: bar, children: [] }])
[     clap::parser::validator]  Validator::gather_requires
[     clap::parser::validator]  Validator::validate_required: is_exclusive_present=false
[     clap::parser::validator]  Validator::validate_required:iter:aog=bar
[     clap::parser::validator]  Validator::validate_required:iter: This is an arg
[     clap::parser::validator]  Validator::is_missing_required_ok: bar
[     clap::parser::validator]  Conflicts::gather_conflicts: arg=bar
[     clap::parser::validator]  Conflicts::gather_conflicts: conflicts=[]
[     clap::parser::validator]  Validator::missing_required_error; incl=[]
[     clap::parser::validator]  Validator::missing_required_error: reqs=ChildGraph([Child { id: bar, children: [] }])
[         clap::output::usage]  Usage::get_required_usage_from: incls=[], matcher=true, incl_last=true
[         clap::output::usage]  Usage::get_required_usage_from: unrolled_reqs={bar}
[         clap::output::usage]  Usage::get_required_usage_from:iter:bar
[         clap::output::usage]  Usage::get_required_usage_from: ret_val={"--bar <BAR>"}
[     clap::parser::validator]  Validator::missing_required_error: req_args=[
    "--bar <BAR>",
]
[         clap::output::usage]  Usage::create_usage_with_title
[         clap::output::usage]  Usage::create_usage_no_title
[         clap::output::usage]  Usage::create_help_usage; incl_reqs=true
[         clap::output::usage]  Usage::get_required_usage_from: incls=[], matcher=false, incl_last=false
[         clap::output::usage]  Usage::get_required_usage_from: unrolled_reqs={bar}
[         clap::output::usage]  Usage::get_required_usage_from:iter:bar
[         clap::output::usage]  Usage::get_required_usage_from: ret_val={"--bar <BAR>"}
[         clap::output::usage]  Usage::needs_options_tag
[         clap::output::usage]  Usage::needs_options_tag:iter: f=bar
[         clap::output::usage]  Usage::needs_options_tag:iter Option is required
[         clap::output::usage]  Usage::needs_options_tag:iter: f=help
[         clap::output::usage]  Usage::needs_options_tag:iter Option is built-in
[         clap::output::usage]  Usage::needs_options_tag: [OPTIONS] not required
[         clap::output::usage]  Usage::create_help_usage: usage=clap_command_for_update_bug foo --bar <BAR>
[      clap::builder::command]  Command::color: Color setting...
[      clap::builder::command]  Auto
error: The following required arguments were not provided:
    --bar <BAR>

USAGE:
    clap_command_for_update_bug foo --bar <BAR>

For more information try --help

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-bugCategory: bug

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions