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..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 @@ -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, + /// 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..0f7e95a1e 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,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, 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.