Make PipelinePhase generic

This avoids a type check and moves a runtime error to compiler time for setting the main phase subsystem, which must be of the correct type r null.

Currently base and generic are in the same file to facilitate review (not having all the code show up as new in commits)

Author: KathleenDollard

src/System.CommandLine.Subsystems/Pipeline.cs

@@ -10,13 +10,13 @@ namespace System.CommandLine;
 public partial class Pipeline
 {
     // TODO: Consider more phases that have obvious meanings, like first and last
-    private PipelinePhase diagramPhase = new(SubsystemKind.Diagram);
-    private PipelinePhase completionPhase = new(SubsystemKind.Completion);
-    private PipelinePhase helpPhase = new(SubsystemKind.Help);
-    private PipelinePhase versionPhase = new(SubsystemKind.Version);
-    private PipelinePhase validationPhase = new(SubsystemKind.Validation);
-    private PipelinePhase invocationPhase = new(SubsystemKind.Invocation);
-    private PipelinePhase errorReportingPhase = new(SubsystemKind.ErrorReporting);
+    private PipelinePhase<DiagramSubsystem> diagramPhase = new(SubsystemKind.Diagram);
+    private PipelinePhase<CompletionSubsystem> completionPhase = new(SubsystemKind.Completion);
+    private PipelinePhase<HelpSubsystem> helpPhase = new(SubsystemKind.Help);
+    private PipelinePhase<VersionSubsystem> versionPhase = new(SubsystemKind.Version);
+    private PipelinePhase<ValidationSubsystem> validationPhase = new(SubsystemKind.Validation);
+    private PipelinePhase<InvocationSubsystem> invocationPhase = new(SubsystemKind.Invocation);
+    private PipelinePhase<ErrorReportingSubsystem> errorReportingPhase = new(SubsystemKind.ErrorReporting);
     // TODO: Consider this naming as it sounds like it is a finishing phase
     private readonly IEnumerable<PipelinePhase> phases = [];
 
@@ -129,107 +129,55 @@ public void AddSubsystem(CliSubsystem subsystem, PhaseTiming timing = PhaseTimin
     /// </summary>
     public DiagramSubsystem? Diagram
     {
-        get
-            => diagramPhase.Subsystem switch
-            {
-                null => null,
-                DiagramSubsystem diagramSubsystem => diagramSubsystem,
-                _ => throw new InvalidOperationException("Version subsystem is not of the correct type")
-            };
-        set
-        {
-            diagramPhase.Subsystem = value;
-        }
+        get => diagramPhase.Subsystem;
+        set => diagramPhase.Subsystem = value;
     }
 
     /// <summary>
     /// Sets or gets the completion subsystem.
     /// </summary>
     public CompletionSubsystem? Completion
     {
-        get
-            => completionPhase.Subsystem switch
-            {
-                null => null,
-                CompletionSubsystem completionSubsystem => completionSubsystem,
-                _ => throw new InvalidOperationException("Version subsystem is not of the correct type")
-            };
-        set
-        {
-            completionPhase.Subsystem = value;
-        }
+        get => completionPhase.Subsystem;
+        set => completionPhase.Subsystem = value;
+
     }
 
     /// <summary>
     /// Sets or gets the help subsystem.
     /// </summary>
     public HelpSubsystem? Help
     {
-        get
-            => helpPhase.Subsystem switch
-            {
-                null => null,
-                HelpSubsystem helpSubsystem => helpSubsystem,
-                _ => throw new InvalidOperationException("Version subsystem is not of the correct type")
-            };
-        set
-        {
-            helpPhase.Subsystem = value;
-        }
+        get => helpPhase.Subsystem;
+        set => helpPhase.Subsystem = value;
+
     }
 
     /// <summary>
     /// Sets or gets the version subsystem.
     /// </summary>
     public VersionSubsystem? Version
     {
-        get
-            => versionPhase.Subsystem switch
-            {
-                null => null,
-                VersionSubsystem versionSubsystem => versionSubsystem,
-                _ => throw new InvalidOperationException("Version subsystem is not of the correct type")
-            };
-        set
-        {
-            versionPhase.Subsystem = value;
-        }
+        get => versionPhase.Subsystem;
+        set => versionPhase.Subsystem = value;
     }
 
     /// <summary>
     /// Sets or gets the error reporting subsystem.
     /// </summary>
     public ErrorReportingSubsystem? ErrorReporting
     {
-        get
-            => errorReportingPhase.Subsystem switch
-            {
-                null => null,
-                ErrorReportingSubsystem errorReportingSubsystem => errorReportingSubsystem,
-                _ => throw new InvalidOperationException("Version subsystem is not of the correct type")
-            };
-        set
-        {
-            errorReportingPhase.Subsystem = value;
-        }
+        get => errorReportingPhase.Subsystem;
+        set => errorReportingPhase.Subsystem = value;
     }
 
     /// <summary>
     /// Sets or gets the validation subsystem
     /// </summary>
     public ValidationSubsystem? Validation
     {
-        get
-            => validationPhase.Subsystem switch
-            {
-                null => null,
-                ValidationSubsystem validationSubsystem => validationSubsystem,
-                _ => throw new InvalidOperationException("Version subsystem is not of the correct type")
-            };
-        set
-        {
-            validationPhase.Subsystem = value;
-        }
+        get => validationPhase.Subsystem;
+        set => validationPhase.Subsystem = value;
     }
 
 
@@ -238,17 +186,8 @@ public ValidationSubsystem? Validation
     /// </summary>
     public InvocationSubsystem? Invocation
     {
-        get
-            => invocationPhase.Subsystem switch
-            {
-                null => null,
-                InvocationSubsystem invocationSubsystem => invocationSubsystem,
-                _ => throw new InvalidOperationException("Version subsystem is not of the correct type")
-            };
-        set
-        {
-            invocationPhase.Subsystem = value;
-        }
+        get => invocationPhase.Subsystem;
+        set => invocationPhase.Subsystem = value;
     }
 
 

src/System.CommandLine.Subsystems/Subsystems/PipelinePhase.cs

@@ -3,28 +3,22 @@
 
 namespace System.CommandLine.Subsystems;
 
-/// <summary>
-/// This struct manages one phase. The most common case is that it is empty, and the most complicated
-/// case of several items before, and several items after will be quite rare.
-/// </summary>
-/// <remarks>
-/// <para>
-/// The most common case is that it is empty, and the most complicated
-/// case of several items before, and several items after will be quite rare. <br/>
-/// </para>
-/// <para>
-/// In the current design, this needs to be a reference type so values are synced.
-/// </para>
-/// </remarks>
 internal class PipelinePhase(SubsystemKind kind)
 {
     private List<CliSubsystem>? before = null;
     private List<CliSubsystem>? after = null;
 
     public readonly SubsystemKind Kind = kind;
-
-    internal CliSubsystem? Subsystem { get; set; }
-
+    protected CliSubsystem? CliSubsystem { get; set; }
+
+    /// <summary>
+    /// Add a subsystem to the phase
+    /// </summary>
+    /// <param name="subsystem">The subsystem to add</param>
+    /// <param name="timing">Whether it should run before or after the key subsystem</param>
+    /// <remarks>
+    /// Adding a subsystem that is not of the normal phase type is expected and OK.
+    /// </remarks>
     public void AddSubsystem(CliSubsystem subsystem, PhaseTiming timing = PhaseTiming.Before)
     {
         List<CliSubsystem>? addToList = timing == PhaseTiming.Before
@@ -48,9 +42,9 @@ private List<CliSubsystem> CreateAfterIfNeeded()
 
     public IEnumerable<CliSubsystem> GetSubsystems()
     {
-        List<CliSubsystem> ret = Subsystem is null
+        List<CliSubsystem> ret = CliSubsystem is null
                                     ? []
-                                    : [Subsystem];
+                                    : [CliSubsystem];
         if (before is not null)
         {
             // TODO: Confirm that we want to reverse the before list.
@@ -64,108 +58,34 @@ public IEnumerable<CliSubsystem> GetSubsystems()
     }
 }
 
+/// <summary>
+/// This struct manages one phase. The most common case is that it is empty, and the most complicated
+/// case of several items before, and several items after will be quite rare.
+/// </summary>
+/// <remarks>
+/// <para>
+/// The most common case is that it is empty, and the most complicated
+/// case of several items before, and several items after will be quite rare. <br/>
+/// </para>
+/// <para>
+/// In the current design, this needs to be a reference type so values are synced.
+/// </para>
+/// </remarks>
+internal class PipelinePhase<TSubsystem> : PipelinePhase
+    where TSubsystem : CliSubsystem
+{
+    private TSubsystem? subsystem;
 
+    public PipelinePhase(SubsystemKind kind) : base(kind)
+    { }
 
-// AddSubsystem(CliSubsystem subsystem, SubsystemPhase phase = SubsystemPhase.NotSpecified);
-
-//public enum SubsystemPhase
-//{
-//    NotSpecified = 0,
-//    BeforeDiagram,
-//    Diagram,
-//    AfterDiagram,
-//    BeforeCompletion,
-//    Completion,
-//    AfterCompletion,
-//    BeforeHelp,
-//    Help,
-//    AfterHelp,
-//    BeforeVersion,
-//    Version,
-//    AfterVersion,
-//    BeforeErrorReporting,
-//    ErrorReporting,
-//    AfterErrorReporting,
-//}
-
-// AddSubsystem(CliSubsystem subsystem, SubsystemPhase phase = SubsystemPhase.NotSpecified, PhaseTiming timing = PhaseTiming.Before);
-
-//public enum SubsystemPhase
-//{
-//    NotSpecified = 0,
-//    Diagram,
-//    Completion,
-//    Help,
-//    Version,
-//    ErrorReporting,
-//}
-
-//public enum PhaseTiming
-//{
-//    Before = 0,
-//    After
-//}
-
-
-///// <summary>
-///// Subsystem phases group subsystems that should be run at specific places in CLI processing and 
-///// are used for high level ordering.
-///// </summary>
-///// <remarks>
-///// Order of operations:
-///// 
-///// * Initialize is called for all subsystems, regardless of phase
-///// * ExecuteIfNeeded is called for subsystems in the EarlyReturn phase
-///// * ExecuteIfNeeded is called for subsystems in the Validate phase
-///// * ExecuteIfNeeded is called for subsystems in the Execute phase
-///// * ExecuteIfNeeded is called for subsystems in the Finish phase
-///// * Teardown is called for all subsystems, regardless of phase
-///// </remarks>
-//public enum SubsystemPhase
-//{
-//    /// <summary>
-//    /// Indicates a subsystem that never runs, and exists to support other subsystems. ValueSubsystem
-//    /// is an example.
-//    /// </summary>
-//    /// <remarks>
-//    /// Initialization runs first, teardown runs last - this is arbitrary and can be changed prior
-//    /// to release if we have scenarios to justify.
-//    /// </remarks>
-//    None,
-
-//    /// <summary>
-//    /// Indicates a subsystem is designed to shortcut execution and perform an action other than the 
-//    /// action indicated by the command. HelpSubsystem and VersionSubsystem are examples.
-//    /// </summary>
-//    /// <remarks>
-//    /// EarlyReturn subsystems are differentiated from other execution because data validation has not
-//    /// occurred. Because of this, data should not be used and should be assume to be questionable.
-//    /// </remarks>
-//    EarlyReturn,
-
-//    /// <summary>
-//    /// Indicates a subsystem that validates data entered by the user. 
-//    /// </summary>
-//    /// <remarks>
-//    /// Errors are not reported, but are rather stored for later display. This may be reconsidered
-//    /// if we keep track of which errors have been reported.
-//    /// </remarks>
-//    Validate,
-
-//    /// <summary>
-//    /// Indicates a subsystem that executes using data entered by the user. The only known case is 
-//    /// the Invocation subsystem.
-//    /// </summary>
-//    Execute,
-
-//    /// <summary>
-//    /// Indicates a subsystem that runs as the CLI part of processing is ending. ErrorReportingSubsystem
-//    /// is an example, although we may rethink when errors are displayed.
-//    /// </summary>
-//    /// <remarks>
-//    /// This is separate from the TearDown step, which is avaiable to all subsystems.
-//    /// </remarks>
-//    Finish,
-//}
-
-
+    internal TSubsystem? Subsystem
+    {
+        get => subsystem;
+        set
+        {
+            CliSubsystem = value;
+            subsystem = value;
+        }
+    }
+}