From ebf39a54786ffb10a8ff5aec5504e4fbd29db19a Mon Sep 17 00:00:00 2001 From: yanglsh Date: Sat, 24 May 2025 21:10:46 +0800 Subject: [PATCH] fix: `unit_arg` wrongly handled macros --- clippy_lints/src/unit_types/unit_arg.rs | 54 ++++++++++++++++++------- clippy_utils/src/lib.rs | 3 +- tests/ui/unit_arg.rs | 24 +++++++++++ tests/ui/unit_arg.stderr | 23 ++++++++++- tests/ui/unit_arg_fixable.fixed | 30 ++++++++++++++ tests/ui/unit_arg_fixable.rs | 27 +++++++++++++ tests/ui/unit_arg_fixable.stderr | 46 ++++++++++++++++++++- 7 files changed, 188 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/unit_types/unit_arg.rs b/clippy_lints/src/unit_types/unit_arg.rs index e09b362800c0..74b3331414c3 100644 --- a/clippy_lints/src/unit_types/unit_arg.rs +++ b/clippy_lints/src/unit_types/unit_arg.rs @@ -1,9 +1,11 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::source::{SourceText, SpanRangeExt, indent_of, reindent_multiline}; +use clippy_utils::source::{SpanRangeExt, indent_of, reindent_multiline}; +use clippy_utils::sugg::Sugg; use clippy_utils::{is_expr_default, is_from_proc_macro}; use rustc_errors::Applicability; use rustc_hir::{Block, Expr, ExprKind, MatchSource, Node, StmtKind}; use rustc_lint::LateContext; +use rustc_span::SyntaxContext; use super::{UNIT_ARG, utils}; @@ -100,14 +102,16 @@ fn lint_unit_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, args_to_ let arg_snippets: Vec<_> = args_to_recover .iter() - .filter_map(|arg| arg.span.get_source_text(cx)) + // If the argument is from an expansion and is a `Default::default()`, we skip it + .filter(|arg| !arg.span.from_expansion() || !is_expr_default_nested(cx, arg)) + .filter_map(|arg| get_expr_snippet(cx, arg)) .collect(); // If the argument is an empty block or `Default::default()`, we can replace it with `()`. let arg_snippets_without_redundant_exprs: Vec<_> = args_to_recover .iter() - .filter(|arg| !is_empty_block(arg) && !is_expr_default(cx, arg)) - .filter_map(|arg| arg.span.get_source_text(cx)) + .filter(|arg| !is_expr_default_nested(cx, arg) && (arg.span.from_expansion() || !is_empty_block(arg))) + .filter_map(|arg| get_expr_snippet(cx, arg)) .collect(); if let Some(call_snippet) = expr.span.get_source_text(cx) { @@ -119,13 +123,17 @@ fn lint_unit_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, args_to_ &arg_snippets_without_redundant_exprs, ); - if arg_snippets_without_redundant_exprs.is_empty() { + if arg_snippets_without_redundant_exprs.is_empty() + && let suggestions = args_to_recover + .iter() + .filter(|arg| !arg.span.from_expansion() || !is_expr_default_nested(cx, arg)) + .map(|arg| (arg.span.parent_callsite().unwrap_or(arg.span), "()".to_string())) + .collect::>() + && !suggestions.is_empty() + { db.multipart_suggestion( format!("use {singular}unit literal{plural} instead"), - args_to_recover - .iter() - .map(|arg| (arg.span, "()".to_string())) - .collect::>(), + suggestions, applicability, ); } else { @@ -146,6 +154,22 @@ fn lint_unit_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, args_to_ ); } +fn is_expr_default_nested<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + is_expr_default(cx, expr) + || matches!(expr.kind, ExprKind::Block(block, _) + if block.expr.is_some() && is_expr_default_nested(cx, block.expr.unwrap())) +} + +fn get_expr_snippet<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option> { + let mut app = Applicability::MachineApplicable; + let snip = Sugg::hir_with_context(cx, expr, SyntaxContext::root(), "..", &mut app); + if app != Applicability::MachineApplicable { + return None; + } + + Some(snip) +} + fn is_empty_block(expr: &Expr<'_>) -> bool { matches!( expr.kind, @@ -164,17 +188,17 @@ fn fmt_stmts_and_call( cx: &LateContext<'_>, call_expr: &Expr<'_>, call_snippet: &str, - args_snippets: &[SourceText], - non_empty_block_args_snippets: &[SourceText], + args_snippets: &[Sugg<'_>], + non_empty_block_args_snippets: &[Sugg<'_>], ) -> String { let call_expr_indent = indent_of(cx, call_expr.span).unwrap_or(0); - let call_snippet_with_replacements = args_snippets - .iter() - .fold(call_snippet.to_owned(), |acc, arg| acc.replacen(arg.as_ref(), "()", 1)); + let call_snippet_with_replacements = args_snippets.iter().fold(call_snippet.to_owned(), |acc, arg| { + acc.replacen(&arg.to_string(), "()", 1) + }); let mut stmts_and_call = non_empty_block_args_snippets .iter() - .map(|it| it.as_ref().to_owned()) + .map(ToString::to_string) .collect::>(); stmts_and_call.push(call_snippet_with_replacements); stmts_and_call = stmts_and_call diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index f3cd22de5229..4e588adf43eb 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -3472,13 +3472,12 @@ pub fn desugar_await<'tcx>(expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { } } -/// Checks if the given expression is the `default` method belonging to the `Default` trait. +/// Checks if the given expression is a call to `Default::default()`. pub fn is_expr_default<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { if let ExprKind::Call(fn_expr, []) = &expr.kind && let ExprKind::Path(qpath) = &fn_expr.kind && let Res::Def(_, def_id) = cx.qpath_res(qpath, fn_expr.hir_id) { - // right hand side of assignment is `Default::default` cx.tcx.is_diagnostic_item(sym::default_fn, def_id) } else { false diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs index 22a6a26dab62..4208efad6774 100644 --- a/tests/ui/unit_arg.rs +++ b/tests/ui/unit_arg.rs @@ -151,3 +151,27 @@ fn main() { bad(); ok(); } + +fn issue14857() { + let fn_take_unit = |_: ()| {}; + fn some_other_fn(_: &i32) {} + + macro_rules! mac { + (def) => { + Default::default() + }; + (func $f:expr) => { + $f() + }; + (nonempty_block $e:expr) => {{ + some_other_fn(&$e); + $e + }}; + } + fn_take_unit(mac!(def)); + //~^ unit_arg + fn_take_unit(mac!(func Default::default)); + //~^ unit_arg + fn_take_unit(mac!(nonempty_block Default::default())); + //~^ unit_arg +} diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr index 6c333d9792d4..0dcfb02b9fa0 100644 --- a/tests/ui/unit_arg.stderr +++ b/tests/ui/unit_arg.stderr @@ -199,5 +199,26 @@ LL ~ foo(1); LL + Some(()) | -error: aborting due to 10 previous errors +error: passing a unit value to a function + --> tests/ui/unit_arg.rs:171:5 + | +LL | fn_take_unit(mac!(def)); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + +error: passing a unit value to a function + --> tests/ui/unit_arg.rs:173:5 + | +LL | fn_take_unit(mac!(func Default::default)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + +error: passing a unit value to a function + --> tests/ui/unit_arg.rs:175:5 + | +LL | fn_take_unit(mac!(nonempty_block Default::default())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + +error: aborting due to 13 previous errors diff --git a/tests/ui/unit_arg_fixable.fixed b/tests/ui/unit_arg_fixable.fixed index 7a25b93e5aee..3ef6a953c729 100644 --- a/tests/ui/unit_arg_fixable.fixed +++ b/tests/ui/unit_arg_fixable.fixed @@ -37,4 +37,34 @@ fn issue14857() { let fn_take_unit = |_: ()| {}; fn_take_unit(()); //~^ unit_arg + + fn some_other_fn(_: &i32) {} + + macro_rules! another_mac { + () => { + some_other_fn(&Default::default()) + }; + ($e:expr) => { + some_other_fn(&$e) + }; + } + + another_mac!(); + fn_take_unit(()); + //~^ unit_arg + another_mac!(1); + fn_take_unit(()); + //~^ unit_arg + + macro_rules! mac { + (nondef $e:expr) => { + $e + }; + (empty_block) => {{}}; + } + fn_take_unit(mac!(nondef ())); + //~^ unit_arg + mac!(empty_block); + fn_take_unit(()); + //~^ unit_arg } diff --git a/tests/ui/unit_arg_fixable.rs b/tests/ui/unit_arg_fixable.rs index 65152abbe7b8..097d51e9481c 100644 --- a/tests/ui/unit_arg_fixable.rs +++ b/tests/ui/unit_arg_fixable.rs @@ -34,4 +34,31 @@ fn issue14857() { let fn_take_unit = |_: ()| {}; fn_take_unit(Default::default()); //~^ unit_arg + + fn some_other_fn(_: &i32) {} + + macro_rules! another_mac { + () => { + some_other_fn(&Default::default()) + }; + ($e:expr) => { + some_other_fn(&$e) + }; + } + + fn_take_unit(another_mac!()); + //~^ unit_arg + fn_take_unit(another_mac!(1)); + //~^ unit_arg + + macro_rules! mac { + (nondef $e:expr) => { + $e + }; + (empty_block) => {{}}; + } + fn_take_unit(mac!(nondef Default::default())); + //~^ unit_arg + fn_take_unit(mac!(empty_block)); + //~^ unit_arg } diff --git a/tests/ui/unit_arg_fixable.stderr b/tests/ui/unit_arg_fixable.stderr index 79afe67e5724..94063f02a3f1 100644 --- a/tests/ui/unit_arg_fixable.stderr +++ b/tests/ui/unit_arg_fixable.stderr @@ -50,5 +50,49 @@ LL | fn_take_unit(Default::default()); | | | help: use a unit literal instead: `()` -error: aborting due to 5 previous errors +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:49:5 + | +LL | fn_take_unit(another_mac!()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL ~ another_mac!(); +LL ~ fn_take_unit(()); + | + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:51:5 + | +LL | fn_take_unit(another_mac!(1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL ~ another_mac!(1); +LL ~ fn_take_unit(()); + | + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:60:5 + | +LL | fn_take_unit(mac!(nondef Default::default())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^------------------^^ + | | + | help: use a unit literal instead: `()` + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:62:5 + | +LL | fn_take_unit(mac!(empty_block)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL ~ mac!(empty_block); +LL ~ fn_take_unit(()); + | + +error: aborting due to 9 previous errors