diff --git a/crates/agent/src/agent_platform.rs b/crates/agent/src/agent_platform.rs index 90fa36b9a4..23a810336d 100644 --- a/crates/agent/src/agent_platform.rs +++ b/crates/agent/src/agent_platform.rs @@ -83,7 +83,7 @@ impl ManagedFile { pub fn ensure_contents(&mut self, contents: &[u8]) -> eyre::Result { let destination_exists = self.path.try_exists().wrap_err_with(|| { format!( - "Couldn't check existence of destination file {f}", + "couldn't check existence of destination file {f}", f = self.path.display() ) })?; @@ -93,7 +93,7 @@ impl ManagedFile { true => { let current_contents = std::fs::read(self.path.as_path()).wrap_err_with(|| { format!( - "Couldn't read current file contents of {f}", + "couldn't read current file contents of {f}", f = self.path.display() ) })?; @@ -124,26 +124,26 @@ fn safe_copy(destination_path: &Path, source_path: &Path) -> eyre::Result<()> { let mut tmp_destination = NamedTempFile::with_suffix_in(".tmp", destination_dirname) .with_context(|| { format!( - "Couldn't create temporary file in destination directory {d}", + "couldn't create temporary file in destination directory {d}", d = destination_dirname.display() ) })?; std::io::copy(&mut source_file, &mut tmp_destination).with_context(|| { format!( - "Couldn't copy file contents from {s} to {d}", + "couldn't copy file contents from {s} to {d}", s = source_path.display(), d = tmp_destination.path().display() ) })?; let destination_file = tmp_destination.persist(destination_path).with_context(|| { format!( - "Couldn't persist contents to destination file {d}", + "couldn't persist contents to destination file {d}", d = destination_path.display() ) })?; destination_file.sync_all().with_context(|| { format!( - "Couldn't sync file data of destination file {d}", + "couldn't sync file data of destination file {d}", d = destination_path.display() ) })?; @@ -167,25 +167,25 @@ fn safe_write(destination_path: &Path, contents: &[u8]) -> eyre::Result<()> { .tempfile_in(destination_dirname) .with_context(|| { format!( - "Couldn't create temporary file in destination directory {d}", + "couldn't create temporary file in destination directory {d}", d = destination_dirname.display() ) })?; tmp_destination.write_all(contents).with_context(|| { format!( - "Couldn't write file contents to {d}", + "couldn't write file contents to {d}", d = destination_path.display() ) })?; let destination_file = tmp_destination.persist(destination_path).with_context(|| { format!( - "Couldn't persist contents to destination file {d}", + "couldn't persist contents to destination file {d}", d = destination_path.display() ) })?; destination_file.sync_all().with_context(|| { format!( - "Couldn't sync file data of destination file {d}", + "couldn't sync file data of destination file {d}", d = destination_path.display() ) })?; diff --git a/crates/agent/src/extension_services/k8s_pod_handler.rs b/crates/agent/src/extension_services/k8s_pod_handler.rs index 4be2358b52..806c21c9e5 100644 --- a/crates/agent/src/extension_services/k8s_pod_handler.rs +++ b/crates/agent/src/extension_services/k8s_pod_handler.rs @@ -287,7 +287,7 @@ impl KubernetesPodServicesHandler { .await .wrap_err_with(|| { format!( - "Failed to rename {} to {}", + "failed to rename {} to {}", tmp_path.display(), pod_spec_path.display() ) @@ -326,7 +326,7 @@ impl KubernetesPodServicesHandler { } Err(e) => { return Err(e).wrap_err_with(|| { - format!("Failed to remove pod spec {}", pod_spec_path.display()) + format!("failed to remove pod spec {}", pod_spec_path.display()) }); } } @@ -1015,13 +1015,13 @@ Environment="NO_PROXY=127.0.0.1,localhost,.svc,.svc.cluster.local" let kubelet_dir = Path::new(KUBERNETES_POD_DIR); std::fs::create_dir_all(kubelet_dir).wrap_err_with(|| { format!( - "Failed to create kubelet directory at {}", + "failed to create kubelet directory at {}", kubelet_dir.display() ) })?; let dir_iter = std::fs::read_dir(kubelet_dir).wrap_err_with(|| { - format!("Failed to read kubelet directory {}", kubelet_dir.display()) + format!("failed to read kubelet directory {}", kubelet_dir.display()) })?; for entry in dir_iter { diff --git a/crates/agent/src/hbn.rs b/crates/agent/src/hbn.rs index 3e57dcca87..f7f31e3a3d 100644 --- a/crates/agent/src/hbn.rs +++ b/crates/agent/src/hbn.rs @@ -98,7 +98,7 @@ pub async fn run_in_container_shell(cmd: &str) -> Result<(), eyre::Report> { run_in_container(&container_id, &["bash", "-c", cmd], check_result) .await .wrap_err_with(|| { - format!("Failed executing '{cmd}' in container. Check logs in /var/log/doca/hbn/") + format!("failed executing '{cmd}' in container. check logs in /var/log/doca/hbn/") })?; Ok(()) } diff --git a/crates/machine-controller/src/scout_firmware_scripts.rs b/crates/machine-controller/src/scout_firmware_scripts.rs index a73e5056b0..7a10878500 100644 --- a/crates/machine-controller/src/scout_firmware_scripts.rs +++ b/crates/machine-controller/src/scout_firmware_scripts.rs @@ -116,7 +116,7 @@ fn find_scout_script_in( let script = fs::read(&script_path).wrap_err_with(|| { format!( - "failed to read Scout firmware script {}", + "failed to read scout firmware script {}", script_path.display() ) })?; @@ -249,14 +249,14 @@ fn script_url(pxe_public_base_url: &str, relative_path: &str) -> String { fn read_metadata(path: &Path) -> eyre::Result { let metadata = fs::read_to_string(path).wrap_err_with(|| { format!( - "failed to read Scout firmware script metadata {}", + "failed to read scout firmware script metadata {}", path.display() ) })?; toml::from_str(&metadata).wrap_err_with(|| { format!( - "failed to parse Scout firmware script metadata {}", + "failed to parse scout firmware script metadata {}", path.display() ) }) @@ -461,7 +461,7 @@ mod tests { assert!( error .to_string() - .contains("failed to parse Scout firmware script metadata") + .contains("failed to parse scout firmware script metadata") ); } } diff --git a/crates/xtask/src/error_message_case.rs b/crates/xtask/src/error_message_case.rs index c8d6d73fa7..9f47c7ce29 100644 --- a/crates/xtask/src/error_message_case.rs +++ b/crates/xtask/src/error_message_case.rs @@ -69,13 +69,13 @@ const CONV_METHODS: &[&str] = &["into", "to_string", "to_owned", "into_owned"]; const FORMAT_MACROS: &[&str] = &["format", "format_args"]; // TODO: `format!(...)` is now peeled when it's the message argument of a -// recognized error slot (see `leading_str_lit`), but a few forms remain out of -// reach: a `format!` behind a *block*-bodied closure (`.wrap_err_with(|| { -// format!(...) })`), struct-literal error fields (`CarbideError::Internal { -// message: "..." }`), other error enums' constructors, a manual `impl Display` -// (`write!`), and an error site nested inside another macro's body (a `bail!` -// inside `tokio::select!`, which `syn` keeps as an opaque token stream). Those -// are left as-is; widening the checker further, and re-sweeping, is a follow-up. +// recognized error slot, through expression- or block-bodied closures (see +// `leading_str_lit`). A few forms still remain out of reach: struct-literal +// error fields (`CarbideError::Internal { message: "..." }`), other error enums' +// constructors, a manual `impl Display` (`write!`), and an error site nested +// inside another macro's body (a `bail!` inside `tokio::select!`, which `syn` +// keeps as an opaque token stream). Those are left as-is; widening the checker +// further, and re-sweeping what it then catches, is a follow-up. pub fn check(fix: bool) -> eyre::Result { let repo_root = PathBuf::from(REPO_ROOT).canonicalize()?; @@ -251,10 +251,18 @@ impl CheckOutcome { Problem::Capitalized => (c + 1, p), Problem::TrailingPeriod => (c, p + 1), }); + let total = self.violations.len(); println!( - "{} error-message style violations ({cap} capitalized, {period} trailing period). \ - Run `cargo xtask lint-error-messages --fix` to fix.", - self.violations.len(), + "\n{total} error-message style violation(s) ({cap} capitalized, {period} trailing period)." + ); + println!( + "Error messages should be a lowercase phrase with no trailing period (C-GOOD-ERR)." + ); + println!("\nTo fix these automatically, run:\n"); + println!(" cargo xtask lint-error-messages --fix"); + println!( + "\nA deliberate capital (an acronym or a case-sensitive token) can be kept with a \ + `// xtask:allow-error-case` comment on that line. See STYLE_GUIDE.md." ); std::process::exit(1); } @@ -408,9 +416,10 @@ fn macro_message(mac: &Macro) -> Option { leading_str_lit(args.iter().nth(index)?) } -/// Peels closures, references, trivial conversions, and a wrapping `format!` to -/// find a leading string literal — the message inside `|| "...".into()`, `&"..."`, -/// `"...".to_string()`, or `CarbideError::invalid_argument(format!("...", x))`. +/// Peels closures (expression- or block-bodied), references, trivial conversions, +/// and a wrapping `format!` to find a leading string literal — the message inside +/// `|| "...".into()`, `&"..."`, `"...".to_string()`, `|| { format!("...") }`, or +/// `CarbideError::invalid_argument(format!("...", x))`. /// Returns an owned `LitStr` (it may be parsed out of a `format!` body), but its /// span still points at the real source literal, so a fix rewrites the right bytes. fn leading_str_lit(expr: &Expr) -> Option { @@ -439,6 +448,13 @@ fn leading_str_lit(expr: &Expr) -> Option { leading_str_lit(args.first()?) } Expr::Closure(closure) => leading_str_lit(&closure.body), + // A block-bodied closure (or a bare block) forwards to its tail + // expression -- the block's value -- so `.wrap_err_with(|| { format!(...) })` + // is reached just like the expression-bodied `|| format!(...)`. + Expr::Block(block) => block.block.stmts.last().and_then(|tail| match tail { + syn::Stmt::Expr(e, None) => leading_str_lit(e), + _ => None, + }), Expr::Reference(reference) => leading_str_lit(&reference.expr), Expr::Paren(paren) => leading_str_lit(&paren.expr), Expr::Group(group) => leading_str_lit(&group.expr), @@ -782,4 +798,31 @@ mod tests { assert!(fixed.contains(r#""Bare log line stays""#), "{fixed}"); assert!(fixed.contains(r#""Plain format stays""#), "{fixed}"); } + + /// A `format!` inside a *block*-bodied closure is reached too -- the common + /// `.wrap_err_with(|| { format!(...) })` form, not just `|| format!(...)`. + #[test] + fn end_to_end_rewrite_reaches_block_closure() { + let src = concat!( + "fn f() -> anyhow::Result<()> {\n", + " thing().wrap_err_with(|| {\n", + " format!(\"Couldn't reach {host}\")\n", + " })?;\n", + " Ok(())\n", + "}\n", + ); + let ast = syn::parse_file(src).unwrap(); + let mut findings = Vec::new(); + let mut collector = Collector { + lines: src.lines().collect(), + out: &mut findings, + }; + collector.visit_file(&ast); + let (fixed, applied) = apply_fixes(src, &findings); + assert_eq!(applied, 1, "{fixed}"); + assert!( + fixed.contains(r#"format!("couldn't reach {host}")"#), + "{fixed}" + ); + } } diff --git a/crates/xtask/src/workspace_deps.rs b/crates/xtask/src/workspace_deps.rs index 8289f0b027..c955f8464e 100644 --- a/crates/xtask/src/workspace_deps.rs +++ b/crates/xtask/src/workspace_deps.rs @@ -185,7 +185,7 @@ impl Workspace { } else if let Some(version) = dep.as_str() { let version = Version::from(version).with_context(|| { format!( - "Error parsing version for dependency `{}` in {}", + "error parsing version for dependency `{}` in {}", dep_name, toml_path.display() )