spectre.console: POSIX: Invalid handling of parameter values starting with double quotes (`"`), e.g. `--order-by='"quoted"` value'

This is the combined PR @sandreas across the various command line parsing issues, in case you want to build and try it out: https://github.com/spectreconsole/spectre.console/pull/1048

I’ve just created this PR https://github.com/spectreconsole/spectre.console/pull/1036 @sandreas (and will fix any build errors that may appear…)

As mentioned in the PR, the following three PR’s when taken jointly represent a significant enhancement to existing command line parsing behaviour: https://github.com/spectreconsole/spectre.console/pull/1036, https://github.com/spectreconsole/spectre.console/pull/1029, https://github.com/spectreconsole/spectre.console/pull/732

If it would be useful, I could merge them all into a local ‘preview’ branch for you to build and test Tone against?

Oh, please don’t do that!

Ok fair enough. I’m just in the middle of mental gymnastics, trying to think of all the unit test cases to support. And what not to support.

My PR isn’t far away now so you will be able to try it out with your Tone soon enough…

Regarding handling of quotes, hyphens and spaces - do you see any cases I’ve missed?

Maybe. I don’t see the following (maybe this is intended):

• "\"quoted\" string" (Quoted string that does not end with quotes)
• " \"quoted\"" (space then quoted string)
• "" (empty string)
• " " (space only)
• " " (multiple spaces - think I found a markdown bug in githubs parser - this also shows only one space 😉)
• "-" (dash only)
• "-size" (simple string with dash)
• "\t" (tab)
• "\r\n\t" (multiple whitespace that should never happen)
• "👋🏻" (an emoji)
• "🐎👋🏻🔥❤️" (multiple emojis)
• "\"🐎👋🏻🔥❤️\" is an emoji sequence" (multiple emojis quoted with additional text)

I’m sorry ¯\_(ツ)_/¯… If you think one of these cases is not necessary, just leave it out 😉

You sir, deserve a cookie!

Maybe it would be a good idea to implement the args parser framework / library agnostic.

class SubCommand {
    public string Name;
}
class Flag {
    public string Name;
    public string[] Aliases; // e.g. for shortName
}

class Option {
    public string Name;
    public string[] Aliases; // e.g. for shortName
    public string? Value; // not a real type because every arg is at first a string - this can be done by the parser
}
class Argument {
    public int index;
    public string? Value; // not a real type because every arg is at first a string - this can be done by the parser
}
class SubCommand {
    public IEnumerable<SubCommand> SubCommands {get; set;}
    public IEnumerable<Flag> Flags {get; set;}
    public IEnumerable<Option> Options {get; set;}
    public IEnumerable<Argument> Arguments {get; set;}
}
var tokenizer = new Tokenizer("=");
var subCommands = new List<SubCommand>();
// configure subcommands with arguments and stuff
var parser = new ArgsParser(subCommands, tokenizer);
var result = parser.Parse(args);

Just a thought…

As part of fixing the ‘flags cannot be set’ issue, I introduced a HadSeparator flag on command tree token which allows the CommandTreeParser some knowledge of context, see here: https://github.com/spectreconsole/spectre.console/blob/dded816d97f5a6cbf9c1831aa00fe46684d1ac5e/src/Spectre.Console.Cli/Internal/Parsing/CommandTreeToken.cs

however, this is more of a tactical implementation when compared to your design suggestion above. I don’t know the codebase well enough to refactor extensively yet, however the command tree parser looks easily backed by an interface and dependency injected.

For now, I’ll see what I can do tactically to fix the unit tests, so it can be released sooner than later. Then look at perhaps a new implementation of the parser following a statemachine or similar. Which can be injected and validated with existing unit tests, instead of attempting open heart surgery on the currrent codebase.

Ok, I can reproduce what you have reported across several bug reports and the discussion thread (https://github.com/spectreconsole/spectre.console/discussions/1025) with the following unit test:

Awesome 😃

I don’t mind having a look into what may be required to fix these failing tests, which if successful, would be a massive step forward in addressing many edge case issues regarding command line argument handling.

Great, thank you very much. Maybe we should keep in mind that fixing these unit-tests are not gonna fix the design problem, that EVERY argument is going through the method CommandTreeTokenizer.Parse:

• https://github.com/spectreconsole/spectre.console/blob/5b2b4375a45d060ab33236787213a8407a4d61ab/src/Spectre.Console.Cli/Internal/Parsing/CommandTreeTokenizer.cs#L50

This design problem does not only result in unnecessary parsings (Performance, Security), but not regarding the previous type of Token for an argument leads to other bugs:

• https://github.com/spectreconsole/spectre.console/issues/193
• https://github.com/spectreconsole/spectre.console/issues/825

So maybe it’s worth also taking a look at:

• https://github.com/spectreconsole/spectre.console/blob/5b2b4375a45d060ab33236787213a8407a4d61ab/src/Spectre.Console.Cli/Internal/Parsing/CommandTreeParser.cs#L35 Here, _configuration.Commands[*].Parameters (class CommandModel, IList<CommandInfo>, IList<CommandParameter>) is not regarded in any way, which in my opinion would be required to check, if the previous argument was one of these specific TokenTypes to check, if it has to be parsed or just used as is:
  • SUBCOMMAND (e.g commit in git commit)
  • FLAG (e.g. --force)
  • OPTION (e.g. --message or -m)
  • VALUE (e.g. "my message" in -m "my message")
  • ARGUMENT (e.g. some-file.txt in git add some-file.txt)
  • OPTION=VALUE (e.g. --message=MyMessage)
  • SHORTOPTION=VALUE (e.g. -mMyMessage)
  • MULTISHORTFLAG (e.g. -fd for --force --debug
• https://github.com/spectreconsole/spectre.console/blob/5b2b4375a45d060ab33236787213a8407a4d61ab/src/Spectre.Console.Cli/Internal/Parsing/CommandTreeTokenizer.cs#L33 Here I think the args array is parsed without knowing the TokenType of the arg before, so every arg is parsed by a tokenizer - even the VALUE args. As I described here, there are several assumptions that can be made depending on the TokenType of the arg before the current one and most of them do not need any parsing.

I’m very excited that now it is clear, what I meant 😃 Thank you for your hard work.

This is a pretty extreme edge case - relying on shells to act consistently with quotes is simply a non-starter as the link in the previous issue talk about.

@phil-scott-78 While I think a problem in the Tokenizer / Parser logic is no extreme edge case any more and fixing this would take me deep knowledge about how spectre console parses, tokenizes and transfers args to Settings, am I really the right person to look at this?

I did not find any unit tests for the tokenizer… did I miss them? If not, how can I ensure I don’t break something when I try to fix this?

Sorry, I just want to prevent putting much work into something that is not useful to anyone.

I took a look some more in this. I still comes down to how each shell is gonna handle the quotes and each shell is wildly different. Cmd.exe, pwsh, and even running through the debugger all vary wildly in the scenarios you posted with no consistency. But, on your shell and only this shell it does include the quotes in the args and theoretically we could include them. Went ahead and created a unit test shows the failure.

    [Fact]
    public void Quote_Tests()
    {
        // Given
        var app = new CommandAppTester();
        app.Configure(config =>
        {
            config.PropagateExceptions();
            config.AddCommand<GenericCommand<FooCommandSettings>>("test");
        });

        // When
        var result = app.Run(new[]
        {
            "test", "\"Rufus\"",
        });

        // Then
        result.ExitCode.ShouldBe(0);
        result.Settings.ShouldBeOfType<FooCommandSettings>().And(foo => foo.Qux.ShouldBe("\"Rufus\""));
    }

This is a pretty extreme edge case - relying on shells to act consistently with quotes is simply a non-starter as the link in the previous issue talk about. If you or anyone else want to give fixing this bug a shot, consider it up for grabs though, and you are welcome to see if you can’t get the parser updated to take this scenario into account. But personally, this is not a behavior I’d ever rely on for end users and I would highly recommend something other than nested quotes in the arguments if you are going to have an app running in multiple shells…