From 4bf602b46fe371bb5214d1693670ff160260336d Mon Sep 17 00:00:00 2001 From: Kevin Bost Date: Sat, 4 May 2024 02:02:39 -0700 Subject: [PATCH] Fix duplicate errors appended Fixes #2392. This is a port from the fix on main. However due to additional work that is currently being done on Powerderhouse it is forcing the creation of the ArgumentConversionResult as part of parsing. This is due to the invocation SymbolResultTree.GetValueResultDictionary() which forces the evaluation of all ArgumentConversionResults. This is adding an additional test to make sure that the behavior is not re-introduced. This also fixes a few NRT issues and a comment typo. --- src/System.CommandLine.Tests/ParserTests.cs | 25 ++++++++++++++++--- .../Parsing/ArgumentResult.cs | 5 ++-- .../Parsing/OptionResult.cs | 2 +- .../Parsing/SymbolResultTree.cs | 2 +- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/System.CommandLine.Tests/ParserTests.cs b/src/System.CommandLine.Tests/ParserTests.cs index 72280dd9c0..209d4f0826 100644 --- a/src/System.CommandLine.Tests/ParserTests.cs +++ b/src/System.CommandLine.Tests/ParserTests.cs @@ -4,13 +4,11 @@ using System.Collections.Generic; using System.CommandLine.Parsing; using System.CommandLine.Tests.Utility; -using System.IO; using FluentAssertions; using FluentAssertions.Equivalency; using System.Linq; using FluentAssertions.Common; using Xunit; -using Xunit.Abstractions; namespace System.CommandLine.Tests { @@ -19,10 +17,10 @@ public partial class ParserTests // TODO: Update testing strategy if we use Location in equality. Some will break private readonly Location dummyLocation = new("", Location.Internal, -1, null); - private T GetValue(ParseResult parseResult, CliOption option) + private static T GetValue(ParseResult parseResult, CliOption option) => parseResult.GetValue(option); - private T GetValue(ParseResult parseResult, CliArgument argument) + private static T GetValue(ParseResult parseResult, CliArgument argument) => parseResult.GetValue(argument); //[Fact] @@ -1921,5 +1919,24 @@ public void ParseResult_contains_option_ValueResults() result1.GetValue().Should().Be("Kirk"); result2.GetValue().Should().Be("Spock"); } + + // https://github.com/dotnet/command-line-api/issues/2392 + [Fact] + public void Getting_argument_value_with_option_argument_error_the_only_a_single_error_is_returned() + { + var option = new CliOption("-x") { Arity = new ArgumentArity(2, 3), AllowMultipleArgumentsPerToken = true }; + var command = new CliCommand("the-command") + { + option + }; + + ParseResult parseResult = CliParser.Parse(command, "-x 1 2 3 4"); + + _ = parseResult.CommandResult.GetValue(option); + + parseResult.Errors + .Should() + .ContainSingle(); + } } } diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs index 9e2248b870..3a09f4e57e 100644 --- a/src/System.CommandLine/Parsing/ArgumentResult.cs +++ b/src/System.CommandLine/Parsing/ArgumentResult.cs @@ -31,10 +31,11 @@ public ValueResult ValueResult { // This is not lazy on the assumption that almost everything the user enters will be used, and ArgumentResult is no longer used for defaults // TODO: Make sure errors are added - var conversionValue = GetArgumentConversionResult().Value; + ArgumentConversionResult argumentConversionValue = GetArgumentConversionResult(); + var conversionValue = argumentConversionValue.Value; var locations = Tokens.Select(token => token.Location).ToArray(); //TODO: Remove this wrapper later - _valueResult = new ValueResult(Argument, conversionValue, locations, ArgumentResult.GetValueResultOutcome(GetArgumentConversionResult()?.Result)); // null is temporary here + _valueResult = new ValueResult(Argument, conversionValue, locations, GetValueResultOutcome(argumentConversionValue.Result)); } return _valueResult; } diff --git a/src/System.CommandLine/Parsing/OptionResult.cs b/src/System.CommandLine/Parsing/OptionResult.cs index ccba50a386..e5f9186a4f 100644 --- a/src/System.CommandLine/Parsing/OptionResult.cs +++ b/src/System.CommandLine/Parsing/OptionResult.cs @@ -37,7 +37,7 @@ public ValueResult ValueResult var conversionValue = ArgumentConversionResult.Value; var locations = Tokens.Select(token => token.Location).ToArray(); //TODO: Remove this wrapper later - _valueResult = new ValueResult(Option, conversionValue, locations, ArgumentResult.GetValueResultOutcome(ArgumentConversionResult?.Result)); // null is temporary here + _valueResult = new ValueResult(Option, conversionValue, locations, ArgumentResult.GetValueResultOutcome(ArgumentConversionResult.Result)); } return _valueResult; } diff --git a/src/System.CommandLine/Parsing/SymbolResultTree.cs b/src/System.CommandLine/Parsing/SymbolResultTree.cs index 37319d604a..fa6092361b 100644 --- a/src/System.CommandLine/Parsing/SymbolResultTree.cs +++ b/src/System.CommandLine/Parsing/SymbolResultTree.cs @@ -15,7 +15,7 @@ internal sealed class SymbolResultTree : Dictionary internal List? UnmatchedTokens; */ - // TODO: Looks like this is a SymboNode/linked list because a symbol may appear multiple + // TODO: Looks like this is a SymbolNode/linked list because a symbol may appear multiple // places in the tree and multiple symbols will have the same short name. The question is // whether creating the multiple node instances is faster than just using lists. Could well be. private Dictionary? _symbolsByName;