Skip to content

Commit

Permalink
Auto merge of #6871 - camsteffen:redundant-closure-macro, r=Manishearth
Browse files Browse the repository at this point in the history 
Fix redundant closure with macros

changelog: Fix redundant_closure FPs with macros

Fixes #6732
Fixes #6850
Fixes #4354 (addresses the error message confusion)
  • Loading branch information
@bors
bors committed Mar 8, 2021
2 parents b207f23 + 8c540dc commit 2cb5bbf
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 40 deletions.
32 changes: 27 additions & 5 deletions clippy_lints/src/eta_reduction.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}
},
_ => (),
Expand All @@ -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;

Expand All @@ -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,
);
Expand Down Expand Up @@ -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,
);
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/eta.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/eta.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
76 changes: 41 additions & 35 deletions tests/ui/eta.stderr
View file
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 2cb5bbf

Please sign in to comment.