feat(lints): lint format_error on anyhow::Error (#15158)

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

lints/src/format_error.rs

@@ -16,7 +16,7 @@ use clippy_utils::diagnostics::span_lint_and_help;
 use clippy_utils::macros::{
     find_format_arg_expr, find_format_args, is_format_macro, macro_backtrace,
 };
-use clippy_utils::ty::implements_trait;
+use clippy_utils::ty::{implements_trait, match_type};
 use clippy_utils::{
     is_in_cfg_test, is_in_test_function, is_trait_method, match_def_path, match_function_call,
 };
@@ -64,6 +64,7 @@ const TRACING_FIELD_DEBUG: [&str; 3] = ["tracing_core", "field", "debug"];
 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"];
 
 impl<'tcx> LateLintPass<'tcx> for FormatError {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
@@ -143,20 +144,27 @@ fn check_fmt_arg_in_anyhow_error(cx: &LateContext<'_>, arg_expr: &Expr<'_>) {
     check_fmt_arg_with_help(
         cx,
         arg_expr,
-        "consider directly wrapping the error with `anyhow::anyhow!(..)` instead of formatting it",
+        (
+            "consider directly wrapping the error with `anyhow::anyhow!(..)` instead of formatting it",
+            "consider removing the redundant wrapping of `anyhow::anyhow!(..)`",
+        ),
     );
 }
 
 fn check_fmt_arg_in_anyhow_context(cx: &LateContext<'_>, arg_expr: &Expr<'_>) {
     check_fmt_arg_with_help(
         cx,
         arg_expr,
-        "consider using `anyhow::Error::context`, `anyhow::Context::(with_)context` to \
+        (
+            "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",
+        ),
     );
 }
 
-fn check_fmt_arg_with_help(cx: &LateContext<'_>, arg_expr: &Expr<'_>, help: &str) {
+fn check_fmt_arg_with_help(cx: &LateContext<'_>, arg_expr: &Expr<'_>, help: impl Help) {
     check_arg(cx, arg_expr, arg_expr.span, help);
 }
 
@@ -169,27 +177,56 @@ fn check_to_string_call(cx: &LateContext<'_>, receiver: &Expr<'_>, to_string_spa
     );
 }
 
-fn check_arg(cx: &LateContext<'_>, arg_expr: &Expr<'_>, span: Span, help: &str) {
+fn check_arg(cx: &LateContext<'_>, arg_expr: &Expr<'_>, span: Span, help: impl Help) {
     let Some(error_trait_id) = cx.tcx.get_diagnostic_item(sym::Error) else {
         return;
     };
 
     let ty = cx.typeck_results().expr_ty(arg_expr).peel_refs();
 
-    if implements_trait(cx, ty, error_trait_id, &[]) {
-        if let Some(span) = core::iter::successors(Some(span), |s| s.parent_callsite())
-            .find(|s| s.can_be_used_for_suggestions())
-        {
-            // TODO: applicable suggestions
-            span_lint_and_help(
-                cx,
-                FORMAT_ERROR,
-                span,
-                "should not format error directly",
-                None,
-                help,
-            );
-        }
+    let help = if implements_trait(cx, ty, error_trait_id, &[]) {
+        help.normal_help()
+    } else if match_type(cx, ty, &ANYHOW_ERROR) {
+        help.anyhow_help()
+    } else {
+        return;
+    };
+
+    if let Some(span) = core::iter::successors(Some(span), |s| s.parent_callsite())
+        .find(|s| s.can_be_used_for_suggestions())
+    {
+        // TODO: applicable suggestions
+        span_lint_and_help(
+            cx,
+            FORMAT_ERROR,
+            span,
+            "should not format error directly",
+            None,
+            help,
+        );
+    }
+}
+
+trait Help {
+    fn normal_help(&self) -> &str;
+    fn anyhow_help(&self) -> &str {
+        self.normal_help()
+    }
+}
+
+impl Help for &str {
+    fn normal_help(&self) -> &str {
+        self
+    }
+}
+
+impl Help for (&str, &str) {
+    fn normal_help(&self) -> &str {
+        self.0
+    }
+
+    fn anyhow_help(&self) -> &str {
+        self.1
     }
 }
 

lints/src/lib.rs

@@ -14,6 +14,7 @@
 
 #![feature(rustc_private)]
 #![feature(let_chains)]
+#![feature(lazy_cell)]
 #![warn(unused_extern_crates)]
 
 extern crate rustc_ast;

lints/ui/format_error.rs

@@ -55,4 +55,22 @@ fn main() {
     let _ = anyhow!("{:?}", err);
     let _ = anyhow!("some error occurred: {}", err);
     let _ = anyhow!("some error occurred: {:?}", err);
+
+    // `anyhow::Error` does not implement `Error` trait, test the special path here.
+    let make_anyhow_err = || anyhow!("foobar");
+    let anyhow_err = make_anyhow_err();
+
+    let _ = format!("{}", anyhow_err);
+    let _ = format!("{}", &anyhow_err);
+    let _ = format!("{}", &&anyhow_err);
+    let _ = format!("{}", Box::new(&anyhow_err)); // TODO: fail to lint
+
+    tracing::field::display(&anyhow_err);
+    tracing::field::debug(make_anyhow_err());
+
+    let _ = anyhow_err.to_string();
+    let _ = (&&anyhow_err).to_string();
+
+    let _ = anyhow!("{}", anyhow_err);
+    let _ = anyhow!("some error occurred: {:?}", anyhow_err);
 }

lints/ui/format_error.stderr

@@ -262,15 +262,87 @@ error: should not format error directly
 LL |     let _ = anyhow!("some error occurred: {}", err);
    |                                                ^^^
    |
-   = help: consider using `anyhow::Error::context`, `anyhow::Context::(with_)context` to attach additional message to the error and make it an error source instead
+   = 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:57:50
    |
 LL |     let _ = anyhow!("some error occurred: {:?}", err);
    |                                                  ^^^
    |
-   = help: consider using `anyhow::Error::context`, `anyhow::Context::(with_)context` to attach additional message to the error and make it an error source instead
+   = 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 34 previous errors
+error: should not format error directly
+  --> $DIR/format_error.rs:63:27
+   |
+LL |     let _ = format!("{}", anyhow_err);
+   |                           ^^^^^^^^^^
+   |
+   = help: consider importing `thiserror_ext::AsReport` and using `.as_report()` instead
+
+error: should not format error directly
+  --> $DIR/format_error.rs:64:27
+   |
+LL |     let _ = format!("{}", &anyhow_err);
+   |                           ^^^^^^^^^^^
+   |
+   = help: consider importing `thiserror_ext::AsReport` and using `.as_report()` instead
+
+error: should not format error directly
+  --> $DIR/format_error.rs:65:27
+   |
+LL |     let _ = format!("{}", &&anyhow_err);
+   |                           ^^^^^^^^^^^^
+   |
+   = help: consider importing `thiserror_ext::AsReport` and using `.as_report()` instead
+
+error: should not format error directly
+  --> $DIR/format_error.rs:68:29
+   |
+LL |     tracing::field::display(&anyhow_err);
+   |                             ^^^^^^^^^^^
+   |
+   = help: consider importing `thiserror_ext::AsReport` and recording the error as a field with `error = %<error>.as_report()` instead
+
+error: should not format error directly
+  --> $DIR/format_error.rs:69:27
+   |
+LL |     tracing::field::debug(make_anyhow_err());
+   |                           ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider importing `thiserror_ext::AsReport` and recording the error as a field with `error = %<error>.as_report()` instead
+
+error: should not format error directly
+  --> $DIR/format_error.rs:71:24
+   |
+LL |     let _ = anyhow_err.to_string();
+   |                        ^^^^^^^^^^^
+   |
+   = help: consider importing `thiserror_ext::AsReport` and using `.to_report_string()` instead
+
+error: should not format error directly
+  --> $DIR/format_error.rs:72:28
+   |
+LL |     let _ = (&&anyhow_err).to_string();
+   |                            ^^^^^^^^^^^
+   |
+   = help: consider importing `thiserror_ext::AsReport` and using `.to_report_string()` instead
+
+error: should not format error directly
+  --> $DIR/format_error.rs:74:27
+   |
+LL |     let _ = anyhow!("{}", anyhow_err);
+   |                           ^^^^^^^^^^
+   |
+   = help: consider removing the redundant wrapping of `anyhow::anyhow!(..)`
+
+error: should not format error directly
+  --> $DIR/format_error.rs:75:50
+   |
+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
 

src/cmd/src/lib.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use risingwave_common::error::v2::AsReport as _;
 use risingwave_compactor::CompactorOpts;
 use risingwave_compute::ComputeNodeOpts;
 use risingwave_ctl::CliOpts as CtlOpts;
@@ -67,13 +68,12 @@ pub fn ctl(opts: CtlOpts) {
     // Note: Use a simple current thread runtime for ctl.
     // When there's a heavy workload, multiple thread runtime seems to respond slowly. May need
     // further investigation.
-    tokio::runtime::Builder::new_current_thread()
+    if let Err(e) = tokio::runtime::Builder::new_current_thread()
         .enable_all()
         .build()
         .unwrap()
         .block_on(risingwave_ctl::start(opts))
-        .inspect_err(|e| {
-            eprintln!("{:#?}", e);
-        })
-        .unwrap();
+    {
+        eprintln!("Error: {:#?}", e.as_report());
+    }
 }

src/cmd_all/Cargo.toml

@@ -58,6 +58,7 @@ workspace-hack = { path = "../workspace-hack" }
 expect-test = "1"
 
 [build-dependencies]
+thiserror-ext = { workspace = true }
 vergen = { version = "8", default-features = false, features = [
     "build",
     "git",

src/cmd_all/build.rs

@@ -12,11 +12,12 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use thiserror_ext::AsReport;
 use vergen::EmitBuilder;
 
 fn main() {
     if let Err(e) = EmitBuilder::builder().git_sha(true).fail_on_error().emit() {
         // Leave the environment variable unset if error occurs.
-        println!("cargo:warning={}", e)
+        println!("cargo:warning={}", e.as_report())
     }
 }

src/common/src/config.rs

@@ -391,9 +391,9 @@ impl<'de> Deserialize<'de> for DefaultParallelism {
                     VirtualNode::COUNT
                 )))?
             } else {
-                NonZeroUsize::new(i)
-                    .context("default parallelism should be greater than 0")
-                    .map_err(|e| serde::de::Error::custom(e.to_string()))?
+                NonZeroUsize::new(i).ok_or_else(|| {
+                    serde::de::Error::custom("default parallelism should be greater than 0")
+                })?
             })),
         }
     }

src/connector/src/sink/nats.rs

@@ -107,12 +107,9 @@ impl Sink for NatsSink {
                 "Nats sink only support append-only mode"
             )));
         }
-        let _client = self
-            .config
-            .common
-            .build_client()
-            .await
-            .context("validate nats sink error")?;
+        let _client = (self.config.common.build_client().await)
+            .context("validate nats sink error")
+            .map_err(SinkError::Nats)?;
         Ok(())
     }
 

src/connector/src/source/cdc/source/reader.rs

@@ -14,7 +14,7 @@
 
 use std::str::FromStr;
 
-use anyhow::anyhow;
+use anyhow::{anyhow, Context};
 use async_trait::async_trait;
 use futures_async_stream::try_stream;
 use itertools::Itertools;
@@ -79,8 +79,8 @@ impl<T: CdcSourceTypeTrait> SplitReader for CdcSplitReader<T> {
         if matches!(T::source_type(), CdcSourceType::Citus)
             && let Some(server_addr) = split.server_addr()
         {
-            let host_addr = HostAddr::from_str(&server_addr)
-                .map_err(|err| anyhow!("invalid server address for cdc split. {}", err))?;
+            let host_addr =
+                HostAddr::from_str(&server_addr).context("invalid server address for cdc split")?;
             properties.insert("hostname".to_string(), host_addr.host);
             properties.insert("port".to_string(), host_addr.port.to_string());
             // rewrite table name with suffix to capture all shards in the split
@@ -218,7 +218,7 @@ impl<T: CdcSourceTypeTrait> CommonSplitReader for CdcSplitReader<T> {
                     GLOBAL_ERROR_METRICS.cdc_source_error.report([
                         source_type.as_str_name().into(),
                         source_id.clone(),
-                        e.to_string(),
+                        e.to_report_string(),
                     ]);
                     Err(e)?;
                 }

src/connector/src/source/kafka/enumerator/client.rs

@@ -15,7 +15,7 @@
 use std::collections::HashMap;
 use std::time::Duration;
 
-use anyhow::{anyhow, Context as _};
+use anyhow::{anyhow, Context};
 use async_trait::async_trait;
 use rdkafka::consumer::{BaseConsumer, Consumer};
 use rdkafka::error::KafkaResult;
@@ -112,6 +112,7 @@ impl SplitEnumerator for KafkaSplitEnumerator {
                 self.broker_address
             )
         })?;
+
         let watermarks = self.get_watermarks(topic_partitions.as_ref()).await?;
         let mut start_offsets = self
             .fetch_start_offset(topic_partitions.as_ref(), &watermarks)

src/error/src/lib.rs

@@ -23,3 +23,6 @@
 
 pub mod anyhow;
 pub mod tonic;
+
+// Re-export the `thiserror-ext` crate.
+pub use thiserror_ext::*;

src/frontend/planner_test/src/lib.rs

@@ -944,7 +944,7 @@ pub async fn run_test_file(file_path: &Path, file_content: &str) -> Result<()> {
                     "Test #{i} (id: {}) failed, SQL:\n{}\nError: {}",
                     c.id().clone().unwrap_or_else(|| "<none>".to_string()),
                     c.sql(),
-                    e
+                    e.as_report()
                 );
                 failed_num += 1;
             }