From 5257420b395ed97c12bad6a379f406454989cf25 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:21:57 +0000 Subject: [PATCH 1/2] feat(derive): hold the bytes a word arrived as MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A value used to reach its field 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. That is the worst shape a bug can take: no error, wrong answer. The partial holds the bytes now and the conversion happens once, where the struct is built. `apply` cannot report anything — it answers whether an event belonged to this command — which is why the bytes have to travel that far before anyone can complain about them. A word that is not UTF-8 is an `InvalidValue` naming the field, showing the value lossily *for the message only*, and saying what was wrong with it. It also retires a hazard review raised on the last PR: `String` was recognised by how it was written so that its value could be moved rather than converted, which broke for an adopter who shadowed the name. There is no identity case left — everything converts — so recognising the spelling only skips a second step now, and getting it wrong is a compile error rather than a mangled value. Costs +656 instructions (1.6%) and one allocation against main, measured on the same fixture: the collecting path rebuilds a `Vec` from bytes rather than moving one. That is what not corrupting a value is worth. Reporting such a value is not the same as accepting it, and accepting it needs `OsStr::from_encoded_bytes_unchecked` — `unsafe`, in a crate that has none. The call would be sound and PLAN.md says why, but that is jdx`s call rather than mine. --- PLAN.md | 17 +++-- conformance/tests/typed.rs | 46 ++++++++++++ derive/src/codegen.rs | 141 ++++++++++++++++++++++--------------- derive/src/lib.rs | 15 ++-- 4 files changed, 147 insertions(+), 72 deletions(-) diff --git a/PLAN.md b/PLAN.md index 24e84305e..aba9f156f 100644 --- a/PLAN.md +++ b/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, diff --git a/conformance/tests/typed.rs b/conformance/tests/typed.rs index a06378d6d..8355ab621 100644 --- a/conformance/tests/typed.rs +++ b/conformance/tests/typed.rs @@ -322,3 +322,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, + /// Anything at all + #[usage(long)] + text: Option, +} + +#[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")); +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index d2d275537..e32820e49 100644 --- a/derive/src/codegen.rs +++ b/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 { + value.to_vec() } pub fn __usage_value_text( value: ::std::option::Option<&[u8]>, - ) -> ::std::string::String { + ) -> ::std::vec::Vec { 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>), + Shape::Required => quote!(::std::vec::Vec), + Shape::Many => quote!(::std::vec::Vec<::std::vec::Vec>), }; 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` 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>` 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 { + value.to_vec() } pub fn __usage_value_text( value: ::std::option::Option<&[u8]>, - ) -> ::std::string::String { + ) -> ::std::vec::Vec { 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,10 @@ 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 not any of the choices, and `build` is where that is reported. + let __usage_text = ::std::str::from_utf8(value).unwrap_or_default(); + if !#choices.contains(&__usage_text) { return ::std::result::Result::Err( ::usage_argv::Error::InvalidChoice { name: #name, diff --git a/derive/src/lib.rs b/derive/src/lib.rs index aa704709d..103f07242 100644 --- a/derive/src/lib.rs +++ b/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. From ea7b427785427d58f7b40a7e3d840248f0aa24a1 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:20:28 +0000 Subject: [PATCH 2/2] fix(derive): report the UTF-8 failure, not the choice list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A field with `choices` or `value_enum` reached the choices check before the struct was built, and that check compared the value as text through `from_utf8(...).unwrap_or_default()` — so a value that is not UTF-8 became the empty string, matched none of the choices, and came back as `InvalidChoice`: a message listing words, about a value that is not a word. The comment beside it already claimed `build` was where this got reported. It was not. Bytes that will not convert are passed over here and left for `build`, which names the field and gives the UTF-8 error as the reason. A value that *is* text and is not one of the choices still gets the list, which is what the check is for. Found by Cursor Bugbot on #841. Co-Authored-By: Claude Opus 5 --- conformance/tests/typed.rs | 36 ++++++++++++++++++++++++++++++++++++ derive/src/codegen.rs | 15 ++++++++++++--- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/conformance/tests/typed.rs b/conformance/tests/typed.rs index 8355ab621..78a5344a1 100644 --- a/conformance/tests/typed.rs +++ b/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 diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index e32820e49..0f7e95a1e 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -1537,9 +1537,18 @@ fn post_binding(cli: &Cli) -> TokenStream { }; Some(quote! { for value in #values { - // Compared as text, since a choice is a word. Bytes that are not UTF-8 - // are not any of the choices, and `build` is where that is reported. - let __usage_text = ::std::str::from_utf8(value).unwrap_or_default(); + // 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 {