From 6969e701502cdf9cfac7767459a62ae4a85bb243 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Fri, 2 Feb 2024 17:37:17 +0800 Subject: [PATCH] feat(lints): improve help message when formatting error in `anyhow` macro Signed-off-by: Bugen Zhao --- lints/Cargo.lock | 1 + lints/Cargo.toml | 1 + lints/src/format_error.rs | 54 ++++++++++++++++++++++++++--------- lints/ui/format_error.rs | 7 +++++ lints/ui/format_error.stderr | 55 +++++++++++++++++++++++++++--------- 5 files changed, 92 insertions(+), 26 deletions(-) diff --git a/lints/Cargo.lock b/lints/Cargo.lock index 5cd984d3cac47..5251d536cd1aa 100644 --- a/lints/Cargo.lock +++ b/lints/Cargo.lock @@ -588,6 +588,7 @@ dependencies = [ name = "lints" version = "0.1.0" dependencies = [ + "anyhow", "clippy_utils", "dylint_linting", "dylint_testing", diff --git a/lints/Cargo.toml b/lints/Cargo.toml index 7974f6970bd90..14019f167147b 100644 --- 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 index b090388d72fca..0d1df649460e8 100644 --- 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 `` 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 = %.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 = %.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 index 1cdc112bd93aa..eeead1306ea3f 100644 --- 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 index 5ee0c1bdd9ec1..8ec6e69b7fcf4 100644 --- a/lints/ui/format_error.stderr +++ b/lints/ui/format_error.stderr @@ -118,8 +118,7 @@ error: should not format error directly LL | info!("{}", err); | ^^^ | - = help: consider importing `thiserror_ext::AsReport` and recording the error as a field - with `error = %.as_report()` instead + = help: consider importing `thiserror_ext::AsReport` and recording the error as a field with `error = %.as_report()` instead error: should not format error directly --> $DIR/format_error.rs:32:20 @@ -127,8 +126,7 @@ error: should not format error directly LL | my_info!("{}", err); | ^^^ | - = help: consider importing `thiserror_ext::AsReport` and recording the error as a field - with `error = %.as_report()` instead + = help: consider importing `thiserror_ext::AsReport` and recording the error as a field with `error = %.as_report()` instead error: should not format error directly --> $DIR/format_error.rs:34:29 @@ -136,7 +134,7 @@ error: should not format error directly 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 = %.as_report()` instead error: should not format error directly --> $DIR/format_error.rs:35:27 @@ -144,7 +142,7 @@ error: should not format error directly 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 = %.as_report()` instead error: should not format error directly --> $DIR/format_error.rs:37:5 @@ -152,7 +150,7 @@ error: should not format error directly 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 = %.as_report()` instead error: should not format error directly --> $DIR/format_error.rs:38:5 @@ -160,7 +158,7 @@ error: should not format error directly 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 = %.as_report()` instead error: should not format error directly --> $DIR/format_error.rs:39:23 @@ -168,8 +166,7 @@ error: should not format error directly LL | info!(%err, "{}", err); | ^^^ | - = help: consider importing `thiserror_ext::AsReport` and recording the error as a field - with `error = %.as_report()` instead + = help: consider importing `thiserror_ext::AsReport` and recording the error as a field with `error = %.as_report()` instead error: should not format error directly --> $DIR/format_error.rs:39:5 @@ -177,7 +174,7 @@ error: should not format error directly 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 = %.as_report()` instead error: should not format error directly --> $DIR/format_error.rs:40:13 @@ -185,7 +182,7 @@ error: should not format error directly 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 = %.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