From 7eeb5e45e5df8580f5a7a209dde0170341b8a266 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 16 Oct 2026 09:15:49 +0000 Subject: [PATCH] fix(builtins): track uucore quoting-style rename in printf shim The weekly Coreutils Args Drift run has failed since 2026-10-05. It re-ports vendored uucore modules against upstream HEAD, and the regenerated `format/` no longer compiles against our handwritten shim: upstream renamed `QuotingStyle::C_NO_QUOTES` to an `Escape` variant and extracted `locale_aware_shell_escape` as a shorthand for `%q`. Both are pure renames. `Escape` and the old `C_NO_QUOTES` both select uucore's `CQuoter` with `CQuotesEnforce::None` and both are what `--quoting-style=escape` resolves to; `locale_aware_shell_escape(x)` is `locale_aware_escape_name(x, SHELL_ESCAPE)` returning `String`. The shim carries both spellings so it builds against the current pin and against upstream HEAD, which is what unblocks the drift run. `printf` reverse-matches the vendored module's numeric diagnostics by re-quoting candidate arguments with the same style, and a drift PR may only touch generated files, so it cannot keep `printf.rs` in step. The style is now named once as `DIAGNOSTIC_QUOTING_STYLE`, and a test scans the generated module for the style it actually uses and fails if that style escapes differently. Claude-Session: https://claude.ai/code/session_0112F7oWpuuyUKiXHbhqRGc1 --- .../src/builtins/generated/format_support.rs | 117 +++++++++++++++++- crates/bashkit/src/builtins/printf.rs | 27 +++- knowledge/runtimes/coreutils-args-port.md | 41 ++++++ 3 files changed, 179 insertions(+), 6 deletions(-) diff --git a/crates/bashkit/src/builtins/generated/format_support.rs b/crates/bashkit/src/builtins/generated/format_support.rs index a8cbdeb35..7944f6ad0 100644 --- a/crates/bashkit/src/builtins/generated/format_support.rs +++ b/crates/bashkit/src/builtins/generated/format_support.rs @@ -72,20 +72,56 @@ pub fn strip_errno(err: &std::io::Error) -> String { msg } +/// Stand-in for `uucore::quoting_style::QuotingStyle`, carrying only the +/// styles the vendored `format` module actually names. +/// +/// Upstream renamed the `C_NO_QUOTES` associated constant to an `Escape` +/// variant; both map to uucore's `CQuoter` with `CQuotesEnforce::None` and +/// both are what `--quoting-style=escape` selects, so the rename carries no +/// behaviour change. Both spellings live here during the window where the +/// pinned `generated/format/` still says `C_NO_QUOTES` while the drift run +/// regenerates it against an upstream HEAD that says `Escape` — a shim that +/// only knew one of them would fail the drift build. Drop `C_NO_QUOTES` once +/// the pin has moved past the rename. #[allow(non_camel_case_types)] #[derive(Clone, Copy, Debug)] pub enum QuotingStyle { + /// Upstream's pre-rename `QuotingStyle::C_NO_QUOTES`. C_NO_QUOTES, + /// Upstream's post-rename `QuotingStyle::Escape`. Same escaping. + Escape, SHELL_ESCAPE, } +/// Style the vendored `format` module quotes a rejected numeric argument +/// with before reporting it. +/// +/// `printf` recovers the raw argument by re-quoting each candidate the same +/// way and prefix-matching the diagnostic +/// ([`crate::builtins::printf`]`::numeric_argument_diagnostics`), so the two +/// sides must agree. Naming the style once keeps them in step across an +/// upstream rename: the drift PR may only touch generated files, so it +/// cannot update `printf.rs` itself. +pub const DIAGNOSTIC_QUOTING_STYLE: QuotingStyle = QuotingStyle::Escape; + pub fn locale_aware_escape_name(input: &OsStr, style: QuotingStyle) -> OsString { match style { - QuotingStyle::C_NO_QUOTES => input.to_os_string(), + // No C escaping: the escaped value is only ever a matching key and + // the text bashkit discards, never user-visible output (printf + // reports the raw argument, as bash does). + QuotingStyle::C_NO_QUOTES | QuotingStyle::Escape => input.to_os_string(), QuotingStyle::SHELL_ESCAPE => shell_quote(&input.to_string_lossy()).into(), } } +/// Mirrors `uucore::quoting_style::locale_aware_shell_escape`, the shorthand +/// upstream extracted for `printf %q`. Shell-escapes and returns `String`. +pub fn locale_aware_shell_escape(input: impl AsRef) -> String { + locale_aware_escape_name(input.as_ref(), QuotingStyle::SHELL_ESCAPE) + .into_string() + .unwrap_or_else(|os| os.to_string_lossy().into_owned()) +} + /// Most diagnostics one [`collect_diagnostics`] call keeps (one per bad /// argument; the cap bounds memory for huge argument lists). const MAX_COLLECTED_DIAGNOSTICS: usize = 64; @@ -200,3 +236,82 @@ fn shell_quote(s: &str) -> String { } out } + +#[cfg(test)] +mod tests { + use super::*; + + /// Upstream renamed `C_NO_QUOTES` to the `Escape` variant; both select + /// uucore's `CQuoter` with `CQuotesEnforce::None`, and both are what + /// `--quoting-style=escape` resolves to. Pin the equivalence so the two + /// spellings cannot drift apart while both are live. + #[test] + fn escape_and_c_no_quotes_escape_identically() { + for input in ["", "abc", "a\tb", "a b", "quote'd", "\u{1b}[0m"] { + let os = OsStr::new(input); + assert_eq!( + locale_aware_escape_name(os, QuotingStyle::C_NO_QUOTES), + locale_aware_escape_name(os, QuotingStyle::Escape), + "styles disagree on {input:?}" // debug-ok: assert-failure message + ); + } + } + + /// `locale_aware_shell_escape` is upstream's shorthand, not a second + /// implementation: it must agree with the style it delegates to. + #[test] + fn shell_escape_shorthand_matches_escape_name() { + for input in ["", "plain", "hello world", "a'b", "tab\there"] { + assert_eq!( + locale_aware_shell_escape(input), + locale_aware_escape_name(OsStr::new(input), QuotingStyle::SHELL_ESCAPE) + .to_string_lossy(), + "shorthand diverged on {input:?}" // debug-ok: assert-failure message + ); + } + } + + /// `printf` reverse-matches the vendored module's numeric diagnostics by + /// re-quoting candidates with [`DIAGNOSTIC_QUOTING_STYLE`], so the style + /// the generated code names must escape the same way. A drift PR may + /// only touch generated files, so this scan is what makes an upstream + /// switch to some other style fail loudly instead of silently dropping + /// `printf: : invalid number`. + #[test] + fn generated_module_quotes_diagnostics_with_the_style_printf_expects() { + const ARGUMENT_RS: &str = include_str!("format/argument.rs"); + + let probe = OsStr::new("a\tb c'd"); + let expected = locale_aware_escape_name(probe, DIAGNOSTIC_QUOTING_STYLE); + + let named: Vec<&str> = ARGUMENT_RS + .match_indices("QuotingStyle::") + .map(|(i, _)| { + let rest = &ARGUMENT_RS[i + "QuotingStyle::".len()..]; + let end = rest + .find(|c: char| !c.is_ascii_alphanumeric() && c != '_') + .unwrap_or(rest.len()); + &rest[..end] + }) + .collect(); + assert!( + !named.is_empty(), + "generated/format/argument.rs no longer quotes its diagnostics; \ + printf's prefix match cannot work" + ); + for name in named { + let style = match name { + "C_NO_QUOTES" => QuotingStyle::C_NO_QUOTES, + "Escape" => QuotingStyle::Escape, + "SHELL_ESCAPE" => QuotingStyle::SHELL_ESCAPE, + other => panic!("generated code names unknown QuotingStyle::{other}"), + }; + assert_eq!( + locale_aware_escape_name(probe, style), + expected, + "generated code quotes with QuotingStyle::{name}, which escapes \ + differently from DIAGNOSTIC_QUOTING_STYLE; update printf.rs" + ); + } + } +} diff --git a/crates/bashkit/src/builtins/printf.rs b/crates/bashkit/src/builtins/printf.rs index 31a42ece2..ff2c37455 100644 --- a/crates/bashkit/src/builtins/printf.rs +++ b/crates/bashkit/src/builtins/printf.rs @@ -16,7 +16,7 @@ use super::generated::format::{ FormatArgument, FormatArguments, FormatError, FormatItem, parse_spec_and_escape, }; use super::generated::format_support::{ - QuotingStyle, collect_diagnostics, locale_aware_escape_name, + DIAGNOSTIC_QUOTING_STYLE, collect_diagnostics, locale_aware_escape_name, }; use super::limits::PRINTF_MAX_DIAG_CHARS as MAX_PRINTF_DIAG_CHARS; use super::{Builtin, BuiltinSideEffect, Context, Date, MAX_FORMAT_WIDTH}; @@ -145,10 +145,9 @@ fn numeric_argument_diagnostics(diags: &[String], args: &[String]) -> (String, i // uucore writes `: `; recover the raw argument by // quoting each candidate the same way. let Some(arg) = args.iter().find(|a| { - let quoted = - locale_aware_escape_name(OsStr::new(a.as_str()), QuotingStyle::C_NO_QUOTES) - .quote() - .to_string(); + let quoted = locale_aware_escape_name(OsStr::new(a.as_str()), DIAGNOSTIC_QUOTING_STYLE) + .quote() + .to_string(); diag.strip_prefix(quoted.as_str()) .is_some_and(|rest| rest.starts_with(": ")) }) else { @@ -970,6 +969,24 @@ mod tests { assert_eq!(err.unwrap_err(), "bad"); } + /// The vendored `format` module quotes a rejected numeric argument + /// before reporting it, and [`numeric_argument_diagnostics`] recovers + /// the raw argument by re-quoting each candidate the same way. A + /// control character is where a style mismatch first shows: plain + /// `abc` escapes to itself under every style, so the + /// `printf_invalid_number_stderr` spec case cannot catch a + /// drift-induced divergence between the two sides. + #[test] + fn numeric_diagnostic_survives_control_characters_in_argument() { + let args = vec!["a\tb".to_string()]; + let (_, diags) = collect_diagnostics(|| render_printf_bytes("%d", &args)); + assert_eq!(diags.len(), 1, "uucore should reject the argument"); + let (stderr, status) = numeric_argument_diagnostics(&diags, &args); + // bash 5.2 prints the argument raw, tab and all. + assert_eq!(stderr, "bash: printf: a\tb: invalid number\n"); + assert_eq!(status, 1); + } + #[test] fn generated_formatter_repeats_format_until_args_exhausted() { assert_eq!( diff --git a/knowledge/runtimes/coreutils-args-port.md b/knowledge/runtimes/coreutils-args-port.md index 509cdad13..fbc0da3d6 100644 --- a/knowledge/runtimes/coreutils-args-port.md +++ b/knowledge/runtimes/coreutils-args-port.md @@ -319,6 +319,47 @@ level, so it cannot see vendored subdirectories). |---|---|---|---| | `format` | `src/uucore/src/lib/features/format` | `crates/bashkit/src/builtins/generated/format/` plus `extendedbigdecimal.rs` and `num_parser.rs` siblings | `crate::format` self-refs rewrite to `crate::builtins::generated::format`; `extendedbigdecimal` and `parser::num_parser` are inlined; `NonUtf8OsStrError`, `os_str_as_bytes`, `UError`, `set_exit_code`, `strip_errno`, `quoting_style`, `show_error`, and `show_warning` rewrite to bashkit-local `format_support` shims; `translate` / `translate_text` resolve at port time from `src/uucore/locales/{,errors/}en-US.ftl`; `host_env = ["POSIXLY_CORRECT"]` folds upstream's host-env probe to unset. | +#### Shim must straddle an upstream rename + +The `format_support` shim is handwritten, so a renamed uucore symbol +breaks the drift run and nothing else: the pinned `generated/format/` +still compiles while the regeneration against upstream HEAD does not. +The run fails at *Verify build still passes*, after regeneration, so the +patch is never uploaded and no PR opens — the weekly job simply stays +red until someone reads the log. + +Rule: when upstream renames a symbol the shim provides, add the new +spelling **without removing the old one**, in its own PR, before the +drift PR lands. The shim then builds against both the current pin and +upstream HEAD, which is the only state in which the drift run can get +past the build step. Drop the old spelling once the pin has moved. + +Worked example (2026-10): upstream renamed +`QuotingStyle::C_NO_QUOTES` to an `Escape` variant and extracted +`locale_aware_shell_escape` as a `%q` shorthand. Both are pure renames — +`Escape` and `C_NO_QUOTES` both select `CQuoter` with +`CQuotesEnforce::None`, and both are what `--quoting-style=escape` +resolves to — so the shim maps them to one arm. + +#### The drift PR cannot fix its own consumers + +The `open-pr` job enforces a generated-only diff, so an upstream change +that invalidates handwritten code **outside** `generated/` can never be +repaired by the drift PR itself. The live instance is `printf`: +`numeric_argument_diagnostics` recovers a rejected argument by +re-quoting each candidate with the same `QuotingStyle` the vendored +module used and prefix-matching the diagnostic. A mismatch drops +`printf: : invalid number` silently, and only for arguments the two +styles escape differently — plain `abc` escapes to itself under every +style, so `printf_invalid_number_stderr` cannot catch it. + +Two guards: the style is named once as +`format_support::DIAGNOSTIC_QUOTING_STYLE`, and +`format_support::tests::generated_module_quotes_diagnostics_with_the_style_printf_expects` +scans `generated/format/argument.rs` for the styles it actually names +and fails if any escapes differently from that constant. A printf unit +test covers a control-character argument, the case the spec suite misses. + ### Output banner Module-mode files carry the same banner shape as args mode: `GENERATED by