Skip to content

Commit bf0aeb2

Browse files
committed
fix #2786
1 parent 12ad062 commit bf0aeb2

3 files changed

Lines changed: 64 additions & 8 deletions

File tree

src/System.CommandLine.Tests/ParsingValidationTests.cs

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,44 @@ public void A_custom_validator_can_be_added_to_a_command()
355355
.Contain("Options '--one' and '--two' cannot be used together.");
356356
}
357357

358+
[Fact]
359+
public void GetValue_in_command_validator_does_not_suppress_argument_validation_errors()
360+
{
361+
var fileArg = new Argument<FileInfo>("file");
362+
fileArg.AcceptExistingOnly();
363+
364+
var root = new RootCommand { fileArg };
365+
366+
root.Validators.Add(result =>
367+
{
368+
_ = result.GetValue(fileArg);
369+
});
370+
371+
var parseResult = root.Parse("nonexistent.xyz");
372+
373+
parseResult.Errors.Should().ContainSingle()
374+
.Which.Message.Should().Be(LocalizationResources.FileDoesNotExist("nonexistent.xyz"));
375+
}
376+
377+
[Fact]
378+
public void GetValue_in_command_validator_does_not_suppress_option_argument_validation_errors()
379+
{
380+
var fileOption = new Option<FileInfo>("--file");
381+
fileOption.AcceptExistingOnly();
382+
383+
var root = new RootCommand { fileOption };
384+
385+
root.Validators.Add(result =>
386+
{
387+
_ = result.GetValue(fileOption);
388+
});
389+
390+
var parseResult = root.Parse("--file nonexistent.xyz");
391+
392+
parseResult.Errors.Should().ContainSingle()
393+
.Which.Message.Should().Be(LocalizationResources.FileDoesNotExist("nonexistent.xyz"));
394+
}
395+
358396
[Fact]
359397
public void A_custom_validator_can_be_added_to_an_option()
360398
{

src/System.CommandLine/Parsing/ArgumentResult.cs

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ namespace System.CommandLine.Parsing
1212
public sealed class ArgumentResult : SymbolResult
1313
{
1414
private ArgumentConversionResult? _conversionResult;
15+
private bool _validatorsHaveBeenRun;
1516
private bool _onlyTakeHasBeenCalled;
1617

1718
internal ArgumentResult(
@@ -31,8 +32,27 @@ internal ArgumentResult(
3132

3233
public bool Implicit { get; private set; }
3334

34-
internal ArgumentConversionResult GetArgumentConversionResult() =>
35-
_conversionResult ??= ValidateAndConvert(useValidators: true);
35+
internal ArgumentConversionResult GetArgumentConversionResult()
36+
{
37+
if (_conversionResult is not null)
38+
{
39+
if (!_validatorsHaveBeenRun && Argument.HasValidators)
40+
{
41+
// GetValueOrDefault was called before GetArgumentConversionResult,
42+
// which cached a conversion result without running validators.
43+
// Run them now so that validators like AcceptExistingOnly() are not skipped.
44+
_validatorsHaveBeenRun = true;
45+
for (var i = 0; i < Argument.Validators.Count; i++)
46+
{
47+
Argument.Validators[i](this);
48+
}
49+
}
50+
51+
return _conversionResult;
52+
}
53+
54+
return _conversionResult = ValidateAndConvert(useValidators: true);
55+
}
3656

3757
/// <summary>
3858
/// Gets the parsed value or the default value for <see cref="Argument"/>.
@@ -141,6 +161,8 @@ private ArgumentConversionResult ValidateAndConvert(bool useValidators)
141161
// => GetValueOrDefault => ValidateAndConvert (again)
142162
if (useValidators && Argument.HasValidators)
143163
{
164+
_validatorsHaveBeenRun = true;
165+
144166
for (var i = 0; i < Argument.Validators.Count; i++)
145167
{
146168
Argument.Validators[i](this);

src/System.CommandLine/Parsing/OptionResult.cs

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@ namespace System.CommandLine.Parsing
1111
/// </summary>
1212
public sealed class OptionResult : SymbolResult
1313
{
14-
private ArgumentConversionResult? _argumentConversionResult;
15-
1614
internal OptionResult(
1715
Option option,
1816
SymbolResultTree symbolResultTree,
@@ -54,15 +52,13 @@ internal OptionResult(
5452
/// </summary>
5553
/// <returns>The parsed value or the default value for <see cref="Option"/></returns>
5654
public T GetValueOrDefault<T>() =>
57-
ArgumentConversionResult
58-
.ConvertIfNeeded(typeof(T))
59-
.GetValueOrDefault<T>();
55+
GetResult(Option.Argument)!.GetValueOrDefault<T>();
6056

6157
internal bool IsArgumentLimitReached
6258
=> Option.Argument.Arity.MaximumNumberOfValues == (Implicit ? Tokens.Count - 1 : Tokens.Count);
6359

6460
internal ArgumentConversionResult ArgumentConversionResult
65-
=> _argumentConversionResult ??= GetResult(Option.Argument)!.GetArgumentConversionResult();
61+
=> GetResult(Option.Argument)!.GetArgumentConversionResult();
6662

6763
internal override bool UseDefaultValueFor(ArgumentResult argumentResult)
6864
{

0 commit comments

Comments
 (0)