Response file parsing behavior change

[tannergooding]

Between 2.0.0-beta3.22114.1 (good) and 2.0.0-beta4.22272.1 (bad) the parsing behavior for response files has changed.

Notably, ClangSharp (https://github.com/dotnet/clangsharp) expects the following to be treated as a single argument: Module32First(HANDLE, LPMODULEENTRY32):BOOL=@Module32FirstA

However, starting with the newer version it is now interepreted as two arguments:

• Module32First(HANDLE,
• LPMODULEENTRY32):BOOL=@Module32FirstA

This breaks ClangSharp and its ability to handle user input specific on the command line.

[jonsequitur]

This was by design (#1665) and an attempt to bring the behavior more in line with some of the more common response file parsing behaviors, but there's unfortunately little consistency out there and it's hard to say what's the right behavior. The good news is you can customize it to get the old behavior back.

There are details here as to what changed and why: #1750

Under the Response file format unification section, the updated parsing behaviors are explained. Under Custom token replacement, there's an example of a mechanism that will allow you to define your own response file behaviors if the new defaults don't work.

[jonsequitur]

Here's an example showing how to restore the old response file parsing behavior, so that each line is treated as a single token and doesn't need to be wrapped in quotes if it contains spaces:

        [Fact]
        public void Legacy_line_delimited_response_file_parsing_can_be_enabled_using_token_replacer()
        {
            var option = new Option<int>("--opt");
            var argument = new Argument<string>();

            var responseFile = CreateResponseFile(" --opt ", "123", "one two three");

            var parser = new CommandLineBuilder(new RootCommand { option, argument })

// Add this line to replace the new response file behavior:
                         .UseTokenReplacer(TryReadResponseFile)
// --------------------------------------------------------                         

                         .Build();

            var result = parser.Parse($"@{responseFile}");

            result.GetValueForArgument(argument)
                  .Should()
                  .Be("one two three");
        }

        private bool TryReadResponseFile(
            string filePath,
            out IReadOnlyList<string> newTokens,
            out string error)
        {
            try
            {
                newTokens = ExpandResponseFile(filePath).ToArray();
                error = null;
                return true;
            }
            catch (FileNotFoundException)
            {
                error = $"Response file not found: '{filePath}'";
            }
            catch (IOException e)
            {
                error = $"Error reading response file '{filePath}': {e}";
            }

            newTokens = null;
            return false;

            static IEnumerable<string> ExpandResponseFile(string filePath)
            {
                var lines = File.ReadAllLines(filePath);

                for (var i = 0; i < lines.Length; i++)
                {
                    var line = lines[i];

#if RescursiveResponseFileSupportIsNeeded
                    foreach (var p in SplitLine(line))
                    {
                        if (GetReplaceableTokenValue(p) is { } path)
                        {
                            foreach (var q in ExpandResponseFile(path))
                            {
                                yield return q;
                            }
                        }
                        else
                        {
                            yield return p;
                        }
                    }
#else // otherwise
                    foreach (var p in SplitLine(line))
                    {
                        yield return p;
                    }
#endif
                }
            }

            static string GetReplaceableTokenValue(string arg) =>
                arg.Length > 1 && arg[0] == '@'
                    ? arg.Substring(1)
                    : null;

            static IEnumerable<string> SplitLine(string line)
            {
                var arg = line.Trim();

                if (arg.Length == 0 || arg[0] == '#')
                {
                    yield break;
                }

                yield return arg;
            }
        }
    }

[tannergooding]

@jonsequitur, that's "a lot" of code for something I expect to be very common, especially in relation to file paths on Windows.

Is this something that could be reasonably be provided as a "built-in" but with a toggle to pick between "separate per-line" or "separate by space"?

[jonsequitur]

We did originally have such a toggle in place (without the open-ended extensibility mechanism). There were still cases that neither of the available options satisfied, though.

Probably orthogonal to this but discoverability of the rules was also a challenge. Having a single ruleset in theory should help converge on a standard. There isn't one today even among popular and well-established command line tools.

[jonsequitur]

There's a more minimalistic version here: https://github.com/dotnet/runtime/pull/76271/files

Note that this will not allow one response file to reference another.

[tannergooding]

There were still cases that neither of the available options satisfied, though.

Sure. But I expect the two most common options are "split by whitespace" and "split by newline".

Having to add 50-100 lines to pick up a syntax that had been part the default is a major pain point. It's not just a minor API tweak while adjusting to feedback and trying to find the ideal shape, but rather a fundamental change to one of the core parts of the external user interface provided by the library.

Probably orthogonal to this but discoverability of the rules was also a challenge. Having a single ruleset in theory should help converge on a standard.

This is very much an like "tabs vs spaces" where you'll have two (or more) competing standards effectively forever (https://xkcd.com/927/). Projects that have already been utilizing one format will likely continue utilizing it to avoid a major break for existing consumers, even if they switch over to System.CommandLine. For cases where this library doesn't match their current scenario, the "ease" in which they can make it match will likely dictate whether they adopt the library or look elsewhere.

There isn't one today even among popular and well-established command line tools.

What makes "separate by space" the right choice over "separate by newline" or vice-versa? As you've indicated, there are numerous tools that do each and there isn't a well-established standard so why make the switch after 3+ years?

The library is still in beta, but more and more tools have adopted or are trying to adopt the library and this is very much a painful break that has caused obvious ripple effects throughout with multiple patches having to go up and the original PR being referenced.

[jonsequitur]

Sure. But I expect the two most common options are "split by whitespace" and "split by newline".

This isn't the case, unfortunately. Within "separate by newline" there are several other nuances on which common implementations differ:

• Are whole-line comments supported?
• Are partial line comments supported?
• Are references to other response files supported within the line?

The above implementation is longer than the one linked to because it supports all three of these, as System.CommandLine always did for the "separate by newline" implementation.

[jonsequitur]

This is very much an like "tabs vs spaces" where you'll have two (or more) competing standards effectively forever

The goal was to try to avoid that by aligning more closely to an existing "standard," in this case MSBuild:

Response (.rsp) files are text files that contain MSBuild.exe command-line switches. Each switch can be on a separate line or all switches can be on one line. Comment lines are prefaced with a # symbol. The @ switch is used to pass another response file to MSBuild.exe.

(Again, these are all emergent rule sets, with little apparent attempt at standardization. Here's a short, incomplete list of some of Microsoft's own documented variations.)

[KathleenDollard]

@jonsequitur I think this is a hard problem, but...

Where things are emerging, the default is likely to be used, nudging folks toward more consistency. However, there is a large ecosystem and a lot of existing response files. I'd like to see us have something that allows the default to be just the one we suggest, and then allow folks like @tannergooding with a large install base to just flip additional switches to get the additional behavior. For example, a flagged enum. And yes, it's not two, it's an open set for the future so maybe we should leave an option for custom, but way more on the wish list.

I think this is also important to protect us against "betting on the wrong horse" with the default.

[tannergooding]

@jonsequitur, @KathleenDollard; there is another issue here in that the new logic has no way to express or escape quotes.

command-line-api/src/System.CommandLine/Parsing/Parser.cs

Line 40 in e26fc94

public static IEnumerable<string> SplitCommandLine(string commandLine)

unanimously looks for start/end pairs with no form of escaping and on top of that unanimously strips quotes from within the specified option.

This means that there is no way to express a command line option:
ID3D12Device5=SupportedOSPlatform("windows10.0.17763.0") and have the consuming application see it as such.

[jonsequitur]

@tannergooding, this looks to me like a pre-existing bug: #1758. Have you noticed a change in the behavior?