Skip to content

Commit 662ec29

Browse files
committed
Switch to storing annotations in a static field
Although it's kind of icky to store instance data on a static field, it is implemented in a robust manner that should prevent any surprises, and the details are hidden from authors of CLIs and authors of subsystems. A subsystem reference is no longer needed when annotating the CliSymbol objects in the grammar, which makes construction of a pipleline much simpler. The fluent grammar construction API now looks like this: ```csharp new Option<bool>("--greeting").WithDescription("The greeting") ``` Eventually we will be able to use extension properties: ```csharp new Option<bool>("--greeting") { Description = "The greeting" } ``` We still support the `IAnnotationProvider` model that allows advanced CLI authors to lazily or dynamically provide annotations.
1 parent 434a14f commit 662ec29

18 files changed

+333
-289
lines changed

src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
33

44
using System.CommandLine.Directives;
5+
using System.CommandLine.Subsystems.Annotations;
56

67
namespace System.CommandLine.Subsystems.Tests
78
{
@@ -30,9 +31,7 @@ public VersionThatUsesHelpData(CliSymbol symbol)
3031

3132
protected override CliExit Execute(PipelineContext pipelineContext)
3233
{
33-
var help = pipelineContext.Pipeline.Help ?? throw new InvalidOperationException("Help cannot be null for this subsystem to work");
34-
help.Description.TryGet(Symbol, out var description);
35-
34+
TryGetAnnotation(Symbol, HelpAnnotations.Description, out string? description);
3635
pipelineContext.ConsoleHack.WriteLine(description);
3736
pipelineContext.AlreadyHandled = true;
3837
return CliExit.SuccessfullyHandled(pipelineContext.ParseResult);

src/System.CommandLine.Subsystems.Tests/PipelineTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ public void Subsystems_can_access_each_others_data()
186186
if (pipeline.Help is null) throw new InvalidOperationException();
187187
var rootCommand = new CliRootCommand
188188
{
189-
symbol.With(pipeline.Help.Description, "Testing")
189+
symbol.WithDescription("Testing")
190190
};
191191

192192
pipeline.Execute(new CliConfiguration(rootCommand), "-v", console);

src/System.CommandLine.Subsystems.Tests/ValueSubsystemTests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public void ValueSubsystem_returns_default_value_when_no_value_is_entered()
6161
var configuration = new CliConfiguration(rootCommand);
6262
var pipeline = Pipeline.CreateEmpty();
6363
pipeline.Value = new ValueSubsystem();
64-
pipeline.Value.DefaultValue.Set(option, 43);
64+
option.SetDefaultValue(43);
6565
const int expected = 43;
6666
var input = $"";
6767

@@ -81,7 +81,7 @@ public void ValueSubsystem_returns_calculated_default_value_when_no_value_is_ent
8181
var pipeline = Pipeline.CreateEmpty();
8282
pipeline.Value = new ValueSubsystem();
8383
var x = 42;
84-
pipeline.Value.DefaultValueCalculation.Set(option, () => x + 2);
84+
option.SetDefaultValueCalculation(() => x + 2);
8585
const int expected = 44;
8686
var input = "";
8787

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Copyright (c) .NET Foundation and contributors. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using System.CommandLine.Subsystems.Annotations;
5+
6+
namespace System.CommandLine;
7+
8+
public static class HelpAnnotationExtensions
9+
{
10+
public static TSymbol WithDescription<TSymbol> (this TSymbol symbol, string description) where TSymbol : CliSymbol
11+
{
12+
symbol.SetDescription(description);
13+
return symbol;
14+
}
15+
16+
public static void SetDescription<TSymbol>(this TSymbol symbol, string description) where TSymbol : CliSymbol
17+
{
18+
symbol.SetAnnotation(HelpAnnotations.Description, description);
19+
}
20+
21+
public static string? GetDescription<TSymbol>(this TSymbol symbol) where TSymbol : CliSymbol
22+
{
23+
return symbol.GetAnnotationOrDefault(HelpAnnotations.Description);
24+
}
25+
}

src/System.CommandLine.Subsystems/HelpSubsystem.cs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,6 @@ public class HelpSubsystem(IAnnotationProvider? annotationProvider = null)
2424
Arity = ArgumentArity.Zero
2525
};
2626

27-
public AnnotationAccessor<string> Description
28-
=> new(this, HelpAnnotations.Description);
29-
3027
protected internal override CliConfiguration Initialize(InitializationContext context)
3128
{
3229
context.Configuration.RootCommand.Add(HelpOption);

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

Lines changed: 0 additions & 32 deletions
This file was deleted.
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Copyright (c) .NET Foundation and contributors. All rights reserved.
22
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
33

4-
namespace System.CommandLine.Subsystems;
4+
namespace System.CommandLine.Subsystems.Annotations;
55

66
/// <summary>
77
/// Describes the ID and type of an annotation.
@@ -10,3 +10,5 @@ public record struct AnnotationId<TValue>(string Prefix, string Id)
1010
{
1111
public override readonly string ToString() => $"{Prefix}.{Id}";
1212
}
13+
14+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// Copyright (c) .NET Foundation and contributors. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using System.Diagnostics.CodeAnalysis;
5+
6+
namespace System.CommandLine.Subsystems.Annotations;
7+
8+
partial class AnnotationStorageExtensions
9+
{
10+
class AnnotationStorage : IAnnotationProvider
11+
{
12+
record struct AnnotationKey(CliSymbol symbol, string annotationId);
13+
14+
readonly Dictionary<AnnotationKey, object> annotations = [];
15+
16+
public bool TryGet<TValue>(CliSymbol symbol, AnnotationId<TValue> id, [NotNullWhen(true)] out TValue? value)
17+
{
18+
if (annotations.TryGetValue(new AnnotationKey(symbol, id.Id), out var obj))
19+
{
20+
value = (TValue)obj;
21+
return true;
22+
}
23+
24+
value = default;
25+
return false;
26+
}
27+
28+
public void Set<TValue>(CliSymbol symbol, AnnotationId<TValue> id, TValue value)
29+
{
30+
if (value is not null)
31+
{
32+
annotations[new AnnotationKey(symbol, id.Id)] = value;
33+
}
34+
else
35+
{
36+
annotations.Remove(new AnnotationKey(symbol, id.Id));
37+
}
38+
}
39+
}
40+
}
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
// Copyright (c) .NET Foundation and contributors. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using System.Diagnostics.CodeAnalysis;
5+
using System.Runtime.CompilerServices;
6+
7+
namespace System.CommandLine.Subsystems.Annotations;
8+
9+
/// <summary>
10+
/// Handles storage of annotations associated with <see cref="CliSymbol"/> instances.
11+
/// </summary>
12+
public static partial class AnnotationStorageExtensions
13+
{
14+
// CliSymbol does not offer any PropertyBag-like storage of arbitrary annotations, so the only way to allow setting
15+
// subsystem-specific annotations on CliSymbol instances (such as help description, default value, etc) via simple
16+
// extension methods is to use a static field with a dictionary that associates annotations with CliSymbol instances.
17+
//
18+
// Using ConditionalWeakTable for this dictionary ensures that the symbols and annotations can be collected when the
19+
// symbols are no longer reachable. Although this is unlikely to happen in a CLI app, it is important not to create
20+
// unexpected, unfixable, unbounded memory leaks in apps that construct multiple grammars/pipelines.
21+
//
22+
// The main use case for System.CommandLine is for a CLI app to construct a single annotated grammar in its entry point,
23+
// construct a pipeline using that grammar, and use the pipeline/grammar only once to parse its arguments. However, it
24+
// is important to have well defined and reasonable threading behavior so that System.CommandLine does not behave in
25+
// surprising ways when used in more advanced cases:
26+
//
27+
// * There may be multiple threads constructing and using completely independent grammars/pipelines. This happens in
28+
// our own unit tests, but might happen e.g. in a multithreaded data processing app or web service that uses
29+
// System.CommandLine to process inputs.
30+
//
31+
// * The grammar/pipeline are reentrant; they do not store they do not store internal state, and may be used to parse
32+
// input multiple times. As this is the case, it is reasonable to expect a grammar/pipeline instance to be
33+
// constructed in one thread then used in multiple threads. This might be done by the aforementioned web service or
34+
// data processing app.
35+
//
36+
// The thread-safe behavior of ConditionalWeakTable ensures this works as expected without us having to worry about
37+
// taking locks directly, even though the instance is on a static field and shared between all threads. Note that
38+
// thread local storage is not useful for this, as that would create unexpected behaviors where a grammar constructed
39+
// in one thread would be missing its annotations when used in another thread.
40+
//
41+
// However, while getting values from ConditionalWeakTable is lock free, setting values internally uses an expensive
42+
// lock, so it is not ideal to store all individual annotations directly in the ConditionalWeakTable. This is especially
43+
// true as we do not want the common case of the CLI app entrypoint to have its performance impacted by multithreading
44+
// support more than absolutely necessary.
45+
//
46+
// Instead, we have a single static ConditionalWeakTable that maps each CliSymbol to an AnnotationStorage dictionary,
47+
// which is lazily created and added to the ConditionalWeakTable a single time for each CliSymbol. The individual
48+
// annotations are stored in the AnnotationStorage dictionary, which uses no locks, so is fast, but is not safe to be
49+
// modified from multiple threads.
50+
//
51+
// This is fine, as we will have the following well-defined threading behavior: an annotated grammar and pipeline may
52+
// only be constructed/modified from a single thread. Once the grammar/pipeline instance is fully constructed, it may
53+
// be safely used from multiple threads.
54+
55+
static readonly ConditionalWeakTable<CliSymbol, AnnotationStorage> symbolToAnnotationStorage = new();
56+
57+
/// <summary>
58+
/// Sets the value for the annotation <paramref name="id"/> associated with the <paramref name="symbol"/> in the internal annotation storage.
59+
/// </summary>
60+
/// <typeparam name="TValue">The type of the annotation value</typeparam>
61+
/// <param name="symbol">The symbol that is annotated</param>
62+
/// <param name="id">
63+
/// The identifier for the annotation. For example, the annotation identifier for the help description is <see cref="HelpAnnotations.Description">.
64+
/// </param>
65+
/// <param name="value">The annotation value</param>
66+
public static void SetAnnotation<TValue>(this CliSymbol symbol, AnnotationId<TValue> annotationId, TValue value)
67+
{
68+
var storage = symbolToAnnotationStorage.GetValue(symbol, static (CliSymbol _) => new AnnotationStorage());
69+
storage.Set(symbol, annotationId, value);
70+
}
71+
72+
/// <summary>
73+
/// Sets the value for the annotation <paramref name="id"/> associated with the <paramref name="symbol"/> in the internal annotation storage,
74+
/// and returns the <paramref name="symbol"> to enable fluent construction of symbols with annotations.
75+
/// </summary>
76+
/// <typeparam name="TValue">The type of the annotation value</typeparam>
77+
/// <param name="symbol">The symbol that is annotated</param>
78+
/// <param name="id">
79+
/// The identifier for the annotation. For example, the annotation identifier for the help description is <see cref="HelpAnnotations.Description">.
80+
/// </param>
81+
/// <param name="value">The annotation value</param>
82+
public static TSymbol WithAnnotation<TSymbol, TValue>(this TSymbol symbol, AnnotationId<TValue> annotationId, TValue value) where TSymbol : CliSymbol
83+
{
84+
symbol.SetAnnotation(annotationId, value);
85+
return symbol;
86+
}
87+
88+
/// <summary>
89+
/// Attempts to get the value for the annotation <paramref name="id"/> associated with the <paramref name="symbol"/>,
90+
/// first from the optional <paramref name="provider"/>, and falling back to the internal annotation storage used to
91+
/// store values set via <see cref="SetAnnotation{TValue}(CliSymbol, AnnotationId{TValue}, TValue)"/>.
92+
/// </summary>
93+
/// <typeparam name="TValue">The type of the annotation value</typeparam>
94+
/// <param name="symbol">The symbol that is annotated</param>
95+
/// <param name="id">
96+
/// The identifier for the annotation. For example, the annotation identifier for the help description is <see cref="HelpAnnotations.Description">.
97+
/// </param>
98+
/// <param name="value">The annotation value, if successful, otherwise <c>default</c></param>
99+
/// <param name="provider">
100+
/// An optional annotation provider that may implement custom or lazy construction of annotation values. Annotation returned by an annotation
101+
/// provider take precedence over those stored in internal annotation storage.
102+
/// </param>
103+
/// <returns>True if successful</returns>
104+
public static bool TryGetAnnotation<TValue>(this CliSymbol symbol, AnnotationId<TValue> annotationId, [NotNullWhen(true)] out TValue? value, IAnnotationProvider? provider = null)
105+
{
106+
if (provider is not null && provider.TryGet(symbol, annotationId, out value))
107+
{
108+
return true;
109+
}
110+
111+
if (symbolToAnnotationStorage.TryGetValue(symbol, out var storage) && storage.TryGet (symbol, annotationId, out value))
112+
{
113+
return true;
114+
}
115+
116+
value = default;
117+
return false;
118+
}
119+
/// <summary>
120+
/// Attempt to retrieve the <paramref name="symbol"/>'s value for the annotation <paramref name="id"/>
121+
/// from the optional <paramref name="provider"/> and the internal annotation storage.
122+
/// </summary>
123+
/// <typeparam name="TValue">The type of the annotation value</typeparam>
124+
/// <param name="symbol">The symbol that is annotated</param>
125+
/// <param name="id">
126+
/// The identifier for the annotation. For example, the annotation identifier for the help description is <see cref="HelpAnnotations.Description">.
127+
/// </param>
128+
/// <param name="provider">
129+
/// An optional annotation provider that may implement custom or lazy construction of annotation values. Annotation returned by an annotation
130+
/// provider take precedence over those stored in internal annotation storage.
131+
/// </param>
132+
/// <returns>The annotation value, if successful, otherwise <c>default</c></returns>
133+
public static TValue? GetAnnotationOrDefault<TValue>(this CliSymbol symbol, AnnotationId<TValue> annotationId, IAnnotationProvider? provider = null)
134+
{
135+
if (symbol.TryGetAnnotation(annotationId, out TValue? value, provider))
136+
{
137+
return value;
138+
}
139+
140+
return default;
141+
}
142+
}

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

Lines changed: 0 additions & 67 deletions
This file was deleted.

0 commit comments

Comments
 (0)