diff --git a/lints/Cargo.lock b/lints/Cargo.lock index 650983eb008f3..b8fd465fecd7b 100644 --- a/lints/Cargo.lock +++ b/lints/Cargo.lock @@ -593,6 +593,7 @@ dependencies = [ "dylint_linting", "dylint_testing", "itertools 0.12.0", + "thiserror-ext", "tracing", ] @@ -985,6 +986,28 @@ dependencies = [ "thiserror-impl", ] +[[package]] +name = "thiserror-ext" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "368fdd73eb5f0df62797d1a5d1dca41ca4137a62f250980787ffe33e530b5d8b" +dependencies = [ + "thiserror", + "thiserror-ext-derive", +] + +[[package]] +name = "thiserror-ext-derive" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "efc1991800eaf856df8d097c909b487fdee13db47ffa293136c91cde6de4abee" +dependencies = [ + "either", + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "thiserror-impl" version = "1.0.50" diff --git a/lints/Cargo.toml b/lints/Cargo.toml index 14019f167147b..14bd09faca184 100644 --- a/lints/Cargo.toml +++ b/lints/Cargo.toml @@ -23,6 +23,7 @@ dylint_testing = "2.6.0" # UI test dependencies anyhow = "1" +thiserror-ext = "0.1" tracing = "0.1" [package.metadata.rust-analyzer] diff --git a/lints/src/format_error.rs b/lints/src/format_error.rs index 8dcbed8cb520d..5d95921e48f38 100644 --- a/lints/src/format_error.rs +++ b/lints/src/format_error.rs @@ -65,6 +65,7 @@ const TRACING_FIELD_DISPLAY: [&str; 3] = ["tracing_core", "field", "display"]; const TRACING_MACROS_EVENT: [&str; 3] = ["tracing", "macros", "event"]; const ANYHOW_MACROS_ANYHOW: [&str; 3] = ["anyhow", "macros", "anyhow"]; const ANYHOW_ERROR: [&str; 2] = ["anyhow", "Error"]; +const THISERROR_EXT_REPORT_REPORT: [&str; 3] = ["thiserror_ext", "report", "Report"]; impl<'tcx> LateLintPass<'tcx> for FormatError { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { @@ -147,6 +148,7 @@ fn check_fmt_arg_in_anyhow_error(cx: &LateContext<'_>, arg_expr: &Expr<'_>) { ( "consider directly wrapping the error with `anyhow::anyhow!(..)` instead of formatting it", "consider removing the redundant wrapping of `anyhow::anyhow!(..)`", + "consider directly wrapping the error with `anyhow::anyhow!(..)` instead of formatting its report", ), ); } @@ -159,6 +161,8 @@ fn check_fmt_arg_in_anyhow_context(cx: &LateContext<'_>, arg_expr: &Expr<'_>) { "consider using `anyhow::Context::(with_)context` to \ attach additional message to the error and make it an error source instead", "consider using `.context(..)` to \ + attach additional message to the error and make it an error source instead", + "consider using `anyhow::Context::(with_)context` to \ attach additional message to the error and make it an error source instead", ), ); @@ -188,6 +192,12 @@ fn check_arg(cx: &LateContext<'_>, arg_expr: &Expr<'_>, span: Span, help: impl H help.normal_help() } else if match_type(cx, ty, &ANYHOW_ERROR) { help.anyhow_help() + } else if match_type(cx, ty, &THISERROR_EXT_REPORT_REPORT) { + if let Some(help) = help.report_help() { + help + } else { + return; + } } else { return; }; @@ -212,6 +222,9 @@ trait Help { fn anyhow_help(&self) -> &str { self.normal_help() } + fn report_help(&self) -> Option<&str> { + None + } } impl Help for &str { @@ -220,7 +233,7 @@ impl Help for &str { } } -impl Help for (&str, &str) { +impl Help for (&str, &str, &str) { fn normal_help(&self) -> &str { self.0 } @@ -228,6 +241,10 @@ impl Help for (&str, &str) { fn anyhow_help(&self) -> &str { self.1 } + + fn report_help(&self) -> Option<&str> { + Some(self.2) + } } #[test] diff --git a/lints/ui/format_error.rs b/lints/ui/format_error.rs index 0e46c72766157..22ea8c5f88e8b 100644 --- a/lints/ui/format_error.rs +++ b/lints/ui/format_error.rs @@ -73,4 +73,11 @@ fn main() { let _ = anyhow!("{}", anyhow_err); let _ = anyhow!("some error occurred: {:?}", anyhow_err); + + use thiserror_ext::AsReport; + + let _ = anyhow!("{}", err.as_report()); + let _ = anyhow!("some error occurred: {}", err.as_report()); + let _ = anyhow!("{:?}", anyhow_err.as_report()); + let _ = anyhow!("some error occurred: {:?}", anyhow_err.as_report()); } diff --git a/lints/ui/format_error.stderr b/lints/ui/format_error.stderr index 0eb4786380a79..adb37afdf6883 100644 --- a/lints/ui/format_error.stderr +++ b/lints/ui/format_error.stderr @@ -344,5 +344,37 @@ LL | let _ = anyhow!("some error occurred: {:?}", anyhow_err); | = help: consider using `.context(..)` to attach additional message to the error and make it an error source instead -error: aborting due to 43 previous errors +error: should not format error directly + --> $DIR/format_error.rs:79:27 + | +LL | let _ = anyhow!("{}", err.as_report()); + | ^^^^^^^^^^^^^^^ + | + = help: consider directly wrapping the error with `anyhow::anyhow!(..)` instead of formatting its report + +error: should not format error directly + --> $DIR/format_error.rs:80:48 + | +LL | let _ = anyhow!("some error occurred: {}", err.as_report()); + | ^^^^^^^^^^^^^^^ + | + = help: consider using `anyhow::Context::(with_)context` to attach additional message to the error and make it an error source instead + +error: should not format error directly + --> $DIR/format_error.rs:81:29 + | +LL | let _ = anyhow!("{:?}", anyhow_err.as_report()); + | ^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider directly wrapping the error with `anyhow::anyhow!(..)` instead of formatting its report + +error: should not format error directly + --> $DIR/format_error.rs:82:50 + | +LL | let _ = anyhow!("some error occurred: {:?}", anyhow_err.as_report()); + | ^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `anyhow::Context::(with_)context` to attach additional message to the error and make it an error source instead + +error: aborting due to 47 previous errors diff --git a/src/frontend/planner_test/src/lib.rs b/src/frontend/planner_test/src/lib.rs index 1578fa5cd0dfe..a30e0cab3810f 100644 --- a/src/frontend/planner_test/src/lib.rs +++ b/src/frontend/planner_test/src/lib.rs @@ -914,17 +914,17 @@ pub async fn run_test_file(file_path: &Path, file_content: &str) -> Result<()> { let mut failed_num = 0; let cases: Vec = serde_yaml::from_str(file_content).map_err(|e| { - if let Some(loc) = e.location() { - anyhow!( - "failed to parse yaml: {}, at {}:{}:{}", - e.as_report(), + let context = if let Some(loc) = e.location() { + format!( + "failed to parse yaml at {}:{}:{}", file_path.display(), loc.line(), loc.column() ) } else { - anyhow!("failed to parse yaml: {}", e.as_report()) - } + "failed to parse yaml".to_owned() + }; + anyhow::anyhow!(e).context(context) })?; let cases = resolve_testcase_id(cases).expect("failed to resolve"); let mut outputs = vec![]; diff --git a/src/frontend/planner_test/tests/planner_test_runner.rs b/src/frontend/planner_test/tests/planner_test_runner.rs index 0ce6bba6d5e66..df6730be9fc08 100644 --- a/src/frontend/planner_test/tests/planner_test_runner.rs +++ b/src/frontend/planner_test/tests/planner_test_runner.rs @@ -14,8 +14,9 @@ use std::ffi::OsStr; -use libtest_mimic::{Arguments, Trial}; +use libtest_mimic::{Arguments, Failed, Trial}; use risingwave_planner_test::{run_test_file, test_data_dir}; +use thiserror_ext::AsReport; use tokio::runtime::Runtime; use walkdir::WalkDir; @@ -47,8 +48,11 @@ fn main() { let path = test_data_dir().join("input").join(file_name); let file_content = std::fs::read_to_string(&path).unwrap(); - build_runtime().block_on(run_test_file(&path, &file_content))?; - Ok(()) + build_runtime() + .block_on(run_test_file(&path, &file_content)) + // Manually convert to `Failed`, otherwise it will use `Display` impl of + // `anyhow::Error` which loses the source chain. + .map_err(|e| Failed::from(e.as_report())) })); } } diff --git a/src/meta/src/barrier/mod.rs b/src/meta/src/barrier/mod.rs index a59eb00c723eb..4d867d266270b 100644 --- a/src/meta/src/barrier/mod.rs +++ b/src/meta/src/barrier/mod.rs @@ -20,7 +20,7 @@ use std::mem::{replace, take}; use std::sync::Arc; use std::time::Duration; -use anyhow::anyhow; +use anyhow::Context; use arc_swap::ArcSwap; use fail::fail_point; use itertools::Itertools; @@ -973,12 +973,11 @@ impl CheckpointControl { } if let CompletingCommand::Completing { join_handle, .. } = &mut self.completing_command { - let join_result = join_handle - .await - .map_err(|e| { - anyhow!("failed to join completing command: {:?}", e.as_report()).into() - }) - .and_then(|result| result); + let join_result: MetaResult<_> = try { + join_handle + .await + .context("failed to join completing command")?? + }; // It's important to reset the completing_command after await no matter the result is err // or not, and otherwise the join handle will be polled again after ready. if let Err(e) = &join_result { diff --git a/src/stream/src/lib.rs b/src/stream/src/lib.rs index 4c452d4bb132b..7b036ed520a1d 100644 --- a/src/stream/src/lib.rs +++ b/src/stream/src/lib.rs @@ -40,6 +40,7 @@ #![feature(is_sorted)] #![feature(btree_cursors)] #![feature(assert_matches)] +#![feature(try_blocks)] #[macro_use] extern crate tracing; diff --git a/src/stream/src/task/barrier_manager.rs b/src/stream/src/task/barrier_manager.rs index faabc8f266ac8..edbd660690049 100644 --- a/src/stream/src/task/barrier_manager.rs +++ b/src/stream/src/task/barrier_manager.rs @@ -16,7 +16,7 @@ use std::collections::{HashMap, HashSet}; use std::sync::Arc; use std::time::Duration; -use anyhow::anyhow; +use anyhow::{anyhow, Context}; use futures::stream::FuturesUnordered; use futures::StreamExt; use parking_lot::Mutex; @@ -162,15 +162,7 @@ impl StreamActorManagerState { let (join_result, sender) = pending_on_none(self.creating_actors.next()).await; ( sender, - join_result - .map_err(|join_error| { - anyhow!( - "failed to join creating actors futures: {:?}", - join_error.as_report() - ) - .into() - }) - .and_then(|result| result), + try { join_result.context("failed to join creating actors futures")?? }, ) } }