feat(derive): hold the bytes a word arrived as

PLAN.md

@@ -126,11 +126,18 @@ manpages, and SDKs — never a runtime dependency of somebody else's program.
       nine of these. The list is declared once, on the type: the spec, the help, the
       completions and the check that rejects a wrong word all read it from there, so none of
       them can drift from the type the way a second list on the field would.
-- [ ] **Values that are not valid UTF-8** — a word reaches a field through
-      `from_utf8_lossy`, so a `PathBuf` holding a non-UTF-8 path gets replacement
-      characters instead of bytes. The partial should hold `OsString` and let `build`
-      decide: exact for `PathBuf` and `OsString`, an error for `String` rather than a
-      silent mangling. Small, and the next thing.
+- [x] **Values that are not valid UTF-8 are reported, not mangled** — the partial holds the
+      bytes a word arrived as, and the conversion happens once where the struct is built, so
+      `--out /tmp/\xff` says so instead of handing a `PathBuf` a path with `U+FFFD` in it —
+      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.
 - [ ] **`usage-derive` v1** — everything mise needs: constraints
       (`requires`/`conflicts`/`overrides`/`required_unless`), `var`, `count`,
       `env`, defaults, delimiters, the `double_dash` modes, global flags, flatten,

conformance/tests/typed.rs

@@ -298,6 +298,42 @@ fn the_words_reach_the_spec_from_the_type() {
     assert_eq!(choices.choices, ["bash", "zsh", "fish", "pwsh"]);
 }
 
+#[test]
+#[cfg(unix)]
+fn a_choice_that_is_not_utf8_reports_the_bytes_not_the_list() {
+    // The checks run before the struct is built, so a value that is not UTF-8 used to be
+    // compared as an empty string, match none of the choices, and come back as
+    // `InvalidChoice` — a message listing words, about a value that was never a word. The
+    // UTF-8 failure is the real problem and the one worth reporting.
+    use std::ffi::OsStr;
+    use std::os::unix::ffi::OsStrExt;
+
+    let bad_bytes = [OsStr::new("--shell"), OsStr::from_bytes(b"ba\xffsh")];
+    match Enumerated::parse_from(&bad_bytes) {
+        Err(Error::InvalidValue(bad)) => {
+            assert_eq!(bad.name, "shell");
+            assert!(
+                bad.reason.contains("utf-8") || bad.reason.contains("UTF-8"),
+                "the reason should be the UTF-8 failure: {}",
+                bad.reason
+            );
+        }
+        Err(Error::InvalidChoice { choices, .. }) => {
+            panic!("reported the choices {choices:?} for a value that is not a word at all")
+        }
+        Err(other) => panic!("wrong error: {other:?}"),
+        Ok(_) => panic!("this should not have parsed"),
+    }
+
+    // A word that *is* text and is not one of the choices still gets the list, which is the
+    // case this check exists for.
+    let a = argv(["--shell", "csh"]);
+    assert!(matches!(
+        Enumerated::parse_from(&a),
+        Err(Error::InvalidChoice { .. })
+    ));
+}
+
 #[test]
 fn a_wrong_word_lists_what_was_expected() {
     // An `InvalidChoice` carrying the list, rather than a conversion error about a type the
@@ -322,3 +358,49 @@ fn the_conversion_stands_on_its_own() {
     let err = Interpreter::from_str("csh").expect_err("not a shell");
     assert!(err.contains("bash, zsh, fish, pwsh"), "{err}");
 }
+
+/// A CLI holding a path, which is where mangling would show
+#[derive(Cli)]
+#[usage(bin = "pathy")]
+struct Pathy {
+    /// Where to write
+    #[usage(long)]
+    out: Option<PathBuf>,
+    /// Anything at all
+    #[usage(long)]
+    text: Option<String>,
+}
+
+#[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;
+    use std::os::unix::ffi::OsStrExt;
+
+    let bad = OsStr::from_bytes(b"/tmp/\xff");
+    let argv = [OsStr::new("--out"), bad];
+    match Pathy::parse_from(&argv) {
+        Err(Error::InvalidValue(bad)) => {
+            assert_eq!(bad.name, "out");
+            assert!(
+                bad.reason.contains("utf-8") || bad.reason.contains("UTF-8"),
+                "the reason should say what was wrong: {}",
+                bad.reason
+            );
+            // Rendered lossily *for the message only*, which is the one place it is right:
+            // the value is being described, not used.
+            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"),
+    }
+}
+
+#[test]
+fn a_path_that_is_utf8_arrives_exactly() {
+    let argv = argv(["--out", "/tmp/x y/z", "--text", "hello"]);
+    let p = Pathy::parse_from(&argv).expect("should parse");
+    assert_eq!(p.out, Some(PathBuf::from("/tmp/x y/z")));
+    assert_eq!(p.text.as_deref(), Some("hello"));
+}

derive/src/codegen.rs

@@ -132,13 +132,13 @@ pub fn emit(cli: &Cli) -> TokenStream {
             // holds them as `String`, and converts lossily, which is what mise
             // already does with its own argv. Rejecting a non-UTF-8 value needs an
             // error type for value conversion, and that arrives with typed fields.
-            pub fn __usage_text(value: &[u8]) -> ::std::string::String {
-                ::std::string::String::from_utf8_lossy(value).into_owned()
+            pub fn __usage_text(value: &[u8]) -> ::std::vec::Vec<u8> {
+                value.to_vec()
             }
 
             pub fn __usage_value_text(
                 value: ::std::option::Option<&[u8]>,
-            ) -> ::std::string::String {
+            ) -> ::std::vec::Vec<u8> {
                 value.map(__usage_text).unwrap_or_default()
             }
 
@@ -663,9 +663,13 @@ fn partial_struct(cli: &Cli) -> TokenStream {
                 let ty = &f.ty;
                 quote!(#ty)
             }
-            Shape::Optional => quote!(::std::option::Option<::std::string::String>),
-            Shape::Required => quote!(::std::string::String),
-            Shape::Many => quote!(::std::vec::Vec<::std::string::String>),
+            // The bytes as typed, rather than text: `apply` cannot fail — it answers
+            // whether an event was this command's — so a word that is not valid UTF-8
+            // cannot be reported when it arrives. Keeping the bytes lets `build` report it,
+            // and it means no value is quietly mangled on the way in.
+            Shape::Optional => quote!(::std::option::Option<::std::vec::Vec<u8>>),
+            Shape::Required => quote!(::std::vec::Vec<u8>),
+            Shape::Many => quote!(::std::vec::Vec<::std::vec::Vec<u8>>),
         };
         let given = format_ident!("__given_{}", ident);
         // Whether a token supplied this, as opposed to a default sitting in it: an
@@ -740,42 +744,61 @@ fn field_final(field: &Field) -> TokenStream {
         return quote!(#ident: partial.#ident);
     };
 
-    // `String` is the identity conversion, and writing it out as one costs an allocation
-    // per value to get back what we already had: 3 allocations become 5 and the parse grows
-    // 2.3% on a three-word invocation, measured.
+    // Every type converts, `String` included: the partial holds the bytes that were typed,
+    // and `String::from_utf8` is where a word that is not UTF-8 is reported rather than
+    // quietly replaced. That also retires the old hazard of recognising `String` by how it
+    // was written — there is no identity case left to recognise, so an adopter who shadows
+    // the name is no longer a problem.
     //
-    // Matched on the whole written path rather than its last segment, so someone's own
-    // `my::String` is not mistaken for this one. What remains is an adopter who *shadows*
-    // the name — `use my_crate::String` — whose field would be handed a
-    // `std::string::String` and fail to compile. A macro cannot resolve a name, so the
-    // choice is this narrow hazard or the allocation for everyone. It stops being a choice
-    // once the partial holds bytes rather than text: a `String` field converts like any
-    // other then, and there is no identity case left to recognise.
+    // `from_utf8` takes the `Vec` by value and does not copy, so this costs a check.
+    // `String` still skips the *second* step, since `from_utf8` has already produced one.
+    // Recognising it by spelling is safe now: if an adopter's own `String` were mistaken for
+    // this one, the mismatch is a compile error rather than a value quietly mangled — and
+    // the check that matters, the UTF-8 one, happens either way.
     let is_std_string = matches!(
         rendered_path(ty).as_str(),
         "String" | "std::string::String" | "::std::string::String" | "alloc::string::String"
     );
     let converted = |value: TokenStream| {
-        if is_std_string {
-            quote!(#value)
-        } else {
-            quote! {
-                match ::std::str::FromStr::from_str(&#value) {
-                    ::std::result::Result::Ok(parsed) => parsed,
-                    ::std::result::Result::Err(reason) => {
-                        return ::std::result::Result::Err(
-                            ::usage_argv::Error::InvalidValue(::std::boxed::Box::new(
-                                ::usage_argv::InvalidValue {
-                                    name: #name,
-                                    value: #value,
-                                    reason: ::std::string::ToString::to_string(&reason),
-                                },
-                            )),
-                        );
-                    }
+        let text = quote! {
+            match ::std::string::String::from_utf8(#value) {
+                ::std::result::Result::Ok(text) => text,
+                ::std::result::Result::Err(bad) => {
+                    return ::std::result::Result::Err(
+                        ::usage_argv::Error::InvalidValue(::std::boxed::Box::new(
+                            ::usage_argv::InvalidValue {
+                                name: #name,
+                                value: ::std::string::String::from_utf8_lossy(
+                                    bad.as_bytes(),
+                                )
+                                .into_owned(),
+                                reason: ::std::string::ToString::to_string(&bad.utf8_error()),
+                            },
+                        )),
+                    );
                 }
             }
+        };
+        if is_std_string {
+            return text;
         }
+        quote! {{
+            let __usage_text = #text;
+            match ::std::str::FromStr::from_str(&__usage_text) {
+                ::std::result::Result::Ok(parsed) => parsed,
+                ::std::result::Result::Err(reason) => {
+                    return ::std::result::Result::Err(
+                        ::usage_argv::Error::InvalidValue(::std::boxed::Box::new(
+                            ::usage_argv::InvalidValue {
+                                name: #name,
+                                value: __usage_text,
+                                reason: ::std::string::ToString::to_string(&reason),
+                            },
+                        )),
+                    );
+                }
+            }
+        }}
     };
 
     match field.shape {
@@ -802,20 +825,15 @@ fn field_final(field: &Field) -> TokenStream {
             // A `Vec<String>` is moved whole. Rebuilding it element by element allocated a
             // second `Vec` to hold what the first already held, which is one allocation per
             // collecting field — and mise's commands collect a lot.
-            let collected = if is_std_string {
-                quote!(partial.#ident)
-            } else {
-                // Built by hand rather than with `collect`, so the error can carry the value
-                // that failed rather than only that one did.
-                quote! {{
-                    let mut __usage_values =
-                        ::std::vec::Vec::with_capacity(partial.#ident.len());
-                    for __usage_value in partial.#ident {
-                        __usage_values.push(#one);
-                    }
-                    __usage_values
-                }}
-            };
+            // Built by hand rather than with `collect`, so the error can carry the value
+            // that failed rather than only that one did.
+            let collected = quote! {{
+                let mut __usage_values = ::std::vec::Vec::with_capacity(partial.#ident.len());
+                for __usage_value in partial.#ident {
+                    __usage_values.push(#one);
+                }
+                __usage_values
+            }};
             if field.optional_collection {
                 let given = format_ident!("__given_{}", ident);
                 // `Option<Vec<T>>` distinguishes "never given" from "given nothing", which
@@ -855,10 +873,10 @@ fn reset_to_default(field: &Field) -> TokenStream {
             let on = default == "true";
             quote!(partial.#ident = #on;)
         }
-        Shape::Optional => {
-            quote!(partial.#ident = ::std::option::Option::Some(#default.to_string());)
-        }
-        Shape::Required => quote!(partial.#ident = #default.to_string();),
+        Shape::Optional => quote! {
+            partial.#ident = ::std::option::Option::Some(#default.as_bytes().to_vec());
+        },
+        Shape::Required => quote!(partial.#ident = #default.as_bytes().to_vec();),
         // Rejected in the model: a count starts at zero, and a default for a collecting
         // field is not applied yet.
         Shape::Count => quote!(partial.#ident = ::std::default::Default::default();),
@@ -1123,13 +1141,13 @@ pub fn emit_args(cli: &Cli) -> TokenStream {
                 ..CommandMeta::EMPTY
             };
 
-            pub fn __usage_text(value: &[u8]) -> ::std::string::String {
-                ::std::string::String::from_utf8_lossy(value).into_owned()
+            pub fn __usage_text(value: &[u8]) -> ::std::vec::Vec<u8> {
+                value.to_vec()
             }
 
             pub fn __usage_value_text(
                 value: ::std::option::Option<&[u8]>,
-            ) -> ::std::string::String {
+            ) -> ::std::vec::Vec<u8> {
                 value.map(__usage_text).unwrap_or_default()
             }
 
@@ -1427,9 +1445,13 @@ fn post_binding(cli: &Cli) -> TokenStream {
         let given = format_ident!("__given_{}", ident);
         let var = f.env.as_deref()?;
         let assign = match f.shape {
-            Shape::Optional => quote!(partial.#ident = ::std::option::Option::Some(value);),
-            Shape::Required => quote!(partial.#ident = value;),
-            Shape::Many => quote!(partial.#ident.push(value);),
+            // `env::var` gives text, which is right for an environment variable: the
+            // partial holds bytes because *argv* may not be UTF-8, and this is not argv.
+            Shape::Optional => quote! {
+                partial.#ident = ::std::option::Option::Some(value.into_bytes());
+            },
+            Shape::Required => quote!(partial.#ident = value.into_bytes();),
+            Shape::Many => quote!(partial.#ident.push(value.into_bytes());),
             // A switch reads as on for anything but the spellings of "off", which is
             // what every tool that takes a boolean from the environment settles on.
             Shape::Bool => quote! {
@@ -1515,7 +1537,19 @@ fn post_binding(cli: &Cli) -> TokenStream {
         };
         Some(quote! {
             for value in #values {
-                if !#choices.contains(&value.as_str()) {
+                // Compared as text, since a choice is a word.
+                //
+                // Bytes that are not UTF-8 are passed over rather than reported here. They
+                // are not any of the choices, but saying so would answer the wrong question:
+                // `InvalidChoice` lists words, and this value is not a word at all. Left
+                // alone, it reaches `build`, which reports the UTF-8 failure with the value
+                // in it. Comparing the empty string instead — which is what `unwrap_or_default`
+                // did — made every such value collide with the choices check first.
+                let ::std::result::Result::Ok(__usage_text) = ::std::str::from_utf8(value)
+                else {
+                    continue;
+                };
+                if !#choices.contains(&__usage_text) {
                     return ::std::result::Result::Err(
                         ::usage_argv::Error::InvalidChoice {
                             name: #name,

derive/src/lib.rs

@@ -185,20 +185,17 @@
 //! should answer to quietly, each accepting several as a list. The parser matches both;
 //! the difference is only whether help and completions mention them.
 //!
+//! A word is held as the bytes it arrived as and converted once, where the struct is built.
+//! So a value that is not valid UTF-8 is **reported** rather than quietly replaced with
+//! `U+FFFD` — which for a `PathBuf` meant a different file, silently. What is still missing
+//! is *accepting* such a value: recovering an `OsString` from those bytes needs
+//! `OsStr::from_encoded_bytes_unchecked`, which is `unsafe`, and this crate has none.
+//!
 //! # What this version does not do
 //!
 //! Published early on purpose, so it can be used and argued with — but these are
 //! real limits, not omissions from the docs.
 //!
-//! - **A field whose type shadows the name `String`.** A word reaching a `String` field is
-//!   moved rather than converted, which costs nothing; the type is recognised by how it is
-//!   written, because a macro cannot resolve a name. So `use my_crate::String` followed by
-//!   a `String` field of that type fails to compile. The same change that fixes the next
-//!   item removes this one.
-//! - **Values that are not valid UTF-8.** A word reaches a field through
-//!   `String::from_utf8_lossy`, so a `PathBuf` field holding a path that is not UTF-8
-//!   gets the replacement character rather than the bytes. Rare, and wrong when it
-//!   happens; holding what was typed rather than a lossy copy of it is the next change.
 //! - **Flattening.** A struct cannot yet borrow another struct's flags, so a set of
 //!   options shared by several commands has to be repeated.