Commands shouldn't be invoked with default(T) values when argument is required and no default value is provided

[johncrim]

It looks like there is a bunch of validation for missing arguments, but it isn't being called when the option isn't specified. The following test fails, I've highlighted what I think are problems with PROBLEM :

        [Fact]
        public void Invoke_command_with_missing_arguments_should_return_error()
        {
            var command = new RootCommand()
                          {
                              new Option("--int-value")
                              {
                                  Argument = new Argument("intValue")
                                             {
                                                 ArgumentType = typeof(int)
                                             }
                              }
                          };

            bool commandInvoked = false;
            command.Handler = CommandHandler.Create<int>((int intValue) =>
                                                         {  // PROBLEM: This handler is called with intValue: 0
                                                             commandInvoked = true;
                                                         });

            // Argument doesn't have a default value, and is required once:
            Assert.False(command.Children.OfType<Option>().First().Argument.HasDefaultValue);
            Assert.Equal(1, command.Children.OfType<Option>().First().Argument.Arity.MinimumNumberOfValues);

            var console = new TestConsole();
            command.Invoke("", console);

            // PROBLEM: Expected: Command not invoked
            Assert.False(commandInvoked);
            // PROBLEM: Expected: Should see errors on the console
        }

I looked at #484 , and while that is the root of the problem, it doesn't make sense from a usability standpoint. In short, if I have a command like the above, I shouldn't have to worry about a required argument being passed in as default(T) when arity is 1, and no default value is specified. If I want a default value for a parameter, I'll specify it, eg (int intValue=0) and/or Argument.SetDefaultValue(0).

Please reconsider this design.

It seems at odds with other behavior, too. For example, here's a test in the codebase that passes:

        [Fact]
        public void When_command_arguments_are_fewer_than_minimum_arity_then_an_error_is_returned()
        {
            var command = new Command("the-command")
            {
                Argument = new Argument
                {
                    Arity = new ArgumentArity(2, 3)
                }
            };

            var result = command.Parse("1");

            result.Errors
                  .Select(e => e.Message)
                  .Should()
                  .Contain(ValidationMessages.Instance.RequiredArgumentMissing(result.CommandResult));
        }

Am I missing something that positional arguments are fundamentally different from named arguments?

Or, if this design is absolutely not going to change, is there a good way to add middleware that changes this behavior, such that all required arguments with arity.min = 1, raise the existing "missing parameter" error messages?

[jonsequitur]

Thanks for detailed writeup, @johncrim. The issue is not with named (i.e. option) arguments versus positional (i.e. command) arguments.

Your example test passes if you change the invocation to this:

command.Invoke("--int-value", console);

Am I correct in understanding that you expect that the user is required to supply --int-value? Per the current API design, this isn't the intention. The minimum arity of 1 (inferred from ArgumentType = typeof(int)) applies to the argument to the --int-value option, if --int-value is specified. For example:

> myapp                        #valid
> myapp --int-value 1   #valid
> myapp --int-value      #invalid

System.CommandLine does not currently have a first-class concept to make the first of my examples invalid on account of --int-option being missing, but we're planning to add it (#560), since this is a common point of confusion. The question will then be does that prevent people from trying to achieve this by looking at Option.Argument.Arity?

[johncrim]

Thank you for the quick response! On a weekend no less... and thank you for all your work on this very useful library.

Yes, if the minimum arity is 1, and no default value is set, I expect the option-argument combination to be required, and consider it a user error if the option+argument is not supplied (same behavior as if the argument is a positional argument instead of a named one).

I understand that the test passes if I use command.Invoke("--int-value", console); - and certainly, this case should result in a command line error. It is definitely a user error to include the option and not the value.

But it also seems obvious to me that, if the option is coupled to an argument, and the argument is "required", that it is an error if the user omits the option + argument. It's confusing to me that the less common case (omitting the named argument value) is an error, but the more common case (omitting a "required" argument) is not considered an error.

On the topic of the option being coupled to an argument, I found the child relationship between Option->Argument to be confusing when figuring out how to setup a command - I consider the name part of the argument definition. In other frameworks (eg Powershell Cmdlet Parameters ), Parameters have a name and a type, and can be positional and/or specified with names/aliases. The tree API that System.CommandLine provides kind of makes sense when building a tree of symbols from parsing the command-line, but for ease of use with the API, I would love to have the ability to just add parameters to a command, and the parameter has an optional name, optional position, type, arity, aliases, etc. (I use the term "parameter" because that's the way most developers think about the definition of the thing that receives an argument.) I'll open another feature request on this if you'd like, but I'm explaining it here because I think the Option -> Argument setup creates room for the problem(s) we're discussing.

On the topic of "required" arguments, this already exists for positional arguments, it's just not called "required". I think the combination of a Arity.Min = 1 and no default value clearly indicates that the argument is required (again - works today for positional arguments). I don't think there's a need to add a separate .Required property, that would just add redundancy to the API. A developer can get the same behavior (optional named parameters) as today just by setting Arity.Min = 0 or setting a default value. What would Option.Required mean in combination with Arity.Min = 0 or an Argument.DefaultValue? I think it would add unnecessary complexity to the API.

Lastly, if you step through the code and watch the invocation of the command delegate, you'll see it's called with null arguments, which are converted to default(T) by Delegate.DynamicInvoke in the .NET framework. Since the user didn't specify the argument, it's a bug that a different value is provided to the delegate. This could be handled by the developer writing some argument validation in the command delegate: they'd need to know to test against default(T), and they'd need to know that the value default(T) might mean that the user didn't specify the required argument, and they'd throw an ArgumentException saying "--intValue is required and can't be 0"; but doing it this way mixes up the layers and doesn't provide the nice command-line "UI" feedback that other parameter validation errors provide.

Hope that helps...

[jonsequitur]

On the topic of the option being coupled to an argument, I found the child relationship between Option->Argument to be confusing when figuring out how to setup a command - I consider the name part of the argument definition.

I think this has been a common point of confusion. The naming is based on GNU and POSIX conventions and a lot of discussion has resulted, especially around how to reconcile this with PowerShell (and more broadly, bridge different expectations among Windows, Linux, and macOS users).

I agree that reducing the concept count would be useful here. We've considered the possibility of introducing an Option<T> class which might be syntactic sugar over an Option with a child Argument<T>.

[johncrim]

This issue is effectively resolved in System.CommandLine 2.0.0-beta4.22272.1, mainly by adding Option.IsRequired to the API. Eg this test passes:

    [Fact]
    public void Invoke_command_with_missing_arguments_should_return_error()
    {
        var intOption = new Option<int>("--int-value");
        intOption.IsRequired = true;
        var command = new RootCommand() { intOption };

        bool commandInvoked = false;
        command.SetHandler((int intValue) =>
                           {
                               commandInvoked = true;
                           },
                           intOption);

        // Argument doesn't have a default value, and is required once:
        Assert.True(command.Children.OfType<Option>().First().IsRequired);
        Assert.Equal(1, command.Children.OfType<Option>().First().Arity.MinimumNumberOfValues);

        var console = new TestConsole();
        command.Invoke("", console);

        Assert.False(commandInvoked);
        Assert.Contains("--int-value <int-value> (REQUIRED)", console.Out.ToString());
    }