Skip to content

Commit 2cb5bbf

Browse files
borsbors
committed
Auto merge of rust-lang#6871 - camsteffen:redundant-closure-macro, r=Manishearth
Fix redundant closure with macros changelog: Fix redundant_closure FPs with macros Fixes rust-lang#6732 Fixes rust-lang#6850 Fixes rust-lang#4354 (addresses the error message confusion)
2 parents b207f23 + 8c540dc commit 2cb5bbf

File tree

4 files changed

+98
-40
lines changed

4 files changed

+98
-40
lines changed

Diff for: ‎clippy_lints/src/eta_reduction.rs

+27-5
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ use crate::utils::{
1010
implements_trait, is_adjusted, iter_input_pats, snippet_opt, span_lint_and_sugg, span_lint_and_then,
1111
type_is_unsafe_function,
1212
};
13+
use clippy_utils::higher;
14+
use clippy_utils::higher::VecArgs;
1315

1416
declare_clippy_lint! {
1517
/// **What it does:** Checks for closures which just call another function where
@@ -74,7 +76,10 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
7476
match expr.kind {
7577
ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) => {
7678
for arg in args {
77-
check_closure(cx, arg)
79+
// skip `foo(macro!())`
80+
if arg.span.ctxt() == expr.span.ctxt() {
81+
check_closure(cx, arg)
82+
}
7883
}
7984
},
8085
_ => (),
@@ -87,6 +92,23 @@ fn check_closure(cx: &LateContext<'_>, expr: &Expr<'_>) {
8792
let body = cx.tcx.hir().body(eid);
8893
let ex = &body.value;
8994

95+
if ex.span.ctxt() != expr.span.ctxt() {
96+
if let Some(VecArgs::Vec(&[])) = higher::vec_macro(cx, ex) {
97+
// replace `|| vec![]` with `Vec::new`
98+
span_lint_and_sugg(
99+
cx,
100+
REDUNDANT_CLOSURE,
101+
expr.span,
102+
"redundant closure",
103+
"replace the closure with `Vec::new`",
104+
"std::vec::Vec::new".into(),
105+
Applicability::MachineApplicable,
106+
);
107+
}
108+
// skip `foo(|| macro!())`
109+
return;
110+
}
111+
90112
if_chain!(
91113
if let ExprKind::Call(ref caller, ref args) = ex.kind;
92114

@@ -107,11 +129,11 @@ fn check_closure(cx: &LateContext<'_>, expr: &Expr<'_>) {
107129
if compare_inputs(&mut iter_input_pats(decl, body), &mut args.iter());
108130

109131
then {
110-
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", |diag| {
132+
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
111133
if let Some(snippet) = snippet_opt(cx, caller.span) {
112134
diag.span_suggestion(
113135
expr.span,
114-
"remove closure as shown",
136+
"replace the closure with the function itself",
115137
snippet,
116138
Applicability::MachineApplicable,
117139
);
@@ -141,8 +163,8 @@ fn check_closure(cx: &LateContext<'_>, expr: &Expr<'_>) {
141163
cx,
142164
REDUNDANT_CLOSURE_FOR_METHOD_CALLS,
143165
expr.span,
144-
"redundant closure found",
145-
"remove closure as shown",
166+
"redundant closure",
167+
"replace the closure with the method itself",
146168
format!("{}::{}", name, path.ident.name),
147169
Applicability::MachineApplicable,
148170
);

Diff for: ‎tests/ui/eta.fixed

+15
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,25 @@
1616

1717
use std::path::PathBuf;
1818

19+
macro_rules! mac {
20+
() => {
21+
foobar()
22+
};
23+
}
24+
25+
macro_rules! closure_mac {
26+
() => {
27+
|n| foo(n)
28+
};
29+
}
30+
1931
fn main() {
2032
let a = Some(1u8).map(foo);
2133
meta(foo);
2234
let c = Some(1u8).map(|a| {1+2; foo}(a));
35+
true.then(|| mac!()); // don't lint function in macro expansion
36+
Some(1).map(closure_mac!()); // don't lint closure in macro expansion
37+
let _: Option<Vec<u8>> = true.then(std::vec::Vec::new); // special case vec!
2338
let d = Some(1u8).map(|a| foo((|b| foo2(b))(a))); //is adjusted?
2439
all(&[1, 2, 3], &2, |x, y| below(x, y)); //is adjusted
2540
unsafe {

Diff for: ‎tests/ui/eta.rs

+15
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,25 @@
1616

1717
use std::path::PathBuf;
1818

19+
macro_rules! mac {
20+
() => {
21+
foobar()
22+
};
23+
}
24+
25+
macro_rules! closure_mac {
26+
() => {
27+
|n| foo(n)
28+
};
29+
}
30+
1931
fn main() {
2032
let a = Some(1u8).map(|a| foo(a));
2133
meta(|a| foo(a));
2234
let c = Some(1u8).map(|a| {1+2; foo}(a));
35+
true.then(|| mac!()); // don't lint function in macro expansion
36+
Some(1).map(closure_mac!()); // don't lint closure in macro expansion
37+
let _: Option<Vec<u8>> = true.then(|| vec![]); // special case vec!
2338
let d = Some(1u8).map(|a| foo((|b| foo2(b))(a))); //is adjusted?
2439
all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted
2540
unsafe {

Diff for: ‎tests/ui/eta.stderr

+41-35
Original file line numberDiff line numberDiff line change
@@ -1,80 +1,86 @@
1-
error: redundant closure found
2-
--> $DIR/eta.rs:20:27
1+
error: redundant closure
2+
--> $DIR/eta.rs:32:27
33
|
44
LL | let a = Some(1u8).map(|a| foo(a));
5-
| ^^^^^^^^^^ help: remove closure as shown: `foo`
5+
| ^^^^^^^^^^ help: replace the closure with the function itself: `foo`
66
|
77
= note: `-D clippy::redundant-closure` implied by `-D warnings`
88

9-
error: redundant closure found
10-
--> $DIR/eta.rs:21:10
9+
error: redundant closure
10+
--> $DIR/eta.rs:33:10
1111
|
1212
LL | meta(|a| foo(a));
13-
| ^^^^^^^^^^ help: remove closure as shown: `foo`
13+
| ^^^^^^^^^^ help: replace the closure with the function itself: `foo`
14+
15+
error: redundant closure
16+
--> $DIR/eta.rs:37:40
17+
|
18+
LL | let _: Option<Vec<u8>> = true.then(|| vec![]); // special case vec!
19+
| ^^^^^^^^^ help: replace the closure with `Vec::new`: `std::vec::Vec::new`
1420

1521
error: this expression borrows a reference (`&u8`) that is immediately dereferenced by the compiler
16-
--> $DIR/eta.rs:24:21
22+
--> $DIR/eta.rs:39:21
1723
|
1824
LL | all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted
1925
| ^^^ help: change this to: `&2`
2026
|
2127
= note: `-D clippy::needless-borrow` implied by `-D warnings`
2228

23-
error: redundant closure found
24-
--> $DIR/eta.rs:31:27
29+
error: redundant closure
30+
--> $DIR/eta.rs:46:27
2531
|
2632
LL | let e = Some(1u8).map(|a| generic(a));
27-
| ^^^^^^^^^^^^^^ help: remove closure as shown: `generic`
33+
| ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `generic`
2834

29-
error: redundant closure found
30-
--> $DIR/eta.rs:74:51
35+
error: redundant closure
36+
--> $DIR/eta.rs:89:51
3137
|
3238
LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo());
33-
| ^^^^^^^^^^^ help: remove closure as shown: `TestStruct::foo`
39+
| ^^^^^^^^^^^ help: replace the closure with the method itself: `TestStruct::foo`
3440
|
3541
= note: `-D clippy::redundant-closure-for-method-calls` implied by `-D warnings`
3642

37-
error: redundant closure found
38-
--> $DIR/eta.rs:76:51
43+
error: redundant closure
44+
--> $DIR/eta.rs:91:51
3945
|
4046
LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo());
41-
| ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `TestTrait::trait_foo`
47+
| ^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `TestTrait::trait_foo`
4248

43-
error: redundant closure found
44-
--> $DIR/eta.rs:79:42
49+
error: redundant closure
50+
--> $DIR/eta.rs:94:42
4551
|
4652
LL | let e = Some(&mut vec![1, 2, 3]).map(|v| v.clear());
47-
| ^^^^^^^^^^^^^ help: remove closure as shown: `std::vec::Vec::clear`
53+
| ^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::vec::Vec::clear`
4854

49-
error: redundant closure found
50-
--> $DIR/eta.rs:84:29
55+
error: redundant closure
56+
--> $DIR/eta.rs:99:29
5157
|
5258
LL | let e = Some("str").map(|s| s.to_string());
53-
| ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `std::string::ToString::to_string`
59+
| ^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::string::ToString::to_string`
5460

55-
error: redundant closure found
56-
--> $DIR/eta.rs:86:27
61+
error: redundant closure
62+
--> $DIR/eta.rs:101:27
5763
|
5864
LL | let e = Some('a').map(|s| s.to_uppercase());
59-
| ^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `char::to_uppercase`
65+
| ^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `char::to_uppercase`
6066

61-
error: redundant closure found
62-
--> $DIR/eta.rs:89:65
67+
error: redundant closure
68+
--> $DIR/eta.rs:104:65
6369
|
6470
LL | let e: std::vec::Vec<char> = vec!['a', 'b', 'c'].iter().map(|c| c.to_ascii_uppercase()).collect();
65-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `char::to_ascii_uppercase`
71+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `char::to_ascii_uppercase`
6672

67-
error: redundant closure found
68-
--> $DIR/eta.rs:172:27
73+
error: redundant closure
74+
--> $DIR/eta.rs:187:27
6975
|
7076
LL | let a = Some(1u8).map(|a| foo_ptr(a));
71-
| ^^^^^^^^^^^^^^ help: remove closure as shown: `foo_ptr`
77+
| ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `foo_ptr`
7278

73-
error: redundant closure found
74-
--> $DIR/eta.rs:177:27
79+
error: redundant closure
80+
--> $DIR/eta.rs:192:27
7581
|
7682
LL | let a = Some(1u8).map(|a| closure(a));
77-
| ^^^^^^^^^^^^^^ help: remove closure as shown: `closure`
83+
| ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `closure`
7884

79-
error: aborting due to 12 previous errors
85+
error: aborting due to 13 previous errors
8086

0 commit comments

Comments
 (0)