Command-line-api: Create a Roslyn analyzer to enforce matching argument/option names with command handler parameter names

[jonsequitur]

The most common issue people encounter with the System.CommandLine API is when option or argument names/aliases do not match the parameters in a command handler:

var command= new RootCommand
{
    new Option<int>("--aaa")
};
command.Handler = CommandHandler.Create<int>(zzz => /*  zzz is always going to be 0  */); 

We haven't seen an approach that enforces a relationship between the parameters and the parser at compile time without an enormous increase in the complexity of the code, so we took a more dynamic approach, based on ASP.NET MVC controller methods. The consequence is that these mismatches can't be flagged by the compiler, and until you're aware of the convention, it's easy to get this wrong.

Programmatic validation that all parameters are matchable on all command handlers at runtime has been proposed. This approach would be very expensive, especially when complex objects rather than scalar values are being bound, and since this won't vary after the code is compiled, it's not ideal to pay this cost every time a program runs.

An analyzer might be a more appropriate tool for the job, since it only needs to be validated when the code is being written. It could also include quick fixes for renaming parameters to match options and arguments.

[jeremyong]

This approach would be very expensive, especially when complex objects rather than scalar values are being bound, and since this won't vary after the code is compiled, it's not ideal to pay this cost every time a program runs.

It seems reasonable to restrict the full validation to Debug builds if performance is a concern, or at the very least, implement a "light version" and "full version" of the reflection-based validation so that the light version kicks in at runtime. An option to disable validation (passed to the CommandHandler) may also make sense.

[jeredm]

Another case to allow in an analyzer would be for any "magic" variables like console.

var addCommmand = new Command("add-item", "Adds a grocery item to the list")
{
    new Option<int>(new string[] { "--quantity", "-q" }, () => 1, "The number of items to add"),
    new Argument<string>("name", "The name of the grocery item")
};
addCommmand.Handler = CommandHandler.Create<IConsole, string, int>(async (console, name, quantity) =>
    await shoppingList.AddItemsAsync(console, name, quantity));

[jonsequitur]

@jeremyong The reason we haven't approached this with something that only runs by default in debug mode is that this introduces a significant behavioral difference between debug and release builds. Unless someone is verifying both builds, it creates a possibility of shipping serious bugs.

[jeremyong]

The "behavior" I would want is a hard crash (in Debug mode). I doubt anyone would compile for Release without first developing in debug mode first.

[ohadschn]

Programmatic validation that all parameters are matchable on all command handlers at runtime has been proposed. This approach would be very expensive, especially when complex objects rather than scalar values are being bound, and since this won't vary after the code is compiled, it's not ideal to pay this cost every time a program runs.

So instead of iterating all command handlers, only do this before calling the actual specific handler. That would still surface the error much sooner than providing the default(T) value which is really bad (especially for integers where 0 could be a valid value with a radically different meaning). You could also exclude complex types and/or provide an opt-in/out flag (could be an enum like NoValidation, FullValidation, ScalarValidation). Not sure if it would help, but a possible easing of the problem could be a count-only approach that only makes sure that if a method has N params, then N matches have been made.

An analyzer would be great for sure (though I would still be hesitant to drop such explicit runtime checks), but until we have it I vote for a runtime compromise to be made, as the current state IMHO is not reasonable.

[ohadschn]

In the meantime, I thought about ways to perform this validation myself. My first idea was something like the following:

command.Handler = CommandHandler.Create<string>(foo => {
    if (foo == null) throw new Exception("commandline mismatch"); 
   // do stuff with foo...
});

But what if foo is optional? This won't discern between the case where the argument wasn't supplied at all and the case that the handler method param name didn't match. So I ended up doing this:

const string optionAlias = "--foo";
var command= new RootCommand
{
    new Option<int?>(optionAlias)
};
command.Handler = CommandHandler.Create((ParseResult pr) => DoSomething(pr.ValueForOption<int?>(optionAlias))); 

Now I can be sure that the option alias will never be mismatched with the handler method's param.

[jonsequitur]

@ohadschn Even performing this validation on a single command handler would add perf overhead on all invocations of that command handler. Since the code only changes during the developer loop, the verification should ideally also be part of the developer loop. Once you know it works and you ship it, your users don't need to be executing this verification when they run your tool. So doing the verification at build/test time points to a couple of approaches I can think of:

• An analyzer
• A test method, e.g. CommandLineConfiguration.MakeSureMyBindingsWork(), that can be called in your tests

Both are effectively static analysis and will of course only address certain cases. For example, custom parsing logic (ParseArgument<T>) must be exercised by running the code.

[ohadschn]

I agree that in a perfect world we'd have static analyzers and unit tests covering everything, but in the real world we don't have the analyzer (at least yet) and test coverage is often much closer to 0% than 100%.

Also in the real world, in my humble opinion, the vast majority of users would be willing to pay 10 milliseconds on what would typically be once on startup, to defend against seemingly harmless refactoring from breaking their code. Especially when the parser _already_ uses reflection to match the parameter names...

[jeremyong]

TBH I would be impressed if the validation even added 1 ms. That's an eternity in CPU time and really speaks to greater issues if true. Agreed that optimizing for speed over correctness for command options parsing seems like the incorrect tradeoff in the grand scheme of things.

[jonsequitur]

@ohadschn The perfect world you speak of might just be C# 9 :smile:

We're working now on perf improvements based on user feedback and most of the cost is in model binding (10-20ms on my not-super-fast laptop), which by default uses reflection heavily, and not in parsing, which avoids it. Within model binding, reflection-based matching can be largely avoided today, though it requires more code. Here's an example of an API for reflection-free binding:

https://github.com/jonsequitur/command-line-api/blob/79e351cb4356252f7dae4acb91f3111db4e53667/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs#L520-L538

Another way of looking at the problem might be to allow people to disable name-based matching entirely, which would require developers to write the code for strict and explicit binding. I've spoken with a number of people with a strong preference for this on the grounds that the convention-based approach is less probably correct. For people unaccustomed to convention-based APIs, the name-based matching isn't intuitive.

In the meantime, I think a method to explicitly do a binding validation check could support both approaches and give us a better basis for discussing performance numbers. You would be able to opt into using it in your app startup, or you could use it only in tests, depending on when in the cycle you want to pay the perf cost. We don't want to default users into this perf hit though because a decent test suite completely avoids this gotcha and because we believe source generators can provide a solution that doesn't require the same tradeoff. When that's in place, the validation is effectively done by the compiler.

[ohadschn]

That's fair, but IMHO the vast majority of clients would be oblivious to this issue and won't have test coverage nor full understanding of the convention based approach.

As such, I would vote for the default behavior to err on the side of correctness, and let performance-conscious clients who care about that typically one-time several millisecond delay turn it off and add tests.

[KathleenDollard]

I have fallen into the opinion of explicit binding and the inference should happen at design time where validation doesn't have a perf hit. I think mostly about app models, and they should do a lot of validation at design time.

I think if we guide people to explicit binding having a default that validates runtime inferred bindings makes sense. Hopefully we can pull that off.

To guide people to explicit binding, I think a extension method is worthwhile. To allow this pattern instead of the separate BindMember line:

            var binder = new ModelBinder<ClassWithMultiLetterSettersTwoInts>();
            var intOption = new Option<int>("--int-property").BindOption(binder, obj => obj.IntOption);
            var stringOption = new Option<string>("--string-property").BindOption(binder, obj => obj.StringOption);
            var int2Option = new Option<int>("--int-property-two").BindOption(binder, obj => obj.IntOptionTwo);
            var parser = new Parser(intOption, int2Option, stringOption);

            var bindingContext = new BindingContext(parser.Parse("--int-property 42 --int-property-two 43 --string-property Hello"));

The extension method I wrote for this is:

        public static Option BindOption<TModel, TValue>(this Option option, ModelBinder<TModel> binder, Expression<Func<TModel, TValue>> property)
        {
            binder.BindMemberFromValue(property, option);
            return option;
        }

Two questions for @jonsequitur

• Does explicit binding work for method parameters yet?
• Shall we split this up into a couple of issues as the "Help Wanted" isn't just for the Roslyn analyzer anymore.

[jonsequitur]

Does explicit binding work for method parameters yet?

We have to find some time to work on #953. I didn't look at it closely because it's still in draft, but I'm happy to help out.

Shall we split this up into a couple of issues as the "Help Wanted" isn't just for the Roslyn analyzer anymore.

Probably, yes. I spoke with @jmarolf a bit about how to approach an analyzer. It was pretty clear it will only be an 80% solution but it would still be a big improvement in that it would raise awareness of this gotcha.

[KathleenDollard]

Yeah, I'm not sure I took the right approach with #953. I have had an orthogonal idea for that. Let's find time to talk directly on this.

Thoughts on adding that extension method?

[jonsequitur]

Potential solution: #1012. Please comment.