From dffdb56e7b51c869543a285a3951cc027d57473a Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 19 Jan 2023 14:07:38 +0100 Subject: [PATCH 1/5] remove RootCommandResult (internal), introduce SymbolResultTree (internal) and make all virtual SymbolResult methods non-virtual and delegating to SymbolResultTree --- src/System.CommandLine.Tests/ArgumentTests.cs | 4 +-- src/System.CommandLine.Tests/CommandTests.cs | 2 +- src/System.CommandLine.Tests/ParserTests.cs | 6 ++-- src/System.CommandLine/Argument.cs | 2 +- src/System.CommandLine/ParseResult.cs | 4 +-- .../Parsing/ArgumentResult.cs | 5 ++-- .../Parsing/CommandResult.cs | 7 +++-- .../Parsing/OptionResult.cs | 3 +- .../Parsing/ParseResultVisitor.cs | 28 ++++++++++++------- .../Parsing/SymbolResult.cs | 28 ++++++------------- ...otCommandResult.cs => SymbolResultTree.cs} | 20 ++++++------- 11 files changed, 53 insertions(+), 56 deletions(-) rename src/System.CommandLine/Parsing/{RootCommandResult.cs => SymbolResultTree.cs} (62%) diff --git a/src/System.CommandLine.Tests/ArgumentTests.cs b/src/System.CommandLine.Tests/ArgumentTests.cs index 937c6d4505..388224ec19 100644 --- a/src/System.CommandLine.Tests/ArgumentTests.cs +++ b/src/System.CommandLine.Tests/ArgumentTests.cs @@ -278,7 +278,7 @@ public void Option_ArgumentResult_parentage_to_root_symbol_is_set_correctly_when .Parent .Parent .Should() - .BeAssignableTo() + .BeOfType() .Which .Command .Should() @@ -340,7 +340,7 @@ public void Command_ArgumentResult_Parent_is_set_correctly_when_token_is_implici argumentResult .Parent .Should() - .BeAssignableTo() + .BeOfType() .Which .Command .Should() diff --git a/src/System.CommandLine.Tests/CommandTests.cs b/src/System.CommandLine.Tests/CommandTests.cs index e40bb071f8..9974354874 100644 --- a/src/System.CommandLine.Tests/CommandTests.cs +++ b/src/System.CommandLine.Tests/CommandTests.cs @@ -46,7 +46,7 @@ public void Outer_command_is_identified_correctly_by_Parent_property() .CommandResult .Parent .Should() - .BeAssignableTo() + .BeOfType() .Which .Command .Name diff --git a/src/System.CommandLine.Tests/ParserTests.cs b/src/System.CommandLine.Tests/ParserTests.cs index 6deae54c4c..127f228cdf 100644 --- a/src/System.CommandLine.Tests/ParserTests.cs +++ b/src/System.CommandLine.Tests/ParserTests.cs @@ -668,7 +668,7 @@ public void When_options_with_the_same_name_are_defined_on_parent_and_child_comm result.CommandResult .Parent .Should() - .BeAssignableTo() + .BeOfType() .Which .Children .Should() @@ -697,7 +697,7 @@ public void When_options_with_the_same_name_are_defined_on_parent_and_child_comm result.CommandResult .Parent .Should() - .BeAssignableTo() + .BeOfType() .Which .Children .Should() @@ -1008,7 +1008,7 @@ public void Option_and_Command_can_have_the_same_alias() .CommandResult .Parent .Should() - .BeAssignableTo() + .BeOfType() .Which .Children .Should() diff --git a/src/System.CommandLine/Argument.cs b/src/System.CommandLine/Argument.cs index 67784e8bd3..42d5de1d7f 100644 --- a/src/System.CommandLine/Argument.cs +++ b/src/System.CommandLine/Argument.cs @@ -113,7 +113,7 @@ private protected override string DefaultName /// Returns the default value for the argument, if defined. Null otherwise. public object? GetDefaultValue() { - return GetDefaultValue(new ArgumentResult(this, null)); + return GetDefaultValue(new ArgumentResult(this, null!, null)); } internal abstract object? GetDefaultValue(ArgumentResult argumentResult); diff --git a/src/System.CommandLine/ParseResult.cs b/src/System.CommandLine/ParseResult.cs index 1616690e49..dca48e3457 100644 --- a/src/System.CommandLine/ParseResult.cs +++ b/src/System.CommandLine/ParseResult.cs @@ -15,14 +15,14 @@ namespace System.CommandLine public class ParseResult { private readonly IReadOnlyList _errors; - private readonly RootCommandResult _rootCommandResult; + private readonly CommandResult _rootCommandResult; private readonly IReadOnlyList _unmatchedTokens; private Dictionary>? _directives; private CompletionContext? _completionContext; internal ParseResult( Parser parser, - RootCommandResult rootCommandResult, + CommandResult rootCommandResult, CommandResult commandResult, Dictionary>? directives, List tokens, diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index 949b9eba72..80e36a81ac 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -16,7 +16,8 @@ public sealed class ArgumentResult : SymbolResult internal ArgumentResult( Argument argument, - SymbolResult? parent) : base(parent) + SymbolResultTree symbolResultTree, + SymbolResult? parent) : base(symbolResultTree, parent) { Argument = argument ?? throw new ArgumentNullException(nameof(argument)); } @@ -110,7 +111,7 @@ Parent is { } && if (Parent!.UseDefaultValueFor(argument)) { - var argumentResult = new ArgumentResult(argument, Parent); + var argumentResult = new ArgumentResult(argument, _symbolResultTree, Parent); var defaultValue = argument.GetDefaultValue(argumentResult); diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index dac4af0b37..555b9887e5 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -8,13 +8,14 @@ namespace System.CommandLine.Parsing /// /// A result produced when parsing a . /// - public class CommandResult : SymbolResult + public sealed class CommandResult : SymbolResult { internal CommandResult( Command command, Token token, + SymbolResultTree symbolResultTree, CommandResult? parent = null) : - base(parent) + base(symbolResultTree, parent) { Command = command ?? throw new ArgumentNullException(nameof(command)); Token = token ?? throw new ArgumentNullException(nameof(token)); @@ -33,7 +34,7 @@ internal CommandResult( /// /// Child symbol results in the parse tree. /// - public IEnumerable Children => GetChildren(this); + public IEnumerable Children => _symbolResultTree.GetChildren(this); internal sealed override int MaximumArgumentCapacity { diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index 60d7dfb849..acbe7db56d 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -15,9 +15,10 @@ public sealed class OptionResult : SymbolResult internal OptionResult( Option option, + SymbolResultTree symbolResultTree, Token? token = null, CommandResult? parent = null) : - base(parent) + base(symbolResultTree, parent) { Option = option ?? throw new ArgumentNullException(nameof(option)); Token = token; diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index a4b89e8bf0..44f4b0ea1d 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -14,16 +14,16 @@ internal sealed class ParseResultVisitor private readonly Parser _parser; private readonly List _tokens; private readonly string? _rawInput; + private readonly Dictionary _symbolResults; + private readonly SymbolResultTree _symbolResultTree; private Dictionary>? _directives; private List? _unmatchedTokens; private List? _errors; - private readonly Dictionary _symbolResults = new(); - private List? _argumentResults; - private RootCommandResult? _rootCommandResult; + private CommandResult? _rootCommandResult; private CommandResult? _innermostCommandResult; private bool _isHelpRequested; @@ -38,6 +38,8 @@ internal ParseResultVisitor( _tokens = tokens; _unmatchedTokens = unmatchedTokens; _rawInput = rawInput; + _symbolResults = new (); + _symbolResultTree = new(_symbolResults, _parser.Configuration.LocalizationResources); if (tokenizeErrors is not null) { @@ -105,11 +107,10 @@ private void AddToResult(ArgumentResult result) private void VisitRootCommandNode(CommandNode rootCommandNode) { - _rootCommandResult = new RootCommandResult( + _rootCommandResult = new CommandResult( rootCommandNode.Command, rootCommandNode.Token, - _symbolResults, - _parser.Configuration.LocalizationResources ?? LocalizationResources.Instance); + _symbolResultTree); _innermostCommandResult = _rootCommandResult; } @@ -119,6 +120,7 @@ private void VisitCommandNode(CommandNode commandNode) var commandResult = new CommandResult( commandNode.Command, commandNode.Token, + _symbolResultTree, _innermostCommandResult); _symbolResults.Add(commandNode.Command, commandResult); @@ -134,6 +136,7 @@ private void VisitCommandArgumentNode(CommandArgumentNode argumentNode) argumentResult = new ArgumentResult( argumentNode.Argument, + _symbolResultTree, _innermostCommandResult); AddToResult(argumentResult); @@ -159,6 +162,7 @@ private void VisitOptionNode(OptionNode optionNode) var optionResult = new OptionResult( optionNode.Option, + _symbolResultTree, optionNode.Token, _innermostCommandResult); @@ -168,7 +172,7 @@ private void VisitOptionNode(OptionNode optionNode) { if (optionResult.Option.Argument.HasCustomParser) { - ArgumentResult argumentResult = new (optionResult.Option.Argument, optionResult); + ArgumentResult argumentResult = new (optionResult.Option.Argument, _symbolResultTree, optionResult); _symbolResults.Add(optionResult.Option.Argument, argumentResult); } } @@ -188,6 +192,7 @@ private void VisitOptionArgumentNode( argumentResult = new ArgumentResult( argumentNode.Argument, + _symbolResultTree, optionResult); _symbolResults.TryAdd(argument, argumentResult); } @@ -273,6 +278,7 @@ private void ValidateAndConvertArgumentResults(IList arguments, List public abstract class SymbolResult { + private protected readonly SymbolResultTree _symbolResultTree; private protected List? _tokens; - private protected SymbolResult(SymbolResult? parent) + private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? parent) { + _symbolResultTree = symbolResultTree; Parent = parent; } @@ -46,7 +47,7 @@ private protected SymbolResult(SymbolResult? parent) /// /// Localization resources used to produce messages for this symbol result. /// - public virtual LocalizationResources LocalizationResources => GetRoot().LocalizationResources; + public LocalizationResources LocalizationResources => _symbolResultTree.LocalizationResources; internal void AddToken(Token token) => (_tokens ??= new()).Add(token); @@ -55,36 +56,23 @@ private protected SymbolResult(SymbolResult? parent) /// /// The argument for which to find a result. /// An argument result if the argument was matched by the parser or has a default value; otherwise, null. - public virtual ArgumentResult? FindResultFor(Argument argument) => GetRoot().FindResultFor(argument); + public ArgumentResult? FindResultFor(Argument argument) => _symbolResultTree.FindResultFor(argument); /// /// Finds a result for the specific command anywhere in the parse tree, including parent and child symbol results. /// /// The command for which to find a result. /// An command result if the command was matched by the parser; otherwise, null. - public virtual CommandResult? FindResultFor(Command command) => GetRoot().FindResultFor(command); + public CommandResult? FindResultFor(Command command) => _symbolResultTree.FindResultFor(command); /// /// Finds a result for the specific option anywhere in the parse tree, including parent and child symbol results. /// /// The option for which to find a result. /// An option result if the option was matched by the parser or has a default value; otherwise, null. - public virtual OptionResult? FindResultFor(Option option) => GetRoot().FindResultFor(option); + public OptionResult? FindResultFor(Option option) => _symbolResultTree.FindResultFor(option); - internal virtual IEnumerable GetChildren(SymbolResult parent) => GetRoot().GetChildren(parent); - - private SymbolResult GetRoot() - { - SymbolResult result = this; - while (result.Parent is not null) - { - result = result.Parent; - } - - Debug.Assert(result is RootCommandResult); - - return result; - } + internal IEnumerable GetChildren(SymbolResult parent) => _symbolResultTree.GetChildren(parent); /// public T GetValue(Argument argument) diff --git a/src/System.CommandLine/Parsing/RootCommandResult.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs similarity index 62% rename from src/System.CommandLine/Parsing/RootCommandResult.cs rename to src/System.CommandLine/Parsing/SymbolResultTree.cs index f6c5461de8..86be15afaf 100644 --- a/src/System.CommandLine/Parsing/RootCommandResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -5,35 +5,33 @@ namespace System.CommandLine.Parsing { - internal sealed class RootCommandResult : CommandResult + internal sealed class SymbolResultTree { private readonly Dictionary _symbolResults; private readonly LocalizationResources _localizationResources; - public RootCommandResult( - Command command, - Token token, + internal SymbolResultTree( Dictionary symbolResults, - LocalizationResources localizationResources) : base(command, token) + LocalizationResources localizationResources) { _symbolResults = symbolResults; _localizationResources = localizationResources; } - public override LocalizationResources LocalizationResources => _localizationResources; + internal LocalizationResources LocalizationResources => _localizationResources; - public override ArgumentResult? FindResultFor(Argument argument) + internal ArgumentResult? FindResultFor(Argument argument) => _symbolResults.TryGetValue(argument, out SymbolResult? result) ? (ArgumentResult)result : default; - public override CommandResult? FindResultFor(Command command) + internal CommandResult? FindResultFor(Command command) => _symbolResults.TryGetValue(command, out SymbolResult? result) ? (CommandResult)result : default; - public override OptionResult? FindResultFor(Option option) + internal OptionResult? FindResultFor(Option option) => _symbolResults.TryGetValue(option, out SymbolResult? result) ? (OptionResult)result : default; - internal override IEnumerable GetChildren(SymbolResult parent) + internal IEnumerable GetChildren(SymbolResult parent) { - foreach (var pair in _symbolResults) + foreach (KeyValuePair pair in _symbolResults) { if (ReferenceEquals(parent, pair.Value.Parent)) { From f2dc82b8e24b8dae2c8b0391bc973d7bd87cfb06 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 19 Jan 2023 14:21:44 +0100 Subject: [PATCH 2/5] remove internal SymbolResult.GetChildren, expose SymbolResult.SymbolResultTree as internal field, optimize SymbolResultTree.GetChildren for AgumentResult --- src/System.CommandLine/Parsing/ArgumentResult.cs | 2 +- src/System.CommandLine/Parsing/CommandResult.cs | 2 +- .../Parsing/ParseResultExtensions.cs | 2 +- src/System.CommandLine/Parsing/SymbolResult.cs | 14 ++++++-------- .../Parsing/SymbolResultExtensions.cs | 2 +- src/System.CommandLine/Parsing/SymbolResultTree.cs | 9 ++++++--- 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index 80e36a81ac..dfe89c4cb4 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -111,7 +111,7 @@ Parent is { } && if (Parent!.UseDefaultValueFor(argument)) { - var argumentResult = new ArgumentResult(argument, _symbolResultTree, Parent); + var argumentResult = new ArgumentResult(argument, SymbolResultTree, Parent); var defaultValue = argument.GetDefaultValue(argumentResult); diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index 555b9887e5..645e7d0388 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -34,7 +34,7 @@ internal CommandResult( /// /// Child symbol results in the parse tree. /// - public IEnumerable Children => _symbolResultTree.GetChildren(this); + public IEnumerable Children => SymbolResultTree.GetChildren(this); internal sealed override int MaximumArgumentCapacity { diff --git a/src/System.CommandLine/Parsing/ParseResultExtensions.cs b/src/System.CommandLine/Parsing/ParseResultExtensions.cs index e678e6b125..d4df35a870 100644 --- a/src/System.CommandLine/Parsing/ParseResultExtensions.cs +++ b/src/System.CommandLine/Parsing/ParseResultExtensions.cs @@ -159,7 +159,7 @@ private static void Diagram( builder.Append("[ "); builder.Append(symbolResult.Token().Value); - foreach (SymbolResult child in symbolResult.GetChildren(symbolResult)) + foreach (SymbolResult child in symbolResult.SymbolResultTree.GetChildren(symbolResult)) { if (child is ArgumentResult arg && (arg.Argument.ValueType == typeof(bool) || diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index 0d3ecf2b58..6ab20d8151 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -12,12 +12,12 @@ namespace System.CommandLine.Parsing /// public abstract class SymbolResult { - private protected readonly SymbolResultTree _symbolResultTree; + internal readonly SymbolResultTree SymbolResultTree; private protected List? _tokens; private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? parent) { - _symbolResultTree = symbolResultTree; + SymbolResultTree = symbolResultTree; Parent = parent; } @@ -47,7 +47,7 @@ private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? /// /// Localization resources used to produce messages for this symbol result. /// - public LocalizationResources LocalizationResources => _symbolResultTree.LocalizationResources; + public LocalizationResources LocalizationResources => SymbolResultTree.LocalizationResources; internal void AddToken(Token token) => (_tokens ??= new()).Add(token); @@ -56,23 +56,21 @@ private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? /// /// The argument for which to find a result. /// An argument result if the argument was matched by the parser or has a default value; otherwise, null. - public ArgumentResult? FindResultFor(Argument argument) => _symbolResultTree.FindResultFor(argument); + public ArgumentResult? FindResultFor(Argument argument) => SymbolResultTree.FindResultFor(argument); /// /// Finds a result for the specific command anywhere in the parse tree, including parent and child symbol results. /// /// The command for which to find a result. /// An command result if the command was matched by the parser; otherwise, null. - public CommandResult? FindResultFor(Command command) => _symbolResultTree.FindResultFor(command); + public CommandResult? FindResultFor(Command command) => SymbolResultTree.FindResultFor(command); /// /// Finds a result for the specific option anywhere in the parse tree, including parent and child symbol results. /// /// The option for which to find a result. /// An option result if the option was matched by the parser or has a default value; otherwise, null. - public OptionResult? FindResultFor(Option option) => _symbolResultTree.FindResultFor(option); - - internal IEnumerable GetChildren(SymbolResult parent) => _symbolResultTree.GetChildren(parent); + public OptionResult? FindResultFor(Option option) => SymbolResultTree.FindResultFor(option); /// public T GetValue(Argument argument) diff --git a/src/System.CommandLine/Parsing/SymbolResultExtensions.cs b/src/System.CommandLine/Parsing/SymbolResultExtensions.cs index 443fdcfe62..86a573f269 100644 --- a/src/System.CommandLine/Parsing/SymbolResultExtensions.cs +++ b/src/System.CommandLine/Parsing/SymbolResultExtensions.cs @@ -13,7 +13,7 @@ internal static IEnumerable AllSymbolResults(this CommandResult co foreach (var item in commandResult .Children - .FlattenBreadthFirst(o => o.GetChildren(o))) + .FlattenBreadthFirst(o => o.SymbolResultTree.GetChildren(o))) { yield return item; } diff --git a/src/System.CommandLine/Parsing/SymbolResultTree.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs index 86be15afaf..760e683f57 100644 --- a/src/System.CommandLine/Parsing/SymbolResultTree.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -31,11 +31,14 @@ internal SymbolResultTree( internal IEnumerable GetChildren(SymbolResult parent) { - foreach (KeyValuePair pair in _symbolResults) + if (parent is not ArgumentResult) { - if (ReferenceEquals(parent, pair.Value.Parent)) + foreach (KeyValuePair pair in _symbolResults) { - yield return pair.Value; + if (ReferenceEquals(parent, pair.Value.Parent)) + { + yield return pair.Value; + } } } } From 2d827e551e9b5c7f90d8f8136a1b81fa756958fe Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 19 Jan 2023 14:28:18 +0100 Subject: [PATCH 3/5] make SymbolResultTree derive from Dictionary * it makes SymbolResultTree more responsible and less artificial (subjective) * it removes one allocation (nit) --- .../Parsing/ParseResultVisitor.cs | 38 +++++++++---------- .../Parsing/SymbolResultTree.cs | 16 +++----- 2 files changed, 24 insertions(+), 30 deletions(-) diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index 44f4b0ea1d..b306d82c1f 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -14,7 +14,6 @@ internal sealed class ParseResultVisitor private readonly Parser _parser; private readonly List _tokens; private readonly string? _rawInput; - private readonly Dictionary _symbolResults; private readonly SymbolResultTree _symbolResultTree; private Dictionary>? _directives; @@ -38,8 +37,7 @@ internal ParseResultVisitor( _tokens = tokens; _unmatchedTokens = unmatchedTokens; _rawInput = rawInput; - _symbolResults = new (); - _symbolResultTree = new(_symbolResults, _parser.Configuration.LocalizationResources); + _symbolResultTree = new(_parser.Configuration.LocalizationResources); if (tokenizeErrors is not null) { @@ -99,7 +97,7 @@ private void VisitInternal(SyntaxNode node) private void AddToResult(ArgumentResult result) { - if (_symbolResults.TryAdd(result.Argument, result)) + if (_symbolResultTree.TryAdd(result.Argument, result)) { (_argumentResults ??= new()).Add(result); } @@ -123,14 +121,14 @@ private void VisitCommandNode(CommandNode commandNode) _symbolResultTree, _innermostCommandResult); - _symbolResults.Add(commandNode.Command, commandResult); + _symbolResultTree.Add(commandNode.Command, commandResult); _innermostCommandResult = commandResult; } private void VisitCommandArgumentNode(CommandArgumentNode argumentNode) { - if (!(_symbolResults.TryGetValue(argumentNode.Argument, out var symbolResult) + if (!(_symbolResultTree.TryGetValue(argumentNode.Argument, out var symbolResult) && symbolResult is ArgumentResult argumentResult)) { argumentResult = @@ -153,7 +151,7 @@ private void VisitCommandArgumentNode(CommandArgumentNode argumentNode) private void VisitOptionNode(OptionNode optionNode) { - if (!_symbolResults.ContainsKey(optionNode.Option)) + if (!_symbolResultTree.ContainsKey(optionNode.Option)) { if (optionNode.Option is HelpOption) { @@ -166,14 +164,14 @@ private void VisitOptionNode(OptionNode optionNode) optionNode.Token, _innermostCommandResult); - _symbolResults.Add(optionNode.Option, optionResult); + _symbolResultTree.Add(optionNode.Option, optionResult); if (optionNode.Children is null) // no Arguments { if (optionResult.Option.Argument.HasCustomParser) { ArgumentResult argumentResult = new (optionResult.Option.Argument, _symbolResultTree, optionResult); - _symbolResults.Add(optionResult.Option.Argument, argumentResult); + _symbolResultTree.Add(optionResult.Option.Argument, argumentResult); } } } @@ -182,11 +180,11 @@ private void VisitOptionNode(OptionNode optionNode) private void VisitOptionArgumentNode( OptionArgumentNode argumentNode) { - OptionResult optionResult = (OptionResult)_symbolResults[argumentNode.ParentOptionNode.Option]; + OptionResult optionResult = (OptionResult)_symbolResultTree[argumentNode.ParentOptionNode.Option]; var argument = argumentNode.Argument; - if (!(_symbolResults.TryGetValue(argument, out SymbolResult? symbolResult) + if (!(_symbolResultTree.TryGetValue(argument, out SymbolResult? symbolResult) && symbolResult is ArgumentResult argumentResult)) { argumentResult = @@ -194,7 +192,7 @@ private void VisitOptionArgumentNode( argumentNode.Argument, _symbolResultTree, optionResult); - _symbolResults.TryAdd(argument, argumentResult); + _symbolResultTree.TryAdd(argument, argumentResult); } argumentResult.AddToken(argumentNode.Token); @@ -240,7 +238,7 @@ private void Stop() ValidateCommandResult(); - foreach (var symbolPair in _symbolResults) + foreach (var symbolPair in _symbolResultTree) { if (symbolPair.Value is OptionResult optionResult) { @@ -294,7 +292,7 @@ private void ValidateAndConvertArgumentResults(IList arguments, List= argumentResults.Count) @@ -477,7 +475,7 @@ private void ValidateAndConvertOptionResult(OptionResult optionResult) } } - foreach (var pair in _symbolResults) + foreach (var pair in _symbolResultTree) { if (object.ReferenceEquals(pair.Value.Parent, optionResult)) { @@ -556,9 +554,9 @@ void Handle(SymbolResult? symbolResult, Symbol symbol) case OptionResult o: if (o.Option.Argument.ValueType == typeof(bool) - && !_symbolResults.ContainsKey(o.Option.Argument)) + && !_symbolResultTree.ContainsKey(o.Option.Argument)) { - _symbolResults.Add(o.Option.Argument, new ArgumentResult(o.Option.Argument, _symbolResultTree, o)); + _symbolResultTree.Add(o.Option.Argument, new ArgumentResult(o.Option.Argument, _symbolResultTree, o)); } break; @@ -574,17 +572,17 @@ void Handle(SymbolResult? symbolResult, Symbol symbol) null, commandResult); - if (_symbolResults.TryAdd(optionResult.Option, optionResult)) + if (_symbolResultTree.TryAdd(optionResult.Option, optionResult)) { ArgumentResult argumentResult = new (optionResult.Option.Argument, _symbolResultTree, optionResult); - _symbolResults.Add(optionResult.Option.Argument, argumentResult); + _symbolResultTree.Add(optionResult.Option.Argument, argumentResult); } break; case Argument { HasDefaultValue: true } argument: - if (!_symbolResults.ContainsKey(argument)) + if (!_symbolResultTree.ContainsKey(argument)) { AddToResult(new ArgumentResult(argument, _symbolResultTree, commandResult)); } diff --git a/src/System.CommandLine/Parsing/SymbolResultTree.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs index 760e683f57..07ba985c7e 100644 --- a/src/System.CommandLine/Parsing/SymbolResultTree.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -5,35 +5,31 @@ namespace System.CommandLine.Parsing { - internal sealed class SymbolResultTree + internal sealed class SymbolResultTree : Dictionary { - private readonly Dictionary _symbolResults; private readonly LocalizationResources _localizationResources; - internal SymbolResultTree( - Dictionary symbolResults, - LocalizationResources localizationResources) + internal SymbolResultTree(LocalizationResources localizationResources) { - _symbolResults = symbolResults; _localizationResources = localizationResources; } internal LocalizationResources LocalizationResources => _localizationResources; internal ArgumentResult? FindResultFor(Argument argument) - => _symbolResults.TryGetValue(argument, out SymbolResult? result) ? (ArgumentResult)result : default; + => TryGetValue(argument, out SymbolResult? result) ? (ArgumentResult)result : default; internal CommandResult? FindResultFor(Command command) - => _symbolResults.TryGetValue(command, out SymbolResult? result) ? (CommandResult)result : default; + => TryGetValue(command, out SymbolResult? result) ? (CommandResult)result : default; internal OptionResult? FindResultFor(Option option) - => _symbolResults.TryGetValue(option, out SymbolResult? result) ? (OptionResult)result : default; + => TryGetValue(option, out SymbolResult? result) ? (OptionResult)result : default; internal IEnumerable GetChildren(SymbolResult parent) { if (parent is not ArgumentResult) { - foreach (KeyValuePair pair in _symbolResults) + foreach (KeyValuePair pair in this) { if (ReferenceEquals(parent, pair.Value.Parent)) { From a8566768618e16812e6cc2db1a3d6cc926e9ca35 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 19 Jan 2023 15:02:32 +0100 Subject: [PATCH 4/5] update the token with missing information during parsing, so later stages don't need to modify it --- src/System.CommandLine/Parsing/ParseOperation.cs | 6 ++++++ src/System.CommandLine/Parsing/ParseResultVisitor.cs | 9 ++------- src/System.CommandLine/Parsing/Token.cs | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/System.CommandLine/Parsing/ParseOperation.cs b/src/System.CommandLine/Parsing/ParseOperation.cs index 34997725bb..da2381f23b 100644 --- a/src/System.CommandLine/Parsing/ParseOperation.cs +++ b/src/System.CommandLine/Parsing/ParseOperation.cs @@ -102,6 +102,12 @@ private void ParseCommandArguments(CommandNode commandNode, ref int currentArgum if (currentArgumentCount < argument.Arity.MaximumNumberOfValues) { + if (CurrentToken.Symbol is null) + { + // update the token with missing information now, so later stages don't need to modify it + CurrentToken.Symbol = argument; + } + var argumentNode = new CommandArgumentNode( CurrentToken, argument, diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index b306d82c1f..67f8b591a7 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -140,13 +140,8 @@ private void VisitCommandArgumentNode(CommandArgumentNode argumentNode) AddToResult(argumentResult); } - var token = argumentNode.Token.Symbol is null - ? new Token(argumentNode.Token.Value, TokenType.Argument, argumentResult.Argument) - : argumentNode.Token; - - argumentResult.AddToken(token); - - _innermostCommandResult?.AddToken(token); + argumentResult.AddToken(argumentNode.Token); + _innermostCommandResult?.AddToken(argumentNode.Token); } private void VisitOptionNode(OptionNode optionNode) diff --git a/src/System.CommandLine/Parsing/Token.cs b/src/System.CommandLine/Parsing/Token.cs index 74dc55a63d..5e195215b1 100644 --- a/src/System.CommandLine/Parsing/Token.cs +++ b/src/System.CommandLine/Parsing/Token.cs @@ -46,7 +46,7 @@ internal Token(string? value, TokenType type, Symbol? symbol, int position) /// /// The Symbol represented by the token (if any). /// - internal Symbol? Symbol { get; } + internal Symbol? Symbol { get; set; } /// public override bool Equals(object? obj) => Equals(obj as Token); From 3575f0921a73879290b1107b98d5c89ca4f7ede8 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 19 Jan 2023 15:09:32 +0100 Subject: [PATCH 5/5] refactor the code so root command and inner most command are never nulls --- .../Parsing/ParseResultVisitor.cs | 50 +++++++------------ src/System.CommandLine/Parsing/Parser.cs | 3 +- 2 files changed, 21 insertions(+), 32 deletions(-) diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index 67f8b591a7..ed94ed50a6 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -15,15 +15,13 @@ internal sealed class ParseResultVisitor private readonly List _tokens; private readonly string? _rawInput; private readonly SymbolResultTree _symbolResultTree; + private readonly CommandResult _rootCommandResult; private Dictionary>? _directives; private List? _unmatchedTokens; private List? _errors; - private List? _argumentResults; - - private CommandResult? _rootCommandResult; - private CommandResult? _innermostCommandResult; + private CommandResult _innermostCommandResult; private bool _isHelpRequested; internal ParseResultVisitor( @@ -31,13 +29,18 @@ internal ParseResultVisitor( List tokens, List? tokenizeErrors, List? unmatchedTokens, - string? rawInput) + string? rawInput, + CommandNode rootCommandNode) { _parser = parser; _tokens = tokens; _unmatchedTokens = unmatchedTokens; _rawInput = rawInput; _symbolResultTree = new(_parser.Configuration.LocalizationResources); + _innermostCommandResult = _rootCommandResult = new CommandResult( + rootCommandNode.Command, + rootCommandNode.Token, + _symbolResultTree); if (tokenizeErrors is not null) { @@ -53,8 +56,6 @@ internal ParseResultVisitor( internal void Visit(CommandNode rootCommandNode) { - VisitRootCommandNode(rootCommandNode); - VisitChildren(rootCommandNode); Stop(); @@ -103,16 +104,6 @@ private void AddToResult(ArgumentResult result) } } - private void VisitRootCommandNode(CommandNode rootCommandNode) - { - _rootCommandResult = new CommandResult( - rootCommandNode.Command, - rootCommandNode.Token, - _symbolResultTree); - - _innermostCommandResult = _rootCommandResult; - } - private void VisitCommandNode(CommandNode commandNode) { var commandResult = new CommandResult( @@ -141,7 +132,7 @@ private void VisitCommandArgumentNode(CommandArgumentNode argumentNode) } argumentResult.AddToken(argumentNode.Token); - _innermostCommandResult?.AddToken(argumentNode.Token); + _innermostCommandResult.AddToken(argumentNode.Token); } private void VisitOptionNode(OptionNode optionNode) @@ -243,7 +234,7 @@ private void Stop() if (_argumentResults is not null) { - ValidateAndConvertArgumentResults(_innermostCommandResult!.Command.Arguments, _argumentResults); + ValidateAndConvertArgumentResults(_innermostCommandResult.Command.Arguments, _argumentResults); } } @@ -276,7 +267,7 @@ private void ValidateAndConvertArgumentResults(IList arguments, List arguments, CommandResult innermos private void ValidateCommandHandler() { - if (_innermostCommandResult!.Command is not { Handler: null } cmd) + if (_innermostCommandResult.Command is not { Handler: null } cmd) { return; } @@ -525,7 +513,7 @@ private void PopulateDefaultValues() for (var i = 0; i < options.Count; i++) { Option option = options[i]; - Handle(_rootCommandResult!.FindResultFor(option), option); + Handle(_rootCommandResult.FindResultFor(option), option); } } @@ -535,7 +523,7 @@ private void PopulateDefaultValues() for (var i = 0; i < arguments.Count; i++) { Argument argument = arguments[i]; - Handle(_rootCommandResult!.FindResultFor(argument), argument); + Handle(_rootCommandResult.FindResultFor(argument), argument); } } @@ -604,8 +592,8 @@ private void AddErrorToResult(SymbolResult symbolResult, ParseError parseError) public ParseResult GetResult() => new(_parser, - _rootCommandResult ?? throw new InvalidOperationException("No root command was found"), - _innermostCommandResult ?? throw new InvalidOperationException("No command was found"), + _rootCommandResult, + _innermostCommandResult, _directives, _tokens, _unmatchedTokens, diff --git a/src/System.CommandLine/Parsing/Parser.cs b/src/System.CommandLine/Parsing/Parser.cs index f20c9701e0..711e9e3e97 100644 --- a/src/System.CommandLine/Parsing/Parser.cs +++ b/src/System.CommandLine/Parsing/Parser.cs @@ -56,7 +56,8 @@ public ParseResult Parse( tokens, tokenizationErrors, operation.UnmatchedTokens, - rawInput); + rawInput, + operation.RootCommandNode!); visitor.Visit(operation.RootCommandNode!);