Value handling cleanup

OpenQuestions.md

@@ -56,16 +56,15 @@ Suggestion: Use internal constructors and leave conditions public
 
 ## Should `ValueCondition` be called `Condition`?
 
-They may apply to commands.
+They may apply to comm
+
 
 ## Can we remove the "Annotations" in xxxxAnnotationExtensions
 
 We have other extensions, such as `AddCalculation`. Where should it go?
 
 They may shift to extension types in the future.
 
-It's a long in Solution Explorer
-
 ## Calculated value design
 
 My first direction on the calculated value design was to derive from CliSymbol and treat them similarly to any other CliSymbol. This results in a technical challenge in the way the `Add` method works for CliSymbols - specifically it does not allow adding anything except options and arguments and the design results in infinite recursion if the exception is ignored. While we might be able to finesse this, it indicates just how thing the ice is if we try to "trick" things in the core parser layer. 

src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs

@@ -21,8 +21,10 @@ public static class ValueAnnotationExtensions
     /// which calculates the actual default value, based on the default value annotation and default value calculation,
     /// whether directly stored on the symbol or from the subsystem's <see cref="IAnnotationProvider"/>.
     /// </remarks>
-    public static bool TryGetDefault(this CliValueSymbol valueSymbol, out ValueSource? defaultValueSource)
-        => valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValue, out defaultValueSource);
+    public static ValueSource? GetDefault(this CliValueSymbol valueSymbol)
+        => valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValue, out var defaultValueSource)
+            ? (ValueSource)defaultValueSource 
+            : null;
 
     /// <summary>
     /// Sets the default value annotation on the <paramref name="option"/>

src/System.CommandLine.Subsystems/ValueProvider.cs

@@ -1,18 +1,16 @@
 // 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.Subsystems.Annotations;
 using System.CommandLine.ValueSources;
+using static System.Net.Mime.MediaTypeNames;
 
 namespace System.CommandLine;
 
 internal class ValueProvider
 {
-    private struct CacheItem
-    {
-        internal object? Value;
-        internal bool WasFound;
-        internal bool IsCalculating;
-    }
+    private record struct CacheItem(object? Value, bool WasFound, bool IsCalculating)
+    { }
 
     private Dictionary<CliSymbol, CacheItem> cachedValues = [];
     private PipelineResult pipelineResult;
@@ -22,17 +20,6 @@ public ValueProvider(PipelineResult pipelineResult)
         this.pipelineResult = pipelineResult;
     }
 
-    private void SetCachedValue(CliSymbol symbol, object? value, bool found)
-    {
-        // TODO: MHutch: Feel free to optimize this and SetIsCalculating. We need the struct or a tuple here for "WasFound" which turns out we need.
-        cachedValues[symbol] = new CacheItem()
-        {
-            Value = value,
-            WasFound = found,
-            IsCalculating = false
-        };
-    }
-
     private void SetIsCalculating(CliSymbol symbol)
     {
         cachedValues[symbol] = new CacheItem()
@@ -79,7 +66,7 @@ private bool TryGetValueInternal<T>(CliValueSymbol valueSymbol, out T? value)
             value = valueResult.GetValue<T>();
             return CacheAndReturn(valueSymbol, value, true);
         }
-        if (valueSymbol.TryGetDefault(out ValueSource? defaultValueSource))
+        if (valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValue, out var defaultValueSource))
         {
             if (defaultValueSource is not ValueSource<T> typedDefaultValueSource)
             {
@@ -90,13 +77,12 @@ private bool TryGetValueInternal<T>(CliValueSymbol valueSymbol, out T? value)
                 return CacheAndReturn(valueSymbol, value, true);
             }
         }
-        // TODO: Determine if we should cache default. If so, additional design is needed to avoid first hit returning false, and remainder returning true
         value = default;
         return CacheAndReturn(valueSymbol, value, false);
 
         bool CacheAndReturn(CliValueSymbol valueSymbol, T? valueToCache, bool valueFound)
         {
-            SetCachedValue(valueSymbol, valueToCache, valueFound);
+            cachedValues[valueSymbol] = new CacheItem(valueToCache, valueFound, false);
             return valueFound;
         }
     }

src/System.CommandLine.Subsystems/ValueSources/CollectionValueSource.cs

@@ -3,9 +3,11 @@
 
 namespace System.CommandLine.ValueSources;
 
+// TODO: Consider creating a set of classes with different arity: T1 and T1, T2 and T1, T2, T3, etc.
+
 /// <summary>
-/// <see cref="ValueSource"/> that returns the value of the specified other symbol.
-/// If the calculation delegate is supplied, the returned value of the calculation is returned.
+/// <see cref="ValueSource"/> that returns the value value calculated from a set of other symbols.
+/// The calculation must be supplied. the returned value of the calculation is returned.
 /// </summary>
 /// <typeparam name="T">The type to be returned, which is almost always the type of the symbol the ValueSource will be used for.</typeparam>
 /// <param name="otherSymbols">The <see cref="CliOption">, <see cref="CliArgument"/>, or <see cref="CalculatedValue"/> to include as sources.</param>

src/System.CommandLine.Subsystems/ValueSources/SymbolValueSource.cs

@@ -14,7 +14,7 @@ namespace System.CommandLine.ValueSources;
 public sealed class SymbolValueSource<T>
     : ValueSource<T>
 {
-    // TODO: API differences between this adn RelativeToSymbols are very annoying
+    // TODO: API differences between this and RelativeToSymbols are very annoying
     internal SymbolValueSource(
        CliValueSymbol otherSymbol,
        Func<object?, (bool success, T? value)>? calculation = null,

src/System.CommandLine/CliCommand.cs

@@ -195,8 +195,7 @@ public void SetAction(Func<ParseResult, CancellationToken, Task<int>> action)
 
         // Hide from IntelliSense as it's only to support initializing via C# collection expression
         // More specific efficient overloads are available for all supported symbol types.
-        //[DebuggerStepThrough]
-        //[EditorBrowsable(EditorBrowsableState.Never)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
         public void Add(CliSymbol symbol)
         {
             if (symbol is CliCommand cmd)
@@ -228,13 +227,11 @@ public void Add(CliSymbol symbol)
        */
         /// <inheritdoc />
         // Hide from IntelliSense as it's only to support C# collection initializer
-        [DebuggerStepThrough]
         [EditorBrowsable(EditorBrowsableState.Never)]
         IEnumerator IEnumerable.GetEnumerator() => Children.GetEnumerator();
 
         /// <inheritdoc />
         // Hide from IntelliSense as it's only to support initializing via C# collection expression
-        [DebuggerStepThrough]
         [EditorBrowsable(EditorBrowsableState.Never)]
         IEnumerator<CliSymbol> IEnumerable<CliSymbol>.GetEnumerator() => Children.GetEnumerator();
         /*