feat(lints): lint on formatting Report in anyhow construction (#15457)

Signed-off-by: Bugen Zhao <[email protected]>

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]

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,14 +233,18 @@ impl Help for &str {
     }
 }
 
-impl Help for (&str, &str) {
+impl Help for (&str, &str, &str) {
     fn normal_help(&self) -> &str {
         self.0
     }
 
     fn anyhow_help(&self) -> &str {
         self.1
     }
+
+    fn report_help(&self) -> Option<&str> {
+        Some(self.2)
+    }
 }
 
 #[test]

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());
 }

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
 

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<TestCase> = 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![];

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()))
             }));
         }
     }

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 {

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;

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")?? },
         )
     }
 }