fix: `unit_arg` wrongly handled macros

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 e09b362800c..74b3331414c 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::<Vec<_>>()
+                    && !suggestions.is_empty()
+                {
                     db.multipart_suggestion(
                         format!("use {singular}unit literal{plural} instead"),
-                        args_to_recover
-                            .iter()
-                            .map(|arg| (arg.span, "()".to_string()))
-                            .collect::<Vec<_>>(),
+                        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<Sugg<'tcx>> {
+    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::<Vec<_>>();
     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 f3cd22de522..4e588adf43e 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 22a6a26dab6..4208efad677 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 6c333d9792d..0dcfb02b9fa 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 7a25b93e5ae..3ef6a953c72 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 65152abbe7b..097d51e9481 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 79afe67e572..94063f02a3f 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