Argument validator did not run because of GetValue within a Command validator

[NPatch]

Not sure if this is a bug, but it's confusing.

I have an Argument<string[]> inputArg which has a validator and this argument is on a command which also has a validator. The Command Validator was calling commandResult.GetResult(inputArg).GetValue<string[]>(inputArg) within. The command line is "command asdf". The argument's validator is expecting one specific string word or 1 or more numbers.

The argument's validator was not being called. I moved through parsing etc and on to validation using the debugger and I noticed that at some point a conversion result was being produced and there's a part where validation is about to be called on that argument, but it doesn't because the code does sth like: _conversionResult ??= ValidateAndConvert(..).

So the argument's validator doesn't trigger. Afterwards I noticed that _conversionResult is being set when I call GetValue.

If I change the GetValue to commandResult.GetResult(inputArg).Tokens, this doesn't happen.

That said, there's not enough documentation on Validators and the only example on the MS docs uses GetValue, so a user will likely default to that, even if there's other examples buried in the Github Issues that uses Tokens instead.

Is this intentional or a bug?

[jonsequitur]

This sounds like a bug. Are you able to give a more complete code example that demonstrates the problem?

[NPatch]

Sure here it is:

internal class Program
{
    static void Main(string[] args)
    {
        string arg_line = @"command asdfg";
        AppMain(arg_line);
    }

    static void AppMain(string args)
    {
        RootCommand rootCommand = new RootCommand();

        Command command = new Command("command");

        Argument<string> inputArg = new Argument<string>("input");

        inputArg.Validators.Add(result =>
        {
            var value = result.Tokens.FirstOrDefault()?.Value;
           if (value == "asdfg")
            {
                result.AddError("Invalid value");
                return;
            }
        });

        command.Arguments.Add(inputArg);

        command.Validators.Add(commandResult =>
        {
            bool hasInput = !string.IsNullOrEmpty(commandResult.GetValue<string>(inputArg));
            
            if (!hasInput)
            {
                commandResult.AddError("Input argument is required.");
            }
        });

        rootCommand.Subcommands.Add(command);

        ParseResult res = rootCommand.Parse(args);
        res.Invoke();
    }
}

I hit this using the latest version, 2.0.0-beta5.25306.1.

Also for what it's worth, this doesn't happen in 2.0.0-beta4.22272.1 where the original code for this subcommand is from. I only migrated it to beta5. I have a project using beta4 and made a command line app to do various tests and add in more subcommands before messing with the large project. The existing subcommand already used FindResult.GetValueOrDefault in command. So after migration, it became like the above.

As for the flow, it will first go into the command.validator and through GetValue<string> it will call GetValueOrDefault which will do a ValidateAndConvert which caches the conversionResult. Then when it's time to ValidateArguments, it will go to argumentResult.GetArgumentConversionResult which has:

_conversionResult ??= ValidateAndConvert(useValidators: true);

and because of ??=, the cached _conversionResult gets returned, so it doesn't go through the argument validator.

Had to check "suppress jit optimizations on module load" in Options->Debugging->General to step inside GetArgumentConversionResult.

[NPatch]

I think this is related?! https://github.com/dotnet/command-line-api/pull/2416

due to the conversionResult being forced during parsing.

[jeffhandley]

We need to at least document the expectations for what is available to validators and how to get the value that is being validated (or other values). If there are unexpected behaviors found during that review/documentation, we can review the behaviors holistically to determine if a change is needed.

[jeffhandley]

We also need to review #2645 as part of that review.

[jeffhandley]

We're going to document the behavior. We will consider improvements to this area in the future.