Clarify the new annotation mechanism

Author: mhutch

src/System.CommandLine.Subsystems/HelpAnnotationExtensions.cs

@@ -1,23 +1,48 @@
 // 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;
 using System.CommandLine.Subsystems.Annotations;
 
 namespace System.CommandLine;
 
 public static class HelpAnnotationExtensions
 {
+    /// <summary>
+    /// Sets the help description on the <paramref name="symbol"/>
+    /// </summary>
+    /// <typeparam name="TSymbol">The type of the symbol</typeparam>
+    /// <param name="symbol">The symbol</param>
+    /// <param name="description">The help description for the symbol</param>
+    /// <returns>The <paramref name="symbol">, to enable fluent construction of symbols with annotations.</returns>
     public static TSymbol WithDescription<TSymbol> (this TSymbol symbol, string description) where TSymbol : CliSymbol
     {
         symbol.SetDescription(description);
         return symbol;
     }
 
+
+    /// <summary>
+    /// Sets the help description on the <paramref name="symbol"/>
+    /// </summary>
+    /// <typeparam name="TSymbol">The type of the symbol</typeparam>
+    /// <param name="symbol">The symbol</param>
+    /// <param name="description">The help description for the symbol</param>
     public static void SetDescription<TSymbol>(this TSymbol symbol, string description) where TSymbol : CliSymbol
     {
         symbol.SetAnnotation(HelpAnnotations.Description, description);
     }
 
+    /// <summary>
+    /// Get the help description on the <paramref name="symbol"/>
+    /// </summary>
+    /// <typeparam name="TSymbol">The type of the symbol</typeparam>
+    /// <param name="symbol">The symbol</param>
+    /// <returns>The symbol description if any, otherwise <see langword="null"/></returns>
+    /// <remarks>
+    /// This is intended to be called by CLI authors. Subsystems should instead call <see cref="HelpSubsystem.TryGetDescription(CliSymbol, out string?)"/>,
+    /// values from the subsystem's <see cref="IAnnotationProvider"/>.
+    /// </remarks>
     public static string? GetDescription<TSymbol>(this TSymbol symbol) where TSymbol : CliSymbol
     {
         return symbol.GetAnnotationOrDefault(HelpAnnotations.Description);

src/System.CommandLine.Subsystems/HelpSubsystem.cs

@@ -40,4 +40,7 @@ protected internal override CliExit Execute(PipelineContext pipelineContext)
         pipelineContext.ConsoleHack.WriteLine("Help me!");
         return CliExit.SuccessfullyHandled(pipelineContext.ParseResult);
     }
+
+    public bool TryGetDescription (CliSymbol symbol, out string? description)
+        => TryGetAnnotation (symbol, HelpAnnotations.Description, out description);
 }

src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.AnnotationStorage.cs

@@ -9,13 +9,17 @@ partial class AnnotationStorageExtensions
 {
     class AnnotationStorage : IAnnotationProvider
     {
-        record struct AnnotationKey(CliSymbol symbol, string annotationId);
+        record struct AnnotationKey(CliSymbol symbol, string prefix, string id)
+        {
+            public static AnnotationKey Create<TAnnotation> (CliSymbol symbol, AnnotationId<TAnnotation> annotationId)
+                => new (symbol, annotationId.Prefix, annotationId.Id);
+        }
 
         readonly Dictionary<AnnotationKey, object> annotations = [];
 
-        public bool TryGet<TValue>(CliSymbol symbol, AnnotationId<TValue> id, [NotNullWhen(true)] out TValue? value)
+        public bool TryGet<TValue>(CliSymbol symbol, AnnotationId<TValue> annotationId, [NotNullWhen(true)] out TValue? value)
         {
-            if (annotations.TryGetValue(new AnnotationKey(symbol, id.Id), out var obj))
+            if (annotations.TryGetValue(AnnotationKey.Create(symbol, annotationId), out var obj))
             {
                 value = (TValue)obj;
                 return true;
@@ -25,15 +29,16 @@ public bool TryGet<TValue>(CliSymbol symbol, AnnotationId<TValue> id, [NotNullWh
             return false;
         }
 
-        public void Set<TValue>(CliSymbol symbol, AnnotationId<TValue> id, TValue value)
+        public void Set<TValue>(CliSymbol symbol, AnnotationId<TValue> annotationId, TValue value)
         {
+            var key = AnnotationKey.Create(symbol, annotationId);
             if (value is not null)
             {
-                annotations[new AnnotationKey(symbol, id.Id)] = value;
+                annotations[key] = value;
             }
             else
             {
-                annotations.Remove(new AnnotationKey(symbol, id.Id));
+                annotations.Remove(key);
             }
         }
     }

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

@@ -50,7 +50,7 @@ public static partial class AnnotationStorageExtensions
     //
     // This is fine, as we will have the following well-defined threading behavior: an annotated grammar and pipeline may
     // only be constructed/modified from a single thread. Once the grammar/pipeline instance is fully constructed, it may
-    // be safely used from multiple threads.
+    // be safely used from multiple threads, but is not safe to further modify once in use.
 
     static readonly ConditionalWeakTable<CliSymbol, AnnotationStorage> symbolToAnnotationStorage = new();
 
@@ -68,7 +68,7 @@ public static void SetAnnotation<TValue>(this CliSymbol symbol, AnnotationId<TVa
         var storage = symbolToAnnotationStorage.GetValue(symbol, static (CliSymbol _) => new AnnotationStorage());
         storage.Set(symbol, annotationId, value);
     }
-    
+
     /// <summary>
     /// Sets the value for the annotation <paramref name="id"/> associated with the <paramref name="symbol"/> in the internal annotation storage,
     /// and returns the <paramref name="symbol"> to enable fluent construction of symbols with annotations.
@@ -79,35 +79,34 @@ public static void SetAnnotation<TValue>(this CliSymbol symbol, AnnotationId<TVa
     /// The identifier for the annotation. For example, the annotation identifier for the help description is <see cref="HelpAnnotations.Description">.
     /// </param>
     /// <param name="value">The annotation value</param>
+    /// <returns>
+    /// The <paramref name="symbol">, to enable fluent construction of symbols with annotations.
+    /// </returns>
     public static TSymbol WithAnnotation<TSymbol, TValue>(this TSymbol symbol, AnnotationId<TValue> annotationId, TValue value) where TSymbol : CliSymbol
     {
         symbol.SetAnnotation(annotationId, value);
         return symbol;
     }
 
     /// <summary>
-    /// Attempts to get the value for the annotation <paramref name="id"/> associated with the <paramref name="symbol"/>,
-    /// first from the optional <paramref name="provider"/>, and falling back to the internal annotation storage used to
-    /// store values set via <see cref="SetAnnotation{TValue}(CliSymbol, AnnotationId{TValue}, TValue)"/>.
+    /// Attempts to get the value for the annotation <paramref name="annotationId"/> associated with the <paramref name="symbol"/> in the internal annotation
+    /// storage used to store values set via <see cref="SetAnnotation{TValue}(CliSymbol, AnnotationId{TValue}, TValue)"/>.
     /// </summary>
     /// <typeparam name="TValue">The type of the annotation value</typeparam>
     /// <param name="symbol">The symbol that is annotated</param>
-    /// <param name="id">
+    /// <param name="annotationId">
     /// The identifier for the annotation. For example, the annotation identifier for the help description is <see cref="HelpAnnotations.Description">.
     /// </param>
     /// <param name="value">The annotation value, if successful, otherwise <c>default</c></param>
-    /// <param name="provider">
-    /// An optional annotation provider that may implement custom or lazy construction of annotation values. Annotation returned by an annotation
-    /// provider take precedence over those stored in internal annotation storage.
-    /// </param>
     /// <returns>True if successful</returns>
-    public static bool TryGetAnnotation<TValue>(this CliSymbol symbol, AnnotationId<TValue> annotationId, [NotNullWhen(true)] out TValue? value, IAnnotationProvider? provider = null)
+    /// <remarks>
+    /// This is intended to be called by specialized ID-specific accessors for CLI authors such as <see cref="HelpAnnotationExtensions.GetDescription{TSymbol}(TSymbol)"/>.
+    /// Subsystems should not call it, as it does not account for values from the subsystem's <see cref="IAnnotationProvider"/>. They should instead call
+    /// <see cref="CliSubsystem.TryGetAnnotation{TValue}(CliSymbol, AnnotationId{TValue}, out TValue?)"/> or an ID-specific accessor on the subsystem such
+    /// <see cref="HelpSubsystem.TryGetDescription(CliSymbol, out string?)"/>.
+    /// </remarks>
+    public static bool TryGetAnnotation<TValue>(this CliSymbol symbol, AnnotationId<TValue> annotationId, [NotNullWhen(true)] out TValue? value)
     {
-        if (provider is not null && provider.TryGet(symbol, annotationId, out value))
-        {
-            return true;
-        }
-
         if (symbolToAnnotationStorage.TryGetValue(symbol, out var storage) && storage.TryGet (symbol, annotationId, out value))
         {
             return true;
@@ -116,27 +115,30 @@ public static bool TryGetAnnotation<TValue>(this CliSymbol symbol, AnnotationId<
         value = default;
         return false;
     }
+
     /// <summary>
-    /// Attempt to retrieve the <paramref name="symbol"/>'s value for the annotation <paramref name="id"/>
-    /// from the optional <paramref name="provider"/> and the internal annotation storage.
+    /// Attempts to get the value for the annotation <paramref name="annotationId"/> associated with the <paramref name="symbol"/> in the internal annotation
+    /// storage used to store values set via <see cref="SetAnnotation{TValue}(CliSymbol, AnnotationId{TValue}, TValue)"/>.
     /// </summary>
     /// <typeparam name="TValue">The type of the annotation value</typeparam>
     /// <param name="symbol">The symbol that is annotated</param>
-    /// <param name="id">
+    /// <param name="annotationId">
     /// The identifier for the annotation. For example, the annotation identifier for the help description is <see cref="HelpAnnotations.Description">.
     /// </param>
-    /// <param name="provider">
-    /// An optional annotation provider that may implement custom or lazy construction of annotation values. Annotation returned by an annotation
-    /// provider take precedence over those stored in internal annotation storage.
-    /// </param>
     /// <returns>The annotation value, if successful, otherwise <c>default</c></returns>
-    public static TValue? GetAnnotationOrDefault<TValue>(this CliSymbol symbol, AnnotationId<TValue> annotationId, IAnnotationProvider? provider = null)
+    /// <remarks>
+    /// This is intended to be called by specialized ID-specific accessors for CLI authors such as <see cref="HelpAnnotationExtensions.GetDescription{TSymbol}(TSymbol)"/>.
+    /// Subsystems should not call it, as it does not account for values from the subsystem's <see cref="IAnnotationProvider"/>. They should instead call
+    /// <see cref="CliSubsystem.TryGetAnnotation{TValue}(CliSymbol, AnnotationId{TValue}, out TValue?)"/> or an ID-specific accessor on the subsystem such
+    /// <see cref="HelpSubsystem.TryGetDescription(CliSymbol, out string?)"/>.
+    /// </remarks>
+    public static TValue? GetAnnotationOrDefault<TValue>(this CliSymbol symbol, AnnotationId<TValue> annotationId)
     {
-        if (symbol.TryGetAnnotation(annotationId, out TValue? value, provider))
+        if (symbol.TryGetAnnotation(annotationId, out TValue? value))
         {
             return value;
         }
 
         return default;
     }
-}
+}

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

@@ -44,11 +44,17 @@ protected CliSubsystem(string name, SubsystemKind subsystemKind, IAnnotationProv
     /// <param name="value">An out parameter to contain the result</param>
     /// <returns>True if successful</returns>
     /// <remarks>
-    /// This value is protected because it is a convenience for subsystem authors. It calls <see cref="SymbolAnnotationStorageExtensions"/>
+    /// Subsystem authors must use this to access annotation values, as it respects the subsystem's <see cref="IAnnotationProvider"/> if it has one.
+    /// This value is protected because it is intended for use only by subsystem authors. It calls <see cref="AnnotationStorageExtensions"/>
     /// </remarks>
-    protected internal bool TryGetAnnotation<TValue>(CliSymbol symbol, AnnotationId<TValue> id, [NotNullWhen(true)] out TValue? value)
+    protected internal bool TryGetAnnotation<TValue>(CliSymbol symbol, AnnotationId<TValue> annotationId, [NotNullWhen(true)] out TValue? value)
     {
-        return symbol.TryGetAnnotation(id, out value, _annotationProvider);
+        if (_annotationProvider is not null && _annotationProvider.TryGet(symbol, annotationId, out value))
+        {
+            return true;
+        }
+
+        return symbol.TryGetAnnotation(annotationId, out value);
     }
 
     /// <summary>