Inherited commands for processing global options

[patriksvensson]

Discussed in spectreconsole/spectre.console#1831

Minimal reproducable example

using Spectre.Console;
using Spectre.Console.Cli;

var app = new CommandApp<DefaultCommand>();
app.Configure(static config =>
{
    config.PropagateExceptions();
    config.ValidateExamples();

    config.AddBranch<UserSettings>("user", static user =>
    {
        user.AddCommand<ListUsers>("list");
    });
});

return await app.RunAsync(args);

public class AppSettings : CommandSettings
{
    [CommandOption("-d|--database-path")]
    public string? DatabasePath { get; set; }
}

public abstract class AppCommand<TSettings> : AsyncCommand<TSettings> where TSettings : AppSettings
{
    public override async Task<int> ExecuteAsync(CommandContext context, TSettings settings)
    {
        AnsiConsole.WriteLine($"Database: {settings.DatabasePath ?? "N/A"}");
        return await ExecuteCommandAsync(context, settings);
    }

    protected abstract Task<int> ExecuteCommandAsync(CommandContext context, TSettings settings);
}


public sealed class DefaultCommand : AppCommand<AppSettings>
{
    protected override Task<int> ExecuteCommandAsync(
        CommandContext context,
        AppSettings settings)
    {
        return Task.FromResult(0);
    }
}

public abstract class UserSettings : AppSettings
{
}

public sealed class ListUsersSettings : UserSettings
{
}

public sealed class ListUsers : AppCommand<ListUsersSettings>
{
    protected override async Task<int> ExecuteCommandAsync(
        CommandContext context,
        ListUsersSettings settings)
    {
        AnsiConsole.MarkupLine("Hello [green]World[/]");
        return await Task.FromResult(0);
    }
}

Output

10:25:30 ❯ dotnet run -- --help
USAGE:
    SpectreInh.dll [OPTIONS] [COMMAND]

OPTIONS:
    -h, --help             Prints help information
    -d, --database-path

COMMANDS:
    user

10:25:44 ❯ dotnet run -- user --help
DESCRIPTION:
Manage users in the database

USAGE:
    SpectreInh.dll user [OPTIONS] <COMMAND>

OPTIONS:
    -h, --help             Prints help information
    -d, --database-path

COMMANDS:
    list

10:25:57 ❯ dotnet run -- user list --help
USAGE:
    SpectreInh.dll user list [OPTIONS]

OPTIONS:
    -h, --help    Prints help information

10:26:34 ❯ dotnet run -- -d foo
Database: foo

10:27:01 ❯ dotnet run -- user list -d foo
Database: N/A
Hello World

[CryoViking]

I'm new to this codebase so I thought I would have a look through the code and run it through a debugger. I arrived at:
Spectre.Console.Cli.Internal.Binding.CommandValueResolver

    public static CommandValueLookup GetParameterValues(CommandTree? tree, ITypeResolver resolver)
    {
        var lookup = new CommandValueLookup();
        var binder = new CommandValueBinder(lookup);

        CommandValidator.ValidateRequiredParameters(tree);

        while (tree != null)
        {
            // Process unmapped parameters.
            foreach (var parameter in tree.Unmapped)
            {
                // Got a value provider?
                if (parameter.ValueProvider != null)
                {
                    var context = new CommandParameterContext(parameter, resolver, null);
                    if (parameter.ValueProvider.TryGetValue(context, out var result))
                    {
                        result = ConvertValue(resolver, lookup, binder, parameter, result);

                        lookup.SetValue(parameter, result);
                        CommandValidator.ValidateParameter(parameter, lookup, resolver);
                        continue;
                    }
                }

                if (parameter.IsFlagValue())
                {
                    // Set the flag value to an empty, not set instance.
                    var instance = Activator.CreateInstance(parameter.ParameterType);
                    lookup.SetValue(parameter, instance);
                }
                else
                {
                    // Is this an option with a default value?
                    if (parameter.DefaultValue != null)
                    {
                        var value = parameter.DefaultValue?.Value;
                        value = ConvertValue(resolver, lookup, binder, parameter, value);

                        binder.Bind(parameter, resolver, value);
                        CommandValidator.ValidateParameter(parameter, lookup, resolver);
                    }
                    else if (Nullable.GetUnderlyingType(parameter.ParameterType) != null ||
                             !parameter.ParameterType.IsValueType)
                    {
                        lookup.SetValue(parameter, null);
                    }
                }
            }

            // Process mapped parameters.
            foreach (var mapped in tree.Mapped)
            {
                if (mapped.Parameter.WantRawValue)
                {
                    // Just try to assign the raw value.
                    binder.Bind(mapped.Parameter, resolver, mapped.Value);
                }
                else
                {
                    if (mapped.Parameter.IsFlagValue() && mapped.Value == null)
                    {
                        if (mapped.Parameter is CommandOption option && option.DefaultValue != null)
                        {
                            // Set the default value.
                            binder.Bind(mapped.Parameter, resolver, option.DefaultValue?.Value);
                        }
                        else
                        {
                            // Set the flag but not the value.
                            binder.Bind(mapped.Parameter, resolver, null);
                        }
                    }
                    else
                    {
                        object? value;
                        var converter = GetConverter(lookup, binder, resolver, mapped.Parameter);
                        var mappedValue = mapped.Value ?? string.Empty;
                        try
                        {
                            value = converter.ConvertFrom(mappedValue);
                        }
                        catch (Exception exception) when (exception is not CommandRuntimeException)
                        {
                            throw CommandRuntimeException.ConversionFailed(mapped, converter.TypeConverter, exception);
                        }

                        // Assign the value to the parameter.
                        binder.Bind(mapped.Parameter, resolver, value);
                    }
                }

                // Got a value provider?
                if (mapped.Parameter.ValueProvider != null)
                {
                    var context = new CommandParameterContext(mapped.Parameter, resolver, mapped.Value);
                    if (mapped.Parameter.ValueProvider.TryGetValue(context, out var result))
                    {
                        lookup.SetValue(mapped.Parameter, result);
                    }
                }

                CommandValidator.ValidateParameter(mapped.Parameter, lookup, resolver);
            }

            tree = tree.Next;
        }

        return lookup;
    }

I noticed the that parameter remained Unmapped in the tree object, which resulted in:

                    else if (Nullable.GetUnderlyingType(parameter.ParameterType) != null ||
                             !parameter.ParameterType.IsValueType)
                    {
                        lookup.SetValue(parameter, null);
                    }

This branch of logic being called. I might be looking up the wrong tree, and not properly understanding what lookup.SetValue(...) is trying to do, but it passing null looks wrong. Especially when it does have a ParameterType that is System.String

I'm probably leading people on a false chase with this since I'm very unfamiliar with the codebase.

[CryoViking]

The issue might also be that it's getting to the unmapped collection in the tree to begin with. Instead of the mapped collection.

[CryoViking]

Have finally determined that it gets to CommandExecutor.cs:138:

            if (lastParsedLeaf != null && lastParsedCommand != null &&
            lastParsedCommand.IsBranch && !lastParsedLeaf.ShowHelp &&
            lastParsedCommand.DefaultCommand != null)
            {
                // Adjust for any parsed remaining arguments by
                // inserting the the default command ahead of them.
                var position = tokenizerResult.Tokens.Position;
                foreach (var parsedRemaining in parsedResult.Remaining.Parsed)
                {
                    position--;
                    position -= parsedRemaining.Count(value => value != null);
                }
                position = position < 0 ? 0 : position;

                // Insert this branch's default command into the command line
                // arguments and try again to see if it will parse.
                var argsWithDefaultCommand = new List<string>(args);
                argsWithDefaultCommand.Insert(position, lastParsedCommand.DefaultCommand.Name);

                (parsedResult, tokenizerResult) = InternalParseCommandLineArguments(model, settings, argsWithDefaultCommand);
            }

With the it being in the Remaining.Parsed collection and because it isn't the DefaultCommand and it isn't a branch, it doesn't process the remaining parsed values, which tells me that it should get mapped at some point, but as to when it should be mapped, I'm unsure.

At this point I don't know enough about the design of the library to come to a solid conclusion as to where it should be.

[CryoViking]

Will also note that I just tried:

public sealed class ListUsersSettings : UserSettings
{
  [CommandOption("-s|--seomthing-else")]
  [DefaultValue("something")]
  public string? SomethingElse { get; set; }
}

public sealed class ListUsers : AppCommand<ListUsersSettings>
{
  protected override async Task<int> ExecuteCommandAsync(
      CommandContext context,
      ListUsersSettings settings)
  {
    AnsiConsole.WriteLine($"Exists? {settings.SomethingElse}");
    AnsiConsole.MarkupLine("Hello [green]World[/]");
    return await Task.FromResult(0);
  }
}

For the ListUsers command, and the following output:

❯ dotnet run -- user list -d app.db -s hello
Database: A test
Exists? hello
Hello World

Suggests that the top-level global options aren't being Mapped to any of the commands nor are being processed with the default command (rightly so because it's not the default command). With that I'll have to bow out for now and let smarter minds take it further.

[CryoViking]

Inside file: CommandInfoExtensions.cs

    public static bool AllowParentOption(this CommandInfo command, CommandOption option)
    {
        // Got an immediate parent?
        if (command?.Parent != null)
        {
            // Is the current node's settings type the same as the previous one?
            if (command.SettingsType == command.Parent.SettingsType)
            {
                var parameters = command.Parent.Parameters.OfType<CommandOption>().ToArray();
                if (parameters.Length > 0)
                {
                    foreach (var parentOption in parameters)
                    {
                        // Is this the same one?
                        if (option.HaveSameBackingPropertyAs(parentOption))
                        {
                            // Is it part of the same settings class.
                            if (option.Property.DeclaringType == command.SettingsType)
                            {
                                // Allow it.
                                return true;
                            }

                            // Don't allow it.
                            return false;
                        }
                    }
                }
            }
        }

        return false;
    }

Identified that is failing on this check here:

 if (command.SettingsType == command.Parent.SettingsType)

I tested with:

using System.ComponentModel;
using Spectre.Console;
using Spectre.Console.Cli;

namespace MinimalReproduction;

public class Program
{
  public static async Task<int> Main(string[] args)
  {
    var app = new CommandApp<DefaultCommand>();
    app.Configure(static config =>
    {
      config.PropagateExceptions();
      config.ValidateExamples();

      config.AddBranch<AppSettings>("user", static user =>
      {
        user.AddCommand<ListUsers>("list");
      });
    });

    return await app.RunAsync(args);
  }
}

public class AppSettings : CommandSettings
{
  [CommandOption("-d|--database-path")]
  public string? DatabasePath { get; set; }
}

public abstract class AppCommand<TSettings> : AsyncCommand<TSettings> where TSettings : AppSettings
{
  public override async Task<int> ExecuteAsync(CommandContext context, TSettings settings)
  {
    AnsiConsole.WriteLine($"Database: {settings.DatabasePath ?? "N/A"}");
    return await ExecuteCommandAsync(context, settings);
  }

  protected abstract Task<int> ExecuteCommandAsync(CommandContext context, TSettings settings);
}


public sealed class DefaultCommand : AppCommand<AppSettings>
{
  protected override Task<int> ExecuteCommandAsync(
      CommandContext context,
      AppSettings settings)
  {
    return Task.FromResult(0);
  }
}

public abstract class UserSettings : AppSettings
{
}

public sealed class ListUsersSettings : AppSettings
{
  [CommandOption("-s|--something-else")]
  [DefaultValue("something")]
  public string? SomethingElse { get; set; }
}

public sealed class ListUsers : AppCommand<AppSettings>
{
  protected override async Task<int> ExecuteCommandAsync(
      CommandContext context,
      AppSettings settings)
  {
    // AnsiConsole.WriteLine($"Exists? {settings.SomethingElse}");
    AnsiConsole.MarkupLine("Hello [green]World[/]");
    return await Task.FromResult(0);
  }
}

And got this result:

❯ dotnet run -- user list -d app.db
Database: app.db
Hello World

Which is kind of the expected result but also not really because it means that that a inherited Settings tree can't be built.

[CryoViking]

I am back again, I decided to have a closer look at the calling function in CommandModelBuilder.cs:

    private static IEnumerable<CommandParameter> GetParameters(CommandInfo command)
    {
        var result = new List<CommandParameter>();
        var argumentPosition = 0;

        // We need to get parameters in order of the class where they were defined.
        // We assign each inheritance level a value that is used to properly sort the
        // arguments when iterating over them.
        IEnumerable<OrderedProperties> GetPropertiesInOrder()
        {
            var current = command.SettingsType;
            var level = 0;
            var sortOrder = 0;
            while (current.BaseType != null)
            {
                yield return new OrderedProperties(level, sortOrder, current.GetProperties(BindingFlags.DeclaredOnly | BindingFlags.Instance | BindingFlags.Public));
                current = current.BaseType;

                // Things get a little bit complicated now.
                // Only consider a setting's base type part of the
                // setting, if there isn't a parent command that implements
                // the setting's base type. This might come back to bite us :)
                var currentCommand = command.Parent;
                while (currentCommand != null)
                {
                    if (currentCommand.SettingsType == current)
                    {
                        level--;
                        break;
                    }

                    currentCommand = currentCommand.Parent;
                }

                sortOrder--;
            }
        }

        var groups = GetPropertiesInOrder();
        foreach (var group in groups.OrderBy(x => x.Level).ThenBy(x => x.SortOrder))
        {
            var parameters = new List<CommandParameter>();

            foreach (var property in group.Properties)
            {
                if (property.IsDefined(typeof(CommandOptionAttribute)))
                {
                    var attribute = property.GetCustomAttribute<CommandOptionAttribute>();
                    if (attribute != null)
                    {
                        var option = BuildOptionParameter(property, attribute);

                        // Any previous command has this option defined?
                        if (command.HaveParentWithOption(option))
                        {
                            // Do we allow it to exist on this command as well?
                            if (command.AllowParentOption(option))
                            {
                                option.IsShadowed = true;
                                parameters.Add(option);
                            }
                        }
                        else
                        {
                            // No parent have this option.
                            parameters.Add(option);
                        }
                    }
                }
                else if (property.IsDefined(typeof(CommandArgumentAttribute)))
                {
                    var attribute = property.GetCustomAttribute<CommandArgumentAttribute>();
                    if (attribute != null)
                    {
                        var argument = BuildArgumentParameter(property, attribute);

                        // Any previous command has this argument defined?
                        // In that case, we should not assign the parameter to this command.
                        if (!command.HaveParentWithArgument(argument))
                        {
                            parameters.Add(argument);
                        }
                    }
                }
            }

            // Update the position for the parameters.
            foreach (var argument in parameters.OfType<CommandArgument>().OrderBy(x => x.Position))
            {
                argument.Position = argumentPosition++;
            }

            // Add all parameters to the result.
            foreach (var groupResult in parameters)
            {
                result.Add(groupResult);
            }
        }

        return result;
    }

and noticed that if I comment out the following like so:

                            // Do we allow it to exist on this command as well?
                            // if (command.AllowParentOption(option))
                            // {
                            option.IsShadowed = true;
                            parameters.Add(option);
                            // }

Then the output of the program is as desired on the minimal reproduce-able version , allowing the inheritance tree of the settings to work.

My questions would then be this:
Why do we need to see if we allow it exist on this command as well? What is the purpose? We have already previously checked if there is at least one parent in the inheritance tree with the option.

If we do need to check if we allow it on this command as well, what does that really mean?
CommandInfoExtensions.AllowParentOption(...) only checks the parent objects and not the whole inheritance tree.
Do we need to loop like CommandInfoExtensions.HaveParentWithOption(...)?

    public static bool HaveParentWithOption(this CommandInfo command, CommandOption option)
    {
        var parent = command?.Parent;
        while (parent != null)
        {
            foreach (var parentOption in parent.Parameters.OfType<CommandOption>())
            {
                if (option.HaveSameBackingPropertyAs(parentOption))
                {
                    return true;
                }
            }

            parent = parent.Parent;
        }

        return false;
    }

Once the logic of that function is figured out, and I can't answer that because I don't have an idea of why the AllowParentOption function exists, the fix should be fairly trivial I should think.

[CryoViking]

I will note that removing the call to AllowParentOption does fail 26 of the current tests. So it is critical to other features, but it's blocking this particular case, so maybe when this gets resolved, part of that process is to add a new test case.

[CryoViking]

Hmmm.... I have a question... why does the library limit the parents options in general? Even in the test cases, branch/default options don't show in the help menus for the sub/final commands.

I'm failing those test cases because for example:

    /usr/local/share/dotnet/sdk/9.0.300/Microsoft.TestPlatform.targets(48,5): error TESTERROR:
      Spectre.Console.Tests.Unit.Cli.CommandAppTests+Help.Should_Output_Leaf_Correctly (37ms): Error Message: VerifyException : Directory: /Users/cryosis/AutomataCrypt/OpenSource/spe
      ctre.console/src/Tests/Spectre.Console.Cli.Tests/Expectations/Help
      NotEqual:
        - Received: Leaf.Output.DotNet9_0.received.txt
          Verified: Leaf.Output.verified.txt

      FileContent:

      NotEqual:

      Received: Leaf.Output.DotNet9_0.received.txt
      DESCRIPTION:
      The lion command

      USAGE:
          myapp cat [LEGS] lion <TEETH> [OPTIONS]

      ARGUMENTS:
          <TEETH>    The number of teeth the lion has

      OPTIONS:
                                   DEFAULT
          -h, --help                                   Prints help information
          -a, --alive                                  Indicates whether or not the animal is alive
          -n, --name <VALUE>
              --agility <VALUE>    10                  The agility between 0 and 100
          -c <CHILDREN>                                The number of children the lion has
          -d <DAY>                 Monday, Thursday    The days the lion goes hunting
          -w, --weight [WEIGHT]                        The weight of the lion, in kgs
      Verified: Leaf.Output.verified.txt
      DESCRIPTION:
      The lion command

      USAGE:
          myapp cat [LEGS] lion <TEETH> [OPTIONS]

      ARGUMENTS:
          <TEETH>    The number of teeth the lion has

      OPTIONS:
                                   DEFAULT
          -h, --help                                   Prints help information
          -c <CHILDREN>                                The number of children the lion has
          -d <DAY>                 Monday, Thursday    The days the lion goes hunting
          -w, --weight [WEIGHT]                        The weight of the lion, in kgs

The help menu has started showing all options from myapp, cat and lion, as well as the arguments (to be expected) however the test strips the parent options from the command. Is that by design or has that slipped through into the test and made the test flawed?

Is it intended behaviour to not have the branch/global command options show in the help menu?

P.S. I have seen this same behaviour scattered through out some of the other tests as well, where parent options don't show in the help menu.

[CryoViking]

@patriksvensson any chance you can point me in the desired direction for applying a bug fix, because from where I'm looking, it looks like the tests are wrong but then that would point towards there being a potential desired design that does not allow for options from parent branches to be used in child branches.

I'm happy to put in the time to fix the tests and the code if that's what's needed but that then appears to be more than a small isolated fix since that changes the nature of potentially so many other use cases.