Skip to content

Fix redundant closure with macros #6871

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 8, 2021
Merged

Fix redundant closure with macros #6871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf98aa6
Fix redundant closure with macros
camsteffen Mar 8, 2021
8c540dc
Improve the redundant_closure message
camsteffen Mar 8, 2021
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
32 changes: 27 additions & 5 deletions clippy_lints/src/eta_reduction.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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