Changes to invocation to remove middleware pipeline

[KathleenDollard]

This is my representation of a bunch of conversations that have happened in imagining a replacement to the middleware pipeline. If you customize the middleware pipeline, please read this and imagine your use case with this approach - or ask us about it. Thanks to @jonsequitur, @adamsitnik, @Keboo, @patriksvensson, @HowardvanRooijen and @mhutch for the conversations that got us this far and to you for feedback, if you customize middleware. If you do not customize middleware, everything will work as before.

Execution changes

We are planning significant changes to the internals of how System.CommandLine determines what to execute.

• This will not affect you unless you customize the middleware pipeline. Most System.CommandLine users will not need to understand the new pipeline or that we have changed it.
• If you only customize the pipeline to add or remove existing elements like version, this will be a small change.
• We need to hear from you if you do customizations to the pipeline that you do not think are supported in the new model!

Why the change

The current System.CommandLine internal execution uses a middleware pipeline which you can alter. There is a default so many programmers have been able to ignore the internal design. That design is a series of steps that can do work and then indicate whether they terminate that runs after parsing on invocation. FOr example, the UseParseErrorReporting step displays validation issues and if there are issues does not call the next step in the pipeline - thus terminating.

A few problems with this approach:

• The code could be intimidating and it was not always obvious how to accomplish things.
• Key steps in the pipeline - like reporting validation issues and help - were tied to invocation and could not easily be used in scenarios that did not use System.CommandLine to invoke actions.
• It had a complicated order dependency. Issues were created because CLI authors incorrectly put their step before or after ParseErrorReporting, for example.
• It put items on the callstack which surprised users and could make stepping awkward during debugging.
• Version and Help options were internal, so it was difficult to customize without copying code.
• There are concerns around performance of this approach.

While these things have proved problematic, the middleware approach has been helpful in us gaining insight into what most people want to do.

One purpose of this issue is to find scenarios we do not yet understand.

This experience, and requests as part of generation and from other sources, has led to a deep redesign. For example, we have confidence in the set of features that are part of the common set:

Name	In default set?	Sometimes terminates?	Modifies
UseVersionOption	Yes	Yes
UseHelp	Yes	Yes
UseParseDirective	Yes	Yes
UseParseErrorReporting	Yes	Yes
UseSuggestDirective	Yes	Yes
CancelOnProcessTermination	Yes	No	Invoked code
UseExceptionHandler	Yes	No	Invoked code
UseEnvironmentVariableDirective	Yes	No	Invoked code
UseHelpBuilder	No	No	Help
UseLocalizationResources	No	No	Validation messages
UseTypoCorrections	No	No	Validation reporting

RegisterWithDotnetSuggest and UseTokenReplacer were part of UseDefaults, but not of the pipeline.

Terminating means that no other actions will be performed. Only one terminating action is ever performed. The non-terminating middleware elements affect the behavior of other steps. This follows a Chain of Responsibility design pattern.

The new design

You'll notice that all of the middleware steps either perform an action and terminate, or modifies another step. Both may also alter the result. To better model this, the new design focuses on the steps - the actions.

flowchart
Configuration --> Parser --> Modifiers --> Action
Modifiers --> complex[Complex Action]

Loading

For example, help will continue to be configured as a default and will alter the command tree to add the help global option. You will be able to turn that off via configuration and you will still be alter how help is displayed.

Since I think in code, I'll describe how invocation will work cia code; this is not indicating an implementation! Among other things, the naming is designed for this document not implementation, and extra parameters are passed for clarity, and aspects are strictly air code. The number two clarifies that these are modified classes, similar to but not identical to the previous classes. The parse result will know what action is to be performed, in this air code, it is done by classes derived from ParseResult2:

public class InvokeResult
{...}

public class InvokeResult<T> 
    where T : ICommandHandler

public static int Invoke(Configuration2 configuration, ParseResult2 parseResult, InvocationResult2 invocationResult)
{
   return parseResult switch
   {
      HelpResult => DisplayHelp(configuration, parseResult),
      VersionResult => DisplayVersion(),
      ParseDirectiveResult => DiagramInput(parseResult),
      ParseErrorReportingResult => DisplayValidationErrors(configuration, parseResult),
      SuggestResult => SuggestResult(parseResult), // this is for tab completion
      InvokeResult => InvokeCommand(configuration, parseResult, invocationResult),
      _ => throw...   
   }
}
 
public static int InvokeCommand(Configuration2 configuration, ParseResult2 parseResult, InvocationResult2 invocationResult)
{
   var handler = invocationResult.Handler
   handler = configuration.UseExceptionHandler
               ? WithTry(handler)
               : handler
   handler = configuration.UseCancelOnProcessTermination
               ? WithCancelOnProcessTermination(handler)
               : handler

   static Func<..., int> WithTry(Func<..., int> handler)
   {
      try
         handler(configuration, parseResult, invocationResult)
      catch (Exception e)
         ReportException(e)
   }
   // WithCancelOnProcessTermination is similar to WithTry, but lots of code.
}

There will obviously be more code to support this, but we think it will solve the problems with the previous approach.

Modifying the pipeline

If you are modifying the pipeline today, you may need to modify the way you think about your solution. Here are some examples to get you started. This is still air code, but relatively close to what you will need do.

Replace version or another step

public static int Invoke(Configuration2 configuration, ParseResult2 parseResult, InvocationResult2 invocationResult)
{
   return parseResult switch
   {
      VersionResult => DisplayMyVersion(parseResult),
      _ => invocationContext.Invoke(configuration, parseResult, invocationResult)
   }
}

Do something before or after invocation

public static int Invoke(Configuration2 configuration, ParseResult2 parseResult, InvocationResult2 invocationResult)
{
   return parseResult switch
   {
      InvokeResult => WrapInLogger(configuration, parseResult, invocationResult),
      _ => invocationResult.Invoke(configuration, parseResult, invocationResult)
   }

   static int WrapInLogger(Configuration2 configuration, ParseResult2 parseResult, InvocationResult2 invocationResult)
   {
       if (LoggingTurnedOn())
       { myLogger.LogStart(parseResult.CommandResult.Name); }
       invocationContext.InvokeCommand(configuration, parseResult, invocationResult);
       if (LoggingTurnedOn())
       { myLogger.LogEnd(parseResult.CommandResult.Name); }
   }
}

Do something for a specific command

public static int Invoke(Configuration2 configuration, ParseResult2 parseResult, InvocationResult2 invocationResult)
{
   return parseResult switch
   {
      InvokeResult<MyCommand> => WrapInLogger(configuration, parseResult, invocationResult),
      _ => invocationResult.Invoke(configuration, parseResult, invocationResult)
   }
}

Summary

We are redesigning the execution pipeline. While other changes will probably affect you, this change only affects people that have modified the middleware pipeline. In all the cases we have considered, this change will make your code easier to understand and probably reduces the code you need to write; for example we may be able to remove the CommandLineBuilder based APIs. We also know that folks have been quite creative with the middleware pipeline and we need you to consider how the new design will work with your needs. If it doesn't, or you have questions, we look forward to hearing from you.

[KalleOlaviNiemitalo]

Is class InvokeResult derived from class ParseResult2?

Having both class InvokeResult and class InvocationResult2 would be very confusing.

Does InvocationResult2.Handler have to synchronously return int, or can it return Task<int>? I haven't really needed async here but I'm surprised if you remove the feature.

If the command line has a [parse] directive followed by validation errors, what controls whether ParseDirectiveResult or ParseErrorReportingResult takes precedence? Does the same apply to custom directives?

Is Invoke(Configuration2, ParseResult2, InvocationResult2) a static method, an instance method on InvocationContext, or both?

How is the InvokeResult<MyCommand> instance constructed; does that involve reflection when no source generator is available?

case InvokeResult<MyCommand> result wouldn't match InvokeResult<MyDerivedCommand> where class MyDerivedCommand is derived from class MyCommand. Cannot change it to class InvokeResult<in T> either, as generic variance only supports delegates and interfaces. Perhaps it won't be a problem in practice.

[eajhnsn1]

Thank you for the air code. If I'm understanding correctly, this has the look of shifting from an aspnet middleware construct to
one more akin to MVC's "pipeline" involving action results and action filters. That's what came to mind, anyway.

In the "examples to get you started", will such code be implemented as static methods (as demonstrated here) or behind some interface, a. la IActionFilter?

Finally, is it a correct assumption that there will still be an async path?

[adamsitnik]

I like the idea, but I wonder if we really need to expose custom result types that derive from ParseResult. I am currently not sure how to make it easy and performant for us to create instances of custom user defined types, moreover it would require creating and exposing a lot of new types (bad for perf, as JIT needs to load and compile all of them and our startup scenario is already dominated by JIT)

ParseResult already defines CommandResult which allows us to identify parsed command:

command-line-api/src/System.CommandLine/ParseResult.cs

Lines 62 to 65 in 6462288

	/// <summary>
	/// A result indicating the command specified in the command line input.
	/// </summary>
	public CommandResult CommandResult { get; }

To support active Options and Directives, we could extend ParseResult with SymbolResult:

public class ParseResult
{
    public SymbolResult SymbolResult { get; }
}

and bring back SymbolResult.Symbol: #2031

or just

public class ParseResult
{
    public Symbol Symbol { get; }
}

Then our users could do sth like this:

public static int Invoke(ParseResult parseResult)
{
   return parseResult.Symbol switch
   {
      HelpOption => DisplayHelp(parseResult),
      VersionOption => DisplayVersion(),
      ParseDirective => DiagramInput(parseResult),
      ParseErrorReporting => DisplayValidationErrors(configuration, parseResult),
      Command cmd => InvokeCommand(cmd, parseResult),
      _ => throw...   
   }
}

We would not need to worry about creating these types, as they have already been created by the user and provided for the parser. It would require making few internal types public (not a big deal, we should consider doing that regardless of that) and implementing Directive symbol type (#2056) which we are also going to do anyway.

[KalleOlaviNiemitalo]

@adamsitnik, treating ParseErrorReporting as a Symbol doesn't feel right to me.

[KalleOlaviNiemitalo]

So Invoke should either check ParseResult.Errors first

public static int Invoke(ParseResult parseResult)
{
   if (parseResult.Errors.Count != 0)
   {
       return DisplayValidationErrors(configuration, parseResult);
   }
    
   return parseResult.Symbol switch
   {
      HelpOption => DisplayHelp(parseResult),
      VersionOption => DisplayVersion(),
      ParseDirective => DiagramInput(parseResult),
      Command cmd => InvokeCommand(cmd, parseResult),
      _ => throw…
   };
}

or use more complex patterns

public static int Invoke(ParseResult parseResult)
{
   return parseResult switch
   {
      { Symbol: HelpOption } => DisplayHelp(parseResult),
      { Symbol: VersionOption } => DisplayVersion(),
      { Symbol: ParseDirective } => DiagramInput(parseResult),
      { Errors: { Count: not 0 } } => DisplayValidationErrors(configuration, parseResult),
      { Symbol: Command cmd } => InvokeCommand(cmd, parseResult),
      _ => throw…
   };
}

but the former would cause Errors to take precedence over ParseDirective, and the latter would be more verbose and more work to port to C# 7 for supported use on .NET Framework.

[KathleenDollard]

@KalleOlaviNiemitalo Thanks for the suggestion of not having ParseErrorResult as an action, but as something that happens around the action. I think there are some interesting possibilities there, including separate display for some commands and future support for warnings and warnings as errors.

On whether we have different ParseResults: I do not want to couple to Symbol. I see several issues here, including that I want folks to use a different help option (a different name, or less/more aliases for example) without changing the invocation.

I would be find if the switch was based on something more abstract. As an example, if we had well known integers or strings for the things we understand how to do (the things in our switch). That still leaves open the fact that different information would be used for the different switch arms. I am still thinking about that.

[KathleenDollard]

I wanted to record a few things from a conversation this morning:

We think there are three kinds of things at invocation that is somewhat orthogonal to what the Symbol is:

• Provide data (--targetframework)
• Request an action (build)
• Request pre or post processing for orthogonal concerns (a localization directive)

I now think that there may be a fourth:

• Do nothing, I'm just a container (dotnet)

Spectre.Console uses a separate concept for non-invokable commands (Branch)

• All Command symbols request an action or do nothing
• All Argument symbols carry data only
• Options may request an action (--version) or provide data (--targetframework)
• Directives may request an action ([parse]), provide data (``) or request pre or post processing

As @KalleOlaviNiemitalo pointed out in this issue, there is also the scenario where the state of the result indicates a request for a different action (ParseErrorReporting).

[adamsitnik]

Middleware has been removed in #2100, for more details please refer to #2071