more flexible error reporting for SymbolResult by adamsitnik · Pull Request #2033 · dotnet/command-line-api

adamsitnik · 2023-01-24T18:07:34Z

I've removed SymbolResult.ErrorMessage as it did not allow for providing multiple errors for single result and introduced SymbolResult.ReportError (name can be changed, I don't have strong preferences about it).

SymbolResult.ReportError adds a new error to the list of errors (so multiple errors per result are supported) and in case of ArgumentResult also sets ArgumentConversionResult (perf, simplicity) and encapsulates the fact that Option.Argument is internal and for Arguments owned by Option sets the parent to OptionResult rather than ArgumentResult

I also did a lot of refactoring related to ArgumentConversionResult, I am going to add comments explaining why.

…tConversionResult

src/System.CommandLine.Tests/ParseDiagramTests.cs

src/System.CommandLine/Binding/ArgumentConversionResult.cs

adamsitnik · 2023-01-24T18:18:37Z

src/System.CommandLine/Parsing/ArgumentResult.cs

-
-            return null;
+            SymbolResultTree.ReportError(new ParseError(errorMessage, Parent is OptionResult option ? option : this));
+            _conversionResult = ArgumentConversionResult.Failure(this, errorMessage, ArgumentConversionResultType.Failed);


at this moment we know that it's a failure

Very nice. As long as we don't later allow clearing of the errors.

...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt

jonsequitur · 2023-01-24T19:46:14Z

Was a test added to pin the expected behavior when ReportError is called more than once?

* rename ReportError to AddError * test coverage improvement (multiple errors for one result)

…one (20% perf gain on simple scenarios)

* move CommandResult validation logic to CommandResult type * simplify command handler validation * don't search for ArgumentResult when we know if up front

…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)

… tests passing again

remove null checks as it's internal API and we are covered by nullable annotations

adamsitnik · 2023-01-26T20:50:42Z

src/System.CommandLine/Parsing/ArgumentResult.cs

-        internal ParseError? CustomError(Argument argument)
-        {
-            for (var i = 0; i < argument.Validators.Count; i++)
+            while (tokensToPass > 0 && nextArgumentIndex < arguments.Count)


previously this method was implemented in ParseResultVisitor, I've moved it here and implemented in a way it does not require an allocation of temporary list

command-line-api/src/System.CommandLine/Parsing/ParseResultVisitor.cs

Lines 251 to 295 in 6524142

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)

{

_unmatchedTokens ??= new List<Token>();

_unmatchedTokens.AddRange(argumentResult.PassedOnTokens);

}

adamsitnik · 2023-01-26T20:52:44Z

src/System.CommandLine/Parsing/ArgumentResult.cs

        }

-        private ArgumentConversionResult Convert(Argument argument)
+        private ArgumentConversionResult ValidateAndConvert(bool potentialRecursion)


this method is now responsible for entire validation. The problem is that it can be called by not only the ParseResultVisitor but also custom parsers and validator which can do things like ArgumentResult.GetDefaultValue(). For the latter case I've added potentialRecursion argument to ensure we don't have endless recursion (validator->getvalue->validator)

The name potentialRecursion is a little unclear to me. Is this saying whether to allow recursion?

adamsitnik · 2023-01-26T20:54:17Z

src/System.CommandLine/Parsing/CommandResult.cs

+            => argumentResult.Argument.HasDefaultValue && argumentResult.Tokens.Count == 0;
+
+        /// <param name="completeValidation">Only the inner most command goes through complete validation.</param>
+        internal void Validate(bool completeValidation)


I've moved this logic from the visitor and re-wrote in a way that we don't iterate over every argument more than once (previously we were setting the default values and then validating).

src/System.CommandLine.Tests/ParsingValidationTests.cs

adamsitnik · 2023-01-26T21:03:04Z

@jonsequitur the PR is ready for review. I hope that you like the refactoring I've made. The side-effects of the refactor is improved performance. For the simple use case Jan is always using it's 1-3% for cold startup time scenario (start a new process, parse the args, print and exit: jkotas/commandline-perf#2), for micro-benchmark it's 20% faster (less important, as it's warm), but the allocations has reduced nicely

…sponding ArgumentResult

src/System.CommandLine/Parsing/ArgumentResult.cs

src/System.CommandLine/Parsing/CommandResult.cs

src/System.CommandLine/Parsing/ParseResultVisitor.cs

Co-authored-by: Jon Sequeira <jonsequeira@gmail.com>

KalleOlaviNiemitalo · 2023-02-02T19:13:31Z

This required changes in pretty much all of my custom parsers. They were easy to implement, though.

adamsitnik added 2 commits January 24, 2023 13:34

remove SymbolResult.ErrorMessage, use the new SymbolResult.ReportError

b0b488d

use all available information when creating error message for Argumen…

3006cb3

…tConversionResult

adamsitnik commented Jan 24, 2023

View reviewed changes

src/System.CommandLine.Tests/ParseDiagramTests.cs Show resolved Hide resolved

adamsitnik commented Jan 24, 2023

View reviewed changes

src/System.CommandLine/Binding/ArgumentConversionResult.cs Show resolved Hide resolved

adamsitnik commented Jan 24, 2023

View reviewed changes

adamsitnik added 2 commits January 24, 2023 19:23

remove unused method

2ef226f

last tweaks

d8f92ef

adamsitnik requested a review from jonsequitur January 24, 2023 18:44

jonsequitur reviewed Jan 24, 2023

View reviewed changes

...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt Outdated Show resolved Hide resolved

adamsitnik added 7 commits January 25, 2023 09:12

address code review feedback:

f8370ae

* rename ReportError to AddError * test coverage improvement (multiple errors for one result)

refactor the code: merge validation and default values creation into …

66c3e4f

…one (20% perf gain on simple scenarios)

refactor:

94f9705

* move CommandResult validation logic to CommandResult type * simplify command handler validation * don't search for ArgumentResult when we know if up front

fix a bug I have introduced: when no Tokens are available the result …

49ab482

…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)

re-implement OnlyTake, remove some virtual properties and get all the…

ecf99fd

… tests passing again

reduce the number of ctors and methods

aa852d1

remove null checks as it's internal API and we are covered by nullable annotations

solve the TODO: use Enum.TryParse when available

bae82fe

adamsitnik commented Jan 26, 2023

View reviewed changes

src/System.CommandLine.Tests/ParsingValidationTests.cs Show resolved Hide resolved

adamsitnik requested a review from jonsequitur January 27, 2023 18:24

adamsitnik added 2 commits January 27, 2023 19:50

with recent changes it's guaranteed that every OptionResult has corre…

4f62218

…sponding ArgumentResult

rename the test to reflect what it does now

172ffe4

jonsequitur reviewed Jan 27, 2023

View reviewed changes

src/System.CommandLine/Parsing/ArgumentResult.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Jan 27, 2023

View reviewed changes

src/System.CommandLine/Parsing/ArgumentResult.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Jan 27, 2023

View reviewed changes

src/System.CommandLine/Parsing/CommandResult.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Jan 27, 2023

View reviewed changes

src/System.CommandLine/Parsing/ParseResultVisitor.cs Show resolved Hide resolved

Apply suggestions from code review

10fdeeb

Co-authored-by: Jon Sequeira <jonsequeira@gmail.com>

Address code review feedback

1307432

adamsitnik requested a review from jonsequitur January 30, 2023 19:10

adamsitnik mentioned this pull request Feb 1, 2023

Remove one layer of abstraction and multiple intermediate types #2039

Closed

KalleOlaviNiemitalo mentioned this pull request Feb 1, 2023

Consider a new localization design #1915

Open

jonsequitur approved these changes Feb 2, 2023

View reviewed changes

adamsitnik merged commit 506cb2f into dotnet:main Feb 2, 2023

adamsitnik deleted the error branch February 2, 2023 16:20

adamsitnik mentioned this pull request Feb 2, 2023

Remove one layer of abstraction and multiple intermediate types #2040

Merged

adamsitnik mentioned this pull request Mar 28, 2023

Is it possible to control the order of an option validations? #1823

Closed

	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)
	{
	_unmatchedTokens ??= new List<Token>();
	_unmatchedTokens.AddRange(argumentResult.PassedOnTokens);
	}

Conversation

adamsitnik commented Jan 24, 2023

Uh oh!

Uh oh!

Uh oh!

adamsitnik Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

jonsequitur Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonsequitur commented Jan 24, 2023

Uh oh!

adamsitnik Jan 26, 2023

Choose a reason for hiding this comment

Uh oh!

adamsitnik Jan 26, 2023

Choose a reason for hiding this comment

Uh oh!

jonsequitur Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

adamsitnik Jan 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamsitnik commented Jan 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KalleOlaviNiemitalo commented Feb 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants