feat(derive): accept a value the OS accepts and UTF-8 does not

.github/workflows/test.yml

@@ -40,6 +40,14 @@ jobs:
           fi
       - run: mise r build
       - run: mise r test
+      # `os_string_from_bytes` has a `cfg` branch per platform, and every test of it is
+      # `#[cfg(unix)]` — so the Windows one is not compiled anywhere else in this pipeline.
+      # A compile check is not coverage, but it is what keeps that branch from rotting
+      # unseen.
+      - name: check the Windows-only conversion still compiles
+        run: |
+          rustup target add x86_64-pc-windows-msvc
+          cargo check -p usage-argv --all-features --target x86_64-pc-windows-msvc
       - run: mise r render
       # Same reasoning as `render`: the shadow is checked in, so a change to the derive's
       # vocabulary that would alter it has to be committed rather than discovered later.

PLAN.md

@@ -132,12 +132,26 @@ manpages, and SDKs — never a runtime dependency of somebody else's program.
       a different file, silently. Costs +656 instructions (1.6%) and one allocation, which
       is what not corrupting a value is worth. It also retired the hazard of recognising
       `String` by its spelling, since there is no identity case left.
-- [ ] **Accepting a value that is not valid UTF-8** — reporting it is not the same as taking
-      it. `PathBuf` could hold the exact bytes, but recovering an `OsString` from them needs
-      `OsStr::from_encoded_bytes_unchecked`, which is `unsafe`, and this crate has none. The
-      call would be sound — the bytes come from `as_encoded_bytes` in the same process, and
-      every split the parser makes is at an ASCII byte, so no multi-byte sequence is ever
-      cut — but introducing `unsafe` is jdx's call to make, not mine.
+- [x] **Accepting a value that is not valid UTF-8** — reporting it was the safe half;
+      accepting it is the whole fix, because the operating system does accept `/tmp/\xff` as a
+      filename and a CLI that cannot receive one cannot open the file. A `PathBuf` or
+      `OsString` field takes the bytes exactly, through `usage_argv::os_string_from_bytes`.
+
+      **And with no `unsafe` anywhere.** On Unix an `OsString` is an arbitrary byte sequence,
+          so this is the safe `OsString::from_vec` and every byte survives — which is the case that
+          matters, since non-UTF-8 filenames are ordinary there. Windows was going to need
+          `from_encoded_bytes_unchecked`, and jdx approved that, but a *safe* function taking a
+          `Vec<u8>` cannot enforce its precondition: there is no way to know the bytes came from
+          `as_encoded_bytes` rather than from anywhere else, and a safe function whose precondition
+          a caller can violate is unsound however carefully today's callers behave. Greptile flagged
+          exactly that on #844. So Windows goes through UTF-8 and reports what will not convert,
+          which gives up only an unpaired-surrogate argument there.
+
+          Cheaper than the text path, not dearer: a `PathBuf` field costs **553 instructions per
+          parse against a `String` field's 674**, since it skips the UTF-8 validation pass, and both
+          allocate once. The gate fixture cannot show this — a spec carries no Rust types, so every
+          shadow field is a `String` — which is why it is measured directly.
+
 - [ ] **`usage-derive` v1** — everything mise needs: constraints
       (`requires`/`conflicts`/`overrides`/`required_unless`), `var`, `count`,
       `env`, defaults, delimiters, the `double_dash` modes, global flags, flatten,

argv/src/lib.rs

@@ -54,8 +54,14 @@
 //! actually looked at can fail to convert.
 //!
 //! Slicing an `OsStr` into `&str` pieces safely is not possible without
-//! allocating or `unsafe`, and this crate forbids `unsafe`. Bytes are what is
-//! left, and they turn out to be the honest interface anyway.
+//! allocating or `unsafe`. Bytes are what is left, and they turn out to be the
+//! honest interface anyway.
+//!
+//! The reverse conversion is [`os_string_from_bytes`], which lets a `PathBuf`
+//! field hold a filename that is not UTF-8 rather than a mangled copy of one. On
+//! Unix that is lossless and safe; on Windows, where WTF-8 makes it partial, a
+//! value that will not convert is reported. Either way this crate contains no
+//! `unsafe`, which a conversion that guessed would have cost.
 //!
 //! # What this crate does not do
 //!
@@ -75,7 +81,7 @@
 
 #![forbid(unsafe_code)]
 
-use std::ffi::OsStr;
+use std::ffi::{OsStr, OsString};
 
 #[cfg(feature = "spec")]
 pub mod spec;
@@ -141,6 +147,13 @@ pub struct Flag<'a> {
     /// Long forms, written without the leading `--`.
     pub longs: &'a [&'a str],
     /// Short forms, as single bytes.
+    ///
+    /// **Should be ASCII.** A cluster like `-xyz` is walked one byte at a time, so a
+    /// non-ASCII short can never be matched, and the remainder after a value-taking one —
+    /// which becomes its value — would begin in the middle of a character.
+    /// `#[derive(Cli)]` rejects a non-ASCII `short`; a table written by hand should keep to
+    /// it. Nothing is unsound if it does not: the value would simply be cut in a place that
+    /// makes no sense, and on Windows would then fail to convert.
     pub shorts: &'a [u8],
     /// A long form that sets the flag to false, written without the `--`.
     pub negate: Option<&'a str>,
@@ -406,6 +419,49 @@ pub fn as_str(value: &[u8]) -> Result<&str, std::str::Utf8Error> {
     std::str::from_utf8(value)
 }
 
+/// Rebuild an [`OsString`] from bytes the parser handed back.
+///
+/// This is the reverse of [`OsStr::as_encoded_bytes`], and it is how a `PathBuf` field
+/// receives a filename the operating system accepts but UTF-8 does not — `/tmp/\xff` stays
+/// `/tmp/\xff` rather than becoming a *different* filename with `U+FFFD` in it.
+///
+/// Where the platform cannot hold those bytes, they are handed back in the `Err` — as
+/// `String::from_utf8` does — so the caller can name the value in its error without this
+/// having to copy it for a case that is nearly never taken.
+///
+/// # Why this is not `unsafe`, and why it is not lossless everywhere
+///
+/// On **Unix** an `OsString` is an arbitrary byte sequence, so the conversion is total and
+/// uses the safe [`OsStringExt::from_vec`]. Every byte survives, which is the case that
+/// matters: non-UTF-8 filenames are ordinary there.
+///
+/// [`OsStringExt::from_vec`]: std::os::unix::ffi::OsStringExt::from_vec
+///
+/// On **Windows** the encoding is WTF-8, where not every byte sequence is valid, and the only
+/// constructor that accepts one is `OsString::from_encoded_bytes_unchecked` — whose
+/// precondition this function cannot enforce. It takes a `Vec<u8>` from a safe caller, so
+/// there is no way to know the bytes came from `as_encoded_bytes` rather than from anywhere
+/// else, and a safe function with a precondition that can be violated is unsound however
+/// carefully its callers behave today.
+///
+/// So on Windows the bytes go through UTF-8, and one that is not valid UTF-8 is refused
+/// rather than assumed. What that gives up is a Windows argument containing an unpaired
+/// surrogate, which is reported instead of accepted; what it buys is that this crate needs no
+/// `unsafe` at all.
+pub fn os_string_from_bytes(value: Vec<u8>) -> Result<OsString, Vec<u8>> {
+    #[cfg(unix)]
+    {
+        Ok(std::os::unix::ffi::OsStringExt::from_vec(value))
+    }
+    #[cfg(not(unix))]
+    {
+        match String::from_utf8(value) {
+            Ok(text) => Ok(OsString::from(text)),
+            Err(bad) => Err(bad.into_bytes()),
+        }
+    }
+}
+
 /// A single-pass parse over `argv`.
 ///
 /// Created with [`Parser::new`] and driven with [`Parser::next_event`].
@@ -831,8 +887,8 @@ impl<'t, 'v> Parser<'t, 'v> {
 /// View a token as bytes.
 ///
 /// `as_encoded_bytes` is a plain accessor with no conversion and no allocation.
-/// It is only the reverse direction that needs `unsafe`, which is why values
-/// come back as bytes.
+/// The reverse direction is the one with a cost — see [`os_string_from_bytes`] —
+/// which is why values come back as bytes.
 fn bytes<'v>(s: &'v &'v OsStr) -> &'v [u8] {
     s.as_encoded_bytes()
 }

conformance/tests/typed.rs

@@ -5,7 +5,7 @@
 //! `PathBuf` 227 times and a tool-version type 83 times in its command structs — and that a
 //! value which will not convert says which value and why.
 
-use std::ffi::OsStr;
+use std::ffi::{OsStr, OsString};
 use std::path::PathBuf;
 use std::str::FromStr;
 
@@ -359,30 +359,123 @@ fn the_conversion_stands_on_its_own() {
     assert!(err.contains("bash, zsh, fish, pwsh"), "{err}");
 }
 
-/// A CLI holding a path, which is where mangling would show
+/// A CLI holding paths, which is where mangling would show
 #[derive(Cli)]
 #[usage(bin = "pathy")]
 struct Pathy {
     /// Where to write
-    #[usage(long)]
+    #[usage(long, short = 'o')]
     out: Option<PathBuf>,
     /// Anything at all
     #[usage(long)]
     text: Option<String>,
+    /// Every input
+    #[usage(long, var)]
+    input: Vec<PathBuf>,
+    /// A raw word
+    #[usage(long)]
+    raw: Option<OsString>,
+    /// Every exclusion, or none given at all
+    #[usage(long, var)]
+    exclude: Option<Vec<PathBuf>>,
+}
+
+/// A filename the operating system accepts and UTF-8 does not.
+#[cfg(unix)]
+fn not_utf8() -> &'static OsStr {
+    use std::os::unix::ffi::OsStrExt;
+    OsStr::from_bytes(b"/tmp/\xff")
+}
+
+#[cfg(unix)]
+fn as_bytes(path: &std::path::Path) -> &[u8] {
+    use std::os::unix::ffi::OsStrExt;
+    path.as_os_str().as_bytes()
 }
 
 #[test]
-fn a_word_that_is_not_utf8_is_reported_rather_than_mangled() {
-    // It used to arrive through `from_utf8_lossy`, so a path with a stray byte in it became
-    // a path with U+FFFD in it — a different file, silently. Now the parse says so.
-    use std::ffi::OsStr;
+#[cfg(unix)]
+fn a_word_that_is_not_utf8_arrives_byte_for_byte() {
+    // It used to arrive through `from_utf8_lossy`, so a path with a stray byte in it became a
+    // path with `U+FFFD` in it — a different file, silently. Reporting it was the safe half
+    // of the fix; this is the whole one, because the operating system does accept this
+    // filename and a CLI that cannot receive it cannot open the file.
+    let argv = [OsStr::new("--out"), not_utf8()];
+    let parsed = Pathy::parse_from(&argv).expect("a filename the OS accepts should parse");
+    let out = parsed.out.expect("given");
+    assert_eq!(as_bytes(&out), b"/tmp/\xff");
+
+    // The point of byte-exactness: it is not the lossy rendering, which names another file.
+    assert_ne!(as_bytes(&out), "/tmp/\u{fffd}".as_bytes());
+}
+
+#[test]
+#[cfg(unix)]
+fn every_form_a_value_can_arrive_in_keeps_its_bytes() {
+    // Each of these reaches the field by a different route through the parser, and each cuts
+    // the token at a different place — which is exactly what `os_string_from_bytes` is
+    // trusting. A detached value is passed through whole; `--out=…` is cut at the `=`.
     use std::os::unix::ffi::OsStrExt;
 
-    let bad = OsStr::from_bytes(b"/tmp/\xff");
-    let argv = [OsStr::new("--out"), bad];
+    for (token, why) in [
+        (
+            &b"--out=/tmp/\xff"[..],
+            "a long form's attached value is cut at the `=`",
+        ),
+        (
+            b"-o/tmp/\xff",
+            "a short flag's value is the rest of the token",
+        ),
+        (
+            b"-o=/tmp/\xff",
+            "a short flag's value may be cut at an `=` too",
+        ),
+    ] {
+        let parsed = Pathy::parse_from(&[OsStr::from_bytes(token)]).expect("should parse");
+        assert_eq!(as_bytes(&parsed.out.expect("given")), b"/tmp/\xff", "{why}");
+    }
+
+    // A collecting field keeps every one of them, and an `OsString` field takes the word
+    // with no interpretation at all.
+    // `var` is repetition rather than a variadic, so each value comes with its own flag.
+    let argv = [
+        OsStr::new("--input"),
+        not_utf8(),
+        OsStr::new("--input"),
+        OsStr::from_bytes(b"\xfe"),
+        OsStr::new("--raw"),
+        not_utf8(),
+    ];
+    let parsed = Pathy::parse_from(&argv).expect("should parse");
+    assert_eq!(
+        parsed.input.iter().map(|p| as_bytes(p)).collect::<Vec<_>>(),
+        [&b"/tmp/\xff"[..], &b"\xfe"[..]]
+    );
+    assert_eq!(parsed.raw.expect("given").as_bytes(), b"/tmp/\xff");
+
+    // An `Option<Vec<_>>` still tells "never given" from "given nothing", which is decided
+    // before any of this and must not have been lost on the way to the byte path.
+    assert!(parsed.exclude.is_none(), "never given");
+    let argv = [OsStr::new("--exclude"), not_utf8()];
+    let parsed = Pathy::parse_from(&argv).expect("should parse");
+    assert_eq!(
+        parsed
+            .exclude
+            .as_deref()
+            .map(|paths| paths.iter().map(|p| as_bytes(p)).collect::<Vec<_>>()),
+        Some(vec![&b"/tmp/\xff"[..]])
+    );
+}
+
+#[test]
+#[cfg(unix)]
+fn a_field_that_is_not_a_path_still_reports_it() {
+    // Byte-exactness is for the types that can hold any byte sequence. A `String` cannot, so
+    // there the honest answer is still to say so rather than to substitute a different word.
+    let argv = [OsStr::new("--text"), not_utf8()];
     match Pathy::parse_from(&argv) {
         Err(Error::InvalidValue(bad)) => {
-            assert_eq!(bad.name, "out");
+            assert_eq!(bad.name, "text");
             assert!(
                 bad.reason.contains("utf-8") || bad.reason.contains("UTF-8"),
                 "the reason should say what was wrong: {}",
@@ -393,7 +486,7 @@ fn a_word_that_is_not_utf8_is_reported_rather_than_mangled() {
             assert!(bad.value.contains("/tmp/"), "{}", bad.value);
         }
         Err(other) => panic!("wrong error: {other:?}"),
-        Ok(_) => panic!("a value that is not UTF-8 should not have been accepted"),
+        Ok(_) => panic!("a `String` cannot hold this, so it should not have been accepted"),
     }
 }