Add ProcessStartInfo.ArgumentList

[mklement0]

^{Note: Updated into an API proposal, with feedback incorporated.}

Rationale

On Unix platforms, external executables (binaries) receive their arguments as an array of string literals, which makes for robust, predictable passing of arguments (see Linux system function execvp(), for instance).

Regrettably, in Windows there is no equivalent mechanism: a pseudo shell "command line" must be passed as a single string encoding all arguments, and it is up to the target executable itself to parse that line into individual arguments.

The ProcessStartInfo class currently only supports the Windows approach directly, by exposing a string Arguments property that expects the whole command line.

On Unix platforms, this means that even if you start out with an array of arguments, you must currently artificially assemble its elements into a single pseudo shell command line, only to have CoreFX split that back into an array of individual arguments behind the scenes so as to be able to invoke the platform-native process-creation function, which takes an array of arguments.

Not only is this an unnecessary burden on the user and inefficient, it is error-prone. It is easy to accidentally assemble a command-line string that does not round-trip as expected.

As a real-world use case, consider the ongoing quoting woes PowerShell experiences.
At least on Unix platforms, PowerShell should be able to simply pass the arguments that are the result of its parsing as-is to external utilities.

Having the ability to pass an array of arguments would be of benefit on Windows too, as it is fair to assume that the more typical use case is to build the desired arguments as a list / array rather than to piece together a single-string command line with intricate quoting.
(The latter should only be needed if you're passing an preexisting string through or if you're invoking an executable that has custom argument-parsing rules.)

Proposed API

Add a IReadOnlyList<String> ArgumentList property (conceptually, an array of argument string literals) to the ProcessStartInfo class, to complement the existing string Arguments (pseudo shell command line) property, and let each update the other lazily, on demand, when accessed:

• If .Arguments was (last) assigned to, do the following when .ArgumentList is accessed:
  Call ParseArgumentsIntoList(), which splits the string into individual arguments based on the rules for MS C++ programs and return the resulting list.

• If .ArgumentList was (last) assigned to, do the following when .Arguments is accessed:
  Synthesize the pseudo shell command line from the individual arguments and assign the result to using the above rules in reverse (enclose in "..." if a given argument has embedded whitespace, ..., as already implemented for internal use in System.PasteArguments.Paste()) and return the resulting string.

  • As @TSlivede proposes, it's worth extending the conversion algorithm to also double-quote arguments that contain ' (single quotes) lest they be interpreted as having syntactic function, which to some programs they do (e.g., Ruby, Cygwin).

That way, both .Arguments and .ArgumentList can be assigned to, and the respective other property contains the equivalent in terms of the official (MS C++) parsing rules.

A ProcessStartInfo instance constructed this way can therefore be used on all supported platforms:

• On Windows, pass the .Arguments property value to the CreateProcess() / ShellExecuteEx() Windows API functions, as before.

• On Unix platforms, pass the .ArgumentList property value via .ToArray() to ForkAndExecProcess().

Additionally, to complement the suggested behind-the-scenes conversion between the array form and the single-string form, public utility methods should be implemented that perform these conversions explicitly, on demand.

@atsushikan proposes the following signatures:

public static IReadOnlyList<String> SplitArguments(String commandLine) { ... }

public static String CombineArguments(IReadOnlyList<String> argumentList) { ... }

Open Questions

• How exactly should the existing, currently (effectively) private utility methods referenced above be surfaced publicly (namespace, class, signature)? Currently, they're in different classes, and one of them is internal (System.PasteArguments); @atsushikan suggests making them public methods of the System.Diagnostics.Process class.

Usage

// The array of arguments to pass.
string[] args = { "hello", "sweet world" };

// ---- New ProcessStartInfo.ArgumentList property.

// Assign it to the new .ArgumentList property of a ProcessStartInfo instance.
var psi = new ProcessStartInfo();
psi.ArgumentList = args;

// Accessing .Arguments now returns the pseudo shell command line that is the
// equivalent of the array of arguments:
//   @"hello ""sweet world"""
string pseudoShellCommandLine = psi.Arguments; 

// ---- New utility methods for conversion on demand.
//         EXACT NAMES AND SIGNATURES TBD.

// Arguments array (list) -> pseudo shell command line
string[] args = { "hello", "sweet world" };
string pseudoShellCommandLine = System.Diagnostics.Process.CombineArguments(args);

// Pseudo shell command line -> arguments array (list)
IReadOnlyList<string> argList = System.Diagnostics.Process.SplitArguments(@"hello ""sweet world""");

[jnm2]

I would find a utility method extremely useful as well for properly escaping an argv array into a single string.

[danmoseley]

/cc @atsushikan who has been interested in this in the past...

[danmoseley]

204522f 1d15089

[svick]

Few comments:

• The type shouldn't be array. .Net historically often used arrays for this kind of properties, but arrays have issues (e.g. directly mutating the array likely wouldn't have the desired effect). I think the type of the property should be one of the System.Collections.Generic collection interfaces (IEnumerable<string>, I(ReadOnly)List<string>, I(ReadOnly)Collection<string>). It's not clear to me which one is the best option.

• The name shouldn't be Argv. I think it's not very clear that Arguments and Argv are closely related and .Net generally doesn't use abbreviations ("DO NOT use abbreviations or contractions as part of identifier names."). Also, the name Argv is confusing, since it's short for "argument vector", but it wouldn't actually be a vector.

  I think a better name would be something like ArgumentCollection, ArgumentList or ArgumentArray (depending on what is the type of the property).

[jnm2]

+1 for IReadOnlyList<string>. For lazy evaluation I use IEnumerable<>, for non-lazy collections where the order (mapping of index to value) is not relevant I use IReadOnlyCollection<> because it has no indexer, and the natural choice in my mind here would be IReadOnlyList<string>.

[jnm2]

This is the implementation I've been using to escape a single argument, with test cases, based on reading the spec and spending a long time experimentally verifying. https://gist.github.com/jnm2/c5c840bf317605a40f5f56f944db4892

I wonder, is there anything you'd improve in this implementation besides accepting and joining multiple arguments?

[ghost]

I also think a standalone api for splitting and recombining argument strings would be the way to go. We should really have had that all along instead of welding them into other apis that launch processes or retreive the system command line. Bad separation of concerns there.

(Though also having this as part of ProcessStartInfo is justified on the grounds that the Unix implementation shouldn't have to do an unnecessary round-trip just to function.)

[ghost]

@jnm2 - We have an implementation in corefx (https://github.com/dotnet/corefx/blob/5d3e42f831ec3d4c5964040e235824f779d5db53/src/Common/src/System/PasteArguments.cs) - we're already using it for the Remote Invoke utility in corefx testing. It's probably implementing the same rules but feel free to check for differences.

[ghost]

I think IReadOnlyList<String> would be the one. It's customarily to use a "List" type when it's a sequence where order matters.

[jnm2]

It's probably implementing the same rules but feel free to check for differences.

Your implementation passes my tests, so that's cool. =)

I think null should be treated as string.Empty and escaped to "" to take up the space of the target argument. Otherwise all parameters get shifted over.

[mklement0]

@svick: argv was a nod to familiar historical usage, but I guess a more descriptive, .NET-conformant name makes sense.

@atsushikan:

(Though also having this as part of ProcessStartInfo is justified on the grounds that the Unix implementation shouldn't have to do an unnecessary round-trip just to function.)

Indeed, but I think this would be of benefit on Windows too, as it is fair to assume that the more typical use case is to build the desired arguments as a list / array rather than to piece together a single-string command line with intricate quoting.
(The latter should only be needed if you're passing an preexisting string through or if you're invoking an executable that has custom argument-parsing rules.)

[ghost]

Note too that argv on C++ includes argv[0], but ProcessStartInfo.Arguments excludes argv[0].

[TSlivede]

Related: https://github.com/dotnet/corefx/issues/21267
Please reuse one of the existing implementations for conversions in both directions :-)
(From list to single string and from single string to list)

[TSlivede]

Maybe the existing implementation for conversion from argument list to single string could be extended, so that tokens containing ' also get quoted.

Reason: Some applications mostly follow the MSVC rules, but additionally allow ' as a string delimiter (AFAIK cygwin, ruby). I see no disadvantage for apps that strictly follow MSVC rules, so it might be worth to also quote arguments containing '.

[TSlivede]

Proposal:
Add a string[] Argv property (array of argument literals) to complement the existing string Arguments (pseudo shell command line) property, and let each update the other on assignment

For performance reasons it might be better to update the other private member variable not on assignment, but to generate the complementing representation only if the specific get-accessor is called.

This way, the conversion is only performed if necessary.

[joperezr]

@mklement0 are you interested in putting up a formal api proposal for this so that we can submit this up for review? Here is a good example of how they should look: #13979 and here is more info on our proposals: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

[mklement0]

@joperezr: Sounds good - see dotnet/corefx#24073; I hope it is in the correct form and detailed enough.

[ghost]

@mklement0 - You can edit the formalized proposal into the top comment on this issue rather than creating a new issue. All the discussion is here...

My one thought is this: I just know that if this api is surfaced in this form, I'll find myself writing these helper methods over and over again. (I know this because I have been writing these helper methods (without BCL support) over and over again for years!)

  public static IReadOnlyList<String> SplitArguments(String commandLine)
  {
      return new ProcessStartInfo() { Arguments = commandLine }.ArgumentList;
  }

  public static String CombineArguments(IReadOnlyList<String> argumentList)
  {
      return new ProcessStartInfo() { ArgumentList = argumentList }.Arguments;
  }

Perhaps, the api should just be surfaced in this form rather than being accessible only through ProcessStartInfo. Of course, it'd be fine for ProcessStartinfo.ArgumentList to also exist for convenience - it would just call the above two.

Another advantage of doing it this way, is that these two apis could be exposed in System.Runtime.Extensions rather than System.Diagnostics.Process. The reason that's an advantage is that we have Environment.CommandLine api that's just crying to take advantage of this. But due to layering, it can't leverage this if it's up in System.Diagnostics.Process.

[jnm2]

Absolutely, please surface that API.

[mklement0]

@atsushikan:

Thanks - I've closed dotnet/corefx#24073 and have updated this issue with its contents.

I've also added the need for utility methods and incorporated the signatures of your helper methods as a suggestions.
We need more detail on how exactly to surface them publicly, however.

[ghost]

We need more detail on how exactly to surface them publicly, however.

I guess as static methods on the Process class itself. That's the placement that makes the most organizational sense to me. Unfortunately, it does mean that Environment.ComandLine can't just call it, but given that we already share the source code for the algorithm through the Common folder, we can still get the sharing benefit without actually moving the implementation.

A second candidate is System.Environment but looking through that, it seems like everything on it affects or is affected by the environment in some way. So it would be an outlier there.

[joperezr]

Thanks @mklement0 and @atsushikan , marking this ready for review.

[svick]

@atsushikan What is the use case for those helper methods? When are the resulting values useful, except for assigning them to the right ProcessStartInfo property?

[jnm2]

@svick For example, literally everywhere in Cake, escaping is done naively (assuming that tacking a quote onto each end is good enough without understanding the underlying mechanism). People need a go-to proper escaping mechanism.

[TSlivede]

@jnm2 Why do you need a conversion method for that? I didn't read the source code of Cake, but wouldn't it be much cleaner to just add elements to an argument list instead of manually escaping each argument and creating a single argument string?

I believe this is a perfect example of what @mklement0 said:

... it is fair to assume that the more typical use case is to build the desired arguments as a list / array rather than to piece together a single-string command line with intricate quoting.
(The latter should only be needed if you're passing an preexisting string through or if you're invoking an executable that has custom argument-parsing rules.)

So unless there are any nonstandard custom argument-parsing rules involved, I think these helper methods should not be used in that case. (IMHO an argument list is less error-prone than "to piece together a single-string")

As I don't really know Cake, I might have missed something - if there is any special reason why Cake can't just use an argument list, I'd be very interested about that!

[ghost]

@svick

SplitArguments is useful for parsing response files when writing command line tools.

CombineArguments - generating response files (useful in generating repro scenarios, or large command arguments to pass to another tool in a build system.)

There's also an issue of orthogonality. The algorithm has been part of the BCL since forever, but it's always been loaded down with unnecessary fine print - You can only access it for the OS command line, or to generate the arguments passed to the assembly main() method. Now, we're finding new uses for it that wasn't anticipated before and having to go through an api review process that wouldn't be necessarily if the core building blocks had been provided from the start without these artificial restrictions. I want to end that cycle here.

[terrajobst]

@mklement0

My concern was that choosing System.Collections.ObjectModel.Collection<string> as the type of the - get-only - .ArgumentList would make it potentially cumbersome to populate the collection of arguments, as it would have to be done one by one via the .Add() method.

You can easily work this around by using collection initialization which I'd argue is even more readable than using AddRange() or a naked setter:

var info = new ProcessStartInfo("bash")
{
    Arguments = {
        "-c",
        "echo hi"
    }
};

The primary reason to use Collection<string> is that this type doesn't expose implementation. If in the future we decided we want to sync the Arguments collection with the the flat string, we can return a derived type of Collection<string> that intercepts modifications. List<string> wouldn't allow that. Exposing a setter equally breaks that.

[jnm2]

So folks who build and pass around the arguments in a IReadOnlyList or ImmutableArray will have to foreach and add one at a time unless that is switched to a derived collection type. I'll probably add an AddRange extension method in projects like this. 👍

[mklement0]

@terrajobst:

I appreciate the explanation.

The syntax you demonstrate is indeed great for initializing from a literal and as simple as it gets.

The ability to initialize from a preexisting collection with something like .AddRange() would indeed make a nice complement, as @jnm2 suggests.

[terrajobst]

The ability to initialize from a preexisting collection with something like .AddRange() would indeed make a nice complement, as @jnm2 suggests.

That's fair but how often will this likely be the case in practice? In my own experience, I have centralized the process creation and string mangling in one place. And that's the place where I'd construct the startup info.

[jnm2]

I'm thinking of things like Cake and process utility classes I've written for build scripts and integration testing. But an extension method will go a long way and Collection<> can be subclassed later if there's demand.

[mklement0]

@terrajobst: Good point; I had actually not realized that the initializer even works with non-literal elements:

var info = new ProcessStartInfo("bash")
{
    ArgumentList = {
        "-c",
        // using an expression here (including a variable) works perfectly fine
        "echo " + Environment.GetEnvironmentVariable("USER") 
    }
};

Thinking more broadly, though (and I'm happy to take this tangent where it belongs if it has any merit), should a public void AddRange (System.Collections.Generic.IEnumerable<T> collection); method be added to type System.Collections.ObjectModel.Collection<T>?

[tobia]

I found this issue after a long search on how to spawn a process in dotnet (I'm new to the platform.) If I'm reading the code and the tests correctly, the current API is supposed to be used like this:

    ProcessStartInfo psi = new ProcessStartInfo("filename");
    psi.ArgumentList.Add("arg1");
    psi.ArgumentList.Add("arg2");

    // and so on, requiring a loop to pass an arbitrary array of arguments!

I would like to express that from the outside, this looks like a very convoluted and hidden way to set the arguments for a process execution, especially since the artificial and error-prone Arguments parameter is still prominently featured in the constructor.

[Anipik]

@tobia @terrajobst already addressed this issue in his comment

That's fair but how often will this likely be the case in practice? In my own experience, I have centralized the process creation and string mangling in one place. And that's the place where I'd construct the startup info.

and you can still use the initializer non-literal elements:

var info = new ProcessStartInfo("bash")
{
    ArgumentList = {
        "-c",
        // using an expression here (including a variable) works perfectly fine
        "echo " + Environment.GetEnvironmentVariable("USER") 
    }
};

or you can just provide the Arguments property.

since the artificial and error-prone Arguments parameter is still prominently featured in the constructor.

In windows, you need to provide the arguments as a string while starting the process. The main purpose of adding the ArgumentList property was for unix, where the arguments are provided through a list. So in order to avoid the unnecessary conversion of list->string->list. we introduced this property.

cc @danmosemsft

[daveyostcom]

I see this in documentation but not in any libraries on Mac.

[karelz]

What do you mean by that? What exactly happens?
Which version of .NET Core do you use?
Did you try a Hello world app using the API?

[daveyostcom]

Net4.7.2, netstandard2.0, netcoreapp2.2

ProcessStartInfo does not have an ArgumentList property.

[karelz]

That is not .NET Core version, but list of TFMs. The fist two won't have it, the last one will.
Create Hello World app targeting .NET Core 2.1+ and see for yourself.

[MrM40]

For any improvement in Arguments / ArgumentsList, please make sure it will include the support for parsing single quotes in the argument value (It's fine it has to be escaped). For the current it doesn't seem to be possible :-(

[jnm2]

@MrM40 Single quotes don't have to be escaped in general as far as I'm aware. The rules being followed aren't the rules of a specific shell like PowerShell but rather the rules of the https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments convention (on Windows).

[MrM40]

But still, the single quotes aren't parsed, escaped or non-escaped :-(

[jnm2]

@MrM40 Single quotes should be treated like any other character. At least, that's the long-standing argv convention on Windows. The only special characters are " and \. Are you seeing something different?

[ForNeVeR]

That would be a compatibility-breaking solution, because everyone is "okay"-ish with the way the command lines have been parsed for last years. For any new cross-platform development, you really really better use the new API with ArgumentList.

And about your request, if we ever want to make any improvements to the API, we better add support for new extendable "parsing engines" (for *nix) and "concatting engines" (for Windows), so everybody could put their own solution into there.

On *nix, there's actually no "preferred" way of parsing the command lines to argument lists, and each shell (or other sort of app) is free to implement their own way of passing/escaping the arguments.

On Windows, there's a "standard" way, but not every program follows it (one notable exception is e.g. Explorer who doesn't like the strings with this format: "C:\Windows\\", and requires them to be in a format of "C:\Windows\").

So, in fact, you'd need multiple parsing/concatting engines when passing arguments to a "standard" Windows program, a Cygwin program, an MSYS program, a WSL program etc. etc. etc.

[MrM40]

I agree you would expect that single quotes would be parsed without problems, but I will claim they are not. See #35222
I've tried to find a solution for the last six month with all the escaping tricks I could come up with.

[jnm2]

@MrM40 The last suggestion at https://github.com/dotnet/corefx/issues/35222#issuecomment-476518970 is that you should use double quotes. If that didn't work, can you follow up in that issue?

[MrM40]

There are programs/commands which require single quotes in the argument, I hope we can agree to that. And for that reason they should be parsed.
I can easily give you some more complex examples of commands, but is actually off-topic and would only move focus from the real topic.
The bottom line is single quotes are not parsed. You can just try the example in 35222 yourself.....the example project is attached.
Or maybe I put it wrong: They seem to be parsed in a very special unpredicted way. The receiving program doesn't accept the argument as just (tried on may different programs, commands and arguments, all the same problem)

[ForNeVeR]

@MrM40 could you please specify your target platform first? Otherwise, the following discussion doesn't make much sense.

[jnm2]

@MrM40 As @TSlivede points out in https://github.com/dotnet/corefx/issues/35222#issuecomment-514244283, this passes three separate arguments to ls: ', /tmp/, and '.

You want ArgumentList = { "/tmp/" } which passes a single argument to ls: /tmp/
Escaping is already done for you on platforms where it is necessary (Windows) in case the single argument has spaces etc.

[MrM40]

Two examples with solution:

In the Linux shell: dpkg-query -W -f=' ${db:Status-Status} ' mariadb*

ProcessStartInfo pi = new ProcessStartInfo("dpkg-query");
pi.ArgumentList.Add("-W");
pi.ArgumentList.Add("-f= ${db:Status-Status} ");
pi.ArgumentList.Add("mariadb*");`

In the Linux shell: virsh qemu-agent-command SRV04 '{"execute":"guest-ping"}'

ProcessStartInfo pi = new ProcessStartInfo("virsh");
pi.ArgumentList.Add("qemu-agent-command");
pi.ArgumentList.Add("SRV01");
pi.ArgumentList.Add("{\"execute\":\"guest-ping\"}");

[ForNeVeR]

@MrM40 and what's the issue? These samples you've posted do something (I'm not sure about the correctness of the first one though because of the way you've removed the ' characters, but whatever). What is your particular problem, why do you require the older string-based API (that honestly should already be considered "legacy" on *nix for the good measure) to somehow "properly parse" your input?

You're saying that your samples work fine "in the Linux shell". If you require the escape/argument parser of the particular shell, then you better use that exact shell to do the work you require. For example,

ProcessStartInfo pi = new ProcessStartInfo("/bin/sh");
pi.ArgumentList.Add("-c");
pi.ArgumentList.Add("dpkg-query -W -f=' ${db:Status-Status} ' mariadb*"); // exact line you've shown in your first example

It will do the processing, *-expansion and anything else for you, and pass the proper (according to the rules of that particular shell, which may differ!) argument collection to the called process.

Could you please explain why either that or the new ArgumentList API doesn't work for you?

[MrM40]

@ForNeVeR: Sorry for the short-info, I've rephrased the example.
I was wrong in my assumption that there was an issue with parsing single quotes in .Arguments and/or .ArgumentList.
It works fine, you just have to specify it correctly, which at least for me, was not obvious at all, and that's why I gave some examples for others who would find themself in the same situation pulling out hairs :-)
Sorry for the noise and inconvenience, and thank you follow up.

[ForNeVeR]

Alright!

No need to apologize, you've done nothing wrong. You had a question and you asked. Thanks for collaboration!