Issue 2279

[samcoppock]

Fix issue #2279

The test covers both the --option value and --option=value formats

[samcoppock]

@dotnet-policy-service agree

[KalleOlaviNiemitalo]

Fix issue 2279

This syntax is not recognised by GitHub. Please see Linking a pull request to an issue

[KalleOlaviNiemitalo]

Does this new test demonstrate the bug, i.e. fail if the bug is not fixed? The call stack of the exception in the issue included ParseResult.GetCompletions, but this test does not seem to call that function.

[samcoppock]

Ive checked and surprisingly - no it does not.

when i manually run the following code manually ( with or without a debugger ) it throws an error before I apply my fix ( only if the argument has a = symbol separating the key value ) and after I apply my fix it works. - IE exactly what it should do.

` CliRootCommand _outerCommand = new CliRootCommand
{
new CliCommand("inner")
{
new CliOption("--optionOne"),
new CliOption("--optionTwo")
}
};

    var result = _outerCommand.Parse("ou\\=ter inner --optionOne argument1 --optionTwo=argument2");
    result.GetCompletions();

`

Within my test I have almost exactly the same code - the only differences are _outerCommand is now named command and the command line string is now defined in a variable before the command is called.
neither of these issues should have changed it.

However when i reverted my fix the test still worked.

I tried adding a check to detect the 'InvalidOperationException' and it made no difference

I tried the following test code and its not failing without my fix being applied ( note i added the manual "throw new InvalidOperationException("hello");" to prove that my test is correctly detecting errors, I also ran it without this line )

` public void CommandLineText_is_parsed_when_option_is_in_name_equals_sign_value_format()
{

        Assert.Throws<InvalidOperationException>(methodThatThrows);
            
        void methodThatThrows()
        {
            CliRootCommand command = new CliRootCommand
            {
                new CliCommand("inner")
                {
                    new CliOption<string>("--optionOne"),
                    new CliOption<string>("--optionTwo")
                }
            };

            var commandLine = "outer inner --optionOne argument1 --optionTwo=argument2";

            var parseResult = command.Parse(commandLine);

            throw new InvalidOperationException("hello");

        }
            
    }`

I dont know what to do here or why my functionally identical code is behaving differently when it runs in test.
Advice would be appreciated.

[KalleOlaviNiemitalo]

Perhaps the argument of GetCompletions makes a difference. In #2279 (comment),

(Note: I found the easiest way to actually reproduce this was by running this with the arguments of: [suggest:30] "CompletionCrashRepro --test=asd" rather than actually doing tab completion.)

which would presumably call GetCompletions(30). Your code above calls result.GetCompletions() instead.

The length of CompletionCrashRepro --test=asd is 31 characters. I think 30 then means the insertion point would have been between --test=as and d.

[samcoppock]

also
i tried adding 'System.Environment.Exit(2);' to 'GetWordToComplete' in 'CompletionContext.cs' and running the test and non of them fail which shows that none of the tests run this piece of code.

They do exit when running the code manually.

[KalleOlaviNiemitalo]

I suggest you run the test in a debugger and trace why it doesn't go to GetWordToComplete.

It might be related to how the CliRootCommand constructor uses CliRootCommand.ExecutableName as the name of the command. CliRootCommand derives that from Environment.GetCommandLineArgs()[0], which would differ between the test runner process and your experimental application.

[samcoppock]

test fixed - its working properly now.
please finish reviwing

[samcoppock]

@KalleOlaviNiemitalo

Tests are fixed, please review again

[elgonzo]

My apologies for barging into the conversion like that, but please note that this is not a proper fix, as it is introducing another bug. = can be a valid character in an option/argument value, however the PR and the test included therein do not consider this fact.

Without the suggested PR, a CLI like outer inner --optionOne -=Yay=- would be consumed without any troubles. But with the PR here, an exception will be thrown in the ParseResult.WillAcceptAnArgument method.

To reproduce, it is enough to change the CLI string the test is using from

var commandLine = "inner --optionOne argument1 --optionTwo=argument2";

to

var commandLine = "inner --optionOne -=Yay=-";

It is important to note that the occurrence of this issue is conditional on the option/argument value containing = being the last token in the CLI string.

A proper fix would ideally also address the related issue that the GetCompletions() method seems to not like the last option/argument value in a CLI string having white-spaces when the CLI parameters have been passed to the Parse method as a single string only. Regardless of whether this PR is applied or not, parseResult.GetCompletions() will throw when a commandline string like "inner --optionOne \"foo bar\"" is provided.

[samcoppock]

My apologies for barging into the conversion like that, but please note that this is not a proper fix, as it is introducing another bug. = can be a valid character in an option/argument value, however the PR and the test included therein do not consider this fact.

Without the suggested PR, a CLI like outer inner --optionOne -=Yay=- would be consumed without any troubles. But with the PR here, an exception will be thrown in the ParseResult.WillAcceptAnArgument method.

To reproduce, it is enough to change the CLI string the test is using from

var commandLine = "inner --optionOne argument1 --optionTwo=argument2";

to

var commandLine = "inner --optionOne -=Yay=-";

It is important to note that the occurrence of this issue is conditional on the option/argument value containing = being the last token in the CLI string.

A proper fix would ideally also address the related issue that the GetCompletions() method seems to not like the last option/argument value in a CLI string having white-spaces when the CLI parameters have been passed to the Parse method as a single string only. Regardless of whether this PR is applied or not, parseResult.GetCompletions() will throw when a commandline string like "inner --optionOne \"foo bar\"" is provided.

I did think of that

the = character is a special character which has to be escaped if used on the CLI

https://mywiki.wooledge.org/BashGuide/SpecialCharacters

I did test this during development but did not include a test for this

[elgonzo]

the = character is a special character which has to be escaped if used on the CLI

Incorrect in the sense that this is not generally true. Whether = is a special character is specific to particular shells or particular shell commands only (or whatever particular script language is being used to execute a program while passing commandline arguments to it). For example, neither the command prompts of cmd.exe nor Powershell (arguably the two most popular shells in the Windows world) treat = as a special character when they appear as part of the commandline parameters for a program invocation.

[KalleOlaviNiemitalo]

Even if = is a special character for the shell and has to be escaped to make the shell treat it as a literal character -- this escaping is then consumed by the shell and not visible to the System.CommandLine library. So the treatment of incoming characters in System.CommandLine should not depend on whether they are special for a shell. (In contrast, the output of dotnet-suggest could usefully depend on such things, if it knows which shell is being used.)

[samcoppock]

var commandLine = "inner --optionOne -=Yay=-";

the "var commandLine = "inner --optionOne -=Yay=-";" issue has been fixed

[samcoppock]

@KalleOlaviNiemitalo
@elgonzo
@radical
@shanselman

can you guys take another look at this please

[jonsequitur]

We generally avoid the use of Assert.IsTrue in this codebase as it provides very little useful feedback when a test fails. Also, when there are multiple assertions, only the first failure is seen in the output, without feedback about whether the other assertions pass or fail.

I'd suggest changing the assertions to the following, which addresses both concerns:

parseResult.Tokens.Should().BeEquivalentSequenceTo("--optionOne", "argument1", "--optionTwo", "argument2");

[samcoppock]

I tried

parseResult.Tokens.Should().BeEquivalentSequenceTo("--optionOne", "argument1", "--optionTwo", "argument2");

and i get this error

Expected item[0] to be System.String, but found System.CommandLine.Parsing.CliToken

I also tried this

parseResult.Tokens.Should().BeEquivalentSequenceTo( new CommandLine.Parsing.CliToken("inner", CommandLine.Parsing.CliTokenType.Command, null), new CommandLine.Parsing.CliToken("--optionOne", CommandLine.Parsing.CliTokenType.Command, null), new CommandLine.Parsing.CliToken("argument1", CommandLine.Parsing.CliTokenType.Command, null), new CommandLine.Parsing.CliToken("--optionTwo", CommandLine.Parsing.CliTokenType.Command, null), new CommandLine.Parsing.CliToken("argument2", CommandLine.Parsing.CliTokenType.Command, null) );

and got the following error

Expected object to be inner, but found inner.
Expected object to be --optionOne, but found --optionOne.
Expected object to be argument1, but found argument1.
Expected object to be --optionTwo, but found --optionTwo.
Expected object to be argument2, but found argument2.

Any ideas how to make this work?

[jonsequitur]

This test name is a little hard to understand, and CommandLinetext_is_parsed isn't very specific about the expectation. I think the expectation is that the tokens don't include the delimiters? (Also, that it doesn't throw, but that typically goes without saying.)

A more sentence-style test name is encouraged, e.g. something like When_equals_sign_is_used_as_delimiter_then_it_is_not_included_in_tokens.

Some of these tests seem to differ only by the arguments being parsed, so combining them into a [Theory] might give you fewer tests to name.