From b0b488dface144b68a88662b92dbfb8e883f2e17 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 23 Jan 2023 17:24:40 +0100 Subject: [PATCH 01/15] remove SymbolResult.ErrorMessage, use the new SymbolResult.ReportError --- ...ommandLine_api_is_not_changed.approved.txt | 2 +- src/System.CommandLine.Tests/ArgumentTests.cs | 14 +-- src/System.CommandLine.Tests/OptionTests.cs | 4 +- .../ParsingValidationTests.cs | 59 ++++++----- src/System.CommandLine/Argument.cs | 2 + src/System.CommandLine/ArgumentArity.cs | 16 ++- src/System.CommandLine/Argument{T}.cs | 10 +- src/System.CommandLine/Help/VersionOption.cs | 2 +- .../Parsing/ArgumentResult.cs | 24 +---- .../Parsing/ParseResultVisitor.cs | 97 +++++++------------ .../Parsing/SymbolResult.cs | 13 +-- .../Parsing/SymbolResultTree.cs | 39 +++++++- src/System.CommandLine/Validate.cs | 9 +- 13 files changed, 144 insertions(+), 147 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 452160cab6..e2d71fa746 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 @@ -423,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.String ErrorMessage { get; set; } public System.CommandLine.LocalizationResources LocalizationResources { get; } public SymbolResult Parent { get; } public System.Collections.Generic.IReadOnlyList Tokens { get; } @@ -434,6 +433,7 @@ System.CommandLine.Parsing public System.Object GetValue(System.CommandLine.Argument argument) public T GetValue(Option option) public System.Object GetValue(System.CommandLine.Option option) + public System.Void ReportError(System.String errorMessage) public System.String ToString() public class Token, System.IEquatable public static System.Boolean op_Equality(Token left, Token right) diff --git a/src/System.CommandLine.Tests/ArgumentTests.cs b/src/System.CommandLine.Tests/ArgumentTests.cs index 388224ec19..242082659e 100644 --- a/src/System.CommandLine.Tests/ArgumentTests.cs +++ b/src/System.CommandLine.Tests/ArgumentTests.cs @@ -124,7 +124,7 @@ public void Validation_failure_message_can_be_specified_when_parsing_tokens() { var argument = new Argument(result => { - result.ErrorMessage = "oops!"; + result.ReportError("oops!"); return null; }); @@ -143,7 +143,7 @@ public void Validation_failure_message_can_be_specified_when_evaluating_default_ { var argument = new Argument(result => { - result.ErrorMessage = "oops!"; + result.ReportError("oops!"); return null; }, true); @@ -164,7 +164,7 @@ public void Validation_failure_message_can_be_specified_when_evaluating_default_ "-x", result => { - result.ErrorMessage = "oops!"; + result.ReportError("oops!"); return null; }, true); @@ -389,7 +389,7 @@ public void Multiple_command_arguments_can_have_custom_parse_delegates() { new Argument("from", argumentResult => { - argumentResult.ErrorMessage = "nope"; + argumentResult.ReportError("nope"); return null; }, true) { @@ -397,7 +397,7 @@ public void Multiple_command_arguments_can_have_custom_parse_delegates() }, new Argument("to", argumentResult => { - argumentResult.ErrorMessage = "UH UH"; + argumentResult.ReportError("UH UH"); return null; }, true) { @@ -426,7 +426,7 @@ public void When_custom_conversion_fails_then_an_option_does_not_accept_further_ new Argument(), new Option("-x", argResult => { - argResult.ErrorMessage = "nope"; + argResult.ReportError("nope"); return default; }) }; @@ -446,7 +446,7 @@ public void When_argument_cannot_be_parsed_as_the_specified_type_then_getting_va return value; } - argumentResult.ErrorMessage = $"'{argumentResult.Tokens.Single().Value}' is not an integer"; + argumentResult.ReportError($"'{argumentResult.Tokens.Single().Value}' is not an integer"); return default; }); diff --git a/src/System.CommandLine.Tests/OptionTests.cs b/src/System.CommandLine.Tests/OptionTests.cs index 5716012e6d..957231f319 100644 --- a/src/System.CommandLine.Tests/OptionTests.cs +++ b/src/System.CommandLine.Tests/OptionTests.cs @@ -287,11 +287,11 @@ public void Option_T_default_value_is_validated() { var option = new Option("-x", () => 123); option.Validators.Add(symbol => - symbol.ErrorMessage = symbol.Tokens + symbol.ReportError(symbol.Tokens .Select(t => t.Value) .Where(v => v == "123") .Select(_ => "ERR") - .FirstOrDefault()); + .First())); option .Parse("-x 123") diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index 523e982b08..a91406cb84 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -60,13 +60,17 @@ public void When_FromAmong_is_used_then_the_OptionResult_ErrorMessage_is_set() var parseResult = command.Parse("test --opt c"); - parseResult.FindResultFor(option) - .ErrorMessage - .Should() - .Be(parseResult.Errors.Single().Message) - .And - .Should() - .NotBeNull(); + var error = parseResult.Errors.Single(); + + error + .Message + .Should() + .Be(parseResult.CommandResult.LocalizationResources.UnrecognizedArgument("c", new []{ "a", "b"})); + error + .SymbolResult + .Should() + .BeOfType(); + } [Fact] // https://github.com/dotnet/command-line-api/issues/1475 @@ -79,13 +83,16 @@ public void When_FromAmong_is_used_then_the_ArgumentResult_ErrorMessage_is_set() var parseResult = command.Parse("test c"); - parseResult.FindResultFor(argument) - .ErrorMessage - .Should() - .Be(parseResult.Errors.Single().Message) - .And - .Should() - .NotBeNull(); + var error = parseResult.Errors.Single(); + + error + .Message + .Should() + .Be(parseResult.CommandResult.LocalizationResources.UnrecognizedArgument("c", new []{ "a", "b"})); + error + .SymbolResult + .Should() + .BeOfType(); } [Fact] // https://github.com/dotnet/command-line-api/issues/1556 @@ -334,7 +341,7 @@ public void A_custom_validator_can_be_added_to_a_command() 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."; + commandResult.ReportError("Options '--one' and '--two' cannot be used together."); } }); @@ -358,7 +365,7 @@ public void A_custom_validator_can_be_added_to_an_option() { var value = r.GetValueOrDefault(); - r.ErrorMessage = $"Option {r.Token.Value} cannot be set to {value}"; + r.ReportError($"Option {r.Token.Value} cannot be set to {value}"); }); var command = new RootCommand { option }; @@ -385,7 +392,7 @@ public void A_custom_validator_can_be_added_to_an_argument() { var value = r.GetValueOrDefault(); - r.ErrorMessage = $"Argument {r.Argument.Name} cannot be set to {value}"; + r.ReportError($"Argument {r.Argument.Name} cannot be set to {value}"); }); var command = new RootCommand { argument }; @@ -449,7 +456,7 @@ public void Validators_on_global_options_are_executed_when_invoking_a_subcommand var option = new Option("--file"); option.Validators.Add(r => { - r.ErrorMessage = "Invoked validator"; + r.ReportError("Invoked validator"); }); var subCommand = new Command("subcommand"); @@ -484,7 +491,7 @@ public async Task A_custom_validator_added_to_a_global_option_is_checked(string var handlerWasCalled = false; var globalOption = new Option("--value"); - globalOption.Validators.Add(r => r.ErrorMessage = "oops!"); + globalOption.Validators.Add(r => r.ReportError("oops!")); var grandchildCommand = new Command("grandchild"); @@ -514,7 +521,7 @@ public void Custom_validator_error_messages_are_not_repeated() { var errorMessage = "that's not right..."; var argument = new Argument(); - argument.Validators.Add(r => r.ErrorMessage = errorMessage); + argument.Validators.Add(r => r.ReportError(errorMessage)); var cmd = new Command("get") { @@ -541,7 +548,7 @@ public void The_parsed_value_of_an_argument_is_available_within_a_validator() if (value < 0 || value > 100) { - result.ErrorMessage = errorMessage; + result.ReportError(errorMessage); } }); @@ -565,7 +572,7 @@ public void The_parsed_value_of_an_option_is_available_within_a_validator() if (value < 0 || value > 100) { - result.ErrorMessage = errorMessage; + result.ReportError(errorMessage); } }); @@ -1196,7 +1203,7 @@ public void Arity_failures_are_not_reported_for_both_an_argument_and_its_parent_ public void Multiple_validators_on_the_same_command_do_not_report_duplicate_errors() { var command = new RootCommand(); - command.Validators.Add(result => result.ErrorMessage = "Wrong"); + command.Validators.Add(result => result.ReportError("Wrong")); command.Validators.Add(_ => { }); var parseResult = command.Parse(""); @@ -1214,7 +1221,7 @@ public void Multiple_validators_on_the_same_command_do_not_report_duplicate_erro public void Multiple_validators_on_the_same_option_do_not_report_duplicate_errors() { var option = new Option("-x"); - option.Validators.Add(result => result.ErrorMessage = "Wrong"); + option.Validators.Add(result => result.ReportError("Wrong")); option.Validators.Add(_ => { }); var command = new RootCommand @@ -1237,7 +1244,7 @@ public void Multiple_validators_on_the_same_option_do_not_report_duplicate_error public void Multiple_validators_on_the_same_argument_do_not_report_duplicate_errors() { var argument = new Argument(); - argument.Validators.Add(result => result.ErrorMessage = "Wrong"); + argument.Validators.Add(result => result.ReportError("Wrong")); argument.Validators.Add(_ => { }); var command = new RootCommand @@ -1262,7 +1269,7 @@ internal void When_there_is_an_arity_error_then_further_errors_are_not_reported( var option = new Option("-o"); option.Validators.Add(result => { - result.ErrorMessage = "OOPS"; + result.ReportError("OOPS"); }); //all good; var command = new Command("comm") diff --git a/src/System.CommandLine/Argument.cs b/src/System.CommandLine/Argument.cs index 42d5de1d7f..b9bf36e259 100644 --- a/src/System.CommandLine/Argument.cs +++ b/src/System.CommandLine/Argument.cs @@ -107,6 +107,8 @@ private protected override string DefaultName /// public List> Validators => _validators ??= new (); + internal bool HasValidators => (_validators?.Count ?? 0) > 0; + /// /// Gets the default value for the argument. /// diff --git a/src/System.CommandLine/ArgumentArity.cs b/src/System.CommandLine/ArgumentArity.cs index 6e3d496af5..a5f46f9c7b 100644 --- a/src/System.CommandLine/ArgumentArity.cs +++ b/src/System.CommandLine/ArgumentArity.cs @@ -73,41 +73,37 @@ public override int GetHashCode() => MaximumNumberOfValues ^ MinimumNumberOfValues ^ IsNonDefault.GetHashCode(); internal static ArgumentConversionResult? Validate( - SymbolResult symbolResult, + SymbolResult parentSymbolResult, Argument argument, int minimumNumberOfValues, int maximumNumberOfValues) { - var argumentResult = symbolResult switch - { - ArgumentResult a => a, - _ => symbolResult.FindResultFor(argument) - }; + var argumentResult = parentSymbolResult.FindResultFor(argument); var tokenCount = argumentResult?.Tokens.Count ?? 0; if (tokenCount < minimumNumberOfValues) { - if (symbolResult.UseDefaultValueFor(argument)) + if (parentSymbolResult.UseDefaultValueFor(argument)) { return null; } return ArgumentConversionResult.Failure( argument, - symbolResult.LocalizationResources.RequiredArgumentMissing(symbolResult), + parentSymbolResult.LocalizationResources.RequiredArgumentMissing(parentSymbolResult), ArgumentConversionResultType.FailedMissingArgument); } if (tokenCount > maximumNumberOfValues) { - if (symbolResult is OptionResult optionResult) + if (parentSymbolResult is OptionResult optionResult) { if (!optionResult.Option.AllowMultipleArgumentsPerToken) { return ArgumentConversionResult.Failure( argument, - symbolResult!.LocalizationResources.ExpectsOneArgument(symbolResult), + parentSymbolResult!.LocalizationResources.ExpectsOneArgument(parentSymbolResult), ArgumentConversionResultType.FailedTooManyArguments); } } diff --git a/src/System.CommandLine/Argument{T}.cs b/src/System.CommandLine/Argument{T}.cs index 35c3d4204d..89e32b96d0 100644 --- a/src/System.CommandLine/Argument{T}.cs +++ b/src/System.CommandLine/Argument{T}.cs @@ -92,9 +92,10 @@ public Argument( ConvertArguments = (ArgumentResult argumentResult, out object? value) => { + int errorsBefore = argumentResult.SymbolResultTree.ErrorCount; var result = parse(argumentResult); - if (string.IsNullOrEmpty(argumentResult.ErrorMessage)) + if (errorsBefore == argumentResult.SymbolResultTree.ErrorCount) { value = result; return true; @@ -194,8 +195,7 @@ void UnrecognizedArgumentError(ArgumentResult argumentResult) { if (Array.IndexOf(values, token.Value) < 0) { - argumentResult.ErrorMessage = argumentResult.LocalizationResources.UnrecognizedArgument(token.Value, values); - break; + argumentResult.ReportError(argumentResult.LocalizationResources.UnrecognizedArgument(token.Value, values)); } } } @@ -221,7 +221,7 @@ public void AcceptLegalFilePathsOnly() if (invalidCharactersIndex >= 0) { - result.ErrorMessage = result.LocalizationResources.InvalidCharactersInPath(token.Value[invalidCharactersIndex]); + result.ReportError(result.LocalizationResources.InvalidCharactersInPath(token.Value[invalidCharactersIndex])); } } }); @@ -244,7 +244,7 @@ public void AcceptLegalFileNamesOnly() if (invalidCharactersIndex >= 0) { - result.ErrorMessage = result.LocalizationResources.InvalidCharactersInFileName(token.Value[invalidCharactersIndex]); + result.ReportError(result.LocalizationResources.InvalidCharactersInFileName(token.Value[invalidCharactersIndex])); } } }); diff --git a/src/System.CommandLine/Help/VersionOption.cs b/src/System.CommandLine/Help/VersionOption.cs index 55b4d9e9c0..1aba232647 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.Option.Name); + result.ReportError(result.LocalizationResources.VersionOptionCannotBeCombinedWithOtherArguments(result.Token?.Value ?? result.Option.Name)); } }); } diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index dfe89c4cb4..a32aa1a6a3 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -80,22 +80,6 @@ public void OnlyTake(int numberOfTokens) /// public override string ToString() => $"{GetType().Name} {Argument.Name}: {string.Join(" ", Tokens.Select(t => $"<{t.Value}>"))}"; - internal ParseError? CustomError(Argument argument) - { - for (var i = 0; i < argument.Validators.Count; i++) - { - var validateSymbolResult = argument.Validators[i]; - validateSymbolResult(this); - - if (!string.IsNullOrWhiteSpace(ErrorMessage)) - { - return new ParseError(ErrorMessage!, Parent is OptionResult option ? option : this); - } - } - - return null; - } - private ArgumentConversionResult Convert(Argument argument) { if (ShouldCheckArity() && @@ -115,7 +99,7 @@ Parent is { } && var defaultValue = argument.GetDefaultValue(argumentResult); - if (string.IsNullOrEmpty(argumentResult.ErrorMessage)) + if (!SymbolResultTree.HasError(argumentResult, out ParseError? error)) { return ArgumentConversionResult.Success( argument, @@ -125,7 +109,7 @@ Parent is { } && { return ArgumentConversionResult.Failure( argument, - argumentResult.ErrorMessage!, + error.Message, ArgumentConversionResultType.Failed); } } @@ -151,9 +135,9 @@ Parent is { } && return ArgumentConversionResult.Success(argument, value); } - if (ErrorMessage is not null) + if (SymbolResultTree.HasError(this, out ParseError? e)) { - return ArgumentConversionResult.Failure(argument, ErrorMessage, ArgumentConversionResultType.Failed); + return ArgumentConversionResult.Failure(argument, e.Message, ArgumentConversionResultType.Failed); } return new ArgumentConversionResult( diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index ed94ed50a6..33ff8b0872 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -19,7 +19,6 @@ internal sealed class ParseResultVisitor private Dictionary>? _directives; private List? _unmatchedTokens; - private List? _errors; private List? _argumentResults; private CommandResult _innermostCommandResult; private bool _isHelpRequested; @@ -36,22 +35,11 @@ internal ParseResultVisitor( _tokens = tokens; _unmatchedTokens = unmatchedTokens; _rawInput = rawInput; - _symbolResultTree = new(_parser.Configuration.LocalizationResources); + _symbolResultTree = new(_parser.Configuration.LocalizationResources, tokenizeErrors); _innermostCommandResult = _rootCommandResult = new CommandResult( rootCommandNode.Command, rootCommandNode.Token, _symbolResultTree); - - if (tokenizeErrors is not null) - { - _errors = new List(tokenizeErrors.Count); - - for (var i = 0; i < tokenizeErrors.Count; i++) - { - var error = tokenizeErrors[i]; - _errors.Add(new ParseError(error)); - } - } } internal void Visit(CommandNode rootCommandNode) @@ -349,11 +337,7 @@ private void ValidateCommandResult() { if (_rootCommandResult.FindResultFor(option) is null) { - AddErrorToResult( - _innermostCommandResult, - new ParseError( - _rootCommandResult.LocalizationResources.RequiredOptionWasNotProvided(option), - _innermostCommandResult)); + _innermostCommandResult.ReportError(_rootCommandResult.LocalizationResources.RequiredOptionWasNotProvided(option)); } } } @@ -371,21 +355,17 @@ private void ValidateCommandResult() private bool UseValidators(Command command, CommandResult innermostCommandResult) { - for (var i = 0; i < command.Validators.Count; i++) + if (!command.HasValidators) { - var validateSymbolResult = command.Validators[i]; - validateSymbolResult(innermostCommandResult); - - if (!string.IsNullOrWhiteSpace(innermostCommandResult.ErrorMessage)) - { - AddErrorToResult( - innermostCommandResult, - new ParseError(innermostCommandResult.ErrorMessage!, _innermostCommandResult)); + return false; + } - return true; - } + int errorCountBefore = _symbolResultTree.ErrorCount; + for (var i = 0; i < command.Validators.Count; i++) + { + command.Validators[i](innermostCommandResult); } - return false; + return _symbolResultTree.ErrorCount != errorCountBefore; } private void ValidateArguments(IList arguments, CommandResult innermostCommandResult) @@ -402,7 +382,7 @@ private void ValidateArguments(IList arguments, CommandResult innermos if (arityFailure is not null) { - AddErrorToResult(innermostCommandResult, new ParseError(arityFailure.ErrorMessage!, innermostCommandResult)); + innermostCommandResult.ReportError(arityFailure.ErrorMessage!); } } } @@ -419,7 +399,7 @@ private void ValidateCommandHandler() return; } - (_errors ??= new()).Insert( + _symbolResultTree.InsertError( 0, new ParseError( _innermostCommandResult.LocalizationResources.RequiredCommandWasNotProvided(), @@ -436,31 +416,30 @@ private void ValidateAndConvertOptionResult(OptionResult optionResult) argument.Arity.MinimumNumberOfValues, argument.Arity.MaximumNumberOfValues); - if (arityFailure is { }) + if (arityFailure is not null) { - AddErrorToResult(optionResult, new ParseError(arityFailure.ErrorMessage!, optionResult)); + optionResult.ReportError(arityFailure.ErrorMessage!); return; } if (optionResult.Option.HasValidators) { + int errorsBefore = _symbolResultTree.ErrorCount; + for (var i = 0; i < optionResult.Option.Validators.Count; i++) { - var validate = optionResult.Option.Validators[i]; - validate(optionResult); - - if (!string.IsNullOrWhiteSpace(optionResult.ErrorMessage)) - { - AddErrorToResult(optionResult, new ParseError(optionResult.ErrorMessage!, optionResult)); + optionResult.Option.Validators[i](optionResult); + } - return; - } + if (errorsBefore != _symbolResultTree.ErrorCount) + { + return; } } foreach (var pair in _symbolResultTree) { - if (object.ReferenceEquals(pair.Value.Parent, optionResult)) + if (ReferenceEquals(pair.Value.Parent, optionResult)) { ValidateAndConvertArgumentResult((ArgumentResult)pair.Value); } @@ -471,12 +450,18 @@ private void ValidateAndConvertArgumentResult(ArgumentResult argumentResult) { var argument = argumentResult.Argument; - var parseError = argumentResult.CustomError(argument); - - if (parseError is { }) + if (argument.HasValidators) { - AddErrorToResult(argumentResult, parseError); - return; + int errorsBefore = _symbolResultTree.ErrorCount; + for (var i = 0; i < argument.Validators.Count; i++) + { + argument.Validators[i](argumentResult); + } + + if (errorsBefore != _symbolResultTree.ErrorCount) + { + return; + } } var argumentConversionResult = argumentResult.GetArgumentConversionResult(); @@ -497,7 +482,7 @@ private void ValidateAndConvertArgumentResult(ArgumentResult argumentResult) } } - AddErrorToResult(argumentResult, new ParseError(argumentConversionResult.ErrorMessage!, argumentResult)); + argumentResult.ReportError(argumentConversionResult.ErrorMessage!); } } @@ -578,18 +563,6 @@ void Handle(SymbolResult? symbolResult, Symbol symbol) } } - private void AddErrorToResult(SymbolResult symbolResult, ParseError parseError) - { - symbolResult.ErrorMessage ??= parseError.Message; - - if (symbolResult.Parent is OptionResult optionResult) - { - optionResult.ErrorMessage ??= symbolResult.ErrorMessage; - } - - (_errors ??= new()).Add(parseError); - } - public ParseResult GetResult() => new(_parser, _rootCommandResult, @@ -597,7 +570,7 @@ public ParseResult GetResult() => _directives, _tokens, _unmatchedTokens, - _errors, + _symbolResultTree.Errors, _rawInput); } } diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index 6ab20d8151..1acd428ac1 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -21,12 +21,6 @@ private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? Parent = parent; } - /// - /// An error message for this symbol result. - /// - /// 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; } - /// /// The parent symbol result in the parse tree. /// @@ -51,6 +45,13 @@ private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? internal void AddToken(Token token) => (_tokens ??= new()).Add(token); + /// + /// Adds an error message for this symbol result to it's parse tree. + /// + /// Setting an error will cause the parser to indicate an error for the user and prevent invocation of the command line. + public void ReportError(string errorMessage) + => SymbolResultTree.ReportError(new ParseError(errorMessage, Parent is OptionResult option ? option : this)); + /// /// Finds a result for the specific argument anywhere in the parse tree, including parent and child symbol results. /// diff --git a/src/System.CommandLine/Parsing/SymbolResultTree.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs index 07ba985c7e..110a707c3e 100644 --- a/src/System.CommandLine/Parsing/SymbolResultTree.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -2,20 +2,34 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; namespace System.CommandLine.Parsing { internal sealed class SymbolResultTree : Dictionary { private readonly LocalizationResources _localizationResources; + internal List? Errors; - internal SymbolResultTree(LocalizationResources localizationResources) + internal SymbolResultTree(LocalizationResources localizationResources, List? tokenizeErrors) { _localizationResources = localizationResources; + + if (tokenizeErrors is not null) + { + Errors = new List(tokenizeErrors.Count); + + for (var i = 0; i < tokenizeErrors.Count; i++) + { + Errors.Add(new ParseError(tokenizeErrors[i])); + } + } } internal LocalizationResources LocalizationResources => _localizationResources; + internal int ErrorCount => Errors?.Count ?? 0; + internal ArgumentResult? FindResultFor(Argument argument) => TryGetValue(argument, out SymbolResult? result) ? (ArgumentResult)result : default; @@ -38,5 +52,28 @@ internal IEnumerable GetChildren(SymbolResult parent) } } } + + internal bool HasError(ArgumentResult argumentResult, [NotNullWhen(true)] out ParseError? error) + { + if (Errors is not null) + { + SymbolResult symbolResult = argumentResult.Parent is OptionResult optionResult ? optionResult : argumentResult; + for (int i = 0; i < Errors.Count; i++) + { + if (ReferenceEquals(Errors[i].SymbolResult, symbolResult)) + { + error = Errors[i]; + return true; + } + } + } + + error = null; + return false; + } + + internal void ReportError(ParseError parseError) => (Errors ??= new()).Add(parseError); + + internal void InsertError(int index, ParseError parseError) => (Errors ??= new()).Insert(index, parseError); } } diff --git a/src/System.CommandLine/Validate.cs b/src/System.CommandLine/Validate.cs index 04a5206a16..591575e786 100644 --- a/src/System.CommandLine/Validate.cs +++ b/src/System.CommandLine/Validate.cs @@ -13,8 +13,7 @@ internal static void FileExists(ArgumentResult result) if (!File.Exists(token.Value)) { - result.ErrorMessage = result.LocalizationResources.FileDoesNotExist(token.Value); - return; + result.ReportError(result.LocalizationResources.FileDoesNotExist(token.Value)); } } } @@ -27,8 +26,7 @@ internal static void DirectoryExists(ArgumentResult result) if (!Directory.Exists(token.Value)) { - result.ErrorMessage = result.LocalizationResources.DirectoryDoesNotExist(token.Value); - return; + result.ReportError(result.LocalizationResources.DirectoryDoesNotExist(token.Value)); } } } @@ -41,8 +39,7 @@ internal static void FileOrDirectoryExists(ArgumentResult result) if (!Directory.Exists(token.Value) && !File.Exists(token.Value)) { - result.ErrorMessage = result.LocalizationResources.FileOrDirectoryDoesNotExist(token.Value); - return; + result.ReportError(result.LocalizationResources.FileOrDirectoryDoesNotExist(token.Value)); } } } From 3006cb33e512c08922d0236fe488dbc43fdb3bdf Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 24 Jan 2023 13:35:54 +0100 Subject: [PATCH 02/15] use all available information when creating error message for ArgumentConversionResult --- ...ommandLine_api_is_not_changed.approved.txt | 3 + .../ModelBinder.cs | 4 +- .../ParseDiagramTests.cs | 2 +- src/System.CommandLine/ArgumentArity.cs | 27 +++--- .../Binding/ArgumentConversionResult.cs | 73 +++++++++++------ .../Binding/ArgumentConverter.cs | 76 ++++++++--------- .../Binding/BindingContext.cs | 14 +++- .../LocalizationResources.cs | 14 ++++ .../Parsing/ArgumentResult.cs | 82 +++++++++---------- .../Parsing/OptionResult.cs | 4 +- .../Parsing/OptionResultExtensions.cs | 16 ---- .../Parsing/ParseResultVisitor.cs | 40 ++------- .../Parsing/SymbolResult.cs | 3 +- .../Properties/Resources.Designer.cs | 18 ++++ .../Properties/Resources.resx | 6 ++ .../Properties/xlf/Resources.cs.xlf | 10 +++ .../Properties/xlf/Resources.de.xlf | 10 +++ .../Properties/xlf/Resources.es.xlf | 10 +++ .../Properties/xlf/Resources.fr.xlf | 10 +++ .../Properties/xlf/Resources.it.xlf | 10 +++ .../Properties/xlf/Resources.ja.xlf | 10 +++ .../Properties/xlf/Resources.ko.xlf | 10 +++ .../Properties/xlf/Resources.pl.xlf | 10 +++ .../Properties/xlf/Resources.pt-BR.xlf | 10 +++ .../Properties/xlf/Resources.ru.xlf | 10 +++ .../Properties/xlf/Resources.tr.xlf | 10 +++ .../Properties/xlf/Resources.zh-Hans.xlf | 10 +++ .../Properties/xlf/Resources.zh-Hant.xlf | 10 +++ 28 files changed, 324 insertions(+), 188 deletions(-) delete mode 100644 src/System.CommandLine/Parsing/OptionResultExtensions.cs 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 e2d71fa746..3ac3f73bbb 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 @@ -146,7 +146,9 @@ System.CommandLine public static LocalizationResources Instance { get; } public System.String ArgumentConversionCannotParse(System.String value, System.Type expectedType) public System.String ArgumentConversionCannotParseForCommand(System.String value, System.String commandAlias, System.Type expectedType) + public System.String ArgumentConversionCannotParseForCommand(System.String value, System.String commandAlias, System.Type expectedType, System.Collections.Generic.IEnumerable completions) public System.String ArgumentConversionCannotParseForOption(System.String value, System.String optionAlias, System.Type expectedType) + public System.String ArgumentConversionCannotParseForOption(System.String value, System.String optionAlias, System.Type expectedType, System.Collections.Generic.IEnumerable completions) public System.String DirectoryDoesNotExist(System.String path) public System.String ErrorReadingResponseFile(System.String filePath, System.IO.IOException e) public System.String ExceptionHandlerHeader() @@ -389,6 +391,7 @@ System.CommandLine.Parsing public System.Object GetValueOrDefault() public T GetValueOrDefault() public System.Void OnlyTake(System.Int32 numberOfTokens) + public System.Void ReportError(System.String errorMessage) public System.String ToString() public class CommandLineStringSplitter public System.Collections.Generic.IEnumerable Split(System.String commandLine) diff --git a/src/System.CommandLine.NamingConventionBinder/ModelBinder.cs b/src/System.CommandLine.NamingConventionBinder/ModelBinder.cs index 56885c878a..08c52d510b 100644 --- a/src/System.CommandLine.NamingConventionBinder/ModelBinder.cs +++ b/src/System.CommandLine.NamingConventionBinder/ModelBinder.cs @@ -132,7 +132,7 @@ private bool ShortCutTheBinding() var valueSource = GetValueSource(bindingSources, bindingContext, ValueDescriptor, EnforceExplicitBinding); return bindingContext.TryBindToScalarValue(ValueDescriptor, valueSource, - bindingContext.ParseResult.CommandResult.LocalizationResources, + bindingContext.ParseResult, out var boundValue) ? (true, boundValue?.Value, true) : (false, null, false); @@ -277,7 +277,7 @@ internal static (BoundValue? boundValue, bool usedNonDefault) GetBoundValue( if (bindingContext.TryBindToScalarValue( valueDescriptor, valueSource, - bindingContext.ParseResult.CommandResult.LocalizationResources, + bindingContext.ParseResult, out var boundValue)) { return (boundValue, true); diff --git a/src/System.CommandLine.Tests/ParseDiagramTests.cs b/src/System.CommandLine.Tests/ParseDiagramTests.cs index baaf6f6c32..2c78cab848 100644 --- a/src/System.CommandLine.Tests/ParseDiagramTests.cs +++ b/src/System.CommandLine.Tests/ParseDiagramTests.cs @@ -59,7 +59,7 @@ public void Parse_diagram_shows_type_conversion_errors() result.Diagram() .Should() - .Be($"[ {RootCommand.ExecutableName} [ -f ! ] ]"); + .Be($"[ {RootCommand.ExecutableName} ![ -f ] ]"); } [Fact] diff --git a/src/System.CommandLine/ArgumentArity.cs b/src/System.CommandLine/ArgumentArity.cs index a5f46f9c7b..3b20d147c0 100644 --- a/src/System.CommandLine/ArgumentArity.cs +++ b/src/System.CommandLine/ArgumentArity.cs @@ -72,38 +72,31 @@ public bool Equals(ArgumentArity other) => public override int GetHashCode() => MaximumNumberOfValues ^ MinimumNumberOfValues ^ IsNonDefault.GetHashCode(); - internal static ArgumentConversionResult? Validate( - SymbolResult parentSymbolResult, - Argument argument, - int minimumNumberOfValues, - int maximumNumberOfValues) + internal static ArgumentConversionResult? Validate(ArgumentResult argumentResult) { - var argumentResult = parentSymbolResult.FindResultFor(argument); - - var tokenCount = argumentResult?.Tokens.Count ?? 0; - - if (tokenCount < minimumNumberOfValues) + int tokenCount = argumentResult.Tokens.Count; + if (tokenCount < argumentResult.Argument.Arity.MinimumNumberOfValues) { - if (parentSymbolResult.UseDefaultValueFor(argument)) + if (argumentResult.Parent!.UseDefaultValueFor(argumentResult.Argument)) { return null; } return ArgumentConversionResult.Failure( - argument, - parentSymbolResult.LocalizationResources.RequiredArgumentMissing(parentSymbolResult), + argumentResult, + argumentResult.LocalizationResources.RequiredArgumentMissing(argumentResult.Parent!), ArgumentConversionResultType.FailedMissingArgument); } - if (tokenCount > maximumNumberOfValues) + if (tokenCount > argumentResult.Argument.Arity.MaximumNumberOfValues) { - if (parentSymbolResult is OptionResult optionResult) + if (argumentResult.Parent is OptionResult optionResult) { if (!optionResult.Option.AllowMultipleArgumentsPerToken) { return ArgumentConversionResult.Failure( - argument, - parentSymbolResult!.LocalizationResources.ExpectsOneArgument(parentSymbolResult), + argumentResult, + argumentResult.LocalizationResources.ExpectsOneArgument(optionResult), ArgumentConversionResultType.FailedTooManyArguments); } } diff --git a/src/System.CommandLine/Binding/ArgumentConversionResult.cs b/src/System.CommandLine/Binding/ArgumentConversionResult.cs index 2e0ac7b25f..ad914320af 100644 --- a/src/System.CommandLine/Binding/ArgumentConversionResult.cs +++ b/src/System.CommandLine/Binding/ArgumentConversionResult.cs @@ -1,73 +1,94 @@ // 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.Completions; +using System.CommandLine.Parsing; using System.Linq; namespace System.CommandLine.Binding { internal sealed class ArgumentConversionResult { - internal readonly Argument Argument; + internal readonly ArgumentResult ArgumentResult; internal readonly object? Value; internal readonly string? ErrorMessage; internal ArgumentConversionResultType Result; - private ArgumentConversionResult(Argument argument, string error, ArgumentConversionResultType failure) + private ArgumentConversionResult(ArgumentResult argumentResult, string error, ArgumentConversionResultType failure) { - Argument = argument ?? throw new ArgumentNullException(nameof(argument)); + ArgumentResult = argumentResult ?? throw new ArgumentNullException(nameof(argumentResult)); ErrorMessage = error ?? throw new ArgumentNullException(nameof(error)); Result = failure; } - private ArgumentConversionResult(Argument argument, object? value) + private ArgumentConversionResult(ArgumentResult argumentResult, object? value) { - Argument = argument ?? throw new ArgumentNullException(nameof(argument)); + ArgumentResult = argumentResult ?? throw new ArgumentNullException(nameof(argumentResult)); Value = value; Result = ArgumentConversionResultType.Successful; } - private ArgumentConversionResult(Argument argument) + private ArgumentConversionResult(ArgumentResult argumentResult) { - Argument = argument ?? throw new ArgumentNullException(nameof(argument)); + ArgumentResult = argumentResult ?? throw new ArgumentNullException(nameof(argumentResult)); Result = ArgumentConversionResultType.NoArgument; } internal ArgumentConversionResult( - Argument argument, + ArgumentResult argumentResult, Type expectedType, - string value, - LocalizationResources localizationResources) : - this(argument, FormatErrorMessage(argument, expectedType, value, localizationResources), ArgumentConversionResultType.FailedType) + string value) : + this(argumentResult, FormatErrorMessage(argumentResult, expectedType, value), ArgumentConversionResultType.FailedType) { } - internal static ArgumentConversionResult Failure(Argument argument, string error, ArgumentConversionResultType reason) => new(argument, error, reason); + internal static ArgumentConversionResult Failure(ArgumentResult argumentResult, string error, ArgumentConversionResultType reason) + => new(argumentResult, error, reason); - public static ArgumentConversionResult Success(Argument argument, object? value) => new(argument, value); + public static ArgumentConversionResult Success(ArgumentResult argumentResult, object? value) + => new(argumentResult, value); - internal static ArgumentConversionResult None(Argument argument) => new(argument); + internal static ArgumentConversionResult None(ArgumentResult argumentResult) + => new(argumentResult); private static string FormatErrorMessage( - Argument argument, + ArgumentResult argumentResult, Type expectedType, - string value, - LocalizationResources localizationResources) + string value) { - if (argument.FirstParent?.Symbol is IdentifierSymbol identifierSymbol && - argument.FirstParent.Next is null) + if (argumentResult.Parent is CommandResult commandResult) { - var alias = identifierSymbol.GetLongestAlias(removePrefix: false); + string alias = commandResult.Command.GetLongestAlias(removePrefix: false); + CompletionItem[] completionItems = argumentResult.Argument.GetCompletions(CompletionContext.Empty).ToArray(); - switch (identifierSymbol) + if (completionItems.Length > 0) { - case Command _: - return localizationResources.ArgumentConversionCannotParseForCommand(value, alias, expectedType); - case Option _: - return localizationResources.ArgumentConversionCannotParseForOption(value, alias, expectedType); + return argumentResult.LocalizationResources.ArgumentConversionCannotParseForCommand( + value, alias, expectedType, completionItems.Select(ci => ci.Label)); + } + else + { + return argumentResult.LocalizationResources.ArgumentConversionCannotParseForCommand(value, alias, expectedType); + } + } + else if (argumentResult.Parent is OptionResult optionResult) + { + string alias = optionResult.Option.GetLongestAlias(removePrefix: false); + CompletionItem[] completionItems = optionResult.Option.GetCompletions(CompletionContext.Empty).ToArray(); + + if (completionItems.Length > 0) + { + return argumentResult.LocalizationResources.ArgumentConversionCannotParseForOption( + value, alias, expectedType, completionItems.Select(ci => ci.Label)); + } + else + { + return argumentResult.LocalizationResources.ArgumentConversionCannotParseForOption(value, alias, expectedType); } } - return localizationResources.ArgumentConversionCannotParse(value, expectedType); + // fake ArgumentResults with no Parent + return argumentResult.LocalizationResources.ArgumentConversionCannotParse(value, expectedType); } } } \ No newline at end of file diff --git a/src/System.CommandLine/Binding/ArgumentConverter.cs b/src/System.CommandLine/Binding/ArgumentConverter.cs index bc86831770..18c6e9f89f 100644 --- a/src/System.CommandLine/Binding/ArgumentConverter.cs +++ b/src/System.CommandLine/Binding/ArgumentConverter.cs @@ -11,46 +11,44 @@ namespace System.CommandLine.Binding internal static partial class ArgumentConverter { internal static ArgumentConversionResult ConvertObject( - Argument argument, + ArgumentResult argumentResult, Type type, - object? value, - LocalizationResources localizationResources) + object? value) { switch (value) { case Token singleValue: - return ConvertToken(argument, type, singleValue, localizationResources); + return ConvertToken(argumentResult, type, singleValue); case IReadOnlyList manyValues: - return ConvertTokens(argument, type, manyValues, localizationResources); + return ConvertTokens(argumentResult, type, manyValues); default: - return None(argument); + return None(argumentResult); } } private static ArgumentConversionResult ConvertToken( - Argument argument, + ArgumentResult argumentResult, Type type, - Token token, - LocalizationResources localizationResources) + Token token) { var value = token.Value; if (type.TryGetNullableType(out var nullableType)) { - return ConvertToken(argument, nullableType, token, localizationResources); + return ConvertToken(argumentResult, nullableType, token); } if (_stringConverters.TryGetValue(type, out var tryConvert)) { if (tryConvert(value, out var converted)) { - return Success(argument, converted); + return Success(argumentResult, converted); } else { - return Failure(argument, type, value, localizationResources); + return Failure(argumentResult, type, value); } } @@ -58,7 +56,7 @@ private static ArgumentConversionResult ConvertToken( { try { - return Success(argument, Enum.Parse(type, value, true)); + return Success(argumentResult, Enum.Parse(type, value, true)); } catch (ArgumentException) { @@ -66,15 +64,13 @@ private static ArgumentConversionResult ConvertToken( } } - return Failure(argument, type, value, localizationResources); + return Failure(argumentResult, type, value); } private static ArgumentConversionResult ConvertTokens( - Argument argument, + ArgumentResult argumentResult, Type type, - IReadOnlyList tokens, - LocalizationResources localizationResources, - ArgumentResult? argumentResult = null) + IReadOnlyList tokens) { var itemType = type.GetElementTypeIfEnumerable() ?? typeof(string); var values = CreateEnumerable(type, itemType, tokens.Count); @@ -84,7 +80,7 @@ private static ArgumentConversionResult ConvertTokens( { var token = tokens[i]; - var result = ConvertToken(argument, itemType, token, localizationResources); + var result = ConvertToken(argumentResult, itemType, token); switch (result.Result) { @@ -101,7 +97,7 @@ private static ArgumentConversionResult ConvertTokens( break; default: // failures - if (argumentResult is { Parent: CommandResult }) + if (argumentResult.Parent is CommandResult) { argumentResult.OnlyTake(i); @@ -113,7 +109,7 @@ private static ArgumentConversionResult ConvertTokens( } } - return Success(argument, values); + return Success(argumentResult, values); } internal static TryConvertArgument? GetConverter(Argument argument) @@ -168,34 +164,33 @@ private static bool CanBeBoundFromScalarValue(this Type type) } private static ArgumentConversionResult Failure( - Argument argument, + ArgumentResult argumentResult, Type expectedType, - string value, - LocalizationResources localizationResources) + string value) { - return new ArgumentConversionResult(argument, expectedType, value, localizationResources); + return new ArgumentConversionResult(argumentResult, expectedType, value); } internal static ArgumentConversionResult ConvertIfNeeded( this ArgumentConversionResult conversionResult, - SymbolResult symbolResult, Type toType) { return conversionResult.Result switch { ArgumentConversionResultType.Successful when !toType.IsInstanceOfType(conversionResult.Value) => - ConvertObject(conversionResult.Argument, + ConvertObject(conversionResult.ArgumentResult, toType, - conversionResult.Value, - symbolResult.LocalizationResources), + conversionResult.Value), - ArgumentConversionResultType.NoArgument when conversionResult.Argument.ValueType == typeof(bool) || conversionResult.Argument.ValueType == typeof(bool?) => - Success(conversionResult.Argument, true), + ArgumentConversionResultType.NoArgument when conversionResult.ArgumentResult.Argument.ValueType == typeof(bool) => + Success(conversionResult.ArgumentResult, true), + ArgumentConversionResultType.NoArgument when conversionResult.ArgumentResult.Argument.ValueType == typeof(bool?) => + Success(conversionResult.ArgumentResult, true), - ArgumentConversionResultType.NoArgument when conversionResult.Argument.Arity.MinimumNumberOfValues > 0 => + ArgumentConversionResultType.NoArgument when conversionResult.ArgumentResult.Argument.Arity.MinimumNumberOfValues > 0 => ArgumentConversionResult.Failure( - conversionResult.Argument, - symbolResult.LocalizationResources.RequiredArgumentMissing(symbolResult), + conversionResult.ArgumentResult, + conversionResult.ArgumentResult.LocalizationResources.RequiredArgumentMissing(conversionResult.ArgumentResult.Parent!), ArgumentConversionResultType.FailedMissingArgument), _ => conversionResult @@ -219,18 +214,15 @@ public static bool TryConvertArgument(ArgumentResult argumentResult, out object? ArgumentConversionResult result = argument.Arity.MaximumNumberOfValues switch { // 0 is an implicit bool, i.e. a "flag" - 0 => Success(argumentResult.Argument, true), - 1 => ConvertObject(argument, + 0 => Success(argumentResult, true), + 1 => ConvertObject(argumentResult, argument.ValueType, argumentResult.Tokens.Count > 0 ? argumentResult.Tokens[argumentResult.Tokens.Count - 1] - : null, - argumentResult.LocalizationResources), - _ => ConvertTokens(argument, + : null), + _ => ConvertTokens(argumentResult, argument.ValueType, - argumentResult.Tokens, - argumentResult.LocalizationResources, - argumentResult) + argumentResult.Tokens) }; value = result; diff --git a/src/System.CommandLine/Binding/BindingContext.cs b/src/System.CommandLine/Binding/BindingContext.cs index 13d3bd6e4b..435decc4f2 100644 --- a/src/System.CommandLine/Binding/BindingContext.cs +++ b/src/System.CommandLine/Binding/BindingContext.cs @@ -3,6 +3,7 @@ using System.CommandLine.Help; using System.CommandLine.Invocation; +using System.CommandLine.Parsing; using System.Diagnostics.CodeAnalysis; using System.Linq; @@ -84,7 +85,7 @@ internal bool TryGetValueSource( internal bool TryBindToScalarValue( IValueDescriptor valueDescriptor, IValueSource valueSource, - LocalizationResources localizationResources, + ParseResult parseResult, out BoundValue? boundValue) { if (valueSource.TryGetValue(valueDescriptor, this, out var value)) @@ -96,11 +97,16 @@ internal bool TryBindToScalarValue( } else { + ArgumentResult argumentResult = valueDescriptor is Argument argument + ? parseResult.FindResultFor(argument) is ArgumentResult found + ? found + : new ArgumentResult(argument, parseResult.RootCommandResult.SymbolResultTree, null) + : new ArgumentResult(new Argument(valueDescriptor.ValueName), parseResult.RootCommandResult.SymbolResultTree, null); + var parsed = ArgumentConverter.ConvertObject( - valueDescriptor as Argument ?? new Argument(valueDescriptor.ValueName), + argumentResult, valueDescriptor.ValueType, - value, - localizationResources); + value); if (parsed.Result == ArgumentConversionResultType.Successful) { diff --git a/src/System.CommandLine/LocalizationResources.cs b/src/System.CommandLine/LocalizationResources.cs index 45cc33b8de..ce1c832e31 100644 --- a/src/System.CommandLine/LocalizationResources.cs +++ b/src/System.CommandLine/LocalizationResources.cs @@ -242,12 +242,26 @@ public virtual string ArgumentConversionCannotParse(string value, Type expectedT public virtual string ArgumentConversionCannotParseForCommand(string value, string commandAlias, Type expectedType) => GetResourceString(Properties.Resources.ArgumentConversionCannotParseForCommand, value, commandAlias, expectedType); + /// + /// Interpolates values into a localized string similar to Cannot parse argument '{0}' for command '{1}' as expected type {2}.. + /// + public virtual string ArgumentConversionCannotParseForCommand(string value, string commandAlias, Type expectedType, IEnumerable completions) + => GetResourceString(Properties.Resources.ArgumentConversionCannotParseForCommand_Completions, + value, commandAlias, expectedType, Environment.NewLine + string.Join(Environment.NewLine, completions)); + /// /// Interpolates values into a localized string similar to Cannot parse argument '{0}' for option '{1}' as expected type {2}.. /// public virtual string ArgumentConversionCannotParseForOption(string value, string optionAlias, Type expectedType) => GetResourceString(Properties.Resources.ArgumentConversionCannotParseForOption, value, optionAlias, expectedType); + /// + /// Interpolates values into a localized string similar to Cannot parse argument '{0}' for option '{1}' as expected type {2}.. + /// + public virtual string ArgumentConversionCannotParseForOption(string value, string optionAlias, Type expectedType, IEnumerable completions) + => GetResourceString(Properties.Resources.ArgumentConversionCannotParseForOption_Completions, + value, optionAlias, expectedType, Environment.NewLine + string.Join(Environment.NewLine, completions)); + /// /// Interpolates values into a localized string. /// diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index a32aa1a6a3..e59dfdc41f 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -34,7 +34,7 @@ internal ArgumentResult( internal IReadOnlyList? PassedOnTokens { get; private set; } internal ArgumentConversionResult GetArgumentConversionResult() => - _conversionResult ??= Convert(Argument); + _conversionResult ??= Convert(); /// public object? GetValueOrDefault() => @@ -46,7 +46,7 @@ internal ArgumentConversionResult GetArgumentConversionResult() => /// The parsed value or the default value for public T GetValueOrDefault() => GetArgumentConversionResult() - .ConvertIfNeeded(this, typeof(T)) + .ConvertIfNeeded(typeof(T)) .GetValueOrDefault(); /// @@ -80,74 +80,66 @@ public void OnlyTake(int numberOfTokens) /// public override string ToString() => $"{GetType().Name} {Argument.Name}: {string.Join(" ", Tokens.Select(t => $"<{t.Value}>"))}"; - private ArgumentConversionResult Convert(Argument argument) + /// + public override void ReportError(string errorMessage) { - if (ShouldCheckArity() && - Parent is { } && - ArgumentArity.Validate( - Parent, - argument, - argument.Arity.MinimumNumberOfValues, - argument.Arity.MaximumNumberOfValues) is { } failed) // returns null on success + SymbolResultTree.ReportError(new ParseError(errorMessage, Parent is OptionResult option ? option : this)); + _conversionResult = ArgumentConversionResult.Failure(this, errorMessage, ArgumentConversionResultType.Failed); + } + + private ArgumentConversionResult Convert() + { + if (Parent is not null && + Parent is not OptionResult { IsImplicit: true } && + ArgumentArity.Validate(this) is { } failed) // returns null on success { return failed; } - if (Parent!.UseDefaultValueFor(argument)) + if (Parent!.UseDefaultValueFor(Argument)) { - var argumentResult = new ArgumentResult(argument, SymbolResultTree, Parent); - - var defaultValue = argument.GetDefaultValue(argumentResult); + var defaultValue = Argument.GetDefaultValue(this); - if (!SymbolResultTree.HasError(argumentResult, out ParseError? error)) - { - return ArgumentConversionResult.Success( - argument, - defaultValue); - } - else - { - return ArgumentConversionResult.Failure( - argument, - error.Message, - ArgumentConversionResultType.Failed); - } + // default value factory provided by the user might report an error, which sets _conversionResult + return _conversionResult ?? ArgumentConversionResult.Success(this, defaultValue); } - if (argument.ConvertArguments is null) + if (Argument.ConvertArguments is null) { - return argument.Arity.MaximumNumberOfValues switch + return Argument.Arity.MaximumNumberOfValues switch { - 1 => ArgumentConversionResult.Success(argument, Tokens.SingleOrDefault()), - _ => ArgumentConversionResult.Success(argument, Tokens) + 1 => ArgumentConversionResult.Success(this, Tokens.SingleOrDefault()), + _ => ArgumentConversionResult.Success(this, Tokens) }; } - var success = argument.ConvertArguments(this, out var value); + var success = Argument.ConvertArguments(this, out var value); - if (value is ArgumentConversionResult conversionResult) + // default value factory provided by the user might report an error, which sets _conversionResult + if (_conversionResult is not null) { - return conversionResult; + return _conversionResult; } - if (success) + if (value is ArgumentConversionResult conversionResult) { - return ArgumentConversionResult.Success(argument, value); + if (conversionResult.Result >= ArgumentConversionResultType.Failed) + { + ReportError(conversionResult.ErrorMessage!); + } + + return conversionResult; } - if (SymbolResultTree.HasError(this, out ParseError? e)) + if (success) { - return ArgumentConversionResult.Failure(argument, e.Message, ArgumentConversionResultType.Failed); + return ArgumentConversionResult.Success(this, value); } - return new ArgumentConversionResult( - argument, - argument.ValueType, - Tokens[0].Value, - LocalizationResources); + ArgumentConversionResult failure = new ArgumentConversionResult(this, Argument.ValueType, Tokens[0].Value); + ReportError(failure.ErrorMessage!); - bool ShouldCheckArity() => - Parent is not OptionResult { IsImplicit: true }; + return failure; } } } diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index acbe7db56d..9b1c0108aa 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -54,7 +54,7 @@ internal OptionResult( /// The parsed value or the default value for [return: MaybeNull] public T GetValueOrDefault() => - this.ConvertIfNeeded(typeof(T)) + ArgumentConversionResult.ConvertIfNeeded(typeof(T)) .GetValueOrDefault(); private protected override int RemainingArgumentCapacity @@ -83,7 +83,7 @@ internal ArgumentConversionResult ArgumentConversionResult return _argumentConversionResult = firstChild.GetArgumentConversionResult(); } - return _argumentConversionResult = ArgumentConversionResult.None(Option.Argument); + return _argumentConversionResult = ArgumentConversionResult.None(new ArgumentResult(Option.Argument, SymbolResultTree, this)); } return _argumentConversionResult; diff --git a/src/System.CommandLine/Parsing/OptionResultExtensions.cs b/src/System.CommandLine/Parsing/OptionResultExtensions.cs deleted file mode 100644 index 2ab289a32f..0000000000 --- a/src/System.CommandLine/Parsing/OptionResultExtensions.cs +++ /dev/null @@ -1,16 +0,0 @@ -// 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.Binding; - -namespace System.CommandLine.Parsing -{ - internal static class OptionResultExtensions - { - internal static ArgumentConversionResult ConvertIfNeeded( - this OptionResult optionResult, - Type type) => - optionResult.ArgumentConversionResult - .ConvertIfNeeded(optionResult, type); - } -} \ No newline at end of file diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index 33ff8b0872..df9e6ca488 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -372,13 +372,11 @@ private void ValidateArguments(IList arguments, CommandResult innermos { for (var i = 0; i < arguments.Count; i++) { - var symbol = arguments[i]; + ArgumentResult argumentResult = _symbolResultTree.TryGetValue(arguments[i], out SymbolResult? symbolResult) + ? (ArgumentResult)symbolResult + : new ArgumentResult(arguments[i], _symbolResultTree, innermostCommandResult); - var arityFailure = ArgumentArity.Validate( - innermostCommandResult, - symbol, - symbol.Arity.MinimumNumberOfValues, - symbol.Arity.MaximumNumberOfValues); + ArgumentConversionResult? arityFailure = ArgumentArity.Validate(argumentResult); if (arityFailure is not null) { @@ -410,12 +408,11 @@ private void ValidateAndConvertOptionResult(OptionResult optionResult) { var argument = optionResult.Option.Argument; - var arityFailure = ArgumentArity.Validate( - optionResult, - argument, - argument.Arity.MinimumNumberOfValues, - argument.Arity.MaximumNumberOfValues); + ArgumentResult argumentResult = _symbolResultTree.TryGetValue(argument, out SymbolResult? symbolResult) + ? (ArgumentResult)symbolResult + : new ArgumentResult(argument, _symbolResultTree, optionResult); + ArgumentConversionResult? arityFailure = ArgumentArity.Validate(argumentResult); if (arityFailure is not null) { optionResult.ReportError(arityFailure.ErrorMessage!); @@ -464,26 +461,7 @@ private void ValidateAndConvertArgumentResult(ArgumentResult argumentResult) } } - var argumentConversionResult = argumentResult.GetArgumentConversionResult(); - - if (argumentConversionResult.Result >= ArgumentConversionResultType.Failed && - argumentConversionResult.Result != ArgumentConversionResultType.FailedArity) - { - if (argument.FirstParent?.Symbol is Option option) - { - var completions = option.GetCompletions(CompletionContext.Empty).ToArray(); - - if (completions.Length > 0) - { - argumentConversionResult = ArgumentConversionResult.Failure( - argumentConversionResult.Argument, - argumentConversionResult.ErrorMessage + " Did you mean one of the following?" + Environment.NewLine + string.Join(Environment.NewLine, completions.Select(c => c.Label)), - argumentConversionResult.Result); - } - } - - argumentResult.ReportError(argumentConversionResult.ErrorMessage!); - } + _ = argumentResult.GetArgumentConversionResult(); } private void PopulateDefaultValues() diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index 1acd428ac1..a14b1b05e9 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -49,8 +49,7 @@ private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? /// Adds an error message for this symbol result to it's parse tree. /// /// Setting an error will cause the parser to indicate an error for the user and prevent invocation of the command line. - public void ReportError(string errorMessage) - => SymbolResultTree.ReportError(new ParseError(errorMessage, Parent is OptionResult option ? option : this)); + public virtual void ReportError(string errorMessage) => SymbolResultTree.ReportError(new ParseError(errorMessage, this)); /// /// Finds a result for the specific argument anywhere in the parse tree, including parent and child symbol results. diff --git a/src/System.CommandLine/Properties/Resources.Designer.cs b/src/System.CommandLine/Properties/Resources.Designer.cs index ddb11c322f..c781eb9f85 100644 --- a/src/System.CommandLine/Properties/Resources.Designer.cs +++ b/src/System.CommandLine/Properties/Resources.Designer.cs @@ -78,6 +78,15 @@ internal static string ArgumentConversionCannotParseForCommand { } } + /// + /// Looks up a localized string similar to Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3}. + /// + internal static string ArgumentConversionCannotParseForCommand_Completions { + get { + return ResourceManager.GetString("ArgumentConversionCannotParseForCommand_Completions", resourceCulture); + } + } + /// /// Looks up a localized string similar to Cannot parse argument '{0}' for option '{1}' as expected type '{2}'.. /// @@ -87,6 +96,15 @@ internal static string ArgumentConversionCannotParseForOption { } } + /// + /// Looks up a localized string similar to Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3}. + /// + internal static string ArgumentConversionCannotParseForOption_Completions { + get { + return ResourceManager.GetString("ArgumentConversionCannotParseForOption_Completions", resourceCulture); + } + } + /// /// Looks up a localized string similar to Command '{0}' expects no more than {1} arguments, but {2} were provided.. /// diff --git a/src/System.CommandLine/Properties/Resources.resx b/src/System.CommandLine/Properties/Resources.resx index f0b101ebba..a5e32aa8dc 100644 --- a/src/System.CommandLine/Properties/Resources.resx +++ b/src/System.CommandLine/Properties/Resources.resx @@ -234,4 +234,10 @@ Option '{0}' is required. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + \ No newline at end of file diff --git a/src/System.CommandLine/Properties/xlf/Resources.cs.xlf b/src/System.CommandLine/Properties/xlf/Resources.cs.xlf index 86c81955a1..ef2f1568d8 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.cs.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.cs.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.de.xlf b/src/System.CommandLine/Properties/xlf/Resources.de.xlf index e47d4bc1ee..6b69459ae5 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.de.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.de.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.es.xlf b/src/System.CommandLine/Properties/xlf/Resources.es.xlf index 2e85b00d2f..8a346c3ed8 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.es.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.es.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.fr.xlf b/src/System.CommandLine/Properties/xlf/Resources.fr.xlf index e8afe2a034..2d611ac68f 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.fr.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.fr.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.it.xlf b/src/System.CommandLine/Properties/xlf/Resources.it.xlf index 040135ce41..146e7b4814 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.it.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.it.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.ja.xlf b/src/System.CommandLine/Properties/xlf/Resources.ja.xlf index aa9c63dce5..0f23a46804 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.ja.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.ja.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.ko.xlf b/src/System.CommandLine/Properties/xlf/Resources.ko.xlf index f5a4950dca..60c9971927 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.ko.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.ko.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.pl.xlf b/src/System.CommandLine/Properties/xlf/Resources.pl.xlf index b5cf7fd3f0..4d74816ac1 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.pl.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.pl.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.pt-BR.xlf b/src/System.CommandLine/Properties/xlf/Resources.pt-BR.xlf index 8f7e7af0d2..c1d25a631a 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.pt-BR.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.pt-BR.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.ru.xlf b/src/System.CommandLine/Properties/xlf/Resources.ru.xlf index 6a8b8128f5..2525b84023 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.ru.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.ru.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.tr.xlf b/src/System.CommandLine/Properties/xlf/Resources.tr.xlf index c43d076b4d..3835a328f0 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.tr.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.tr.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.zh-Hans.xlf b/src/System.CommandLine/Properties/xlf/Resources.zh-Hans.xlf index ca955abebc..0b5a526be0 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.zh-Hans.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.zh-Hans.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. diff --git a/src/System.CommandLine/Properties/xlf/Resources.zh-Hant.xlf b/src/System.CommandLine/Properties/xlf/Resources.zh-Hant.xlf index 3ceabfdb3d..e52a13023e 100644 --- a/src/System.CommandLine/Properties/xlf/Resources.zh-Hant.xlf +++ b/src/System.CommandLine/Properties/xlf/Resources.zh-Hant.xlf @@ -12,11 +12,21 @@ Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for command '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. + + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3} + + Command '{0}' expects no more than {1} arguments, but {2} were provided. Command '{0}' expects no more than {1} arguments, but {2} were provided. From 2ef226f723575a7b3d550d4312bb3cd6d12e5aa1 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 24 Jan 2023 19:23:26 +0100 Subject: [PATCH 03/15] remove unused method --- .../Parsing/SymbolResultTree.cs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/System.CommandLine/Parsing/SymbolResultTree.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs index 110a707c3e..8fc3823e3f 100644 --- a/src/System.CommandLine/Parsing/SymbolResultTree.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -53,25 +53,6 @@ internal IEnumerable GetChildren(SymbolResult parent) } } - internal bool HasError(ArgumentResult argumentResult, [NotNullWhen(true)] out ParseError? error) - { - if (Errors is not null) - { - SymbolResult symbolResult = argumentResult.Parent is OptionResult optionResult ? optionResult : argumentResult; - for (int i = 0; i < Errors.Count; i++) - { - if (ReferenceEquals(Errors[i].SymbolResult, symbolResult)) - { - error = Errors[i]; - return true; - } - } - } - - error = null; - return false; - } - internal void ReportError(ParseError parseError) => (Errors ??= new()).Add(parseError); internal void InsertError(int index, ParseError parseError) => (Errors ??= new()).Insert(index, parseError); From d8f92ef7f6eca29cff8cf84353c41368dd6e6cf8 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 24 Jan 2023 19:33:35 +0100 Subject: [PATCH 04/15] last tweaks --- .../Parsing/ArgumentResult.cs | 22 ++++++++++--------- .../Parsing/SymbolResultTree.cs | 1 - 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index e59dfdc41f..bd21b76a31 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -93,7 +93,7 @@ private ArgumentConversionResult Convert() Parent is not OptionResult { IsImplicit: true } && ArgumentArity.Validate(this) is { } failed) // returns null on success { - return failed; + return ReportErrorIfNeeded(failed); } if (Parent!.UseDefaultValueFor(Argument)) @@ -123,12 +123,7 @@ private ArgumentConversionResult Convert() if (value is ArgumentConversionResult conversionResult) { - if (conversionResult.Result >= ArgumentConversionResultType.Failed) - { - ReportError(conversionResult.ErrorMessage!); - } - - return conversionResult; + return ReportErrorIfNeeded(conversionResult); } if (success) @@ -136,10 +131,17 @@ private ArgumentConversionResult Convert() return ArgumentConversionResult.Success(this, value); } - ArgumentConversionResult failure = new ArgumentConversionResult(this, Argument.ValueType, Tokens[0].Value); - ReportError(failure.ErrorMessage!); + return ReportErrorIfNeeded(new ArgumentConversionResult(this, Argument.ValueType, Tokens[0].Value)); + + ArgumentConversionResult ReportErrorIfNeeded(ArgumentConversionResult result) + { + if (result.Result >= ArgumentConversionResultType.Failed) + { + SymbolResultTree.ReportError(new ParseError(result.ErrorMessage!, Parent is OptionResult option ? option : this)); + } - return failure; + return result; + } } } } diff --git a/src/System.CommandLine/Parsing/SymbolResultTree.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs index 8fc3823e3f..f251be7f35 100644 --- a/src/System.CommandLine/Parsing/SymbolResultTree.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -2,7 +2,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; namespace System.CommandLine.Parsing { From f8370ae7cbdff89cd4ac51c6f56fe8d411b0dfaa Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 25 Jan 2023 09:08:48 +0100 Subject: [PATCH 05/15] address code review feedback: * rename ReportError to AddError * test coverage improvement (multiple errors for one result) --- ...ommandLine_api_is_not_changed.approved.txt | 4 +- src/System.CommandLine.Tests/ArgumentTests.cs | 14 +++---- src/System.CommandLine.Tests/OptionTests.cs | 2 +- .../ParsingValidationTests.cs | 39 +++++++++++-------- src/System.CommandLine/Argument{T}.cs | 6 +-- src/System.CommandLine/Help/VersionOption.cs | 2 +- .../Parsing/ArgumentResult.cs | 2 +- .../Parsing/ParseResultVisitor.cs | 6 +-- .../Parsing/SymbolResult.cs | 2 +- src/System.CommandLine/Validate.cs | 6 +-- 10 files changed, 44 insertions(+), 39 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 3ac3f73bbb..46238e35f8 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 @@ -388,10 +388,10 @@ System.CommandLine.IO System.CommandLine.Parsing public class ArgumentResult : SymbolResult public System.CommandLine.Argument Argument { get; } + public System.Void AddError(System.String errorMessage) public System.Object GetValueOrDefault() public T GetValueOrDefault() public System.Void OnlyTake(System.Int32 numberOfTokens) - public System.Void ReportError(System.String errorMessage) public System.String ToString() public class CommandLineStringSplitter public System.Collections.Generic.IEnumerable Split(System.String commandLine) @@ -429,6 +429,7 @@ System.CommandLine.Parsing public System.CommandLine.LocalizationResources LocalizationResources { get; } public SymbolResult Parent { get; } public System.Collections.Generic.IReadOnlyList Tokens { get; } + public System.Void AddError(System.String errorMessage) public ArgumentResult FindResultFor(System.CommandLine.Argument argument) public CommandResult FindResultFor(System.CommandLine.Command command) public OptionResult FindResultFor(System.CommandLine.Option option) @@ -436,7 +437,6 @@ System.CommandLine.Parsing public System.Object GetValue(System.CommandLine.Argument argument) public T GetValue(Option option) public System.Object GetValue(System.CommandLine.Option option) - public System.Void ReportError(System.String errorMessage) public System.String ToString() public class Token, System.IEquatable public static System.Boolean op_Equality(Token left, Token right) diff --git a/src/System.CommandLine.Tests/ArgumentTests.cs b/src/System.CommandLine.Tests/ArgumentTests.cs index 242082659e..a32a0afa71 100644 --- a/src/System.CommandLine.Tests/ArgumentTests.cs +++ b/src/System.CommandLine.Tests/ArgumentTests.cs @@ -124,7 +124,7 @@ public void Validation_failure_message_can_be_specified_when_parsing_tokens() { var argument = new Argument(result => { - result.ReportError("oops!"); + result.AddError("oops!"); return null; }); @@ -143,7 +143,7 @@ public void Validation_failure_message_can_be_specified_when_evaluating_default_ { var argument = new Argument(result => { - result.ReportError("oops!"); + result.AddError("oops!"); return null; }, true); @@ -164,7 +164,7 @@ public void Validation_failure_message_can_be_specified_when_evaluating_default_ "-x", result => { - result.ReportError("oops!"); + result.AddError("oops!"); return null; }, true); @@ -389,7 +389,7 @@ public void Multiple_command_arguments_can_have_custom_parse_delegates() { new Argument("from", argumentResult => { - argumentResult.ReportError("nope"); + argumentResult.AddError("nope"); return null; }, true) { @@ -397,7 +397,7 @@ public void Multiple_command_arguments_can_have_custom_parse_delegates() }, new Argument("to", argumentResult => { - argumentResult.ReportError("UH UH"); + argumentResult.AddError("UH UH"); return null; }, true) { @@ -426,7 +426,7 @@ public void When_custom_conversion_fails_then_an_option_does_not_accept_further_ new Argument(), new Option("-x", argResult => { - argResult.ReportError("nope"); + argResult.AddError("nope"); return default; }) }; @@ -446,7 +446,7 @@ public void When_argument_cannot_be_parsed_as_the_specified_type_then_getting_va return value; } - argumentResult.ReportError($"'{argumentResult.Tokens.Single().Value}' is not an integer"); + argumentResult.AddError($"'{argumentResult.Tokens.Single().Value}' is not an integer"); return default; }); diff --git a/src/System.CommandLine.Tests/OptionTests.cs b/src/System.CommandLine.Tests/OptionTests.cs index 957231f319..f87e74c5c0 100644 --- a/src/System.CommandLine.Tests/OptionTests.cs +++ b/src/System.CommandLine.Tests/OptionTests.cs @@ -287,7 +287,7 @@ public void Option_T_default_value_is_validated() { var option = new Option("-x", () => 123); option.Validators.Add(symbol => - symbol.ReportError(symbol.Tokens + symbol.AddError(symbol.Tokens .Select(t => t.Value) .Where(v => v == "123") .Select(_ => "ERR") diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index a91406cb84..4fd384b3bf 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -192,12 +192,17 @@ public void When_FromAmong_is_used_and_multiple_invalid_inputs_are_provided_the_ var result = command.Parse("list --columns c1 c2"); - // Currently there is no possibility for a single validator to produce multiple errors, - // so only the first one is checked. + result.Errors.Count.Should().Be(2); + result.Errors[0] - .Message - .Should() - .Be(LocalizationResources.Instance.UnrecognizedArgument("c1", new[] { "author", "language", "tags", "type" })); + .Message + .Should() + .Be(LocalizationResources.Instance.UnrecognizedArgument("c1", new[] { "author", "language", "tags", "type" })); + + result.Errors[1] + .Message + .Should() + .Be(LocalizationResources.Instance.UnrecognizedArgument("c2", new[] { "author", "language", "tags", "type" })); } [Fact] @@ -341,7 +346,7 @@ public void A_custom_validator_can_be_added_to_a_command() if (commandResult.Children.Any(sr => ((OptionResult)sr).Option.HasAlias("--one")) && commandResult.Children.Any(sr => ((OptionResult)sr).Option.HasAlias("--two"))) { - commandResult.ReportError("Options '--one' and '--two' cannot be used together."); + commandResult.AddError("Options '--one' and '--two' cannot be used together."); } }); @@ -365,7 +370,7 @@ public void A_custom_validator_can_be_added_to_an_option() { var value = r.GetValueOrDefault(); - r.ReportError($"Option {r.Token.Value} cannot be set to {value}"); + r.AddError($"Option {r.Token.Value} cannot be set to {value}"); }); var command = new RootCommand { option }; @@ -392,7 +397,7 @@ public void A_custom_validator_can_be_added_to_an_argument() { var value = r.GetValueOrDefault(); - r.ReportError($"Argument {r.Argument.Name} cannot be set to {value}"); + r.AddError($"Argument {r.Argument.Name} cannot be set to {value}"); }); var command = new RootCommand { argument }; @@ -456,7 +461,7 @@ public void Validators_on_global_options_are_executed_when_invoking_a_subcommand var option = new Option("--file"); option.Validators.Add(r => { - r.ReportError("Invoked validator"); + r.AddError("Invoked validator"); }); var subCommand = new Command("subcommand"); @@ -491,7 +496,7 @@ public async Task A_custom_validator_added_to_a_global_option_is_checked(string var handlerWasCalled = false; var globalOption = new Option("--value"); - globalOption.Validators.Add(r => r.ReportError("oops!")); + globalOption.Validators.Add(r => r.AddError("oops!")); var grandchildCommand = new Command("grandchild"); @@ -521,7 +526,7 @@ public void Custom_validator_error_messages_are_not_repeated() { var errorMessage = "that's not right..."; var argument = new Argument(); - argument.Validators.Add(r => r.ReportError(errorMessage)); + argument.Validators.Add(r => r.AddError(errorMessage)); var cmd = new Command("get") { @@ -548,7 +553,7 @@ public void The_parsed_value_of_an_argument_is_available_within_a_validator() if (value < 0 || value > 100) { - result.ReportError(errorMessage); + result.AddError(errorMessage); } }); @@ -572,7 +577,7 @@ public void The_parsed_value_of_an_option_is_available_within_a_validator() if (value < 0 || value > 100) { - result.ReportError(errorMessage); + result.AddError(errorMessage); } }); @@ -1203,7 +1208,7 @@ public void Arity_failures_are_not_reported_for_both_an_argument_and_its_parent_ public void Multiple_validators_on_the_same_command_do_not_report_duplicate_errors() { var command = new RootCommand(); - command.Validators.Add(result => result.ReportError("Wrong")); + command.Validators.Add(result => result.AddError("Wrong")); command.Validators.Add(_ => { }); var parseResult = command.Parse(""); @@ -1221,7 +1226,7 @@ public void Multiple_validators_on_the_same_command_do_not_report_duplicate_erro public void Multiple_validators_on_the_same_option_do_not_report_duplicate_errors() { var option = new Option("-x"); - option.Validators.Add(result => result.ReportError("Wrong")); + option.Validators.Add(result => result.AddError("Wrong")); option.Validators.Add(_ => { }); var command = new RootCommand @@ -1244,7 +1249,7 @@ public void Multiple_validators_on_the_same_option_do_not_report_duplicate_error public void Multiple_validators_on_the_same_argument_do_not_report_duplicate_errors() { var argument = new Argument(); - argument.Validators.Add(result => result.ReportError("Wrong")); + argument.Validators.Add(result => result.AddError("Wrong")); argument.Validators.Add(_ => { }); var command = new RootCommand @@ -1269,7 +1274,7 @@ internal void When_there_is_an_arity_error_then_further_errors_are_not_reported( var option = new Option("-o"); option.Validators.Add(result => { - result.ReportError("OOPS"); + result.AddError("OOPS"); }); //all good; var command = new Command("comm") diff --git a/src/System.CommandLine/Argument{T}.cs b/src/System.CommandLine/Argument{T}.cs index 89e32b96d0..ce51ca4156 100644 --- a/src/System.CommandLine/Argument{T}.cs +++ b/src/System.CommandLine/Argument{T}.cs @@ -195,7 +195,7 @@ void UnrecognizedArgumentError(ArgumentResult argumentResult) { if (Array.IndexOf(values, token.Value) < 0) { - argumentResult.ReportError(argumentResult.LocalizationResources.UnrecognizedArgument(token.Value, values)); + argumentResult.AddError(argumentResult.LocalizationResources.UnrecognizedArgument(token.Value, values)); } } } @@ -221,7 +221,7 @@ public void AcceptLegalFilePathsOnly() if (invalidCharactersIndex >= 0) { - result.ReportError(result.LocalizationResources.InvalidCharactersInPath(token.Value[invalidCharactersIndex])); + result.AddError(result.LocalizationResources.InvalidCharactersInPath(token.Value[invalidCharactersIndex])); } } }); @@ -244,7 +244,7 @@ public void AcceptLegalFileNamesOnly() if (invalidCharactersIndex >= 0) { - result.ReportError(result.LocalizationResources.InvalidCharactersInFileName(token.Value[invalidCharactersIndex])); + result.AddError(result.LocalizationResources.InvalidCharactersInFileName(token.Value[invalidCharactersIndex])); } } }); diff --git a/src/System.CommandLine/Help/VersionOption.cs b/src/System.CommandLine/Help/VersionOption.cs index 1aba232647..c2238c627b 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.ReportError(result.LocalizationResources.VersionOptionCannotBeCombinedWithOtherArguments(result.Token?.Value ?? result.Option.Name)); + result.AddError(result.LocalizationResources.VersionOptionCannotBeCombinedWithOtherArguments(result.Token?.Value ?? result.Option.Name)); } }); } diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index bd21b76a31..c2355b2ad5 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -81,7 +81,7 @@ public void OnlyTake(int numberOfTokens) public override string ToString() => $"{GetType().Name} {Argument.Name}: {string.Join(" ", Tokens.Select(t => $"<{t.Value}>"))}"; /// - public override void ReportError(string errorMessage) + public override void AddError(string errorMessage) { SymbolResultTree.ReportError(new ParseError(errorMessage, Parent is OptionResult option ? option : this)); _conversionResult = ArgumentConversionResult.Failure(this, errorMessage, ArgumentConversionResultType.Failed); diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index df9e6ca488..eaee5e7fa7 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -337,7 +337,7 @@ private void ValidateCommandResult() { if (_rootCommandResult.FindResultFor(option) is null) { - _innermostCommandResult.ReportError(_rootCommandResult.LocalizationResources.RequiredOptionWasNotProvided(option)); + _innermostCommandResult.AddError(_rootCommandResult.LocalizationResources.RequiredOptionWasNotProvided(option)); } } } @@ -380,7 +380,7 @@ private void ValidateArguments(IList arguments, CommandResult innermos if (arityFailure is not null) { - innermostCommandResult.ReportError(arityFailure.ErrorMessage!); + innermostCommandResult.AddError(arityFailure.ErrorMessage!); } } } @@ -415,7 +415,7 @@ private void ValidateAndConvertOptionResult(OptionResult optionResult) ArgumentConversionResult? arityFailure = ArgumentArity.Validate(argumentResult); if (arityFailure is not null) { - optionResult.ReportError(arityFailure.ErrorMessage!); + optionResult.AddError(arityFailure.ErrorMessage!); return; } diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index a14b1b05e9..d706fb38b0 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -49,7 +49,7 @@ private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? /// Adds an error message for this symbol result to it's parse tree. /// /// Setting an error will cause the parser to indicate an error for the user and prevent invocation of the command line. - public virtual void ReportError(string errorMessage) => SymbolResultTree.ReportError(new ParseError(errorMessage, this)); + public virtual void AddError(string errorMessage) => SymbolResultTree.ReportError(new ParseError(errorMessage, this)); /// /// Finds a result for the specific argument anywhere in the parse tree, including parent and child symbol results. diff --git a/src/System.CommandLine/Validate.cs b/src/System.CommandLine/Validate.cs index 591575e786..0b339174ed 100644 --- a/src/System.CommandLine/Validate.cs +++ b/src/System.CommandLine/Validate.cs @@ -13,7 +13,7 @@ internal static void FileExists(ArgumentResult result) if (!File.Exists(token.Value)) { - result.ReportError(result.LocalizationResources.FileDoesNotExist(token.Value)); + result.AddError(result.LocalizationResources.FileDoesNotExist(token.Value)); } } } @@ -26,7 +26,7 @@ internal static void DirectoryExists(ArgumentResult result) if (!Directory.Exists(token.Value)) { - result.ReportError(result.LocalizationResources.DirectoryDoesNotExist(token.Value)); + result.AddError(result.LocalizationResources.DirectoryDoesNotExist(token.Value)); } } } @@ -39,7 +39,7 @@ internal static void FileOrDirectoryExists(ArgumentResult result) if (!Directory.Exists(token.Value) && !File.Exists(token.Value)) { - result.ReportError(result.LocalizationResources.FileOrDirectoryDoesNotExist(token.Value)); + result.AddError(result.LocalizationResources.FileOrDirectoryDoesNotExist(token.Value)); } } } From 66c3e4f6f248b744e568d13a344e17abec19e340 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 25 Jan 2023 16:08:08 +0100 Subject: [PATCH 06/15] refactor the code: merge validation and default values creation into one (20% perf gain on simple scenarios) --- src/System.CommandLine/ArgumentArity.cs | 26 +- .../Parsing/ArgumentResult.cs | 33 +- .../Parsing/OptionResult.cs | 1 - .../Parsing/ParseResultVisitor.cs | 448 +++++------------- .../Parsing/SymbolResult.cs | 2 +- .../Parsing/SymbolResultTree.cs | 8 +- 6 files changed, 169 insertions(+), 349 deletions(-) diff --git a/src/System.CommandLine/ArgumentArity.cs b/src/System.CommandLine/ArgumentArity.cs index 3b20d147c0..7609df9e94 100644 --- a/src/System.CommandLine/ArgumentArity.cs +++ b/src/System.CommandLine/ArgumentArity.cs @@ -5,6 +5,7 @@ using System.CommandLine.Binding; using System.CommandLine.Parsing; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; namespace System.CommandLine { @@ -72,20 +73,29 @@ public bool Equals(ArgumentArity other) => public override int GetHashCode() => MaximumNumberOfValues ^ MinimumNumberOfValues ^ IsNonDefault.GetHashCode(); - internal static ArgumentConversionResult? Validate(ArgumentResult argumentResult) + internal static bool Validate(ArgumentResult argumentResult, [NotNullWhen(false)] out ArgumentConversionResult? error) { + error = null; + + if (argumentResult.Parent is null || argumentResult.Parent is OptionResult { IsImplicit: true }) + { + return true; + } + int tokenCount = argumentResult.Tokens.Count; if (tokenCount < argumentResult.Argument.Arity.MinimumNumberOfValues) { - if (argumentResult.Parent!.UseDefaultValueFor(argumentResult.Argument)) + if (argumentResult.Parent.UseDefaultValueFor(argumentResult.Argument)) { - return null; + return true; } - return ArgumentConversionResult.Failure( + error = ArgumentConversionResult.Failure( argumentResult, - argumentResult.LocalizationResources.RequiredArgumentMissing(argumentResult.Parent!), + argumentResult.LocalizationResources.RequiredArgumentMissing(argumentResult.Parent), ArgumentConversionResultType.FailedMissingArgument); + + return false; } if (tokenCount > argumentResult.Argument.Arity.MaximumNumberOfValues) @@ -94,15 +104,17 @@ public override int GetHashCode() { if (!optionResult.Option.AllowMultipleArgumentsPerToken) { - return ArgumentConversionResult.Failure( + error = ArgumentConversionResult.Failure( argumentResult, argumentResult.LocalizationResources.ExpectsOneArgument(optionResult), ArgumentConversionResultType.FailedTooManyArguments); + + return false; } } } - return null; + return true; } /// diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index c2355b2ad5..4a08c2e71c 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.CommandLine.Binding; +using System.Diagnostics; using System.Linq; namespace System.CommandLine.Parsing @@ -34,7 +35,7 @@ internal ArgumentResult( internal IReadOnlyList? PassedOnTokens { get; private set; } internal ArgumentConversionResult GetArgumentConversionResult() => - _conversionResult ??= Convert(); + _conversionResult ??= ValidateAndConvert(potentialRecursion: false); /// public object? GetValueOrDefault() => @@ -45,7 +46,7 @@ internal ArgumentConversionResult GetArgumentConversionResult() => /// /// The parsed value or the default value for public T GetValueOrDefault() => - GetArgumentConversionResult() + (_conversionResult ??= ValidateAndConvert(potentialRecursion: true)) .ConvertIfNeeded(typeof(T)) .GetValueOrDefault(); @@ -83,17 +84,31 @@ public void OnlyTake(int numberOfTokens) /// public override void AddError(string errorMessage) { - SymbolResultTree.ReportError(new ParseError(errorMessage, Parent is OptionResult option ? option : this)); + SymbolResultTree.AddError(new ParseError(errorMessage, Parent is OptionResult option ? option : this)); _conversionResult = ArgumentConversionResult.Failure(this, errorMessage, ArgumentConversionResultType.Failed); } - private ArgumentConversionResult Convert() + private ArgumentConversionResult ValidateAndConvert(bool potentialRecursion) { - if (Parent is not null && - Parent is not OptionResult { IsImplicit: true } && - ArgumentArity.Validate(this) is { } failed) // returns null on success + Debug.Assert(_conversionResult is null); + + if (!ArgumentArity.Validate(this, out ArgumentConversionResult? arityFailure)) { - return ReportErrorIfNeeded(failed); + return ReportErrorIfNeeded(arityFailure); + } + + if (!potentialRecursion && Argument.HasValidators) + { + for (var i = 0; i < Argument.Validators.Count; i++) + { + Argument.Validators[i](this); + } + + // validator provided by the user might report an error, which sets _conversionResult + if (_conversionResult is not null) + { + return _conversionResult; + } } if (Parent!.UseDefaultValueFor(Argument)) @@ -137,7 +152,7 @@ ArgumentConversionResult ReportErrorIfNeeded(ArgumentConversionResult result) { if (result.Result >= ArgumentConversionResultType.Failed) { - SymbolResultTree.ReportError(new ParseError(result.ErrorMessage!, Parent is OptionResult option ? option : this)); + SymbolResultTree.AddError(new ParseError(result.ErrorMessage!, Parent is OptionResult option ? option : this)); } return result; diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index 9b1c0108aa..b3e6e12cfb 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -82,7 +82,6 @@ internal ArgumentConversionResult ArgumentConversionResult { return _argumentConversionResult = firstChild.GetArgumentConversionResult(); } - return _argumentConversionResult = ArgumentConversionResult.None(new ArgumentResult(Option.Argument, SymbolResultTree, this)); } diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index eaee5e7fa7..50b3f99ffe 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -2,10 +2,7 @@ // 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.CommandLine.Completions; using System.CommandLine.Help; -using System.Linq; namespace System.CommandLine.Parsing { @@ -19,7 +16,6 @@ internal sealed class ParseResultVisitor private Dictionary>? _directives; private List? _unmatchedTokens; - private List? _argumentResults; private CommandResult _innermostCommandResult; private bool _isHelpRequested; @@ -51,58 +47,27 @@ internal void Visit(CommandNode rootCommandNode) private void VisitInternal(SyntaxNode node) { - switch (node) - { - case DirectiveNode directiveNode: - VisitDirectiveNode(directiveNode); - - break; - - case CommandNode commandNode: - VisitCommandNode(commandNode); - - VisitChildren(commandNode); - - break; - - case OptionNode optionNode: - VisitOptionNode(optionNode); - - VisitChildren(optionNode); - - break; - - case CommandArgumentNode commandArgumentNode: - VisitCommandArgumentNode(commandArgumentNode); - - break; - - case OptionArgumentNode optionArgumentNode: - VisitOptionArgumentNode(optionArgumentNode); - - break; - } - } - - private void AddToResult(ArgumentResult result) - { - if (_symbolResultTree.TryAdd(result.Argument, result)) - { - (_argumentResults ??= new()).Add(result); - } + if (node is OptionNode optionNode) + VisitOptionNode(optionNode); + else if (node is OptionArgumentNode optionArgumentNode) + VisitOptionArgumentNode(optionArgumentNode); + else if (node is CommandArgumentNode commandArgumentNode) + VisitCommandArgumentNode(commandArgumentNode); + else if (node is CommandNode commandNode) + VisitCommandNode(commandNode); + else if (node is DirectiveNode directiveNode) + VisitDirectiveNode(directiveNode); } private void VisitCommandNode(CommandNode commandNode) { - var commandResult = new CommandResult( + _symbolResultTree.Add(commandNode.Command, _innermostCommandResult = new CommandResult( commandNode.Command, commandNode.Token, _symbolResultTree, - _innermostCommandResult); - - _symbolResultTree.Add(commandNode.Command, commandResult); + _innermostCommandResult)); - _innermostCommandResult = commandResult; + VisitChildren(commandNode); } private void VisitCommandArgumentNode(CommandArgumentNode argumentNode) @@ -116,7 +81,7 @@ private void VisitCommandArgumentNode(CommandArgumentNode argumentNode) _symbolResultTree, _innermostCommandResult); - AddToResult(argumentResult); + _symbolResultTree.Add(argumentNode.Argument, argumentResult); } argumentResult.AddToken(argumentNode.Token); @@ -142,17 +107,15 @@ private void VisitOptionNode(OptionNode optionNode) if (optionNode.Children is null) // no Arguments { - if (optionResult.Option.Argument.HasCustomParser) - { - ArgumentResult argumentResult = new (optionResult.Option.Argument, _symbolResultTree, optionResult); - _symbolResultTree.Add(optionResult.Option.Argument, argumentResult); - } + ArgumentResult argumentResult = new (optionResult.Option.Argument, _symbolResultTree, optionResult); + _symbolResultTree.Add(optionResult.Option.Argument, argumentResult); } } + + VisitChildren(optionNode); } - private void VisitOptionArgumentNode( - OptionArgumentNode argumentNode) + private void VisitOptionArgumentNode(OptionArgumentNode argumentNode) { OptionResult optionResult = (OptionResult)_symbolResultTree[argumentNode.ParentOptionNode.Option]; @@ -161,12 +124,12 @@ private void VisitOptionArgumentNode( if (!(_symbolResultTree.TryGetValue(argument, out SymbolResult? symbolResult) && symbolResult is ArgumentResult argumentResult)) { - argumentResult = - new ArgumentResult( + argumentResult = new ArgumentResult( argumentNode.Argument, _symbolResultTree, optionResult); - _symbolResultTree.TryAdd(argument, argumentResult); + + _symbolResultTree.Add(argument, argumentResult); } argumentResult.AddToken(argumentNode.Token); @@ -208,336 +171,169 @@ private void Stop() ValidateCommandHandler(); - PopulateDefaultValues(); - - ValidateCommandResult(); + ValidateCommandResults(); + } - foreach (var symbolPair in _symbolResultTree) + private void ValidateCommandHandler() + { + if (_innermostCommandResult.Command is not { Handler: null } cmd) { - if (symbolPair.Value is OptionResult optionResult) - { - ValidateAndConvertOptionResult(optionResult); - } + return; } - if (_argumentResults is not null) + if (!cmd.HasSubcommands) { - ValidateAndConvertArgumentResults(_innermostCommandResult.Command.Arguments, _argumentResults); + return; } + + _symbolResultTree.InsertError( + 0, + new ParseError( + _innermostCommandResult.LocalizationResources.RequiredCommandWasNotProvided(), + _innermostCommandResult)); } - private void ValidateAndConvertArgumentResults(IList arguments, List argumentResults) + private void ValidateCommandResults() { - int commandArgumentResultCount = argumentResults.Count; - - for (var i = 0; i < arguments.Count; i++) + CommandResult? currentResult = _innermostCommandResult; + while (currentResult is not null) { - if (i > 0 && argumentResults.Count > i) + Command command = currentResult.Command; + // Only the inner most command goes through full check, for other commands only global options are checked + bool performFullCheck = currentResult == _innermostCommandResult; + + if (performFullCheck && command.HasValidators) { - var previousArgumentResult = argumentResults[i - 1]; - - var passedOnTokensCount = previousArgumentResult.PassedOnTokens?.Count; - - if (passedOnTokensCount > 0) - { - ShiftPassedOnTokensToNextResult(previousArgumentResult, argumentResults[i], passedOnTokensCount); - } - } - - // If this is the current last result but there are more arguments, see if we can shift tokens to the next argument - if (commandArgumentResultCount == i) - { - var nextArgument = arguments[i]; - var nextArgumentResult = new ArgumentResult( - nextArgument, - _symbolResultTree, - _innermostCommandResult); - - var previousArgumentResult = argumentResults[i - 1]; - - var passedOnTokensCount = _innermostCommandResult.Tokens.Count; - - ShiftPassedOnTokensToNextResult(previousArgumentResult, nextArgumentResult, passedOnTokensCount); - - argumentResults.Add(nextArgumentResult); - - if (previousArgumentResult.Parent is CommandResult) - { - AddToResult(nextArgumentResult); - } - - _symbolResultTree.TryAdd(nextArgumentResult.Argument, nextArgumentResult); - } - - if (commandArgumentResultCount >= argumentResults.Count) - { - var argumentResult = argumentResults[i]; - - ValidateAndConvertArgumentResult(argumentResult); - - if (argumentResult.PassedOnTokens is { } && - i == arguments.Count - 1) + if (ValidateCommand(currentResult)) { - _unmatchedTokens ??= new List(); - _unmatchedTokens.AddRange(argumentResult.PassedOnTokens); + return; } } - } - if (argumentResults.Count > arguments.Count) - { - for (var i = arguments.Count; i < argumentResults.Count - 1; i++) + if (command.HasOptions) { - var result = argumentResults[i]; - - if (result.Parent is CommandResult) - { - ValidateAndConvertArgumentResult(result); - } - } - } - - void ShiftPassedOnTokensToNextResult( - ArgumentResult previous, - ArgumentResult next, - int? numberOfTokens) - { - for (var j = previous.Tokens.Count; j < numberOfTokens; j++) - { - if (next.IsArgumentLimitReached) - { - break; - } - - next.AddToken(_innermostCommandResult.Tokens[j]); + ValidateCommandOptions(currentResult, performFullCheck); } - } - } - - private void ValidateCommandResult() - { - var command = _innermostCommandResult.Command; - - if (command.HasValidators && UseValidators(command, _innermostCommandResult)) - { - return; - } - bool checkOnlyGlobalOptions = false; - Command? currentCommand = command; - while (currentCommand is not null) - { - if (currentCommand.HasOptions) + if (command.HasArguments) { - var options = currentCommand.Options; - for (var i = 0; i < options.Count; i++) - { - var option = options[i]; - if (option.IsRequired && (!checkOnlyGlobalOptions || (checkOnlyGlobalOptions && option.IsGlobal))) - { - if (_rootCommandResult.FindResultFor(option) is null) - { - _innermostCommandResult.AddError(_rootCommandResult.LocalizationResources.RequiredOptionWasNotProvided(option)); - } - } - } + ValidateCommandArguments(currentResult, performFullCheck); } - currentCommand = currentCommand.FirstParent?.Symbol as Command; - checkOnlyGlobalOptions = true; - } - - if (command.HasArguments) - { - ValidateArguments(command.Arguments, _innermostCommandResult); + currentResult = currentResult.Parent as CommandResult; } } - private bool UseValidators(Command command, CommandResult innermostCommandResult) + private bool ValidateCommand(CommandResult commandResult) { - if (!command.HasValidators) - { - return false; - } - int errorCountBefore = _symbolResultTree.ErrorCount; - for (var i = 0; i < command.Validators.Count; i++) + for (var i = 0; i < commandResult.Command.Validators.Count; i++) { - command.Validators[i](innermostCommandResult); + commandResult.Command.Validators[i](commandResult); } return _symbolResultTree.ErrorCount != errorCountBefore; } - private void ValidateArguments(IList arguments, CommandResult innermostCommandResult) + private void ValidateCommandOptions(CommandResult commandResult, bool performFullCheck) { - for (var i = 0; i < arguments.Count; i++) + var options = commandResult.Command.Options; + for (var i = 0; i < options.Count; i++) { - ArgumentResult argumentResult = _symbolResultTree.TryGetValue(arguments[i], out SymbolResult? symbolResult) - ? (ArgumentResult)symbolResult - : new ArgumentResult(arguments[i], _symbolResultTree, innermostCommandResult); - - ArgumentConversionResult? arityFailure = ArgumentArity.Validate(argumentResult); + var option = options[i]; - if (arityFailure is not null) + if (!performFullCheck && !(option.IsGlobal || option.Argument.HasDefaultValue || option.DisallowBinding)) { - innermostCommandResult.AddError(arityFailure.ErrorMessage!); + continue; } - } - } - - private void ValidateCommandHandler() - { - if (_innermostCommandResult.Command is not { Handler: null } cmd) - { - return; - } - - if (!cmd.HasSubcommands) - { - return; - } - - _symbolResultTree.InsertError( - 0, - new ParseError( - _innermostCommandResult.LocalizationResources.RequiredCommandWasNotProvided(), - _innermostCommandResult)); - } - - private void ValidateAndConvertOptionResult(OptionResult optionResult) - { - var argument = optionResult.Option.Argument; - - ArgumentResult argumentResult = _symbolResultTree.TryGetValue(argument, out SymbolResult? symbolResult) - ? (ArgumentResult)symbolResult - : new ArgumentResult(argument, _symbolResultTree, optionResult); - ArgumentConversionResult? arityFailure = ArgumentArity.Validate(argumentResult); - if (arityFailure is not null) - { - optionResult.AddError(arityFailure.ErrorMessage!); - return; - } + OptionResult optionResult; + ArgumentResult argumentResult; - if (optionResult.Option.HasValidators) - { - int errorsBefore = _symbolResultTree.ErrorCount; + if (!_symbolResultTree.TryGetValue(option, out SymbolResult? symbolResult)) + { + if (option.IsRequired) + { + commandResult.AddError(commandResult.LocalizationResources.RequiredOptionWasNotProvided(option)); + continue; + } + else if (option.Argument.HasDefaultValue) + { + optionResult = new (option, _symbolResultTree, null, commandResult); + _symbolResultTree.Add(optionResult.Option, optionResult); - for (var i = 0; i < optionResult.Option.Validators.Count; i++) + argumentResult = new (optionResult.Option.Argument, _symbolResultTree, optionResult); + _symbolResultTree.Add(optionResult.Option.Argument, argumentResult); + } + else + { + continue; + } + } + else { - optionResult.Option.Validators[i](optionResult); + optionResult = (OptionResult)symbolResult; + argumentResult = (ArgumentResult)_symbolResultTree[option.Argument]; } - if (errorsBefore != _symbolResultTree.ErrorCount) + // When_there_is_an_arity_error_then_further_errors_are_not_reported + if (!ArgumentArity.Validate(argumentResult, out var error)) { - return; + optionResult.AddError(error.ErrorMessage!); + continue; } - } - foreach (var pair in _symbolResultTree) - { - if (ReferenceEquals(pair.Value.Parent, optionResult)) + if (optionResult.Option.HasValidators) { - ValidateAndConvertArgumentResult((ArgumentResult)pair.Value); + int errorsBefore = _symbolResultTree.ErrorCount; + + for (var j = 0; j < optionResult.Option.Validators.Count; j++) + { + optionResult.Option.Validators[j](optionResult); + } + + if (errorsBefore != _symbolResultTree.ErrorCount) + { + break; + } } + + _ = argumentResult.GetArgumentConversionResult(); } } - private void ValidateAndConvertArgumentResult(ArgumentResult argumentResult) + private void ValidateCommandArguments(CommandResult commandResult, bool performFullCheck) { - var argument = argumentResult.Argument; - - if (argument.HasValidators) + var arguments = commandResult.Command.Arguments; + for (var i = 0; i < arguments.Count; i++) { - int errorsBefore = _symbolResultTree.ErrorCount; - for (var i = 0; i < argument.Validators.Count; i++) + Argument argument = arguments[i]; + + if (!performFullCheck && !argument.HasDefaultValue) { - argument.Validators[i](argumentResult); + continue; } - if (errorsBefore != _symbolResultTree.ErrorCount) + ArgumentResult? argumentResult; + if (_symbolResultTree.TryGetValue(argument, out SymbolResult? symbolResult)) { - return; + argumentResult = (ArgumentResult)symbolResult; } - } - - _ = argumentResult.GetArgumentConversionResult(); - } - - private void PopulateDefaultValues() - { - var commandResult = _innermostCommandResult; - - while (commandResult is not null) - { - if (commandResult.Command.HasOptions) + else if (argument.HasDefaultValue) { - var options = commandResult.Command.Options; - for (var i = 0; i < options.Count; i++) - { - Option option = options[i]; - Handle(_rootCommandResult.FindResultFor(option), option); - } + argumentResult = new ArgumentResult(argument, _symbolResultTree, commandResult); + _symbolResultTree[argument] = argumentResult; } - - if (commandResult.Command.HasArguments) + else if (argument.Arity.MinimumNumberOfValues > 0) { - var arguments = commandResult.Command.Arguments; - for (var i = 0; i < arguments.Count; i++) - { - Argument argument = arguments[i]; - Handle(_rootCommandResult.FindResultFor(argument), argument); - } + commandResult.AddError(commandResult.LocalizationResources.RequiredArgumentMissing(commandResult)); + continue; } - - commandResult = commandResult.Parent as CommandResult; - } - - void Handle(SymbolResult? symbolResult, Symbol symbol) - { - switch (symbolResult) + else { - case OptionResult o: - - if (o.Option.Argument.ValueType == typeof(bool) - && !_symbolResultTree.ContainsKey(o.Option.Argument)) - { - _symbolResultTree.Add(o.Option.Argument, new ArgumentResult(o.Option.Argument, _symbolResultTree, o)); - } - - break; - - case null: - switch (symbol) - { - case Option option when option.Argument.HasDefaultValue: - - var optionResult = new OptionResult( - option, - _symbolResultTree, - null, - commandResult); - - if (_symbolResultTree.TryAdd(optionResult.Option, optionResult)) - { - ArgumentResult argumentResult = new (optionResult.Option.Argument, _symbolResultTree, optionResult); - _symbolResultTree.Add(optionResult.Option.Argument, argumentResult); - } - - break; - - case Argument { HasDefaultValue: true } argument: - - if (!_symbolResultTree.ContainsKey(argument)) - { - AddToResult(new ArgumentResult(argument, _symbolResultTree, commandResult)); - } - - break; - } - - break; + continue; } + + _ = argumentResult.GetArgumentConversionResult(); } } diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index d706fb38b0..d9f13cbf9b 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -49,7 +49,7 @@ private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? /// Adds an error message for this symbol result to it's parse tree. /// /// Setting an error will cause the parser to indicate an error for the user and prevent invocation of the command line. - public virtual void AddError(string errorMessage) => SymbolResultTree.ReportError(new ParseError(errorMessage, this)); + public virtual void AddError(string errorMessage) => SymbolResultTree.AddError(new ParseError(errorMessage, this)); /// /// Finds a result for the specific argument anywhere in the parse tree, including parent and child symbol results. diff --git a/src/System.CommandLine/Parsing/SymbolResultTree.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs index f251be7f35..f8188f7a97 100644 --- a/src/System.CommandLine/Parsing/SymbolResultTree.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -7,12 +7,12 @@ namespace System.CommandLine.Parsing { internal sealed class SymbolResultTree : Dictionary { - private readonly LocalizationResources _localizationResources; + internal readonly LocalizationResources LocalizationResources; internal List? Errors; internal SymbolResultTree(LocalizationResources localizationResources, List? tokenizeErrors) { - _localizationResources = localizationResources; + LocalizationResources = localizationResources; if (tokenizeErrors is not null) { @@ -25,8 +25,6 @@ internal SymbolResultTree(LocalizationResources localizationResources, List _localizationResources; - internal int ErrorCount => Errors?.Count ?? 0; internal ArgumentResult? FindResultFor(Argument argument) @@ -52,7 +50,7 @@ internal IEnumerable GetChildren(SymbolResult parent) } } - internal void ReportError(ParseError parseError) => (Errors ??= new()).Add(parseError); + internal void AddError(ParseError parseError) => (Errors ??= new()).Add(parseError); internal void InsertError(int index, ParseError parseError) => (Errors ??= new()).Insert(index, parseError); } From 94f9705cedd0768fef047467a7b46128256acdf2 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 26 Jan 2023 17:27:30 +0100 Subject: [PATCH 07/15] refactor: * move CommandResult validation logic to CommandResult type * simplify command handler validation * don't search for ArgumentResult when we know if up front --- src/System.CommandLine/ArgumentArity.cs | 2 +- .../Parsing/ArgumentResult.cs | 2 +- .../Parsing/CommandResult.cs | 147 ++++++++++++- .../Parsing/OptionResult.cs | 2 +- .../Parsing/ParseResultVisitor.cs | 203 ++---------------- src/System.CommandLine/Parsing/Parser.cs | 2 +- .../Parsing/SymbolResult.cs | 2 +- .../Parsing/SymbolResultTree.cs | 2 +- 8 files changed, 170 insertions(+), 192 deletions(-) diff --git a/src/System.CommandLine/ArgumentArity.cs b/src/System.CommandLine/ArgumentArity.cs index 7609df9e94..a779882ad7 100644 --- a/src/System.CommandLine/ArgumentArity.cs +++ b/src/System.CommandLine/ArgumentArity.cs @@ -85,7 +85,7 @@ internal static bool Validate(ArgumentResult argumentResult, [NotNullWhen(false) int tokenCount = argumentResult.Tokens.Count; if (tokenCount < argumentResult.Argument.Arity.MinimumNumberOfValues) { - if (argumentResult.Parent.UseDefaultValueFor(argumentResult.Argument)) + if (argumentResult.Parent.UseDefaultValueFor(argumentResult)) { return true; } diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index 4a08c2e71c..2b609b0b9e 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -111,7 +111,7 @@ private ArgumentConversionResult ValidateAndConvert(bool potentialRecursion) } } - if (Parent!.UseDefaultValueFor(Argument)) + if (Parent!.UseDefaultValueFor(this)) { var defaultValue = Argument.GetDefaultValue(this); diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index 645e7d0388..ab26c59c31 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -56,12 +56,147 @@ internal sealed override int MaximumArgumentCapacity } } - internal override bool UseDefaultValueFor(Argument argument) => - FindResultFor(argument) switch + internal override bool UseDefaultValueFor(ArgumentResult argumentResult) + => argumentResult.Argument.HasDefaultValue && argumentResult.Tokens.Count == 0; + + /// Only the inner most command goes through complete validation. + internal void Validate(bool completeValidation) + { + if (completeValidation) + { + if (Command.Handler is null && Command.HasSubcommands) + { + SymbolResultTree.InsertFirstError( + new ParseError(LocalizationResources.RequiredCommandWasNotProvided(), this)); + } + } + + if (completeValidation && Command.HasValidators) + { + int errorCountBefore = SymbolResultTree.ErrorCount; + for (var i = 0; i < Command.Validators.Count; i++) + { + Command.Validators[i](this); + } + + if (SymbolResultTree.ErrorCount != errorCountBefore) + { + return; + } + } + + if (Command.HasOptions) + { + ValidateOptions(completeValidation); + } + + if (Command.HasArguments) + { + ValidateArguments(completeValidation); + } + } + + private void ValidateOptions(bool completeValidation) + { + var options = Command.Options; + for (var i = 0; i < options.Count; i++) + { + var option = options[i]; + + if (!completeValidation && !(option.IsGlobal || option.Argument.HasDefaultValue || option.DisallowBinding)) + { + continue; + } + + OptionResult optionResult; + ArgumentResult argumentResult; + + if (!SymbolResultTree.TryGetValue(option, out SymbolResult? symbolResult)) + { + if (option.IsRequired) + { + AddError(LocalizationResources.RequiredOptionWasNotProvided(option)); + continue; + } + else if (option.Argument.HasDefaultValue) + { + optionResult = new(option, SymbolResultTree, null, this); + SymbolResultTree.Add(optionResult.Option, optionResult); + + argumentResult = new(optionResult.Option.Argument, SymbolResultTree, optionResult); + SymbolResultTree.Add(optionResult.Option.Argument, argumentResult); + } + else + { + continue; + } + } + else + { + optionResult = (OptionResult)symbolResult; + argumentResult = (ArgumentResult)SymbolResultTree[option.Argument]; + } + + // When_there_is_an_arity_error_then_further_errors_are_not_reported + if (!ArgumentArity.Validate(argumentResult, out var error)) + { + optionResult.AddError(error.ErrorMessage!); + continue; + } + + if (optionResult.Option.HasValidators) + { + int errorsBefore = SymbolResultTree.ErrorCount; + + for (var j = 0; j < optionResult.Option.Validators.Count; j++) + { + optionResult.Option.Validators[j](optionResult); + } + + if (errorsBefore != SymbolResultTree.ErrorCount) + { + break; + } + } + + _ = argumentResult.GetArgumentConversionResult(); + } + } + + private void ValidateArguments(bool completeValidation) + { + var arguments = Command.Arguments; + for (var i = 0; i < arguments.Count; i++) { - ArgumentResult arg => arg.Argument.HasDefaultValue && - arg.Tokens.Count == 0, - _ => false - }; + Argument argument = arguments[i]; + + if (!completeValidation && !argument.HasDefaultValue) + { + continue; + } + + ArgumentResult? argumentResult; + if (SymbolResultTree.TryGetValue(argument, out SymbolResult? symbolResult)) + { + argumentResult = (ArgumentResult)symbolResult; + } + else if (argument.HasDefaultValue) + { + argumentResult = new ArgumentResult(argument, SymbolResultTree, this); + SymbolResultTree[argument] = argumentResult; + } + else if (argument.Arity.MinimumNumberOfValues > 0) + { + AddError(LocalizationResources.RequiredArgumentMissing(this)); + continue; + } + else + { + continue; + } + + _ = argumentResult.GetArgumentConversionResult(); + } + } } } diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index b3e6e12cfb..0d3b4da522 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -89,6 +89,6 @@ internal ArgumentConversionResult ArgumentConversionResult } } - internal override bool UseDefaultValueFor(Argument argument) => IsImplicit; + internal override bool UseDefaultValueFor(ArgumentResult argument) => IsImplicit; } } diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index 50b3f99ffe..978558c385 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -42,10 +42,23 @@ internal void Visit(CommandNode rootCommandNode) { VisitChildren(rootCommandNode); - Stop(); + if (!_isHelpRequested) + { + Validate(); + } } - private void VisitInternal(SyntaxNode node) + internal ParseResult CreateResult() => + new(_parser, + _rootCommandResult, + _innermostCommandResult, + _directives, + _tokens, + _unmatchedTokens, + _symbolResultTree.Errors, + _rawInput); + + private void VisitSyntaxNode(SyntaxNode node) { if (node is OptionNode optionNode) VisitOptionNode(optionNode); @@ -92,7 +105,7 @@ private void VisitOptionNode(OptionNode optionNode) { if (!_symbolResultTree.ContainsKey(optionNode.Option)) { - if (optionNode.Option is HelpOption) + if (optionNode.Option.DisallowBinding && optionNode.Option is HelpOption) { _isHelpRequested = true; } @@ -157,194 +170,24 @@ private void VisitChildren(NonterminalSyntaxNode parentNode) { for (var i = 0; i < parentNode.Children.Count; i++) { - VisitInternal(parentNode.Children[i]); + VisitSyntaxNode(parentNode.Children[i]); } } } - private void Stop() + private void Validate() { - if (_isHelpRequested) - { - return; - } - - ValidateCommandHandler(); - - ValidateCommandResults(); - } - - private void ValidateCommandHandler() - { - if (_innermostCommandResult.Command is not { Handler: null } cmd) - { - return; - } - - if (!cmd.HasSubcommands) - { - return; - } + // Only the inner most command goes through complete validation, + // for other commands only a subset of options is checked. + _innermostCommandResult.Validate(completeValidation: true); - _symbolResultTree.InsertError( - 0, - new ParseError( - _innermostCommandResult.LocalizationResources.RequiredCommandWasNotProvided(), - _innermostCommandResult)); - } - - private void ValidateCommandResults() - { - CommandResult? currentResult = _innermostCommandResult; + CommandResult? currentResult = _innermostCommandResult.Parent as CommandResult; while (currentResult is not null) { - Command command = currentResult.Command; - // Only the inner most command goes through full check, for other commands only global options are checked - bool performFullCheck = currentResult == _innermostCommandResult; - - if (performFullCheck && command.HasValidators) - { - if (ValidateCommand(currentResult)) - { - return; - } - } - - if (command.HasOptions) - { - ValidateCommandOptions(currentResult, performFullCheck); - } - - if (command.HasArguments) - { - ValidateCommandArguments(currentResult, performFullCheck); - } + currentResult.Validate(completeValidation: false); currentResult = currentResult.Parent as CommandResult; } } - - private bool ValidateCommand(CommandResult commandResult) - { - int errorCountBefore = _symbolResultTree.ErrorCount; - for (var i = 0; i < commandResult.Command.Validators.Count; i++) - { - commandResult.Command.Validators[i](commandResult); - } - return _symbolResultTree.ErrorCount != errorCountBefore; - } - - private void ValidateCommandOptions(CommandResult commandResult, bool performFullCheck) - { - var options = commandResult.Command.Options; - for (var i = 0; i < options.Count; i++) - { - var option = options[i]; - - if (!performFullCheck && !(option.IsGlobal || option.Argument.HasDefaultValue || option.DisallowBinding)) - { - continue; - } - - OptionResult optionResult; - ArgumentResult argumentResult; - - if (!_symbolResultTree.TryGetValue(option, out SymbolResult? symbolResult)) - { - if (option.IsRequired) - { - commandResult.AddError(commandResult.LocalizationResources.RequiredOptionWasNotProvided(option)); - continue; - } - else if (option.Argument.HasDefaultValue) - { - optionResult = new (option, _symbolResultTree, null, commandResult); - _symbolResultTree.Add(optionResult.Option, optionResult); - - argumentResult = new (optionResult.Option.Argument, _symbolResultTree, optionResult); - _symbolResultTree.Add(optionResult.Option.Argument, argumentResult); - } - else - { - continue; - } - } - else - { - optionResult = (OptionResult)symbolResult; - argumentResult = (ArgumentResult)_symbolResultTree[option.Argument]; - } - - // When_there_is_an_arity_error_then_further_errors_are_not_reported - if (!ArgumentArity.Validate(argumentResult, out var error)) - { - optionResult.AddError(error.ErrorMessage!); - continue; - } - - if (optionResult.Option.HasValidators) - { - int errorsBefore = _symbolResultTree.ErrorCount; - - for (var j = 0; j < optionResult.Option.Validators.Count; j++) - { - optionResult.Option.Validators[j](optionResult); - } - - if (errorsBefore != _symbolResultTree.ErrorCount) - { - break; - } - } - - _ = argumentResult.GetArgumentConversionResult(); - } - } - - private void ValidateCommandArguments(CommandResult commandResult, bool performFullCheck) - { - var arguments = commandResult.Command.Arguments; - for (var i = 0; i < arguments.Count; i++) - { - Argument argument = arguments[i]; - - if (!performFullCheck && !argument.HasDefaultValue) - { - continue; - } - - ArgumentResult? argumentResult; - if (_symbolResultTree.TryGetValue(argument, out SymbolResult? symbolResult)) - { - argumentResult = (ArgumentResult)symbolResult; - } - else if (argument.HasDefaultValue) - { - argumentResult = new ArgumentResult(argument, _symbolResultTree, commandResult); - _symbolResultTree[argument] = argumentResult; - } - else if (argument.Arity.MinimumNumberOfValues > 0) - { - commandResult.AddError(commandResult.LocalizationResources.RequiredArgumentMissing(commandResult)); - continue; - } - else - { - continue; - } - - _ = argumentResult.GetArgumentConversionResult(); - } - } - - public ParseResult GetResult() => - new(_parser, - _rootCommandResult, - _innermostCommandResult, - _directives, - _tokens, - _unmatchedTokens, - _symbolResultTree.Errors, - _rawInput); } } diff --git a/src/System.CommandLine/Parsing/Parser.cs b/src/System.CommandLine/Parsing/Parser.cs index 711e9e3e97..acc05bea1d 100644 --- a/src/System.CommandLine/Parsing/Parser.cs +++ b/src/System.CommandLine/Parsing/Parser.cs @@ -61,7 +61,7 @@ public ParseResult Parse( visitor.Visit(operation.RootCommandNode!); - return visitor.GetResult(); + return visitor.CreateResult(); } } } diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index d9f13cbf9b..d390fd73fe 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -120,7 +120,7 @@ public T GetValue(Argument argument) return ArgumentConverter.GetDefaultValue(option.Argument.ValueType); } - internal virtual bool UseDefaultValueFor(Argument argument) => false; + internal virtual bool UseDefaultValueFor(ArgumentResult argumentResult) => false; /// public override string ToString() => $"{GetType().Name}: {this.Token()} {string.Join(" ", Tokens.Select(t => t.Value))}"; diff --git a/src/System.CommandLine/Parsing/SymbolResultTree.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs index f8188f7a97..410b6e06e5 100644 --- a/src/System.CommandLine/Parsing/SymbolResultTree.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -52,6 +52,6 @@ internal IEnumerable GetChildren(SymbolResult parent) internal void AddError(ParseError parseError) => (Errors ??= new()).Add(parseError); - internal void InsertError(int index, ParseError parseError) => (Errors ??= new()).Insert(index, parseError); + internal void InsertFirstError(ParseError parseError) => (Errors ??= new()).Insert(0, parseError); } } From 49ab48243f632d3a349907417bacb88706715ab5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 26 Jan 2023 19:01:32 +0100 Subject: [PATCH 08/15] fix a bug I have introduced: when no Tokens are available the result should be None improve perf: don't use .Single as it's on hot path and here we know it's only 1 (Arity has been validated) --- src/System.CommandLine/Parsing/ArgumentResult.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index 2b609b0b9e..cb809adc88 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -123,7 +123,8 @@ private ArgumentConversionResult ValidateAndConvert(bool potentialRecursion) { return Argument.Arity.MaximumNumberOfValues switch { - 1 => ArgumentConversionResult.Success(this, Tokens.SingleOrDefault()), + 1 when _tokens is null => ArgumentConversionResult.None(this), + 1 when _tokens is not null => ArgumentConversionResult.Success(this, _tokens[0]), _ => ArgumentConversionResult.Success(this, Tokens) }; } From ecf99fd869b66b959c45eddbd3c3660b535e926b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 26 Jan 2023 19:30:22 +0100 Subject: [PATCH 09/15] re-implement OnlyTake, remove some virtual properties and get all the tests passing again --- src/System.CommandLine/ParseResult.cs | 2 +- .../Parsing/ArgumentResult.cs | 65 +++++++++++++++---- .../Parsing/CommandResult.cs | 20 ------ .../Parsing/OptionResult.cs | 22 ++----- .../Parsing/ParseResultVisitor.cs | 6 +- .../Parsing/SymbolResult.cs | 7 -- .../Parsing/SymbolResultTree.cs | 6 +- 7 files changed, 65 insertions(+), 63 deletions(-) diff --git a/src/System.CommandLine/ParseResult.cs b/src/System.CommandLine/ParseResult.cs index dca48e3457..fb1fe74b92 100644 --- a/src/System.CommandLine/ParseResult.cs +++ b/src/System.CommandLine/ParseResult.cs @@ -241,8 +241,8 @@ currentSymbol is not null static string[] OptionsWithArgumentLimitReached(CommandResult commandResult) => commandResult .Children - .Where(c => c.IsArgumentLimitReached) .OfType() + .Where(c => c.IsArgumentLimitReached) .Select(o => o.Option) .SelectMany(c => c.Aliases) .ToArray(); diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index cb809adc88..4f8b009b30 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -1,9 +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.Collections.Generic; using System.CommandLine.Binding; -using System.Diagnostics; using System.Linq; namespace System.CommandLine.Parsing @@ -14,6 +12,7 @@ namespace System.CommandLine.Parsing public sealed class ArgumentResult : SymbolResult { private ArgumentConversionResult? _conversionResult; + private bool _passedOnHasBeenCalled; internal ArgumentResult( Argument argument, @@ -28,12 +27,10 @@ internal ArgumentResult( /// public Argument Argument { get; } - internal override int MaximumArgumentCapacity => Argument.Arity.MaximumNumberOfValues; + internal bool IsArgumentLimitReached => Argument.Arity.MaximumNumberOfValues == (_tokens?.Count ?? 0); internal bool IsImplicit => Argument.HasDefaultValue && Tokens.Count == 0; - internal IReadOnlyList? PassedOnTokens { get; private set; } - internal ArgumentConversionResult GetArgumentConversionResult() => _conversionResult ??= ValidateAndConvert(potentialRecursion: false); @@ -56,6 +53,7 @@ public T GetValueOrDefault() => /// The number of tokens to take. The rest are passed on. /// numberOfTokens - Value must be at least 1. /// Thrown if this method is called more than once. + /// Thrown if this method is called by Option-owned ArgumentResult. public void OnlyTake(int numberOfTokens) { if (numberOfTokens < 0) @@ -63,18 +61,63 @@ public void OnlyTake(int numberOfTokens) throw new ArgumentOutOfRangeException(nameof(numberOfTokens), numberOfTokens, "Value must be at least 1."); } - if (PassedOnTokens is { }) + if (_passedOnHasBeenCalled) { throw new InvalidOperationException($"{nameof(OnlyTake)} can only be called once."); } - if (_tokens is not null) + if (Parent is OptionResult) + { + throw new NotSupportedException($"TakeOnly is supported only by Command-owned ArgumentResults"); + } + + _passedOnHasBeenCalled = true; + + if (_tokens is null || numberOfTokens >= _tokens.Count) + { + return; + } + + CommandResult parent = (CommandResult)Parent!; + var arguments = parent.Command.Arguments; + int argumentIndex = arguments.IndexOf(Argument); + int nextArgumentIndex = argumentIndex + 1; + int tokensToPass = _tokens.Count - numberOfTokens; + + while (tokensToPass > 0 && nextArgumentIndex < arguments.Count) { - var passedOnTokensCount = _tokens.Count - numberOfTokens; + Argument nextArgument = parent.Command.Arguments[nextArgumentIndex]; + ArgumentResult nextArgumentResult; + + if (SymbolResultTree.TryGetValue(nextArgument, out SymbolResult? symbolResult)) + { + nextArgumentResult = (ArgumentResult)symbolResult; + } + else + { + // it might have not been parsed yet or due too few arguments, so we add it now + nextArgumentResult = new ArgumentResult(nextArgument, SymbolResultTree, Parent); + SymbolResultTree.Add(nextArgument, nextArgumentResult); + } + + while (!nextArgumentResult.IsArgumentLimitReached && tokensToPass > 0) + { + Token toPass = _tokens[numberOfTokens]; + _tokens.RemoveAt(numberOfTokens); + nextArgumentResult.AddToken(toPass); + --tokensToPass; + } - PassedOnTokens = new List(_tokens.GetRange(numberOfTokens, passedOnTokensCount)); + nextArgumentIndex++; + } - _tokens.RemoveRange(numberOfTokens, passedOnTokensCount); + // When_tokens_are_passed_on_by_custom_parser_on_last_argument_then_they_become_unmatched_tokens + while (tokensToPass > 0) + { + Token unmatched = _tokens[numberOfTokens]; + _tokens.RemoveAt(numberOfTokens); + SymbolResultTree.AddUnmatchedToken(unmatched); + --tokensToPass; } } @@ -90,8 +133,6 @@ public override void AddError(string errorMessage) private ArgumentConversionResult ValidateAndConvert(bool potentialRecursion) { - Debug.Assert(_conversionResult is null); - if (!ArgumentArity.Validate(this, out ArgumentConversionResult? arityFailure)) { return ReportErrorIfNeeded(arityFailure); diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index ab26c59c31..d0bf79c171 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -36,26 +36,6 @@ internal CommandResult( /// public IEnumerable Children => SymbolResultTree.GetChildren(this); - 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(ArgumentResult argumentResult) => argumentResult.Argument.HasDefaultValue && argumentResult.Tokens.Count == 0; diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index 0d3b4da522..5360b9fe5e 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -40,8 +40,6 @@ internal OptionResult( /// public Token? Token { get; } - internal override int MaximumArgumentCapacity => Option.Argument.Arity.MaximumNumberOfValues; - /// public object? GetValueOrDefault() => Option.ValueType == typeof(bool) @@ -57,20 +55,8 @@ public T GetValueOrDefault() => ArgumentConversionResult.ConvertIfNeeded(typeof(T)) .GetValueOrDefault(); - private protected override int RemainingArgumentCapacity - { - get - { - var capacity = base.RemainingArgumentCapacity; - - if (IsImplicit && capacity < int.MaxValue) - { - capacity += 1; - } - - return capacity; - } - } + internal bool IsArgumentLimitReached + => Option.Argument.Arity.MaximumNumberOfValues == (IsImplicit ? Tokens.Count - 1 : Tokens.Count); internal ArgumentConversionResult ArgumentConversionResult { @@ -78,9 +64,9 @@ internal ArgumentConversionResult ArgumentConversionResult { if (_argumentConversionResult is null) { - if (FindResultFor(Option.Argument) is ArgumentResult firstChild) + if (FindResultFor(Option.Argument) is ArgumentResult child) { - return _argumentConversionResult = firstChild.GetArgumentConversionResult(); + return _argumentConversionResult = child.GetArgumentConversionResult(); } return _argumentConversionResult = ArgumentConversionResult.None(new ArgumentResult(Option.Argument, SymbolResultTree, this)); } diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index 978558c385..44fa087308 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -15,7 +15,6 @@ internal sealed class ParseResultVisitor private readonly CommandResult _rootCommandResult; private Dictionary>? _directives; - private List? _unmatchedTokens; private CommandResult _innermostCommandResult; private bool _isHelpRequested; @@ -29,9 +28,8 @@ internal ParseResultVisitor( { _parser = parser; _tokens = tokens; - _unmatchedTokens = unmatchedTokens; _rawInput = rawInput; - _symbolResultTree = new(_parser.Configuration.LocalizationResources, tokenizeErrors); + _symbolResultTree = new(_parser.Configuration.LocalizationResources, tokenizeErrors, unmatchedTokens); _innermostCommandResult = _rootCommandResult = new CommandResult( rootCommandNode.Command, rootCommandNode.Token, @@ -54,7 +52,7 @@ internal ParseResult CreateResult() => _innermostCommandResult, _directives, _tokens, - _unmatchedTokens, + _symbolResultTree.UnmatchedTokens, _symbolResultTree.Errors, _rawInput); diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index d390fd73fe..a7ae8d5202 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -31,13 +31,6 @@ private protected SymbolResult(SymbolResultTree symbolResultTree, SymbolResult? /// public IReadOnlyList Tokens => _tokens is not null ? _tokens : Array.Empty(); - internal bool IsArgumentLimitReached => RemainingArgumentCapacity == 0; - - private protected virtual int RemainingArgumentCapacity => - MaximumArgumentCapacity - Tokens.Count; - - internal abstract int MaximumArgumentCapacity { get; } - /// /// Localization resources used to produce messages for this symbol result. /// diff --git a/src/System.CommandLine/Parsing/SymbolResultTree.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs index 410b6e06e5..e31a5237b2 100644 --- a/src/System.CommandLine/Parsing/SymbolResultTree.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -9,10 +9,12 @@ internal sealed class SymbolResultTree : Dictionary { internal readonly LocalizationResources LocalizationResources; internal List? Errors; + internal List? UnmatchedTokens; - internal SymbolResultTree(LocalizationResources localizationResources, List? tokenizeErrors) + internal SymbolResultTree(LocalizationResources localizationResources, List? tokenizeErrors, List? unmatchedTokens) { LocalizationResources = localizationResources; + UnmatchedTokens = unmatchedTokens; if (tokenizeErrors is not null) { @@ -53,5 +55,7 @@ internal IEnumerable GetChildren(SymbolResult parent) internal void AddError(ParseError parseError) => (Errors ??= new()).Add(parseError); internal void InsertFirstError(ParseError parseError) => (Errors ??= new()).Insert(0, parseError); + + internal void AddUnmatchedToken(Token token) => (UnmatchedTokens ??= new()).Add(token); } } From aa852d11c239cc6d91f61504a236de0bcc305e3c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 26 Jan 2023 20:52:56 +0100 Subject: [PATCH 10/15] reduce the number of ctors and methods remove null checks as it's internal API and we are covered by nullable annotations --- .../Binding/ArgumentConversionResult.cs | 31 ++++++------------- .../Binding/ArgumentConverter.cs | 12 ++----- .../Parsing/ArgumentResult.cs | 2 +- 3 files changed, 13 insertions(+), 32 deletions(-) diff --git a/src/System.CommandLine/Binding/ArgumentConversionResult.cs b/src/System.CommandLine/Binding/ArgumentConversionResult.cs index ad914320af..6c103e1d3c 100644 --- a/src/System.CommandLine/Binding/ArgumentConversionResult.cs +++ b/src/System.CommandLine/Binding/ArgumentConversionResult.cs @@ -16,40 +16,29 @@ internal sealed class ArgumentConversionResult private ArgumentConversionResult(ArgumentResult argumentResult, string error, ArgumentConversionResultType failure) { - ArgumentResult = argumentResult ?? throw new ArgumentNullException(nameof(argumentResult)); - ErrorMessage = error ?? throw new ArgumentNullException(nameof(error)); + ArgumentResult = argumentResult; + ErrorMessage = error; Result = failure; } - private ArgumentConversionResult(ArgumentResult argumentResult, object? value) + private ArgumentConversionResult(ArgumentResult argumentResult, object? value, ArgumentConversionResultType result) { - ArgumentResult = argumentResult ?? throw new ArgumentNullException(nameof(argumentResult)); + ArgumentResult = argumentResult; Value = value; - Result = ArgumentConversionResultType.Successful; - } - - private ArgumentConversionResult(ArgumentResult argumentResult) - { - ArgumentResult = argumentResult ?? throw new ArgumentNullException(nameof(argumentResult)); - Result = ArgumentConversionResultType.NoArgument; - } - - internal ArgumentConversionResult( - ArgumentResult argumentResult, - Type expectedType, - string value) : - this(argumentResult, FormatErrorMessage(argumentResult, expectedType, value), ArgumentConversionResultType.FailedType) - { + Result = result; } internal static ArgumentConversionResult Failure(ArgumentResult argumentResult, string error, ArgumentConversionResultType reason) => new(argumentResult, error, reason); + internal static ArgumentConversionResult ArgumentConversionCannotParse(ArgumentResult argumentResult, Type expectedType, string value) + => new(argumentResult, FormatErrorMessage(argumentResult, expectedType, value), ArgumentConversionResultType.FailedType); + public static ArgumentConversionResult Success(ArgumentResult argumentResult, object? value) - => new(argumentResult, value); + => new(argumentResult, value, ArgumentConversionResultType.Successful); internal static ArgumentConversionResult None(ArgumentResult argumentResult) - => new(argumentResult); + => new(argumentResult, value: null, ArgumentConversionResultType.NoArgument); private static string FormatErrorMessage( ArgumentResult argumentResult, diff --git a/src/System.CommandLine/Binding/ArgumentConverter.cs b/src/System.CommandLine/Binding/ArgumentConverter.cs index 18c6e9f89f..3087d8ea24 100644 --- a/src/System.CommandLine/Binding/ArgumentConverter.cs +++ b/src/System.CommandLine/Binding/ArgumentConverter.cs @@ -48,7 +48,7 @@ private static ArgumentConversionResult ConvertToken( } else { - return Failure(argumentResult, type, value); + return ArgumentConversionCannotParse(argumentResult, type, value); } } @@ -64,7 +64,7 @@ private static ArgumentConversionResult ConvertToken( } } - return Failure(argumentResult, type, value); + return ArgumentConversionCannotParse(argumentResult, type, value); } private static ArgumentConversionResult ConvertTokens( @@ -163,14 +163,6 @@ private static bool CanBeBoundFromScalarValue(this Type type) } } - private static ArgumentConversionResult Failure( - ArgumentResult argumentResult, - Type expectedType, - string value) - { - return new ArgumentConversionResult(argumentResult, expectedType, value); - } - internal static ArgumentConversionResult ConvertIfNeeded( this ArgumentConversionResult conversionResult, Type toType) diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index 4f8b009b30..9969cd3bdd 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -188,7 +188,7 @@ private ArgumentConversionResult ValidateAndConvert(bool potentialRecursion) return ArgumentConversionResult.Success(this, value); } - return ReportErrorIfNeeded(new ArgumentConversionResult(this, Argument.ValueType, Tokens[0].Value)); + return ReportErrorIfNeeded(ArgumentConversionResult.ArgumentConversionCannotParse(this, Argument.ValueType, Tokens[0].Value)); ArgumentConversionResult ReportErrorIfNeeded(ArgumentConversionResult result) { From bae82fecac7c73b722c126f419875c98e8de484b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 26 Jan 2023 20:55:58 +0100 Subject: [PATCH 11/15] solve the TODO: use Enum.TryParse when available --- src/System.CommandLine/Binding/ArgumentConverter.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/System.CommandLine/Binding/ArgumentConverter.cs b/src/System.CommandLine/Binding/ArgumentConverter.cs index 3087d8ea24..c982a66fdf 100644 --- a/src/System.CommandLine/Binding/ArgumentConverter.cs +++ b/src/System.CommandLine/Binding/ArgumentConverter.cs @@ -54,14 +54,20 @@ private static ArgumentConversionResult ConvertToken( if (type.IsEnum) { +#if NET7_0_OR_GREATER + if (Enum.TryParse(type, value, ignoreCase: true, out var converted)) + { + return Success(argumentResult, converted); + } +#else try { return Success(argumentResult, Enum.Parse(type, value, true)); } catch (ArgumentException) { - // TODO: find a way to do this without the try..catch } +#endif } return ArgumentConversionCannotParse(argumentResult, type, value); From 4f622182d62d5874983adcab4443646400a4d530 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 27 Jan 2023 19:50:35 +0100 Subject: [PATCH 12/15] with recent changes it's guaranteed that every OptionResult has corresponding ArgumentResult --- src/System.CommandLine/Parsing/OptionResult.cs | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index 5360b9fe5e..3f540cdfb5 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -59,21 +59,7 @@ internal bool IsArgumentLimitReached => Option.Argument.Arity.MaximumNumberOfValues == (IsImplicit ? Tokens.Count - 1 : Tokens.Count); internal ArgumentConversionResult ArgumentConversionResult - { - get - { - if (_argumentConversionResult is null) - { - if (FindResultFor(Option.Argument) is ArgumentResult child) - { - return _argumentConversionResult = child.GetArgumentConversionResult(); - } - return _argumentConversionResult = ArgumentConversionResult.None(new ArgumentResult(Option.Argument, SymbolResultTree, this)); - } - - return _argumentConversionResult; - } - } + => _argumentConversionResult ??= FindResultFor(Option.Argument)!.GetArgumentConversionResult(); internal override bool UseDefaultValueFor(ArgumentResult argument) => IsImplicit; } From 172ffe4fc4c6c6e472d98308c7ef6339f02112bd Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 27 Jan 2023 20:49:02 +0100 Subject: [PATCH 13/15] rename the test to reflect what it does now --- src/System.CommandLine.Tests/ParsingValidationTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index 4fd384b3bf..66ce605324 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -178,7 +178,7 @@ public void When_FromAmong_is_used_for_multiple_arguments_and_invalid_input_is_p } [Fact] - public void When_FromAmong_is_used_and_multiple_invalid_inputs_are_provided_the_error_mentions_first_invalid_argument() + public void When_FromAmong_is_used_and_multiple_invalid_inputs_are_provided_the_errors_mention_all_invalid_arguments() { Option option = new(new[] { "--columns" }); option.AcceptOnlyFromAmong("author", "language", "tags", "type"); From 10fdeeb5c2866f75201f92bf6b82f17728076789 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 30 Jan 2023 19:46:53 +0100 Subject: [PATCH 14/15] Apply suggestions from code review Co-authored-by: Jon Sequeira --- src/System.CommandLine/Parsing/ArgumentResult.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index 9969cd3bdd..86be164c16 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -68,7 +68,7 @@ public void OnlyTake(int numberOfTokens) if (Parent is OptionResult) { - throw new NotSupportedException($"TakeOnly is supported only by Command-owned ArgumentResults"); + throw new NotSupportedException($"{nameof(OnlyTake)} is supported only for a {nameof(Command)}-owned {nameof(ArgumentResult)}"); } _passedOnHasBeenCalled = true; From 1307432e5a6a0b9bd9ad0dc755beb815d18543b8 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 30 Jan 2023 19:57:48 +0100 Subject: [PATCH 15/15] Address code review feedback --- .../Parsing/ArgumentResult.cs | 18 ++++++++++------- .../Parsing/CommandResult.cs | 20 +++++++++---------- .../Parsing/ParseResultVisitor.cs | 10 ++++++++++ 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index 86be164c16..c77bacdecd 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -12,7 +12,7 @@ namespace System.CommandLine.Parsing public sealed class ArgumentResult : SymbolResult { private ArgumentConversionResult? _conversionResult; - private bool _passedOnHasBeenCalled; + private bool _onlyTakeHasBeenCalled; internal ArgumentResult( Argument argument, @@ -32,7 +32,7 @@ internal ArgumentResult( internal bool IsImplicit => Argument.HasDefaultValue && Tokens.Count == 0; internal ArgumentConversionResult GetArgumentConversionResult() => - _conversionResult ??= ValidateAndConvert(potentialRecursion: false); + _conversionResult ??= ValidateAndConvert(useValidators: true); /// public object? GetValueOrDefault() => @@ -43,7 +43,7 @@ internal ArgumentConversionResult GetArgumentConversionResult() => /// /// The parsed value or the default value for public T GetValueOrDefault() => - (_conversionResult ??= ValidateAndConvert(potentialRecursion: true)) + (_conversionResult ??= ValidateAndConvert(useValidators: false)) .ConvertIfNeeded(typeof(T)) .GetValueOrDefault(); @@ -61,7 +61,7 @@ public void OnlyTake(int numberOfTokens) throw new ArgumentOutOfRangeException(nameof(numberOfTokens), numberOfTokens, "Value must be at least 1."); } - if (_passedOnHasBeenCalled) + if (_onlyTakeHasBeenCalled) { throw new InvalidOperationException($"{nameof(OnlyTake)} can only be called once."); } @@ -71,7 +71,7 @@ public void OnlyTake(int numberOfTokens) throw new NotSupportedException($"{nameof(OnlyTake)} is supported only for a {nameof(Command)}-owned {nameof(ArgumentResult)}"); } - _passedOnHasBeenCalled = true; + _onlyTakeHasBeenCalled = true; if (_tokens is null || numberOfTokens >= _tokens.Count) { @@ -131,14 +131,18 @@ public override void AddError(string errorMessage) _conversionResult = ArgumentConversionResult.Failure(this, errorMessage, ArgumentConversionResultType.Failed); } - private ArgumentConversionResult ValidateAndConvert(bool potentialRecursion) + private ArgumentConversionResult ValidateAndConvert(bool useValidators) { if (!ArgumentArity.Validate(this, out ArgumentConversionResult? arityFailure)) { return ReportErrorIfNeeded(arityFailure); } - if (!potentialRecursion && Argument.HasValidators) + // There is nothing that stops user-defined Validator from calling ArgumentResult.GetValueOrDefault. + // In such cases, we can't call the validators again, as it would create infinite recursion. + // GetArgumentConversionResult => ValidateAndConvert => Validator + // => GetValueOrDefault => ValidateAndConvert (again) + if (useValidators && Argument.HasValidators) { for (var i = 0; i < Argument.Validators.Count; i++) { diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index d0bf79c171..28e71fbda7 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -49,19 +49,19 @@ internal void Validate(bool completeValidation) SymbolResultTree.InsertFirstError( new ParseError(LocalizationResources.RequiredCommandWasNotProvided(), this)); } - } - if (completeValidation && Command.HasValidators) - { - int errorCountBefore = SymbolResultTree.ErrorCount; - for (var i = 0; i < Command.Validators.Count; i++) + if (Command.HasValidators) { - Command.Validators[i](this); - } + int errorCountBefore = SymbolResultTree.ErrorCount; + for (var i = 0; i < Command.Validators.Count; i++) + { + Command.Validators[i](this); + } - if (SymbolResultTree.ErrorCount != errorCountBefore) - { - return; + if (SymbolResultTree.ErrorCount != errorCountBefore) + { + return; + } } } diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index 44fa087308..923b1dd479 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -59,15 +59,25 @@ internal ParseResult CreateResult() => private void VisitSyntaxNode(SyntaxNode node) { if (node is OptionNode optionNode) + { VisitOptionNode(optionNode); + } else if (node is OptionArgumentNode optionArgumentNode) + { VisitOptionArgumentNode(optionArgumentNode); + } else if (node is CommandArgumentNode commandArgumentNode) + { VisitCommandArgumentNode(commandArgumentNode); + } else if (node is CommandNode commandNode) + { VisitCommandNode(commandNode); + } else if (node is DirectiveNode directiveNode) + { VisitDirectiveNode(directiveNode); + } } private void VisitCommandNode(CommandNode commandNode)