From e6cc70edf84dcbdfc0ab5fb440c9f29d8027eb1c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 18 Jan 2023 11:41:51 +0100 Subject: [PATCH 1/5] LocalizationResources should be readonly and same for entire symbol tree --- ...ests.System_CommandLine_api_is_not_changed.approved.txt | 2 +- src/System.CommandLine/Parsing/ParseResultVisitor.cs | 5 ++--- src/System.CommandLine/Parsing/RootCommandResult.cs | 7 ++++++- src/System.CommandLine/Parsing/SymbolResult.cs | 7 +------ 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 609adccecc..0467a52a2d 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -424,7 +424,7 @@ System.CommandLine.Parsing public abstract class SymbolResult public System.Collections.Generic.IReadOnlyList Children { get; } public System.String ErrorMessage { get; set; } - public System.CommandLine.LocalizationResources LocalizationResources { get; set; } + public System.CommandLine.LocalizationResources LocalizationResources { get; } public SymbolResult Parent { get; } public System.CommandLine.Symbol Symbol { get; } public System.Collections.Generic.IReadOnlyList Tokens { get; } diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index 53d7d5b48b..a5b52ed438 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -121,9 +121,8 @@ private void VisitRootCommandNode(CommandNode rootCommandNode) _rootCommandResult = new RootCommandResult( rootCommandNode.Command, rootCommandNode.Token, - _symbolResults); - - _rootCommandResult.LocalizationResources = _parser.Configuration.LocalizationResources; + _symbolResults, + _parser.Configuration.LocalizationResources ?? LocalizationResources.Instance); _innermostCommandResult = _rootCommandResult; } diff --git a/src/System.CommandLine/Parsing/RootCommandResult.cs b/src/System.CommandLine/Parsing/RootCommandResult.cs index 2f7888005e..680db15df5 100644 --- a/src/System.CommandLine/Parsing/RootCommandResult.cs +++ b/src/System.CommandLine/Parsing/RootCommandResult.cs @@ -8,15 +8,20 @@ namespace System.CommandLine.Parsing internal sealed class RootCommandResult : CommandResult { private readonly Dictionary _symbolResults; + private readonly LocalizationResources _localizationResources; public RootCommandResult( Command command, Token token, - Dictionary symbolResults) : base(command, token) + Dictionary symbolResults, + LocalizationResources localizationResources) : base(command, token) { _symbolResults = symbolResults; + _localizationResources = localizationResources; } + public override LocalizationResources LocalizationResources => _localizationResources; + public override ArgumentResult? FindResultFor(Argument argument) => _symbolResults.TryGetValue(argument, out SymbolResult? result) ? (ArgumentResult)result : default; diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index 433477ece7..367e28ad52 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -15,7 +15,6 @@ public abstract class SymbolResult { private List? _children; private protected List? _tokens; - private LocalizationResources? _resources; private protected SymbolResult( Symbol symbol, @@ -95,11 +94,7 @@ internal int MaximumArgumentCapacity /// /// Localization resources used to produce messages for this symbol result. /// - public LocalizationResources LocalizationResources - { - get => _resources ??= Parent?.LocalizationResources ?? LocalizationResources.Instance; - set => _resources = value; - } + public virtual LocalizationResources LocalizationResources => GetRoot().LocalizationResources; internal void AddToken(Token token) => (_tokens ??= new()).Add(token); From 4d93e7a0bcce00a31f8228a463d311cd8ee37586 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 18 Jan 2023 13:00:31 +0100 Subject: [PATCH 2/5] remove Symbol from SymbolResult, as each derived type defines more specific property anyway --- ...ommandLine_api_is_not_changed.approved.txt | 1 - src/System.CommandLine.Tests/ArgumentTests.cs | 23 +++++--- src/System.CommandLine.Tests/CommandTests.cs | 19 +++++-- .../ParseResultTests.cs | 4 +- .../ParserTests.MultiplePositions.cs | 11 +++- src/System.CommandLine.Tests/ParserTests.cs | 20 +++---- .../ParsingValidationTests.cs | 56 +++++++++---------- .../Builder/CommandLineBuilderExtensions.cs | 4 +- src/System.CommandLine/Help/VersionOption.cs | 6 +- src/System.CommandLine/ParseResult.cs | 9 ++- .../Parsing/ArgumentResult.cs | 6 +- .../Parsing/CommandResult.cs | 25 ++++++++- .../Parsing/OptionResult.cs | 7 ++- .../Parsing/SymbolResult.cs | 44 +-------------- 14 files changed, 124 insertions(+), 111 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 0467a52a2d..d4fb931a66 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -426,7 +426,6 @@ System.CommandLine.Parsing public System.String ErrorMessage { get; set; } public System.CommandLine.LocalizationResources LocalizationResources { get; } public SymbolResult Parent { get; } - public System.CommandLine.Symbol Symbol { get; } public System.Collections.Generic.IReadOnlyList Tokens { get; } public ArgumentResult FindResultFor(System.CommandLine.Argument argument) public CommandResult FindResultFor(System.CommandLine.Command command) diff --git a/src/System.CommandLine.Tests/ArgumentTests.cs b/src/System.CommandLine.Tests/ArgumentTests.cs index bd4db062ee..937c6d4505 100644 --- a/src/System.CommandLine.Tests/ArgumentTests.cs +++ b/src/System.CommandLine.Tests/ArgumentTests.cs @@ -131,7 +131,7 @@ public void Validation_failure_message_can_be_specified_when_parsing_tokens() argument.Parse("x") .Errors .Should() - .ContainSingle(e => e.SymbolResult.Symbol == argument) + .ContainSingle(e => ((ArgumentResult)e.SymbolResult).Argument == argument) .Which .Message .Should() @@ -150,7 +150,7 @@ public void Validation_failure_message_can_be_specified_when_evaluating_default_ argument.Parse("") .Errors .Should() - .ContainSingle(e => e.SymbolResult.Symbol == argument) + .ContainSingle(e => ((ArgumentResult)e.SymbolResult).Argument == argument) .Which .Message .Should() @@ -248,7 +248,10 @@ public void Option_ArgumentResult_Parent_is_set_correctly_when_token_is_implicit argumentResult .Parent - .Symbol + .Should() + .BeOfType() + .Which + .Option .Should() .Be(command.Options.Single()); } @@ -274,9 +277,12 @@ public void Option_ArgumentResult_parentage_to_root_symbol_is_set_correctly_when argumentResult .Parent .Parent - .Symbol .Should() - .Be(command); + .BeAssignableTo() + .Which + .Command + .Should() + .BeSameAs(command); } [Theory] @@ -333,9 +339,12 @@ public void Command_ArgumentResult_Parent_is_set_correctly_when_token_is_implici argumentResult .Parent - .Symbol .Should() - .Be(command); + .BeAssignableTo() + .Which + .Command + .Should() + .BeSameAs(command); } [Fact] diff --git a/src/System.CommandLine.Tests/CommandTests.cs b/src/System.CommandLine.Tests/CommandTests.cs index 6cb58c8d40..e40bb071f8 100644 --- a/src/System.CommandLine.Tests/CommandTests.cs +++ b/src/System.CommandLine.Tests/CommandTests.cs @@ -31,7 +31,7 @@ public void Outer_command_is_identified_correctly_by_RootCommand() result .RootCommandResult - .Symbol + .Command .Name .Should() .Be("outer"); @@ -45,7 +45,10 @@ public void Outer_command_is_identified_correctly_by_Parent_property() result .CommandResult .Parent - .Symbol + .Should() + .BeAssignableTo() + .Which + .Command .Name .Should() .Be("outer"); @@ -57,7 +60,10 @@ public void Inner_command_is_identified_correctly() var result = _parser.Parse("outer inner --option argument1"); result.CommandResult - .Symbol + .Should() + .BeOfType() + .Which + .Command .Name .Should() .Be("inner"); @@ -71,7 +77,10 @@ public void Inner_command_option_is_identified_correctly() result.CommandResult .Children .ElementAt(0) - .Symbol + .Should() + .BeOfType() + .Which + .Option .Name .Should() .Be("option"); @@ -195,7 +204,7 @@ public void ParseResult_Command_identifies_innermost_command(string input, strin var result = outer.Parse(input); - result.CommandResult.Symbol.Name.Should().Be(expectedCommand); + result.CommandResult.Command.Name.Should().Be(expectedCommand); } [Fact] diff --git a/src/System.CommandLine.Tests/ParseResultTests.cs b/src/System.CommandLine.Tests/ParseResultTests.cs index d8f622b630..c8d87b6df4 100644 --- a/src/System.CommandLine.Tests/ParseResultTests.cs +++ b/src/System.CommandLine.Tests/ParseResultTests.cs @@ -75,12 +75,12 @@ public void Command_will_not_accept_a_command_if_a_sibling_command_has_already_b var result = new Parser(command).Parse("outer inner-one inner-two"); - result.CommandResult.Symbol.Name.Should().Be("inner-one"); + result.CommandResult.Command.Name.Should().Be("inner-one"); result.Errors.Count.Should().Be(1); var result2 = new Parser(command).Parse("outer inner-two inner-one"); - result2.CommandResult.Symbol.Name.Should().Be("inner-two"); + result2.CommandResult.Command.Name.Should().Be("inner-two"); result2.Errors.Count.Should().Be(1); } } diff --git a/src/System.CommandLine.Tests/ParserTests.MultiplePositions.cs b/src/System.CommandLine.Tests/ParserTests.MultiplePositions.cs index 5d0652953f..a915c08b4c 100644 --- a/src/System.CommandLine.Tests/ParserTests.MultiplePositions.cs +++ b/src/System.CommandLine.Tests/ParserTests.MultiplePositions.cs @@ -1,6 +1,7 @@ // 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.Linq; using FluentAssertions; using Xunit; @@ -141,7 +142,15 @@ public void A_command_can_be_specified_in_more_than_one_position( var result = outer.Parse(commandLine); result.Errors.Should().BeEmpty(); - result.CommandResult.Parent.Symbol.Name.Should().Be(expectedParent); + result.CommandResult + .Parent + .Should() + .BeOfType() + .Which + .Command + .Name + .Should() + .Be(expectedParent); } [Fact] diff --git a/src/System.CommandLine.Tests/ParserTests.cs b/src/System.CommandLine.Tests/ParserTests.cs index 17a4f02301..0c039a6bb4 100644 --- a/src/System.CommandLine.Tests/ParserTests.cs +++ b/src/System.CommandLine.Tests/ParserTests.cs @@ -129,7 +129,7 @@ public void Option_short_forms_can_be_bundled() result.CommandResult .Children - .Select(o => o.Symbol.Name) + .Select(o => ((OptionResult)o).Option.Name) .Should() .BeEquivalentTo("x", "y", "z"); } @@ -172,7 +172,7 @@ public void Option_long_forms_do_not_get_unbundled() result.CommandResult .Children - .Select(o => o.Symbol.Name) + .Select(o => ((OptionResult)o).Option.Name) .Should() .BeEquivalentTo("xyz"); } @@ -427,13 +427,13 @@ public void Command_with_multiple_options_is_parsed_correctly() .Children .Should() .ContainSingle(o => - o.Symbol.Name == "inner1" && + ((OptionResult)o).Option.Name == "inner1" && o.Tokens.Single().Value == "argument1"); result.CommandResult .Children .Should() .ContainSingle(o => - o.Symbol.Name == "inner2" && + ((OptionResult)o).Option.Name == "inner2" && o.Tokens.Single().Value == "argument2"); } @@ -669,11 +669,11 @@ public void When_options_with_the_same_name_are_defined_on_parent_and_child_comm .Parent .Children .Should() - .NotContain(o => o.Symbol.Name == "x"); + .AllBeAssignableTo(); result.CommandResult .Children .Should() - .ContainSingle(o => o.Symbol.Name == "x"); + .ContainSingle(o => ((OptionResult)o).Option.Name == "x"); } [Fact] @@ -695,7 +695,7 @@ public void When_options_with_the_same_name_are_defined_on_parent_and_child_comm .Parent .Children .Should() - .ContainSingle(o => o.Symbol.Name == "x"); + .ContainSingle(o => o is OptionResult && ((OptionResult)o).Option.Name == "x"); } [Fact] @@ -1003,7 +1003,7 @@ public void Option_and_Command_can_have_the_same_alias() .Parent .Children .Should() - .Contain(c => c.Symbol == option); + .Contain(o => ((OptionResult)o).Option == option); } [Fact] @@ -1020,12 +1020,12 @@ public void Options_can_have_the_same_alias_differentiated_only_by_prefix() parser.Parse("-a").CommandResult .Children - .Select(s => s.Symbol) + .Select(s => ((OptionResult)s).Option) .Should() .BeEquivalentTo(option1); parser.Parse("--a").CommandResult .Children - .Select(s => s.Symbol) + .Select(s => ((OptionResult)s).Option) .Should() .BeEquivalentTo(option2); } diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index 913fcb057f..f0e7fba9a9 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -48,7 +48,7 @@ public void When_an_option_has_en_error_then_the_error_has_a_reference_to_the_op result.Errors .Where(e => e.SymbolResult != null) .Should() - .Contain(e => e.SymbolResult.Symbol.Name == option.Name); + .Contain(e => ((OptionResult)e.SymbolResult).Option.Name == option.Name); } [Fact] // https://github.com/dotnet/command-line-api/issues/1475 @@ -201,7 +201,7 @@ public void When_a_required_option_is_not_supplied_then_an_error_is_returned() .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol == command) + .Contain(e => ((CommandResult)e.SymbolResult).Command == command) .Which .Message .Should() @@ -225,7 +225,7 @@ public void When_a_required_option_has_multiple_aliases_the_error_message_uses_l .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol == command) + .Contain(e => ((CommandResult)e.SymbolResult).Command == command) .Which .Message .Should() @@ -308,8 +308,8 @@ public void A_custom_validator_can_be_added_to_a_command() command.Validators.Add(commandResult => { - if (commandResult.Children.Any(sr => sr.Symbol is IdentifierSymbol id && id.HasAlias("--one")) && - commandResult.Children.Any(sr => sr.Symbol is IdentifierSymbol id && id.HasAlias("--two"))) + if (commandResult.Children.Any(sr => ((OptionResult)sr).Option.HasAlias("--one")) && + commandResult.Children.Any(sr => ((OptionResult)sr).Option.HasAlias("--two"))) { commandResult.ErrorMessage = "Options '--one' and '--two' cannot be used together."; } @@ -346,7 +346,7 @@ public void A_custom_validator_can_be_added_to_an_option() .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol == option) + .Contain(e => ((OptionResult)e.SymbolResult).Option == option) .Which .Message .Should() @@ -373,7 +373,7 @@ public void A_custom_validator_can_be_added_to_an_argument() .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol == argument) + .Contain(e => ((ArgumentResult)e.SymbolResult).Argument == argument) .Which .Message .Should() @@ -442,7 +442,7 @@ public void Validators_on_global_options_are_executed_when_invoking_a_subcommand .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol == option) + .Contain(e => ((OptionResult)e.SymbolResult).Option == option) .Which .Message .Should() @@ -575,7 +575,7 @@ public void LegalFilePathsOnly_rejects_command_arguments_containing_invalid_path .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol == command.Arguments.First() && + .Contain(e => ((ArgumentResult)e.SymbolResult).Argument == command.Arguments.First() && e.Message == $"Character not allowed in a path: '{invalidCharacter}'."); } @@ -597,7 +597,7 @@ public void LegalFilePathsOnly_rejects_option_arguments_containing_invalid_path_ .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol.Name == "x" && + .Contain(e => ((OptionResult)e.SymbolResult).Option.Name == "x" && e.Message == $"Character not allowed in a path: '{invalidCharacter}'."); } @@ -660,7 +660,7 @@ public void LegalFileNamesOnly_rejects_command_arguments_containing_invalid_file .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol == command.Arguments.First() && + .Contain(e => ((ArgumentResult)e.SymbolResult).Argument == command.Arguments.First() && e.Message == $"Character not allowed in a file name: '{invalidCharacter}'."); } @@ -683,7 +683,7 @@ public void LegalFileNamesOnly_rejects_option_arguments_containing_invalid_file_ .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol.Name == "x" && + .Contain(e => ((OptionResult)e.SymbolResult).Option.Name == "x" && e.Message == $"Character not allowed in a file name: '{invalidCharacter}'."); } @@ -743,7 +743,7 @@ public void A_command_argument_can_be_invalid_based_on_file_existence() .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol.Name == "to" && + .Contain(e => ((ArgumentResult)e.SymbolResult).Argument.Name == "to" && e.Message == $"File does not exist: '{path}'."); } @@ -762,7 +762,7 @@ public void An_option_argument_can_be_invalid_based_on_file_existence() .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol.Name == "to" && + .Contain(e => ((OptionResult)e.SymbolResult).Option.Name == "to" && e.Message == $"File does not exist: '{path}'."); } @@ -781,7 +781,7 @@ public void A_command_argument_can_be_invalid_based_on_directory_existence() .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol.Name == "to" && + .Contain(e => ((ArgumentResult)e.SymbolResult).Argument.Name == "to" && e.Message == $"Directory does not exist: '{path}'."); } @@ -800,7 +800,7 @@ public void An_option_argument_can_be_invalid_based_on_directory_existence() .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol.Name == "to" && + .Contain(e => ((OptionResult)e.SymbolResult).Option.Name == "to" && e.Message == $"Directory does not exist: '{path}'."); } @@ -819,7 +819,7 @@ public void A_command_argument_can_be_invalid_based_on_file_or_directory_existen .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol == command.Arguments.First() && + .Contain(e => ((ArgumentResult)e.SymbolResult).Argument == command.Arguments.First() && e.Message == $"File or directory does not exist: '{path}'."); } @@ -838,7 +838,7 @@ public void An_option_argument_can_be_invalid_based_on_file_or_directory_existen .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol.Name == "to" && + .Contain(e => ((OptionResult)e.SymbolResult).Option.Name == "to" && e.Message == $"File or directory does not exist: '{path}'."); } @@ -857,7 +857,7 @@ public void A_command_argument_with_multiple_files_can_be_invalid_based_on_file_ .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol.Name == "to" && + .Contain(e => ((ArgumentResult)e.SymbolResult).Argument.Name == "to" && e.Message == $"File does not exist: '{path}'."); } @@ -876,7 +876,7 @@ public void An_option_argument_with_multiple_files_can_be_invalid_based_on_file_ .Should() .HaveCount(1) .And - .Contain(e => e.SymbolResult.Symbol.Name == "to" && + .Contain(e => ((OptionResult)e.SymbolResult).Option.Name == "to" && e.Message == $"File does not exist: '{path}'."); } @@ -895,7 +895,7 @@ public void A_command_argument_with_multiple_directories_can_be_invalid_based_on .Should() .HaveCount(1) .And - .ContainSingle(e => e.SymbolResult.Symbol.Name == "to" && + .ContainSingle(e => ((ArgumentResult)e.SymbolResult).Argument.Name == "to" && e.Message == $"Directory does not exist: '{path}'."); } @@ -914,7 +914,7 @@ public void An_option_argument_with_multiple_directories_can_be_invalid_based_on .Should() .HaveCount(1) .And - .ContainSingle(e => e.SymbolResult.Symbol.Name == "to" && + .ContainSingle(e => ((OptionResult)e.SymbolResult).Option.Name == "to" && e.Message == $"Directory does not exist: '{path}'."); } @@ -935,7 +935,7 @@ public void A_command_argument_with_multiple_FileSystemInfos_can_be_invalid_base result.Errors .Should() - .ContainSingle(e => e.SymbolResult.Symbol.Name == "to" && + .ContainSingle(e => ((ArgumentResult)e.SymbolResult).Argument.Name == "to" && e.Message == $"File or directory does not exist: '{path}'."); } @@ -954,7 +954,7 @@ public void An_option_argument_with_multiple_FileSystemInfos_can_be_invalid_base result.Errors .Should() - .ContainSingle(e => e.SymbolResult.Symbol.Name == "to" && + .ContainSingle(e => ((OptionResult)e.SymbolResult).Option.Name == "to" && e.Message == $"File or directory does not exist: '{path}'."); } @@ -973,7 +973,7 @@ public void A_command_argument_with_multiple_FileSystemInfos_can_be_invalid_base .Should() .HaveCount(1) .And - .ContainSingle(e => e.SymbolResult.Symbol.Name == "to" && + .ContainSingle(e => ((ArgumentResult)e.SymbolResult).Argument.Name == "to" && e.Message == $"File or directory does not exist: '{path}'."); } @@ -992,7 +992,7 @@ public void An_option_argument_with_multiple_FileSystemInfos_can_be_invalid_base .Should() .HaveCount(1) .And - .ContainSingle(e => e.SymbolResult.Symbol.Name == "to" && + .ContainSingle(e => ((OptionResult)e.SymbolResult).Option.Name == "to" && e.Message == $"File or directory does not exist: '{path}'."); } @@ -1083,7 +1083,7 @@ public void A_command_with_subcommands_is_invalid_to_invoke_if_it_has_no_handler .Should() .ContainSingle( e => e.Message.Equals(LocalizationResources.Instance.RequiredCommandWasNotProvided()) && - e.SymbolResult.Symbol.Name.Equals("inner")); + ((CommandResult)e.SymbolResult).Command.Name.Equals("inner")); } [Fact] @@ -1099,7 +1099,7 @@ public void A_root_command_is_invalid_if_it_has_no_handler() .Should() .ContainSingle( e => e.Message.Equals(LocalizationResources.Instance.RequiredCommandWasNotProvided()) && - e.SymbolResult.Symbol == rootCommand); + ((CommandResult)e.SymbolResult).Command == rootCommand); } [Fact] diff --git a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs index 29ac9fef5e..54039b04bb 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs @@ -647,7 +647,7 @@ public static CommandLineBuilder UseVersionOption( { if (context.ParseResult.FindResultFor(versionOption) is { }) { - if (context.ParseResult.Errors.Any(e => e.SymbolResult?.Symbol is VersionOption)) + if (context.ParseResult.Errors.Any(e => e.SymbolResult is OptionResult optionResult && optionResult.Option is VersionOption)) { context.InvocationResult = static ctx => ParseErrorResult.Apply(ctx, null); } @@ -688,7 +688,7 @@ public static CommandLineBuilder UseVersionOption( { if (context.ParseResult.FindResultFor(versionOption) is { }) { - if (context.ParseResult.Errors.Any(e => e.SymbolResult?.Symbol is VersionOption)) + if (context.ParseResult.Errors.Any(e => e.SymbolResult is OptionResult optionResult && optionResult.Option is VersionOption)) { context.InvocationResult = static ctx => ParseErrorResult.Apply(ctx, null); } diff --git a/src/System.CommandLine/Help/VersionOption.cs b/src/System.CommandLine/Help/VersionOption.cs index 800cbfb12e..70dd04639f 100644 --- a/src/System.CommandLine/Help/VersionOption.cs +++ b/src/System.CommandLine/Help/VersionOption.cs @@ -31,13 +31,13 @@ public VersionOption(string[] aliases, CommandLineBuilder builder) : base(aliase private void AddValidators() { - Validators.Add(result => + Validators.Add(static result => { if (result.Parent is { } parent && - parent.Children.Where(r => r.Symbol is not VersionOption) + parent.Children.Where(r => !(r is OptionResult optionResult && optionResult.Option is VersionOption)) .Any(IsNotImplicit)) { - result.ErrorMessage = result.LocalizationResources.VersionOptionCannotBeCombinedWithOtherArguments(result.Token?.Value ?? result.Symbol.Name); + result.ErrorMessage = result.LocalizationResources.VersionOptionCannotBeCombinedWithOtherArguments(result.Token?.Value); } }); } diff --git a/src/System.CommandLine/ParseResult.cs b/src/System.CommandLine/ParseResult.cs index b34f2ffb22..1616690e49 100644 --- a/src/System.CommandLine/ParseResult.cs +++ b/src/System.CommandLine/ParseResult.cs @@ -207,9 +207,14 @@ public T GetValue(Argument argument) public IEnumerable GetCompletions( int? position = null) { - var currentSymbolResult = SymbolToComplete(position); + SymbolResult currentSymbolResult = SymbolToComplete(position); - var currentSymbol = currentSymbolResult.Symbol; + Symbol currentSymbol = currentSymbolResult switch + { + ArgumentResult argumentResult => argumentResult.Argument, + OptionResult optionResult => optionResult.Option, + _ => ((CommandResult)currentSymbolResult).Command + }; var context = GetCompletionContext(); diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index 0b8450eaf7..949b9eba72 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -16,9 +16,9 @@ public sealed class ArgumentResult : SymbolResult internal ArgumentResult( Argument argument, - SymbolResult? parent) : base(argument, parent) + SymbolResult? parent) : base(parent) { - Argument = argument; + Argument = argument ?? throw new ArgumentNullException(nameof(argument)); } /// @@ -26,6 +26,8 @@ internal ArgumentResult( /// public Argument Argument { get; } + internal override int MaximumArgumentCapacity => Argument.Arity.MaximumNumberOfValues; + internal bool IsImplicit => Argument.HasDefaultValue && Tokens.Count == 0; internal IReadOnlyList? PassedOnTokens { get; private set; } diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index 0c341b9fb9..56d22b2b07 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -16,10 +16,9 @@ internal CommandResult( Command command, Token token, CommandResult? parent = null) : - base(command ?? throw new ArgumentNullException(nameof(command)), - parent) + base(parent) { - Command = command; + Command = command ?? throw new ArgumentNullException(nameof(command)); Token = token ?? throw new ArgumentNullException(nameof(token)); } @@ -33,6 +32,26 @@ internal CommandResult( /// public Token Token { get; } + internal sealed override int MaximumArgumentCapacity + { + get + { + var value = 0; + + if (Command.HasArguments) + { + var arguments = Command.Arguments; + + for (var i = 0; i < arguments.Count; i++) + { + value += arguments[i].Arity.MaximumNumberOfValues; + } + } + + return value; + } + } + internal override bool UseDefaultValueFor(Argument argument) => FindResultFor(argument) switch { diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index 38419b1b0c..87725424f0 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -19,10 +19,9 @@ internal OptionResult( Option option, Token? token = null, CommandResult? parent = null) : - base(option ?? throw new ArgumentNullException(nameof(option)), - parent) + base(parent) { - Option = option; + Option = option ?? throw new ArgumentNullException(nameof(option)); Token = token; } @@ -42,6 +41,8 @@ internal OptionResult( /// public Token? Token { get; } + internal override int MaximumArgumentCapacity => Option.Argument.Arity.MaximumNumberOfValues; + /// public object? GetValueOrDefault() => Option.ValueType == typeof(bool) diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index 367e28ad52..9e0c9f66dc 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -16,12 +16,8 @@ public abstract class SymbolResult private List? _children; private protected List? _tokens; - private protected SymbolResult( - Symbol symbol, - SymbolResult? parent) + private protected SymbolResult(SymbolResult? parent) { - Symbol = symbol ?? throw new ArgumentNullException(nameof(symbol)); - Parent = parent; } @@ -43,11 +39,6 @@ private protected SymbolResult( /// public SymbolResult? Parent { get; } - /// - /// The symbol to which the result applies. - /// - public Symbol Symbol { get; } - /// /// The list of tokens associated with this symbol result during parsing. /// @@ -58,38 +49,7 @@ private protected SymbolResult( private protected virtual int RemainingArgumentCapacity => MaximumArgumentCapacity - Tokens.Count; - internal int MaximumArgumentCapacity - { - get - { - switch (Symbol) - { - case Option option: - return option.Argument.Arity.MaximumNumberOfValues; - - case Argument argument: - return argument.Arity.MaximumNumberOfValues; - - case Command command: - var value = 0; - - if (command.HasArguments) - { - var arguments = command.Arguments; - - for (var i = 0; i < arguments.Count; i++) - { - value += arguments[i].Arity.MaximumNumberOfValues; - } - } - - return value; - - default: - throw new NotSupportedException(); - } - } - } + internal abstract int MaximumArgumentCapacity { get; } /// /// Localization resources used to produce messages for this symbol result. From 5b57add6473746bbfac8fb64d3b6ba00f92c85c2 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 18 Jan 2023 13:34:13 +0100 Subject: [PATCH 3/5] not every SymbolResult can have children, move the property to derived types as well --- src/System.CommandLine/Help/VersionOption.cs | 2 +- src/System.CommandLine/Parsing/CommandResult.cs | 8 ++++++++ src/System.CommandLine/Parsing/OptionResult.cs | 14 ++++++-------- .../Parsing/ParseResultVisitor.cs | 7 ++----- src/System.CommandLine/Parsing/SymbolResult.cs | 8 -------- 5 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/System.CommandLine/Help/VersionOption.cs b/src/System.CommandLine/Help/VersionOption.cs index 70dd04639f..88153c2d64 100644 --- a/src/System.CommandLine/Help/VersionOption.cs +++ b/src/System.CommandLine/Help/VersionOption.cs @@ -33,7 +33,7 @@ private void AddValidators() { Validators.Add(static result => { - if (result.Parent is { } parent && + if (result.Parent is CommandResult parent && parent.Children.Where(r => !(r is OptionResult optionResult && optionResult.Option is VersionOption)) .Any(IsNotImplicit)) { diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index 56d22b2b07..0abaf2fa77 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -11,6 +11,7 @@ namespace System.CommandLine.Parsing public class CommandResult : SymbolResult { private Dictionary? _defaultArgumentValues; + private List? _children; internal CommandResult( Command command, @@ -32,6 +33,11 @@ internal CommandResult( /// public Token Token { get; } + /// + /// Child symbol results in the parse tree. + /// + public IReadOnlyList Children => _children is not null ? _children : Array.Empty(); + internal sealed override int MaximumArgumentCapacity { get @@ -52,6 +58,8 @@ internal sealed override int MaximumArgumentCapacity } } + internal void AddChild(SymbolResult symbolResult) => (_children ??= new()).Add(symbolResult); + internal override bool UseDefaultValueFor(Argument argument) => FindResultFor(argument) switch { diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index 87725424f0..5ecb4dfb0f 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -12,6 +12,7 @@ namespace System.CommandLine.Parsing /// public sealed class OptionResult : SymbolResult { + private List? _children; private ArgumentConversionResult? _argumentConversionResult; private Dictionary? _defaultArgumentValues; @@ -79,14 +80,9 @@ internal ArgumentConversionResult ArgumentConversionResult { if (_argumentConversionResult is null) { - for (var i = 0; i < Children.Count; i++) + if (_children is not null) { - var child = Children[i]; - - if (child is ArgumentResult argumentResult) - { - return _argumentConversionResult = argumentResult.GetArgumentConversionResult(); - } + return _argumentConversionResult = _children[0].GetArgumentConversionResult(); } return _argumentConversionResult = ArgumentConversionResult.None(Option.Argument); @@ -95,7 +91,9 @@ internal ArgumentConversionResult ArgumentConversionResult return _argumentConversionResult; } } - + + internal void AddChild(ArgumentResult argumentResult) => (_children ??= new()).Add(argumentResult); + internal override bool UseDefaultValueFor(Argument argument) => IsImplicit; internal ArgumentResult GetOrCreateDefaultArgumentResult(Argument argument) => diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index a5b52ed438..05d455deab 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -185,11 +185,8 @@ private void VisitOptionNode(OptionNode optionNode) private void VisitOptionArgumentNode( OptionArgumentNode argumentNode) { - _symbolResults.TryGetValue( - argumentNode.ParentOptionNode.Option, - out var optionResult); - - if (optionResult is not OptionResult) + if (!(_symbolResults.TryGetValue(argumentNode.ParentOptionNode.Option, out SymbolResult? symbolResult) + && symbolResult is OptionResult optionResult)) { return; } diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index 9e0c9f66dc..e2337c4563 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -13,7 +13,6 @@ namespace System.CommandLine.Parsing /// public abstract class SymbolResult { - private List? _children; private protected List? _tokens; private protected SymbolResult(SymbolResult? parent) @@ -27,13 +26,6 @@ private protected SymbolResult(SymbolResult? parent) /// Setting this value to a non-null during parsing will cause the parser to indicate an error for the user and prevent invocation of the command line. public string? ErrorMessage { get; set; } - /// - /// Child symbol results in the parse tree. - /// - public IReadOnlyList Children => _children is not null ? _children : Array.Empty(); - - internal void AddChild(SymbolResult symbolResult) => (_children ??= new()).Add(symbolResult); - /// /// The parent symbol result in the parse tree. /// From ebd03955cd5ecb269c7f7839eb9b51a064345b01 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 18 Jan 2023 16:51:38 +0100 Subject: [PATCH 4/5] do not store the list of children per symbol result, use Parent property to find children using symbol dictionary --- ...ommandLine_api_is_not_changed.approved.txt | 2 +- src/System.CommandLine.Tests/ParserTests.cs | 9 ++ .../Parsing/CommandResult.cs | 12 +-- .../Parsing/OptionResult.cs | 14 +-- .../Parsing/ParseResultExtensions.cs | 4 +- .../Parsing/ParseResultVisitor.cs | 92 +++++++------------ .../Parsing/RootCommandResult.cs | 11 +++ .../Parsing/SymbolResult.cs | 2 + .../Parsing/SymbolResultExtensions.cs | 10 +- 9 files changed, 64 insertions(+), 92 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index d4fb931a66..452160cab6 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -393,6 +393,7 @@ System.CommandLine.Parsing public class CommandLineStringSplitter public System.Collections.Generic.IEnumerable Split(System.String commandLine) public class CommandResult : SymbolResult + public System.Collections.Generic.IEnumerable Children { get; } public System.CommandLine.Command Command { get; } public Token Token { get; } public class OptionResult : SymbolResult @@ -422,7 +423,6 @@ System.CommandLine.Parsing public static System.Threading.Tasks.Task InvokeAsync(this Parser parser, System.String[] args, System.CommandLine.IConsole console = null, System.Threading.CancellationToken cancellationToken = null) public static System.CommandLine.ParseResult Parse(this Parser parser, System.String commandLine) public abstract class SymbolResult - public System.Collections.Generic.IReadOnlyList Children { get; } public System.String ErrorMessage { get; set; } public System.CommandLine.LocalizationResources LocalizationResources { get; } public SymbolResult Parent { get; } diff --git a/src/System.CommandLine.Tests/ParserTests.cs b/src/System.CommandLine.Tests/ParserTests.cs index 0c039a6bb4..6deae54c4c 100644 --- a/src/System.CommandLine.Tests/ParserTests.cs +++ b/src/System.CommandLine.Tests/ParserTests.cs @@ -667,6 +667,9 @@ public void When_options_with_the_same_name_are_defined_on_parent_and_child_comm result.CommandResult .Parent + .Should() + .BeAssignableTo() + .Which .Children .Should() .AllBeAssignableTo(); @@ -693,6 +696,9 @@ public void When_options_with_the_same_name_are_defined_on_parent_and_child_comm .BeEmpty(); result.CommandResult .Parent + .Should() + .BeAssignableTo() + .Which .Children .Should() .ContainSingle(o => o is OptionResult && ((OptionResult)o).Option.Name == "x"); @@ -1001,6 +1007,9 @@ public void Option_and_Command_can_have_the_same_alias() parser.Parse("outer --inner inner") .CommandResult .Parent + .Should() + .BeAssignableTo() + .Which .Children .Should() .Contain(o => ((OptionResult)o).Option == option); diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index 0abaf2fa77..dac4af0b37 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -10,9 +10,6 @@ namespace System.CommandLine.Parsing /// public class CommandResult : SymbolResult { - private Dictionary? _defaultArgumentValues; - private List? _children; - internal CommandResult( Command command, Token token, @@ -36,7 +33,7 @@ internal CommandResult( /// /// Child symbol results in the parse tree. /// - public IReadOnlyList Children => _children is not null ? _children : Array.Empty(); + public IEnumerable Children => GetChildren(this); internal sealed override int MaximumArgumentCapacity { @@ -58,8 +55,6 @@ internal sealed override int MaximumArgumentCapacity } } - internal void AddChild(SymbolResult symbolResult) => (_children ??= new()).Add(symbolResult); - internal override bool UseDefaultValueFor(Argument argument) => FindResultFor(argument) switch { @@ -67,10 +62,5 @@ internal override bool UseDefaultValueFor(Argument argument) => arg.Tokens.Count == 0, _ => false }; - - internal ArgumentResult GetOrCreateDefaultArgumentResult(Argument argument) => - (_defaultArgumentValues ??= new()).GetOrAdd( - argument, - arg => new ArgumentResult(arg, this)); } } diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index 5ecb4dfb0f..60d7dfb849 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -1,7 +1,6 @@ // 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.Collections.Generic; using System.CommandLine.Binding; using System.Diagnostics.CodeAnalysis; @@ -12,9 +11,7 @@ namespace System.CommandLine.Parsing /// public sealed class OptionResult : SymbolResult { - private List? _children; private ArgumentConversionResult? _argumentConversionResult; - private Dictionary? _defaultArgumentValues; internal OptionResult( Option option, @@ -80,9 +77,9 @@ internal ArgumentConversionResult ArgumentConversionResult { if (_argumentConversionResult is null) { - if (_children is not null) + if (FindResultFor(Option.Argument) is ArgumentResult firstChild) { - return _argumentConversionResult = _children[0].GetArgumentConversionResult(); + return _argumentConversionResult = firstChild.GetArgumentConversionResult(); } return _argumentConversionResult = ArgumentConversionResult.None(Option.Argument); @@ -92,13 +89,6 @@ internal ArgumentConversionResult ArgumentConversionResult } } - internal void AddChild(ArgumentResult argumentResult) => (_children ??= new()).Add(argumentResult); - internal override bool UseDefaultValueFor(Argument argument) => IsImplicit; - - internal ArgumentResult GetOrCreateDefaultArgumentResult(Argument argument) => - (_defaultArgumentValues ??= new()).GetOrAdd( - argument, - arg => new ArgumentResult(arg, this)); } } diff --git a/src/System.CommandLine/Parsing/ParseResultExtensions.cs b/src/System.CommandLine/Parsing/ParseResultExtensions.cs index f58592d802..e678e6b125 100644 --- a/src/System.CommandLine/Parsing/ParseResultExtensions.cs +++ b/src/System.CommandLine/Parsing/ParseResultExtensions.cs @@ -159,10 +159,8 @@ private static void Diagram( builder.Append("[ "); builder.Append(symbolResult.Token().Value); - for (var i = 0; i < symbolResult.Children.Count; i++) + foreach (SymbolResult child in symbolResult.GetChildren(symbolResult)) { - var child = symbolResult.Children[i]; - if (child is ArgumentResult arg && (arg.Argument.ValueType == typeof(bool) || arg.Argument.Arity.MaximumNumberOfValues == 0)) diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index 05d455deab..a4b89e8bf0 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -95,21 +95,8 @@ private void VisitInternal(SyntaxNode node) } } - private void AddToResult(CommandResult result) - { - _innermostCommandResult?.AddChild(result); - _symbolResults.Add(result.Command, result); - } - - private void AddToResult(OptionResult result) - { - _innermostCommandResult?.AddChild(result); - _symbolResults.TryAdd(result.Option, result); - } - private void AddToResult(ArgumentResult result) { - _innermostCommandResult?.AddChild(result); if (_symbolResults.TryAdd(result.Argument, result)) { (_argumentResults ??= new()).Add(result); @@ -134,16 +121,15 @@ private void VisitCommandNode(CommandNode commandNode) commandNode.Token, _innermostCommandResult); - AddToResult(commandResult); + _symbolResults.Add(commandNode.Command, commandResult); _innermostCommandResult = commandResult; } private void VisitCommandArgumentNode(CommandArgumentNode argumentNode) { - _symbolResults.TryGetValue(argumentNode.Argument, out var symbolResult); - - if (symbolResult is not ArgumentResult argumentResult) + if (!(_symbolResults.TryGetValue(argumentNode.Argument, out var symbolResult) + && symbolResult is ArgumentResult argumentResult)) { argumentResult = new ArgumentResult( @@ -164,9 +150,7 @@ private void VisitCommandArgumentNode(CommandArgumentNode argumentNode) private void VisitOptionNode(OptionNode optionNode) { - _symbolResults.TryGetValue(optionNode.Option, out var symbolResult); - - if (symbolResult is not OptionResult) + if (!_symbolResults.ContainsKey(optionNode.Option)) { if (optionNode.Option is HelpOption) { @@ -178,28 +162,33 @@ private void VisitOptionNode(OptionNode optionNode) optionNode.Token, _innermostCommandResult); - AddToResult(optionResult); + _symbolResults.Add(optionNode.Option, optionResult); + + if (optionNode.Children is null) // no Arguments + { + if (optionResult.Option.Argument.HasCustomParser) + { + ArgumentResult argumentResult = new (optionResult.Option.Argument, optionResult); + _symbolResults.Add(optionResult.Option.Argument, argumentResult); + } + } } } private void VisitOptionArgumentNode( OptionArgumentNode argumentNode) { - if (!(_symbolResults.TryGetValue(argumentNode.ParentOptionNode.Option, out SymbolResult? symbolResult) - && symbolResult is OptionResult optionResult)) - { - return; - } + OptionResult optionResult = (OptionResult)_symbolResults[argumentNode.ParentOptionNode.Option]; var argument = argumentNode.Argument; - if (!_symbolResults.TryGetValue(argument, out var argumentResult)) + if (!(_symbolResults.TryGetValue(argument, out SymbolResult? symbolResult) + && symbolResult is ArgumentResult argumentResult)) { argumentResult = new ArgumentResult( argumentNode.Argument, optionResult); - optionResult.AddChild(argumentResult); _symbolResults.TryAdd(argument, argumentResult); } @@ -482,27 +471,11 @@ private void ValidateAndConvertOptionResult(OptionResult optionResult) } } - if (optionResult.Children.Count == 0) + foreach (var pair in _symbolResults) { - if (optionResult.Option.Argument is { HasCustomParser: true }) + if (object.ReferenceEquals(pair.Value.Parent, optionResult)) { - if (optionResult.Option is { } opt) - { - var argResult = optionResult.GetOrCreateDefaultArgumentResult(opt.Argument); - optionResult.AddChild(argResult); - ValidateAndConvertArgumentResult(argResult); - } - } - } - else - { - for (var i = 0; i < optionResult.Children.Count; i++) - { - var result = optionResult.Children[i]; - if (result is ArgumentResult argumentResult) - { - ValidateAndConvertArgumentResult(argumentResult); - } + ValidateAndConvertArgumentResult((ArgumentResult)pair.Value); } } } @@ -576,11 +549,10 @@ void Handle(SymbolResult? symbolResult, Symbol symbol) { case OptionResult o: - if (o.Children.Count == 0 && - o.Option.Argument.ValueType == typeof(bool)) + if (o.Option.Argument.ValueType == typeof(bool) + && !_symbolResults.ContainsKey(o.Option.Argument)) { - o.AddChild( - new ArgumentResult(o.Option.Argument, o)); + _symbolResults.Add(o.Option.Argument, new ArgumentResult(o.Option.Argument, o)); } break; @@ -595,20 +567,20 @@ void Handle(SymbolResult? symbolResult, Symbol symbol) null, commandResult); - var childArgumentResult = optionResult.GetOrCreateDefaultArgumentResult( - option.Argument); - - optionResult.AddChild(childArgumentResult); - commandResult.AddChild(optionResult); - _symbolResults.TryAdd(optionResult.Option, optionResult); + if (_symbolResults.TryAdd(optionResult.Option, optionResult)) + { + _symbolResults.Add(optionResult.Option.Argument, new ArgumentResult(optionResult.Option.Argument, optionResult)); + } break; case Argument { HasDefaultValue: true } argument: - var argumentResult = commandResult.GetOrCreateDefaultArgumentResult(argument); - - AddToResult(argumentResult); + if (!_symbolResults.ContainsKey(argument)) + { + AddToResult(new ArgumentResult(argument, commandResult)); + } + break; } diff --git a/src/System.CommandLine/Parsing/RootCommandResult.cs b/src/System.CommandLine/Parsing/RootCommandResult.cs index 680db15df5..f6c5461de8 100644 --- a/src/System.CommandLine/Parsing/RootCommandResult.cs +++ b/src/System.CommandLine/Parsing/RootCommandResult.cs @@ -30,5 +30,16 @@ public RootCommandResult( public override OptionResult? FindResultFor(Option option) => _symbolResults.TryGetValue(option, out SymbolResult? result) ? (OptionResult)result : default; + + internal override IEnumerable GetChildren(SymbolResult parent) + { + foreach (var pair in _symbolResults) + { + if (ReferenceEquals(parent, pair.Value.Parent)) + { + yield return pair.Value; + } + } + } } } diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index e2337c4563..674c288523 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -71,6 +71,8 @@ private protected SymbolResult(SymbolResult? parent) /// 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); + internal virtual IEnumerable GetChildren(SymbolResult parent) => GetRoot().GetChildren(parent); + private SymbolResult GetRoot() { SymbolResult result = this; diff --git a/src/System.CommandLine/Parsing/SymbolResultExtensions.cs b/src/System.CommandLine/Parsing/SymbolResultExtensions.cs index e447f37ada..443fdcfe62 100644 --- a/src/System.CommandLine/Parsing/SymbolResultExtensions.cs +++ b/src/System.CommandLine/Parsing/SymbolResultExtensions.cs @@ -7,13 +7,13 @@ namespace System.CommandLine.Parsing { internal static class SymbolResultExtensions { - internal static IEnumerable AllSymbolResults(this SymbolResult symbolResult) + internal static IEnumerable AllSymbolResults(this CommandResult commandResult) { - yield return symbolResult; + yield return commandResult; - foreach (var item in symbolResult + foreach (var item in commandResult .Children - .FlattenBreadthFirst(o => o.Children)) + .FlattenBreadthFirst(o => o.GetChildren(o))) { yield return item; } @@ -28,7 +28,7 @@ internal static Token Token(this SymbolResult symbolResult) _ => throw new ArgumentOutOfRangeException(nameof(symbolResult)) }; - Token CreateImplicitToken(Option option) + static Token CreateImplicitToken(Option option) { return new Token(option.GetLongestAlias(removePrefix: false), TokenType.Option, option, Parsing.Token.ImplicitPosition); } From a10c271225b47209ad91e5f61200c2ddd9be2d22 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 18 Jan 2023 20:38:21 +0100 Subject: [PATCH 5/5] fix the build ;) --- src/System.CommandLine/Help/VersionOption.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.CommandLine/Help/VersionOption.cs b/src/System.CommandLine/Help/VersionOption.cs index 88153c2d64..55b4d9e9c0 100644 --- a/src/System.CommandLine/Help/VersionOption.cs +++ b/src/System.CommandLine/Help/VersionOption.cs @@ -37,7 +37,7 @@ private void AddValidators() parent.Children.Where(r => !(r is OptionResult optionResult && optionResult.Option is VersionOption)) .Any(IsNotImplicit)) { - result.ErrorMessage = result.LocalizationResources.VersionOptionCannotBeCombinedWithOtherArguments(result.Token?.Value); + result.ErrorMessage = result.LocalizationResources.VersionOptionCannotBeCombinedWithOtherArguments(result.Token?.Value ?? result.Option.Name); } }); }