Auto merge of #6871 - camsteffen:redundant-closure-macro, r=Manishearth

Fix redundant closure with macros

changelog: Fix redundant_closure FPs with macros

Fixes #6732
Fixes #6850
Fixes #4354 (addresses the error message confusion)

4 files changed, 98 insertions, 40 deletions

diff --git a/clippy_lints/src/eta_reduction.rs b/clippy_lints/src/eta_reduction.rs
index 1a722d39f73..c461732fd36 100644
--- a/clippy_lints/src/eta_reduction.rs
+++ b/clippy_lints/src/eta_reduction.rs
@@ -10,6 +10,8 @@ use crate::utils::{
     implements_trait, is_adjusted, iter_input_pats, snippet_opt, span_lint_and_sugg, span_lint_and_then,
     type_is_unsafe_function,
 };
+use clippy_utils::higher;
+use clippy_utils::higher::VecArgs;
 
 declare_clippy_lint! {
     /// **What it does:** Checks for closures which just call another function where
@@ -74,7 +76,10 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
         match expr.kind {
             ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) => {
                 for arg in args {
-                    check_closure(cx, arg)
+                    // skip `foo(macro!())`
+                    if arg.span.ctxt() == expr.span.ctxt() {
+                        check_closure(cx, arg)
+                    }
                 }
             },
             _ => (),
@@ -87,6 +92,23 @@ fn check_closure(cx: &LateContext<'_>, expr: &Expr<'_>) {
         let body = cx.tcx.hir().body(eid);
         let ex = &body.value;
 
+        if ex.span.ctxt() != expr.span.ctxt() {
+            if let Some(VecArgs::Vec(&[])) = higher::vec_macro(cx, ex) {
+                // replace `|| vec![]` with `Vec::new`
+                span_lint_and_sugg(
+                    cx,
+                    REDUNDANT_CLOSURE,
+                    expr.span,
+                    "redundant closure",
+                    "replace the closure with `Vec::new`",
+                    "std::vec::Vec::new".into(),
+                    Applicability::MachineApplicable,
+                );
+            }
+            // skip `foo(|| macro!())`
+            return;
+        }
+
         if_chain!(
             if let ExprKind::Call(ref caller, ref args) = ex.kind;
 
@@ -107,11 +129,11 @@ fn check_closure(cx: &LateContext<'_>, expr: &Expr<'_>) {
             if compare_inputs(&mut iter_input_pats(decl, body), &mut args.iter());
 
             then {
-                span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", |diag| {
+                span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
                     if let Some(snippet) = snippet_opt(cx, caller.span) {
                         diag.span_suggestion(
                             expr.span,
-                            "remove closure as shown",
+                            "replace the closure with the function itself",
                             snippet,
                             Applicability::MachineApplicable,
                         );
@@ -141,8 +163,8 @@ fn check_closure(cx: &LateContext<'_>, expr: &Expr<'_>) {
                     cx,
                     REDUNDANT_CLOSURE_FOR_METHOD_CALLS,
                     expr.span,
-                    "redundant closure found",
-                    "remove closure as shown",
+                    "redundant closure",
+                    "replace the closure with the method itself",
                     format!("{}::{}", name, path.ident.name),
                     Applicability::MachineApplicable,
                 );
diff --git a/tests/ui/eta.fixed b/tests/ui/eta.fixed
index 1b34c2f74eb..2be2283e3fd 100644
--- a/tests/ui/eta.fixed
+++ b/tests/ui/eta.fixed
@@ -16,10 +16,25 @@
 
 use std::path::PathBuf;
 
+macro_rules! mac {
+    () => {
+        foobar()
+    };
+}
+
+macro_rules! closure_mac {
+    () => {
+        |n| foo(n)
+    };
+}
+
 fn main() {
     let a = Some(1u8).map(foo);
     meta(foo);
     let c = Some(1u8).map(|a| {1+2; foo}(a));
+    true.then(|| mac!()); // don't lint function in macro expansion
+    Some(1).map(closure_mac!()); // don't lint closure in macro expansion
+    let _: Option<Vec<u8>> = true.then(std::vec::Vec::new); // special case vec!
     let d = Some(1u8).map(|a| foo((|b| foo2(b))(a))); //is adjusted?
     all(&[1, 2, 3], &2, |x, y| below(x, y)); //is adjusted
     unsafe {
diff --git a/tests/ui/eta.rs b/tests/ui/eta.rs
index 4f050bd8479..f0373f9ccf6 100644
--- a/tests/ui/eta.rs
+++ b/tests/ui/eta.rs
@@ -16,10 +16,25 @@
 
 use std::path::PathBuf;
 
+macro_rules! mac {
+    () => {
+        foobar()
+    };
+}
+
+macro_rules! closure_mac {
+    () => {
+        |n| foo(n)
+    };
+}
+
 fn main() {
     let a = Some(1u8).map(|a| foo(a));
     meta(|a| foo(a));
     let c = Some(1u8).map(|a| {1+2; foo}(a));
+    true.then(|| mac!()); // don't lint function in macro expansion
+    Some(1).map(closure_mac!()); // don't lint closure in macro expansion
+    let _: Option<Vec<u8>> = true.then(|| vec![]); // special case vec!
     let d = Some(1u8).map(|a| foo((|b| foo2(b))(a))); //is adjusted?
     all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted
     unsafe {
diff --git a/tests/ui/eta.stderr b/tests/ui/eta.stderr
index 16aa1b07733..57ed6527966 100644
--- a/tests/ui/eta.stderr
+++ b/tests/ui/eta.stderr
@@ -1,80 +1,86 @@
-error: redundant closure found
-  --> $DIR/eta.rs:20:27
+error: redundant closure
+  --> $DIR/eta.rs:32:27
    |
 LL |     let a = Some(1u8).map(|a| foo(a));
-   |                           ^^^^^^^^^^ help: remove closure as shown: `foo`
+   |                           ^^^^^^^^^^ help: replace the closure with the function itself: `foo`
    |
    = note: `-D clippy::redundant-closure` implied by `-D warnings`
 
-error: redundant closure found
-  --> $DIR/eta.rs:21:10
+error: redundant closure
+  --> $DIR/eta.rs:33:10
    |
 LL |     meta(|a| foo(a));
-   |          ^^^^^^^^^^ help: remove closure as shown: `foo`
+   |          ^^^^^^^^^^ help: replace the closure with the function itself: `foo`
+
+error: redundant closure
+  --> $DIR/eta.rs:37:40
+   |
+LL |     let _: Option<Vec<u8>> = true.then(|| vec![]); // special case vec!
+   |                                        ^^^^^^^^^ help: replace the closure with `Vec::new`: `std::vec::Vec::new`
 
 error: this expression borrows a reference (`&u8`) that is immediately dereferenced by the compiler
-  --> $DIR/eta.rs:24:21
+  --> $DIR/eta.rs:39:21
    |
 LL |     all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted
    |                     ^^^ help: change this to: `&2`
    |
    = note: `-D clippy::needless-borrow` implied by `-D warnings`
 
-error: redundant closure found
-  --> $DIR/eta.rs:31:27
+error: redundant closure
+  --> $DIR/eta.rs:46:27
    |
 LL |     let e = Some(1u8).map(|a| generic(a));
-   |                           ^^^^^^^^^^^^^^ help: remove closure as shown: `generic`
+   |                           ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `generic`
 
-error: redundant closure found
-  --> $DIR/eta.rs:74:51
+error: redundant closure
+  --> $DIR/eta.rs:89:51
    |
 LL |     let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo());
-   |                                                   ^^^^^^^^^^^ help: remove closure as shown: `TestStruct::foo`
+   |                                                   ^^^^^^^^^^^ help: replace the closure with the method itself: `TestStruct::foo`
    |
    = note: `-D clippy::redundant-closure-for-method-calls` implied by `-D warnings`
 
-error: redundant closure found
-  --> $DIR/eta.rs:76:51
+error: redundant closure
+  --> $DIR/eta.rs:91:51
    |
 LL |     let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo());
-   |                                                   ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `TestTrait::trait_foo`
+   |                                                   ^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `TestTrait::trait_foo`
 
-error: redundant closure found
-  --> $DIR/eta.rs:79:42
+error: redundant closure
+  --> $DIR/eta.rs:94:42
    |
 LL |     let e = Some(&mut vec![1, 2, 3]).map(|v| v.clear());
-   |                                          ^^^^^^^^^^^^^ help: remove closure as shown: `std::vec::Vec::clear`
+   |                                          ^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::vec::Vec::clear`
 
-error: redundant closure found
-  --> $DIR/eta.rs:84:29
+error: redundant closure
+  --> $DIR/eta.rs:99:29
    |
 LL |     let e = Some("str").map(|s| s.to_string());
-   |                             ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `std::string::ToString::to_string`
+   |                             ^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::string::ToString::to_string`
 
-error: redundant closure found
-  --> $DIR/eta.rs:86:27
+error: redundant closure
+  --> $DIR/eta.rs:101:27
    |
 LL |     let e = Some('a').map(|s| s.to_uppercase());
-   |                           ^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `char::to_uppercase`
+   |                           ^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `char::to_uppercase`
 
-error: redundant closure found
-  --> $DIR/eta.rs:89:65
+error: redundant closure
+  --> $DIR/eta.rs:104:65
    |
 LL |     let e: std::vec::Vec<char> = vec!['a', 'b', 'c'].iter().map(|c| c.to_ascii_uppercase()).collect();
-   |                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `char::to_ascii_uppercase`
+   |                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `char::to_ascii_uppercase`
 
-error: redundant closure found
-  --> $DIR/eta.rs:172:27
+error: redundant closure
+  --> $DIR/eta.rs:187:27
    |
 LL |     let a = Some(1u8).map(|a| foo_ptr(a));
-   |                           ^^^^^^^^^^^^^^ help: remove closure as shown: `foo_ptr`
+   |                           ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `foo_ptr`
 
-error: redundant closure found
-  --> $DIR/eta.rs:177:27
+error: redundant closure
+  --> $DIR/eta.rs:192:27
    |
 LL |     let a = Some(1u8).map(|a| closure(a));
-   |                           ^^^^^^^^^^^^^^ help: remove closure as shown: `closure`
+   |                           ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `closure`
 
-error: aborting due to 12 previous errors
+error: aborting due to 13 previous errors