Use generic annotation accessors

Added Func<T> generic annotation accesssor
Moved value annotation accessor to own file
Removed Try<specific thing> from ValueSubsystem to encourage <annotationId>.<specific thing>.Get/Set
Cleanup, took some analyzer suggestions
XML Docs improved

Author: KathleenDollard

src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationAccessor.cs

@@ -36,21 +36,3 @@ public struct AnnotationAccessor<TValue>(CliSubsystem owner, AnnotationId<TValue
     /// <returns>True if the value was found, false otherwise.</returns>
     public readonly bool TryGet(CliSymbol symbol, [NotNullWhen(true)] out TValue? value) => owner.TryGetAnnotation(symbol, Id, out value);
 }
-
-/// <summary>
-/// Allows associating an annotation with a <see cref="CliSymbol"/>. The annotation will be stored by the accessor's owner <see cref="CliSubsystem"/>.
-/// </summary>
-public struct ValueAnnotationAccessor<TValue>(CliSubsystem owner, AnnotationId<TValue> id)
-{
-    /// <summary>
-    /// The ID of the annotation
-    /// </summary>
-    public AnnotationId<TValue> Id { get; }
-    public readonly void Set<TSymbolValue>(CliOption<TSymbolValue> symbol, TSymbolValue value)
-        where TSymbolValue : TValue
-        => owner.SetAnnotation(symbol, id, value);
-    public readonly void Set<TSymbolValue>(CliArgument<TSymbolValue> symbol, TSymbolValue value)
-        where TSymbolValue : TValue
-        => owner.SetAnnotation(symbol, id, value); 
-    public readonly bool TryGet(CliSymbol symbol, [NotNullWhen(true)] out TValue? value) => owner.TryGetAnnotation(symbol, id, out value);
-}

src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotationAccessor.cs

@@ -0,0 +1,63 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Diagnostics.CodeAnalysis;
+
+namespace System.CommandLine.Subsystems.Annotations;
+
+/// <summary>
+/// Associates an annotation with a <see cref="CliSymbol"/>. The symbol must be an option or argument and the value must be of the same type as the symbol./>. 
+/// </summary>
+/// <remarks>
+/// The annotation will be stored by the accessor's owner <see cref="CliSubsystem"/>. 
+/// </remarks>
+/// <typeparam name="TValue">The type of value to be stored</typeparam>
+/// <param name="owner">The subsystem that this annotation store data for.</param>
+/// <param name="id">The identifier for this annotation, since subsystems may have multiple annotations.</param>
+public struct ValueAnnotationAccessor<TValue>(CliSubsystem owner, AnnotationId<TValue> id)
+{
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Id"/>>
+    public AnnotationId<TValue> Id { get; }
+
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Set"/>>
+    public readonly void Set<TSymbolValue>(CliOption<TSymbolValue> symbol, TSymbolValue value)
+        where TSymbolValue : TValue
+        => owner.SetAnnotation(symbol, id, value);
+
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Set"/>>
+    public readonly void Set<TSymbolValue>(CliArgument<TSymbolValue> symbol, TSymbolValue value)
+        where TSymbolValue : TValue
+        => owner.SetAnnotation(symbol, id, value);
+
+    // TODO: Consider whether we need a version that takes a CliSymbol (ValueSymbol)
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Get"/>>
+    public readonly bool TryGet<TSymbolValue>(CliOption<TSymbolValue> symbol, [NotNullWhen(true)] out TValue? value)
+        where TSymbolValue : TValue
+        => TryGetInternal<TSymbolValue>(symbol, out value);
+
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Get"/>>
+    public readonly bool TryGet<TSymbolValue>(CliArgument<TSymbolValue> symbol, [NotNullWhen(true)] out TValue? value)
+        where TSymbolValue : TValue
+        => TryGetInternal<TSymbolValue>(symbol, out value);
+
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Get"/>>
+    /// <remarks>
+    /// This overload will throw if the stored value cannot be converted to the type.
+    /// </remarks>
+    /// <exception cref="InvalidCastException"/>
+    public readonly bool TryGet<TSymbolValue>(CliSymbol symbol, [NotNullWhen(true)] out TValue? value)
+        where TSymbolValue : TValue
+        => TryGetInternal<TSymbolValue>(symbol, out value);
+
+    private readonly bool TryGetInternal<TSymbolValue>(CliSymbol symbol, [NotNullWhen(true)] out TValue? value)
+        where TSymbolValue : TValue
+    {
+        if (owner.TryGetAnnotation(symbol, id, out var storedValue))
+        {
+            value = (TSymbolValue)storedValue;
+            return true;
+        }
+        value = default;
+        return false;
+    }
+}

src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs

@@ -1,18 +1,22 @@
 // Copyright (c) .NET Foundation and contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
-using System.CommandLine.Parsing;
-
 namespace System.CommandLine.Subsystems.Annotations;
 
 /// <summary>
 /// IDs for well-known Version annotations.
 /// </summary>
 public static class ValueAnnotations
 {
-    public static string Prefix { get; } = nameof(SubsystemKind.Value);
+    internal static string Prefix { get; } = nameof(SubsystemKind.Value);
 
+    /// <summary>
+    /// Provides Set and Get for default values
+    /// </summary>
     public static AnnotationId<object?> DefaultValue { get; } = new(Prefix, nameof(DefaultValue));
-    public static AnnotationId<Func<object?>?> DefaultValueCalculation { get; } = new(Prefix, nameof(DefaultValueCalculation));
-    public static AnnotationId<object?> Value { get; } = new(Prefix, nameof(Value));
+
+    /// <summary>
+    /// Provides Set and Get for default value calculations
+    /// </summary>
+    public static AnnotationId<Func<object?>> DefaultValueCalculation { get; } = new(Prefix, nameof(DefaultValueCalculation));
 }

src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueFuncAnnotationAccessor.cs

@@ -0,0 +1,63 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Diagnostics.CodeAnalysis;
+
+namespace System.CommandLine.Subsystems.Annotations;
+
+/// <summary>
+/// Associates an annotation with a <see cref="CliSymbol"/>. The symbol must be an option or argument and the delegate must return a value of the same type as the symbol./>. 
+/// </summary>
+/// <remarks>
+/// The annotation will be stored by the accessor's owner <see cref="CliSubsystem"/>. 
+/// </remarks>
+/// <typeparam name="TValue">The type of value to be stored</typeparam>
+/// <param name="owner">The subsystem that this annotation store data for.</param>
+/// <param name="id">The identifier for this annotation, since subsystems may have multiple annotations.</param>
+public struct ValueFuncAnnotationAccessor<TValue>(CliSubsystem owner, AnnotationId<Func<TValue>> id)
+{
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Id"/>>
+    public AnnotationId<Func<TValue>> Id { get; }
+
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Set"/>>
+    public readonly void Set<TSymbolValue>(CliOption<TSymbolValue> symbol, Func<TValue> value)
+        where TSymbolValue : TValue
+        => owner.SetAnnotation(symbol, id, value);
+
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Set"/>>
+    public readonly void Set<TSymbolValue>(CliArgument<TSymbolValue> symbol, Func<TValue> value)
+        where TSymbolValue : TValue
+        => owner.SetAnnotation(symbol, id, value);
+
+    // TODO: Consider whether we need a version that takes a CliSymbol (ValueSymbol)
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Get"/>>
+    public readonly bool TryGet<TSymbolValue>(CliOption<TSymbolValue> symbol, [NotNullWhen(true)] out Func<TValue>? value)
+        where TSymbolValue : TValue
+        => TryGetInternal<TSymbolValue>(symbol, out value);
+
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Get"/>>
+    public readonly bool TryGet<TSymbolValue>(CliArgument<TSymbolValue> symbol, [NotNullWhen(true)] out Func<TValue>? value)
+        where TSymbolValue : TValue
+        => TryGetInternal<TSymbolValue>(symbol, out value);
+
+    /// <inheritdoc cref="AnnotationAccessor{TValue}.Get"/>>
+    /// <remarks>
+    /// This overload will throw if the stored value cannot be converted to the type.
+    /// </remarks>
+    /// <exception cref="InvalidCastException"/>
+    public readonly bool TryGet<TSymbolValue>(CliSymbol symbol, [NotNullWhen(true)] out Func<TValue>? value)
+        where TSymbolValue : TValue
+        => TryGetInternal<TSymbolValue>(symbol, out value);
+
+    private readonly bool TryGetInternal<TSymbolValue>(CliSymbol symbol, [NotNullWhen(true)] out Func<TValue>? value)
+        where TSymbolValue : TValue
+    {
+        if (owner.TryGetAnnotation(symbol, id, out Func<TValue>? storedValue))
+        {
+            value = storedValue;
+            return true;
+        }
+        value = default;
+        return false;
+    }
+}

src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs

@@ -22,10 +22,14 @@ protected CliSubsystem(string name, SubsystemKind subsystemKind, IAnnotationProv
     /// The name of the subsystem.
     /// </summary>
     public string Name { get; }
+
+    /// <summary>
+    /// Defines the kind of subsystem, such as help or version
+    /// </summary>
     public SubsystemKind SubsystemKind { get; }
 
-    DefaultAnnotationProvider? _defaultProvider;
-    readonly IAnnotationProvider? _annotationProvider;
+    private DefaultAnnotationProvider? _defaultProvider;
+   private  readonly IAnnotationProvider? _annotationProvider;
 
     /// <summary>
     /// Attempt to retrieve the value for the symbol and annotation ID from the annotation provider.
@@ -53,6 +57,24 @@ protected internal bool TryGetAnnotation<TValue>(CliSymbol symbol, AnnotationId<
         return false;
     }
 
+    /// <inheritdoc cref="CliSubsystem.SetAnnotation{TValue}(CliSymbol, AnnotationId{TValue}, TValue)"/>
+    /// <returns>A delegate that returns the value.</returns>
+    protected internal bool TryGetAnnotation<TValue>(CliSymbol symbol, AnnotationId<Func<TValue>> id, [NotNullWhen(true)] out Func<TValue>? value)
+    {
+        if (_defaultProvider is not null && _defaultProvider.TryGet(symbol, id, out var storedValue))
+        {
+            value = storedValue;
+            return true;
+        }
+        if (_annotationProvider is not null && _annotationProvider.TryGet(symbol, id, out var storedValue2))
+        {
+            value = storedValue2;
+            return true;
+        }
+        value = default;
+        return false;
+    }
+
     /// <summary>
     /// Set the value for the symbol and annotation ID in the annotation provider.
     /// </summary>
@@ -69,24 +91,34 @@ protected internal void SetAnnotation<TValue>(CliSymbol symbol, AnnotationId<TVa
         (_defaultProvider ??= new DefaultAnnotationProvider()).Set(symbol, id, value);
     }
 
+    /// <inheritdoc cref="CliSubsystem.SetAnnotation{TValue}(CliSymbol, AnnotationId{TValue}, TValue)"/>
+    /// <param name="value">A delegate that returns the value.</param>
+    protected internal void SetAnnotation<TValue>(CliSymbol symbol, AnnotationId<Func<TValue>> id, Func<TValue> value)
+    {
+        (_defaultProvider ??= new DefaultAnnotationProvider()).Set<Func<TValue>>(symbol, id, value);
+    }
+
+    /// <summary>
+    /// The subystem executes, even if another subsystem has handled the operation. This is expected to be used in things like error reporting.
+    /// </summary>
     protected internal virtual bool RunsEvenIfAlreadyHandled { get; protected set; }
 
     /// <summary>
     /// Executes the behavior of the subsystem. For example, help would write information to the console.
     /// </summary>
     /// <param name="pipelineContext">The context contains data like the ParseResult, and allows setting of values like whether execution was handled and the CLI should terminate </param>
     /// <returns>A CliExit object with information such as whether the CLI should terminate</returns>
-    protected internal virtual CliExit Execute(PipelineContext pipelineContext) 
+    protected internal virtual CliExit Execute(PipelineContext pipelineContext)
         => CliExit.NotRun(pipelineContext.ParseResult);
 
     internal PipelineContext ExecuteIfNeeded(PipelineContext pipelineContext)
-        => ExecuteIfNeeded(pipelineContext.ParseResult,  pipelineContext);
+        => ExecuteIfNeeded(pipelineContext.ParseResult, pipelineContext);
 
     internal PipelineContext ExecuteIfNeeded(ParseResult? parseResult, PipelineContext pipelineContext)
     {
-        if( GetIsActivated(parseResult))
+        if (GetIsActivated(parseResult))
         {
-            Execute(pipelineContext );
+            Execute(pipelineContext);
         }
         return pipelineContext;
     }
@@ -120,7 +152,7 @@ protected internal virtual CliConfiguration Initialize(InitializationContext con
         => context.Configuration;
 
     // TODO: Determine if this is needed.
-    protected internal virtual CliExit TearDown(CliExit cliExit) 
+    protected internal virtual CliExit TearDown(CliExit cliExit)
         => cliExit;
 
 }

src/System.CommandLine.Subsystems/ValueSubsystem.cs

@@ -1,65 +1,52 @@
 // Copyright (c) .NET Foundation and contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
-using System.CommandLine.Parsing;
 using System.CommandLine.Subsystems;
 using System.CommandLine.Subsystems.Annotations;
 
 namespace System.CommandLine;
 
-public class ValueSubsystem : CliSubsystem
+public class ValueSubsystem(IAnnotationProvider? annotationProvider = null) 
+    : CliSubsystem(ValueAnnotations.Prefix, SubsystemKind.Value, annotationProvider)
 {
-    // @mhutch: Is the TryGet on the sparse dictionaries how we should handle a case where the annotations will be sparse to support lazy? If so, should we have another method on
-    // the annotation wrapper, or an alternative struct when there a TryGet makes sense? This API needs review, maybe next Tuesday.
-    //private PipelineContext? pipelineContext = null;
-    private Dictionary<CliSymbol, object?> cachedValues = new();
+    private Dictionary<CliSymbol, object?> cachedValues = [];
     private ParseResult? parseResult = null;
 
-    public ValueSubsystem(IAnnotationProvider? annotationProvider = null)
-        : base(ValueAnnotations.Prefix, SubsystemKind.Version, annotationProvider)
-    { }
-
-    //internal void SetDefaultValue(CliSymbol symbol, object? defaultValue)
-    //    => SetAnnotation(symbol, ValueAnnotations.DefaultValue, defaultValue);
-    //internal object? GetDefaultValue(CliSymbol symbol)
-    //  => TryGetAnnotation(symbol, ValueAnnotations.DefaultValue, out var defaultValue)
-    //            ? defaultValue
-    //            : "";
-    private bool TryGetDefaultValue<T>(CliSymbol symbol, out T? defaultValue)
-    {
-        if (TryGetAnnotation(symbol, ValueAnnotations.DefaultValue, out var objectValue))
-        {
-            defaultValue = (T)objectValue;
-            return true;
-        }
-        defaultValue = default;
-        return false;
-    }
-    public AnnotationAccessor<object?> DefaultValue
+    /// <summary>
+    /// Provides access to Get and Set methods for default values for symbols
+    /// </summary>
+    public ValueAnnotationAccessor<object?> DefaultValue
       => new(this, ValueAnnotations.DefaultValue);
 
-    internal void SetDefaultValueCalculation(CliSymbol symbol, Func<object?> factory)
-        => SetAnnotation(symbol, ValueAnnotations.DefaultValueCalculation, factory);
-    internal Func<object?>? GetDefaultValueCalculation(CliSymbol symbol)
-        => TryGetAnnotation<Func<object?>?>(symbol, ValueAnnotations.DefaultValueCalculation, out var value)
-                    ? value
-                    : null;
-    public AnnotationAccessor<Func<object?>?> DefaultValueCalculation
-      => new(this, ValueAnnotations.DefaultValueCalculation);
+    /// <summary>
+    /// Provides access to Get and Set methods for default value calculations for symbols
+    /// </summary>
+    public ValueFuncAnnotationAccessor<object?> DefaultValueCalculation
+      => new (this, ValueAnnotations.DefaultValueCalculation);
 
+    // It is possible that another subsystems GetIsActivated method will access a value. 
+    // If this is called from a GetIsActivated method of a subsystem in the early termination group, 
+    // it will fail. That is not an expected scenario.
+    /// <inheritdoc cref="CliSubsystem.GetIsActivated"/>
+    /// <remarks>
+    /// Note to inheritors: Call base for all ValueSubsystem methods that you override to ensure correct behavior
+    /// </remarks>
     protected internal override bool GetIsActivated(ParseResult? parseResult)
     {
         this.parseResult = parseResult;
         return true;
     }
 
+    /// <inheritdoc cref="CliSubsystem.Execute"/>
+    /// <remarks>
+    /// Note to inheritors: Call base for all ValueSubsystem methods that you override to ensure correct behavior
+    /// </remarks>
     protected internal override CliExit Execute(PipelineContext pipelineContext)
     {
         parseResult ??= pipelineContext.ParseResult;
         return base.Execute(pipelineContext);
     }
 
-    // TODO: Do it! Consider using a simple dictionary instead of the annotation (@mhutch) because with is not useful here
     private void SetValue<T>(CliSymbol symbol, object? value)
         => cachedValues.Add(symbol, value);
     private bool TryGetValue<T>(CliSymbol symbol, out T? value)
@@ -78,7 +65,7 @@ private bool TryGetValue<T>(CliSymbol symbol, out T? value)
     public T? GetValue<T>(CliOption option)
         => GetValueInternal<T>(option);
     public T? GetValue<T>(CliArgument argument)
-    => GetValueInternal<T>(argument);
+        => GetValueInternal<T>(argument);
 
     private T? GetValueInternal<T>(CliSymbol? symbol)
         => symbol switch
@@ -90,10 +77,10 @@ not null when TryGetValue<T>(symbol, out var value)
             CliOption option when parseResult?.GetValueResult(option) is {} valueResult  // GetValue not used because it would always return a value
                 => UseValue(symbol, valueResult.GetValue<T>()), // Value was supplied during parsing
             // Value was not supplied during parsing, determine default now
-            not null when DefaultValueCalculation.TryGet(symbol, out var  defaultValueCalculation)
+            not null when DefaultValueCalculation.TryGet<T>(symbol, out var  defaultValueCalculation)
                 => UseValue(symbol, CalculatedDefault<T>(symbol, defaultValueCalculation)),
-            not null when TryGetDefaultValue<T>(symbol, out var explicitValue) 
-                => UseValue(symbol, explicitValue),
+            not null when DefaultValue.TryGet<T>(symbol, out var explicitValue) 
+                => UseValue<T>(symbol, (T)explicitValue),
             //not null when GetDefaultFromEnvironmentVariable<T>(symbol, out var envName)
             //    => UseValue(symbol, GetEnvByName(envName)),
             null => throw new ArgumentNullException(nameof(symbol)),