unix: Relax escaping in Debug impl on Command

The Debug output of Command is very useful for showing to the user the
command that was executed when something went wrong. This is done for
example by rustc when invoking an external tool like the linker fails.

It is also overly verbose, since everything is quoted, which makes it
harder to read. Instead, we now first check if we're reasonably sure
that an argument is simple enough that using it in the shell wouldn't
need quoting, and then output it without quotes if possible.

Before and example output could look like this:

    PATH="/a:/b" "cc" "foo.o" "-target" "arm64-apple-darwin11.0"

Now it looks like this:

    PATH=/a:/b cc foo.o -target arm64-apple-darwin11.0

Author: madsmtm

library/std/src/lib.rs

@@ -285,6 +285,7 @@
 #![feature(cfg_target_thread_local)]
 #![feature(cfi_encoding)]
 #![feature(concat_idents)]
+#![feature(debug_closure_helpers)]
 #![feature(decl_macro)]
 #![feature(deprecated_suggestion)]
 #![feature(doc_cfg)]

library/std/src/process/tests.rs

@@ -552,7 +552,7 @@ fn debug_print() {
 
     let mut command = Command::new("some-boring-name");
 
-    assert_eq!(format!("{command:?}"), format!(r#""some-boring-name""#));
+    assert_eq!(format!("{command:?}"), format!("some-boring-name"));
 
     assert_eq!(
         format!("{command:#?}"),
@@ -568,7 +568,7 @@ fn debug_print() {
 
     command.args(&["1", "2", "3"]);
 
-    assert_eq!(format!("{command:?}"), format!(r#""some-boring-name" "1" "2" "3""#));
+    assert_eq!(format!("{command:?}"), format!("some-boring-name 1 2 3"));
 
     assert_eq!(
         format!("{command:#?}"),
@@ -587,10 +587,7 @@ fn debug_print() {
 
     crate::os::unix::process::CommandExt::arg0(&mut command, "exciting-name");
 
-    assert_eq!(
-        format!("{command:?}"),
-        format!(r#"["some-boring-name"] "exciting-name" "1" "2" "3""#)
-    );
+    assert_eq!(format!("{command:?}"), format!("[some-boring-name] exciting-name 1 2 3"));
 
     assert_eq!(
         format!("{command:#?}"),
@@ -609,10 +606,7 @@ fn debug_print() {
 
     let mut command_with_env_and_cwd = Command::new("boring-name");
     command_with_env_and_cwd.current_dir("/some/path").env("FOO", "bar");
-    assert_eq!(
-        format!("{command_with_env_and_cwd:?}"),
-        r#"cd "/some/path" && FOO="bar" "boring-name""#
-    );
+    assert_eq!(format!("{command_with_env_and_cwd:?}"), "cd /some/path && FOO=bar boring-name");
     assert_eq!(
         format!("{command_with_env_and_cwd:#?}"),
         format!(
@@ -638,7 +632,7 @@ fn debug_print() {
 
     let mut command_with_removed_env = Command::new("boring-name");
     command_with_removed_env.env_remove("FOO").env_remove("BAR");
-    assert_eq!(format!("{command_with_removed_env:?}"), r#"env -u BAR -u FOO "boring-name""#);
+    assert_eq!(format!("{command_with_removed_env:?}"), "env -u BAR -u FOO boring-name");
     assert_eq!(
         format!("{command_with_removed_env:#?}"),
         format!(
@@ -660,7 +654,7 @@ fn debug_print() {
 
     let mut command_with_cleared_env = Command::new("boring-name");
     command_with_cleared_env.env_clear().env("BAR", "val").env_remove("FOO");
-    assert_eq!(format!("{command_with_cleared_env:?}"), r#"env -i BAR="val" "boring-name""#);
+    assert_eq!(format!("{command_with_cleared_env:?}"), "env -i BAR=val boring-name");
     assert_eq!(
         format!("{command_with_cleared_env:#?}"),
         format!(

library/std/src/sys/pal/unix/process/process_common.rs

@@ -580,8 +580,56 @@ impl fmt::Debug for Command {
 
             debug_command.finish()
         } else {
+            /// Only escape arguments when necessary, otherwise output it
+            /// unescaped to make the output easier to read.
+            ///
+            /// NOTE: This is best effort only!
+            fn relaxed_escaping(bytes: &[u8], is_arg: bool) -> impl fmt::Display + use<'_> {
+                // Don't escape if all the characters are likely
+                // to not be a problem when copied to the shell.
+                let can_print_safely = move |&c| {
+                    // See: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02
+                    match c {
+                        // The documentation linked above says that `=`:
+                        // > may need to be quoted under certain circumstances
+                        //
+                        // Because they may be parsed as a variable assignment.
+                        // But in argument position to a command, they
+                        // shouldn't need escaping.
+                        b'=' if is_arg => true,
+                        // As per documentation linked above:
+                        // > The application shall quote the following characters
+                        b'|' | b'&' | b';' | b'<' | b'>' | b'(' | b')' | b'$' | b'`' | b'\\'
+                        | b'"' | b'\'' | b' ' | b'\t' | b'\n' => false,
+                        // As per documentation linked above:
+                        // > may need to be quoted under certain circumstances
+                        b'*' | b'?' | b'[' | b'#' | b'~' | b'=' | b'%' => false,
+                        // It'd be weird to quote `[` and not quote `]`.
+                        b']' => false,
+                        // ! does history expansion in Bash, require quoting for that as well.
+                        //
+                        // NOTE: This doesn't currently work properly, we'd need to backslash-escape
+                        // `!` as well (which `escape_ascii` doesn't do).
+                        b'!' => false,
+                        // Assume all other printable characters may be output.
+                        0x20..0x7e => true,
+                        // Unprintable or not ASCII.
+                        _ => false,
+                    }
+                };
+                fmt::from_fn(move |f| {
+                    if !bytes.is_empty() && bytes.iter().all(can_print_safely) {
+                        // Unwrap is fine, we've checked above that the bytes only contains ascii.
+                        let ascii = crate::str::from_utf8(bytes).unwrap();
+                        write!(f, "{ascii}")
+                    } else {
+                        write!(f, "\"{}\"", bytes.escape_ascii())
+                    }
+                })
+            }
+
             if let Some(ref cwd) = self.cwd {
-                write!(f, "cd {cwd:?} && ")?;
+                write!(f, "cd {} && ", relaxed_escaping(cwd.to_bytes(), true))?;
             }
             if self.env.does_clear() {
                 write!(f, "env -i ")?;
@@ -595,23 +643,29 @@ impl fmt::Debug for Command {
                             write!(f, "env ")?;
                             any_removed = true;
                         }
-                        write!(f, "-u {} ", key.to_string_lossy())?;
+                        write!(f, "-u {} ", relaxed_escaping(key.as_encoded_bytes(), false))?;
                     }
                 }
             }
             // Altered env vars can just be added in front of the program.
             for (key, value_opt) in self.get_envs() {
                 if let Some(value) = value_opt {
-                    write!(f, "{}={value:?} ", key.to_string_lossy())?;
+                    write!(
+                        f,
+                        "{}={} ",
+                        relaxed_escaping(key.as_encoded_bytes(), false),
+                        relaxed_escaping(value.as_encoded_bytes(), false)
+                    )?;
                 }
             }
             if self.program != self.args[0] {
-                write!(f, "[{:?}] ", self.program)?;
+                write!(f, "[{}] ", relaxed_escaping(self.program.to_bytes(), false))?;
             }
-            write!(f, "{:?}", self.args[0])?;
+
+            write!(f, "{}", relaxed_escaping(self.args[0].to_bytes(), false))?;
 
             for arg in &self.args[1..] {
-                write!(f, " {:?}", arg)?;
+                write!(f, " {}", relaxed_escaping(arg.to_bytes(), true))?;
             }
             Ok(())
         }

library/std/src/sys/pal/unix/process/process_common/tests.rs

@@ -190,3 +190,38 @@ fn unix_exit_statuses() {
         }
     }
 }
+
+#[test]
+fn command_debug_escaping() {
+    #[track_caller]
+    fn check(arg: impl AsRef<OsStr>, expected: &str) {
+        let cmd = Command::new(arg.as_ref());
+        assert_eq!(format!("{cmd:?}"), expected);
+    }
+
+    // Escaped.
+    check("", r#""""#);
+    check("é", r#""\xc3\xa9""#);
+    check("a'", r#""a\'""#);
+    check("a\"", r#""a\"""#);
+    check("'\"", r#""\'\"""#);
+    check(" ", r#"" ""#);
+    check(
+        "/Users/The user's name/.cargo/bin/cargo",
+        r#""/Users/The user\'s name/.cargo/bin/cargo""#,
+    );
+    check("!a", r#""!a""#);
+
+    // Simple enough that we don't escape them.
+    check("a", "a");
+    check("/my/simple/path", "/my/simple/path");
+    check("abc-defg_1234", "abc-defg_1234");
+
+    // Real-world use-case, immediately clear that two of the arguments are passed as one.
+    let mut cmd = crate::process::Command::new("ld");
+    cmd.arg("my/file.o");
+    cmd.arg("-macos_version_min");
+    cmd.arg("13.0");
+    cmd.arg("-arch arm64");
+    assert_eq!(format!("{cmd:?}"), r#"ld my/file.o -macos_version_min 13.0 "-arch arm64""#);
+}

tests/run-make/link-args-order/rmake.rs

@@ -13,17 +13,19 @@ fn main() {
         .linker_flavor(linker)
         .link_arg("a")
         .link_args("b c")
-        .link_args("d e")
-        .link_arg("f")
+        .link_arg("d e")
+        .link_args("f g")
+        .link_arg("h")
         .run_fail()
-        .assert_stderr_contains(r#""a" "b" "c" "d" "e" "f""#);
+        .assert_stderr_contains(r#"a b c "d e" f g h"#);
     rustc()
         .input("empty.rs")
         .linker_flavor(linker)
         .arg("-Zpre-link-arg=a")
         .arg("-Zpre-link-args=b c")
-        .arg("-Zpre-link-args=d e")
-        .arg("-Zpre-link-arg=f")
+        .arg("-Zpre-link-arg=d e")
+        .arg("-Zpre-link-args=f g")
+        .arg("-Zpre-link-arg=h")
         .run_fail()
-        .assert_stderr_contains(r#""a" "b" "c" "d" "e" "f""#);
+        .assert_stderr_contains(r#"a b c "d e" f g h"#);
 }

tests/run-make/link-dedup/rmake.rs

@@ -31,9 +31,9 @@ fn needle_from_libs(libs: &[&str]) -> String {
     let mut needle = String::new();
     for lib in libs {
         if is_msvc() {
-            let _ = needle.write_fmt(format_args!(r#""{lib}.lib" "#));
+            let _ = needle.write_fmt(format_args!("{lib}.lib "));
         } else {
-            let _ = needle.write_fmt(format_args!(r#""-l{lib}" "#));
+            let _ = needle.write_fmt(format_args!("-l{lib} "));
         }
     }
     needle.pop(); // remove trailing space

tests/run-make/pass-linker-flags-flavor/rmake.rs

@@ -72,8 +72,8 @@ fn main() {
         .stdout_utf8();
 
     let no_verbatim = regex::Regex::new("l1.*-Wl,a1.*l2.*-Wl,a2.*d1.*-Wl,a3").unwrap();
-    let one_verbatim = regex::Regex::new(r#"l1.*"a1".*l2.*-Wl,a2.*d1.*-Wl,a3"#).unwrap();
-    let ld = regex::Regex::new(r#"l1.*"a1".*l2.*"a2".*d1.*"a3""#).unwrap();
+    let one_verbatim = regex::Regex::new("l1.*a1.*l2.*-Wl,a2.*d1.*-Wl,a3").unwrap();
+    let ld = regex::Regex::new("l1.*a1.*l2.*a2.*d1.*a3").unwrap();
 
     assert!(no_verbatim.is_match(&out_gnu));
     assert!(no_verbatim.is_match(&out_att_gnu));