enable CliAction to be non-exclusive

[jonsequitur]

The previous middleware design allowed for middleware components to pass through invocation to the next component in the pipeline by calling a continuation delegate. This continuation is the next parameter in this example:

commandLineBuilder.AddMiddleware(async (context, next) =>
{
  // do something before the main action
  await next(context);
  // do something after the main action
});

This complexity was removed as part of the CliAction design (#2071) but the initial implementation (#2095) omitted this behavior except in the case of one built-in directive, EnvironmentVariablesDirective / [env], which was hard-coded to call the main command's CliAction after setting the specified environment variables.

This PR adds the property CliAction.Exclusive (defaulting to true) and when actions have this property set to false, those actions are aggregated in a list rather than being used as the main action. When the command line is invoked, each non-exclusive action is called in turn prior to the main action.

This provides a generalized way to do something before the main action in the code sample above.

As no built-in examples of do something after the main action existed in the code, I haven't added a generalization of that, though the design should potentially take into account that this is something we might want to add later.

[KathleenDollard]

Using the word "Exclusive" both results in a default of true (I know my dislike of that is not universal) and also makes it more difficult for us to add ending actions in the future. If the startup began a log stream, it might need to be flushed. I have not worked very hard to come up with scenarios - but one is enough to plan for this.

Ideas for naming:

Prefixing
[Suffixing]

Preceding
[Following]

Precursor
[Successor]

Precursor
[Completion]

BeforeStart
[AfterEnd]

We could use "Main" or "Normal" if we need a name for a normal action.

[KathleenDollard]

Additional thoughts on this..

I would like the vision of super simple extensibility to stay front and center as we consider this. I'm thinking of that as a switch statement, but obviously not the only solution. It would seem that the simplest approach would be a single action and collections of pre and post actions (with post actions potentially coming later).

If we push all actions into a single list, I do not see how we keep extensibility super simple.

And also...we do not know all the ways where people used middleware. Some part of those were closely tied to the CLI itself, and those might be best handled by extra code in a custom method responding to a ParseResult. But where the action is orthogonal and may be supplied by a directive in a NuGet package, we need this capability to allow pre and post actions.

It will be great when we have the next preview out so it is easier for folks that had middleware to see how well it converts.

[adamsitnik]

I like your proposal as it's very simple and has minimal effect on the public APIs!

I've left some comments that are related to perf and style rather than the main design concept.

[adamsitnik]

I know that it's very unlikely for the cancellation to be requested so quickly, but it's possible, so we should keep the checks and ensure that all actions handle cancellation properly.

In theory we could move this logic to the InvocationPipeline, but it would not cover a scenario where the users do parseResult.Action.InvokeAsync(token) rather than the usual parseResult.InvokeAsync(token)

[jonsequitur]

This specific action wouldn't be cancellable in any useful way though, would it? It's not doing any asynchronous work, and the idea that our async handling code would actually vary from one CliAction to another represent a significant increase in complexity and test surface.

[jonsequitur]

Related, I see we have a few clones of this code:

command-line-api/src/System.CommandLine/Help/HelpOptionAction.cs

Lines 33 to 36 in 72ace16

	public override Task<int> InvokeAsync(ParseResult parseResult, CancellationToken cancellationToken = default)
	=> cancellationToken.IsCancellationRequested
	? Task.FromCanceled<int>(cancellationToken)
	: Task.FromResult(Invoke(parseResult));

I'd like to find a way to prevent CliAction implementations from incidentally differing from one another, because we are testing them as if they're all the same.

[adamsitnik]

We should avoid using value tuples, as they are structs so the JIT needs to perform dedicated compilation and it hurts startup scenario.

How about moving all the new logic to the existing try/catch blocks?

[jonsequitur]

Good call.

[adamsitnik]

thank you for making it optional by default and avoiding the allocations for cases where it's not needed 👍

[adamsitnik]

nit: we can save some space here

Suggested change

	if (_passThroughActions is null)
	{
	_passThroughActions = new();
	}
	_passThroughActions.Add(directive.Action);
	(_passThroughActions ??= new()).Add(directive.Action);

[KalleOlaviNiemitalo]

For a moment, I thought this token was a System.CommandLine.Parsing.Token.

[KathleenDollard]

NOTE: This comment represents a point in time understanding and is no longer fully correct. I am striking out the code and leaving the rest as part of the history.

I am very concerned about the complexity added to the use of invocation. The work done in the current Invoke method will mostly need to be repeated in even trivial customizations which will be very common. Customizing invocations (switches that grab individual actions) are central to this design, and we are losing there simplicity with this approach. One of piece of complexity will come from piping the return code. For this and other reasons, I strongly believe we should move that to the ParseResult. Core to my opinion is that the return code (exit code) is often set by the parser itself (for example, user syntax error).

This design was intended for even lightly experienced C# users will be able to customize key actions like help. I love the work ya'll did on that design and I think we need to remain true to it. Customizing implementations do not need to look like this, but I believe they can be no more complicated than this:

internal static int InvokeSimple(ParseResult parseResult)
{
try
{
HandleStart(parseResult);
var returnCode = parseResult.Action switch
{
null => 0,
// customize here
_ => HandleActions(parseResult),
};
HandleEnd(parseResult);
}
catch (Exception ex)
{
HandleException(parseResult, ex);
}
return parseResult.ReturnCode;
}

Of course, there are other ways to approach this. For example, the user could specify only the switch in a way that we slipped into the invocation. Just simple. Just a switch.

You can extrapolate how much complexity the user has to add to pipe the return code, and asking users to write the code in the Invoke method means they copy key functionality making our implementation very brittle. If they mess up - for example skipping the prework of NonExclusive actions - they will have issues that will look like System.CommandLine bugs. Specifically, if they later add environment variable setting, they will not be set. And by bad luck that is particularly challenging because its a "works on my machine" (but not yours) kind of problem.

[KathleenDollard]

Addition to my previous comment on simplified customization.

• Regardless of whether it does anything now, I would like to have the start and end handling so we can evolve.
• HandleStart, HandleEnd, and HandleException should all be customizable. That is probably a delegate for each.

I would like us to support user's customizing their actions with the same code whether they are using the default System.ComandLine or have an alternate wrapping library. A wrapping library may want to want to customize start, end and exceptions.

[jonsequitur]

I believe customizable start, end, and exception for each CliAction is a lot of additional complexity that I'd like to avoid. It seems hard to reason about.

For the sake of comparison, the old middleware design accommodated all of these with a single function signature. It proved too abstract for many people, but the flow within each pipeline stage was at least obvious for folks who are comfortable with functional composition. Decomposing these into properties will lead to an increase in invocations and allocations that I don't think we want.

For this reason, the following are non-goals for a nonexclusive CliAction:

• It's not intended for people to be able to match and intercept like they can with the ParseResult.Action.
• It's not intended to be able to set the return code.

I believe that if people want to take on that additional complexity and support that additional functionality (and I anticipate that richer app models will want to), then using a custom CliConfiguration-derived type (#2148) should provide the necessary extension points.

[KathleenDollard]

Can we create a test that is exactly what we think someone needs to write if they wish to intercept a specific exclusive action. This is a core user case for our design, and copying the existing code for even the sync case (much less async) is not an approach that I think is desirable long term. That code proliferating will block evolution.

I am willing to set aside some scenarios, such as intercepting non-exclusive actions, for now. But I want to ensure we do not inadvertently block growth in this area in the future.

[jonsequitur]

Can we create a test that is exactly what we think someone needs to write if they wish to intercept a specific exclusive action.

Yes, we'll have a number of tests over this pattern. Nothing in this PR changes the examples in the original proposal, and you can see it in use in some tests in main, e.g.

command-line-api/src/System.CommandLine.Tests/Help/HelpBuilderTests.Customization.cs

Lines 351 to 360 in 6a1b077

	ParseResult parseResult = rootCommand.Parse("-h", config);
	if (parseResult.Action is HelpAction helpAction)
	{
	helpAction.Builder.CustomizeSymbol(subcommand, secondColumnText: "The custom command description");
	helpAction.Builder.CustomizeSymbol(option, secondColumnText: "The custom option description");
	helpAction.Builder.CustomizeSymbol(argument, secondColumnText: "The custom argument description");
	}
	parseResult.Invoke();

In this example, the nonexclusive actions will be executed transparently.

[KathleenDollard]

@jonsequitur The problem with that test is that it does not accommodate the things we add in this PR. I don't think we can just ignore non-exclusive pre-actions here. If we hard code pre-actions into the test it would make it more complicated, which is bad, but the real problem comes in the fact that the test would have to be changed when we add post actions or we do some thing else to change the normal way CLI actions are run.

Tests are standing in for users. We do not want to copies of the code in InvocationPipeline in everyone's code, nor do we want people ignoring the orthogonal issues added there.

This problem is not created by this PR. I just failed to see it before this PR. The logic in the InvocationPipeline class, particularly the async version which I had not paid attention to, was already code we are likely to evolve, and as such we should make it very uncommon for people to need to replace it.

This is probably not terribly difficult to fix, we just need to switch the two InvocationPipeline methods to use a functional approach where f is the actual call to CLI action. If the user does not supply f we just call CLI action as now. If the user does supply f, we call it.

We assume f will call parseResult.Action.Invoke(parseResult), or that they return an indication of whether it was handled, or we have some callback mechanism. Any approach works for me.

In C#, I think of f handled as either an optional delegate (pay for play) or a protected overridable method named something like RunAction that extended but by default does nothing but parseResult.Action.Invoke(parseResult).

Invoke gets used in a lot of ways, but I am find if it is InvokeAction. Whatever the name, the protected overridable method seems a sweet approach because it is not expensive and the "callback if you don't handle it" part of the user's code is just a well understood call to base. However, that means we give up the pipeline being static, and might have to move code around so, not sure the best approach.

To summarize, it should be possible but highly unusual for anyone to avoid using InvocationPiplein.Invoke. We are putting important logic in there which we have a strong opinion about how it runs. Users are expected to extend only one part of this, a conditional around the action, which we expect to be most often used for HelpAction

[KathleenDollard]

I'm writing the issue I am raising from a more user focused (PM) perspective and plan to add it as a discussion. It is orthogonal to this PR and does not need to block this PR.

I remain convinced that NonExclusiveAction is a very bad name. These are pre-actions by whatever name implies a pre-action. Unless I misunderstand, no action in System.CommandLine is guaranteed to be exclusive after this PR.

The CliAction design is great, but I do not think we are quite done tweaking it.