Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(lints): improve help message when formatting error in anyhow macro #14957

Merged
merged 1 commit into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lints/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions lints/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ itertools = "0.12"
dylint_testing = "2.6.0"

# UI test dependencies
anyhow = "1"
tracing = "0.1"

[package.metadata.rust-analyzer]
Expand Down
54 changes: 41 additions & 13 deletions lints/src/format_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<'_>) {
Expand All @@ -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)
Expand All @@ -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);
}
Expand All @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions lints/ui/format_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
55 changes: 42 additions & 13 deletions lints/ui/format_error.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Loading