dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.39k stars 381 forks source link

Type mismatch between string and string array to error out #787

Open am11 opened 4 years ago

am11 commented 4 years ago

Noticed at https://github.com/dotnet/format/pull/551#discussion_r379924103, if option.Argument = new Argument<string>(() => null) is used and the target delegate's signature has string[], it can be an error so the user knows that API is not going to work as expected.

myApp --myStringArrayArg val1 val2 val3

will only function properly at runtime with option.Argument = new Argument<string[]>(() => null).

jonsequitur commented 4 years ago

We don't currently validate the binding, so things like type mismatches and argument name mismatches (the most common gotcha with the current API) will only be discovered at runtime. (This is consistent with ASP.NET's model binding behavior, for what it's worth.)

It's unfortunate that these kinds of things are a little hard to discover. But part of why we haven't addressed it is that it's not clear the performance hit would be justified, since it's pretty easily sorted out in unit testing. Also, app models layered on top (like DragonFruit) make this kind of validation unnecessary, since they'll typically derive the parser configuration from the handler, so the parameter types and names are always aligned.

All that said, if you have any suggestions for how to better validate this, we're happy to try things out.

am11 commented 4 years ago

Thanks @jonsequitur. In this particular case (string[] and string), we do not get runtime error either, but the undesired behavior. From the example above val2 val3 will be silently ignored.

I was thinking could this be detected at binding time (still during the runtime) and thrown to the consumer (so we get a chance to add forgotten [] in Argument<string>)? (unless we explicitly prefer the current params like experience between string and string[])

jonsequitur commented 4 years ago

I'm not sure I fully understand the issue. It sounds like val2 val3 should be ignored (or treated as unmatched by that specific argument) only for the string case, but should be present in the string[] case. This is because the arity for string will be set to ExactlyOne, whereas for string[] it will be set to ZeroOrMore.

Can you provide an example in the form of a unit test that displays the unexpected behavior?

am11 commented 4 years ago

If there is a mismatch is between argument description (using string):

var rootCommand = new RootCommand
{
    new Option(new[] { "--include"})
    {
        Argument = new Argument<string>(() => null)
    }
};
rootCommand.Handler = CommandHandler.Create(GetType().GetMethod(nameof(Run)));

and the command handler (using string[]):

public static async Task<int> Run(string[] include)

there is no run-time error. As a consumer I would expect that binder will throw error or warn somehow, as string[] is not the exact typed argument of Run (but assignable).