Unescaped command-line arguments for Windows

[kornelski]

Some Windows commands, expecially cmd.exe /c, have unusual quoting requirements which are incompatible with default rules assumed for .arg().

This adds .unquoted_arg() to Command via Windows CommandExt trait.

Fixes #29494

[rust-highfive]

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

[kornelski]

There's an existing unstable feature force_quotes() that is the opposite of this feature — used to quote args that have * which aren't quoted by default in .arg().

I went for .unquoted_arg() approach, because it's more flexible. A single .unquoted_arg() can be used to build an arbitrary command-line, but also can be mixed with regular .arg(), so that users who need only some arguments with special syntax don't have to reinvent their own quoting implementation for the rest (the escaping rules for Windows are very weird, so it's better not to have to do it).

I think .force_quotes(true) could be replaced by .quoted_arg(val) to have both features with consistent interface, but I'm leaving it out of scope of this PR.

[dgrunwald]

I haven't tried this branch; but if I read the diff correctly, unquoted_arg will still perform escaping?
Quote::Never doesn't add quotes around the argument, but it still escapes those within the argument.

You don't have any test cases where escaping would trigger (only when the argument contains ").

[dgrunwald]

Also, isn't there a bug where Quote::Auto ends up discarding trailing backslashes from the argument if the argument doesn't contain any whitespace?

[kornelski]

I've disabled inner escaping for unquoted arg.

I haven't changed escaping logic. I think it's fine — the rules of CommandLineToArgvW that it uses are just very counter-intuitive.

[dgrunwald]

Try a test case with Arg::Quoted(OsString::from("c:\\")), (without force_quotes).
I think it'll end up passing c: without the trailing backslash to the target program. That's because backslashes is non-zero at the end, but gets ignored when the if quote is not taken.

[yaahc]

Why'd you go with calling it unquoted_arg instead of raw_arg as suggested in the source issue? To me the former implies that it would still do things other than quoting, such as escaping, where as the later makes it pretty clear to me that the arg is passed directly as is.

[kornelski]

That's because I've noticed that there's force_quotes, and wanted to use a similar teminology. However, you're right that this makes escaping ambiguous. I've changed back to raw_arg

[yaahc]

Try a test case with Arg::Quoted(OsString::from("c:\\")), (without force_quotes).
I think it'll end up passing c: without the trailing backslash to the target program. That's because backslashes is non-zero at the end, but gets ignored when the if quote is not taken.

@kornelski have you tried the test case suggested by @dgrunwald? I'm curious to know if this is an issue.

[bors]

☔ The latest upstream changes (presumably #85270) made this pull request unmergeable. Please resolve the merge conflicts.

[kornelski]

@yaahc I've added tests cases for the issue raised. They're fine, because escaping rules on Windows don't double the number of backslashes, only number of backslashes before a quote (the syntax is illogical like that by design).

[ChrisDenton]

To recap the default rules, a backslash is treated literally unless they are directly followed by a double quote. When a double quote is encountered the rules are:

• Any directly preceding backslashes are counted (represented here by n).
  • If n is even then replace with n/2 backslashes and toggle "quote mode".
  • If n is odd then replace with (n-1)/2 backslashes and add a literal ".

For example, the string C:\Program Files\Rust can be encoded in an argument in several ways:

"C:\Program Files\Rust"
C:\"Program Files"\Rust
C:\"Program Files\\"Rust

These rules are based on the old 80's DOS shell and can't be changed too much due to backwards compatibility. However, some applications do use their own rules, hence the need for raw_arg.

[dgrunwald]

I misread the code; I missed that the first copy of the \ is already inserted by the loop.
So the code works fine, but thanks for adding the test.

Yeah the rules are weird. Fun fact: the interpretation of double double-quotes "" is another (undocumented) special case where the behavior changed in the 2008 update of the Microsoft C runtime. But that doesn't matter for us as we never emit those. This site has the full story: http://daviddeley.com/autohotkey/parameters/parameters.htm#WINCRULES

[yaahc]

Sounds like everyone is happy with the PR in it's current state ^_^

@bors r+

[bors]

⌛ Testing commit b86d121c3af1d82425074ae8c005fa96b824aa1d with merge 61e56dc608b0077df1cdc469c0eea3e9a72275ce...

[bors]

💔 Test failed - checks-actions

[kornelski]

I think mingw uses debug output in tests? I've changed debug output to look exactly like before, and rebased.

I can't test mingw, because I don't know how to make x.py use it instead of msvc.

[yaahc]

I'm not really familiar with windows at all so I can't really help with the mingw/msvc setup. For now though I can delegate bors permissions so you can restart the build as needed

@bors delegate+
@bors r+

[bors]

✌️ @kornelski can now approve this pull request

[bors]

📌 Commit bc67f6b has been approved by yaahc

[bors]

⌛ Testing commit bc67f6b with merge 3eff244...

[bors]

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing 3eff244 to master...

[Xaeroxe]

What's the path for stabilizing this feature? The unstable attribute for it doesn't point at a tracking issue.

[kornelski]

@Xaeroxe There haven't been any objections to this feature, so I think you could make a tracking issue and make a PR to stabilize this feature.

[Xaeroxe]

Tracking issue: #92939