feat(lints): improve help message when formatting error in anyhow m…

…acro (#14957) Signed-off-by: Bugen Zhao <[email protected]>
risingwavelabs · Feb 5, 2024 · f003f28 · f003f28
1 parent 2349211
commit f003f28
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 26 deletions.
diff --git a/lints/Cargo.lock b/lints/Cargo.lock
diff --git a/lints/Cargo.toml b/lints/Cargo.toml
@@ -22,6 +22,7 @@ itertools = "0.12"
 dylint_testing = "2.6.0"
 
 # UI test dependencies
+anyhow = "1"
 tracing = "0.1"
 
 [package.metadata.rust-analyzer]

diff --git a/lints/src/format_error.rs b/lints/src/format_error.rs
@@ -62,7 +62,8 @@ impl_lint_pass!(FormatError => [FORMAT_ERROR]);
 
 const TRACING_FIELD_DEBUG: [&str; 3] = ["tracing_core", "field", "debug"];
 const TRACING_FIELD_DISPLAY: [&str; 3] = ["tracing_core", "field", "display"];
-const TRACING_MACRO_EVENT: [&str; 3] = ["tracing", "macros", "event"];
+const TRACING_MACROS_EVENT: [&str; 3] = ["tracing", "macros", "event"];
+const ANYHOW_MACROS_ANYHOW: [&str; 3] = ["anyhow", "macros", "anyhow"];
 
 impl<'tcx> LateLintPass<'tcx> for FormatError {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
@@ -76,12 +77,14 @@ impl<'tcx> LateLintPass<'tcx> for FormatError {
             .or_else(|| match_function_call(cx, expr, &TRACING_FIELD_DISPLAY))
             && let [arg_expr, ..] = args
         {
-            check_fmt_arg(cx, arg_expr);
+            check_fmt_arg_in_tracing_event(cx, arg_expr);
         }
 
-        // `{}`, `{:?}` in format macros.
-        let in_tracing_macro_event = macro_backtrace(expr.span)
-            .any(|macro_call| match_def_path(cx, macro_call.def_id, &TRACING_MACRO_EVENT));
+        // Indirect `{}`, `{:?}` from other macros.
+        let in_tracing_event_macro = macro_backtrace(expr.span)
+            .any(|macro_call| match_def_path(cx, macro_call.def_id, &TRACING_MACROS_EVENT));
+        let in_anyhow_macro = macro_backtrace(expr.span)
+            .any(|macro_call| match_def_path(cx, macro_call.def_id, &ANYHOW_MACROS_ANYHOW));
 
         for macro_call in macro_backtrace(expr.span) {
             if is_format_macro(cx, macro_call.def_id)
@@ -93,8 +96,14 @@ impl<'tcx> LateLintPass<'tcx> for FormatError {
                         && let Some(arg) = format_args.arguments.all_args().get(index)
                         && let Ok(arg_expr) = find_format_arg_expr(expr, arg)
                     {
-                        if in_tracing_macro_event {
+                        if in_tracing_event_macro {
                             check_fmt_arg_in_tracing_event(cx, arg_expr);
+                        } else if in_anyhow_macro {
+                            if format_args.template.len() == 1 {
+                                check_fmt_arg_in_anyhow_error(cx, arg_expr);
+                            } else {
+                                check_fmt_arg_in_anyhow_context(cx, arg_expr);
+                            }
                         } else {
                             check_fmt_arg(cx, arg_expr);
                         }
@@ -113,25 +122,44 @@ impl<'tcx> LateLintPass<'tcx> for FormatError {
 }
 
 fn check_fmt_arg(cx: &LateContext<'_>, arg_expr: &Expr<'_>) {
-    check_arg(
+    check_fmt_arg_with_help(
         cx,
         arg_expr,
-        arg_expr.span,
         "consider importing `thiserror_ext::AsReport` and using `.as_report()` instead",
-    );
+    )
 }
 
 fn check_fmt_arg_in_tracing_event(cx: &LateContext<'_>, arg_expr: &Expr<'_>) {
     // TODO: replace `<error>` with the actual code snippet.
-    check_arg(
+    check_fmt_arg_with_help(
+        cx,
+        arg_expr,
+        "consider importing `thiserror_ext::AsReport` and recording the error as a field \
+        with `error = %<error>.as_report()` instead",
+    );
+}
+
+fn check_fmt_arg_in_anyhow_error(cx: &LateContext<'_>, arg_expr: &Expr<'_>) {
+    check_fmt_arg_with_help(
         cx,
         arg_expr,
-        arg_expr.span,
-        "consider importing `thiserror_ext::AsReport` and recording the error as a field
-with `error = %<error>.as_report()` instead",
+        "consider directly wrapping the error with `anyhow::anyhow!(..)` instead of formatting it",
     );
 }
 
+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 \
+        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) {
+    check_arg(cx, arg_expr, arg_expr.span, help);
+}
+
 fn check_to_string_call(cx: &LateContext<'_>, receiver: &Expr<'_>, to_string_span: Span) {
     check_arg(
         cx,

diff --git a/lints/ui/format_error.rs b/lints/ui/format_error.rs
@@ -48,4 +48,11 @@ fn main() {
     let _ = (err.clone()).to_string();
     let _ = err.to_string().to_string();
     let _ = (&&err).to_string();
+
+    use anyhow::anyhow;
+
+    let _ = anyhow!("{}", err);
+    let _ = anyhow!("{:?}", err);
+    let _ = anyhow!("some error occurred: {}", err);
+    let _ = anyhow!("some error occurred: {:?}", err);
 }
diff --git a/lints/ui/format_error.stderr b/lints/ui/format_error.stderr
@@ -118,74 +118,71 @@ error: should not format error directly
 LL |     info!("{}", err);
    |                 ^^^
    |
-   = help: consider importing `thiserror_ext::AsReport` and recording the error as a field
-           with `error = %<error>.as_report()` instead
+   = 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:32:20
    |
 LL |     my_info!("{}", err);
    |                    ^^^
    |
-   = help: consider importing `thiserror_ext::AsReport` and recording the error as a field
-           with `error = %<error>.as_report()` instead
+   = 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:34:29
    |
 LL |     tracing::field::display(&err);
    |                             ^^^^
    |
-   = help: consider importing `thiserror_ext::AsReport` and using `.as_report()` instead
+   = 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:35:27
    |
 LL |     tracing::field::debug(err.clone());
    |                           ^^^^^^^^^^^
    |
-   = help: consider importing `thiserror_ext::AsReport` and using `.as_report()` instead
+   = 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:37:5
    |
 LL |     info!(%err, "233");
    |     ^^^^^^^^^^^^^^^^^^
    |
-   = help: consider importing `thiserror_ext::AsReport` and using `.as_report()` instead
+   = 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:38:5
    |
 LL |     info!(?err, "233");
    |     ^^^^^^^^^^^^^^^^^^
    |
-   = help: consider importing `thiserror_ext::AsReport` and using `.as_report()` instead
+   = 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:39:23
    |
 LL |     info!(%err, "{}", err);
    |                       ^^^
    |
-   = help: consider importing `thiserror_ext::AsReport` and recording the error as a field
-           with `error = %<error>.as_report()` instead
+   = 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:39:5
    |
 LL |     info!(%err, "{}", err);
    |     ^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: consider importing `thiserror_ext::AsReport` and using `.as_report()` instead
+   = 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:40:13
    |
 LL |     let _ = info_span!("span", %err);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: consider importing `thiserror_ext::AsReport` and using `.as_report()` instead
+   = 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:44:9
@@ -243,5 +240,37 @@ LL |     let _ = (&&err).to_string();
    |
    = help: consider importing `thiserror_ext::AsReport` and using `.to_report_string()` instead
 
-error: aborting due to 30 previous errors
+error: should not format error directly
+  --> $DIR/format_error.rs:54:27
+   |
+LL |     let _ = anyhow!("{}", err);
+   |                           ^^^
+   |
+   = help: consider directly wrapping the error with `anyhow::anyhow!(..)` instead of formatting it
+
+error: should not format error directly
+  --> $DIR/format_error.rs:55:29
+   |
+LL |     let _ = anyhow!("{:?}", err);
+   |                             ^^^
+   |
+   = help: consider directly wrapping the error with `anyhow::anyhow!(..)` instead of formatting it
+
+error: should not format error directly
+  --> $DIR/format_error.rs:56:48
+   |
+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
+
+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
+
+error: aborting due to 34 previous errors