Skip to content

Commit 7280ad9

Browse files
y21y21
committed
[redundant_closure_call]: handle nested closures
1 parent 60258b0 commit 7280ad9

File tree

5 files changed

+243
-51
lines changed

5 files changed

+243
-51
lines changed

‎clippy_lints/src/lib.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -791,7 +791,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
791791
store.register_early_pass(|| Box::new(int_plus_one::IntPlusOne));
792792
store.register_early_pass(|| Box::new(formatting::Formatting));
793793
store.register_early_pass(|| Box::new(misc_early::MiscEarlyLints));
794-
store.register_early_pass(|| Box::new(redundant_closure_call::RedundantClosureCall));
795794
store.register_late_pass(|_| Box::new(redundant_closure_call::RedundantClosureCall));
796795
store.register_early_pass(|| Box::new(unused_unit::UnusedUnit));
797796
store.register_late_pass(|_| Box::new(returns::Return));

‎clippy_lints/src/redundant_closure_call.rs

Lines changed: 120 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
1+
use crate::rustc_lint::LintContext;
12
use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
3+
use clippy_utils::get_parent_expr;
24
use clippy_utils::sugg::Sugg;
35
use if_chain::if_chain;
4-
use rustc_ast::ast;
5-
use rustc_ast::visit as ast_visit;
6-
use rustc_ast::visit::Visitor as AstVisitor;
76
use rustc_errors::Applicability;
87
use rustc_hir as hir;
98
use rustc_hir::intravisit as hir_visit;
109
use rustc_hir::intravisit::Visitor as HirVisitor;
11-
use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};
10+
use rustc_hir::intravisit::Visitor;
11+
use rustc_lint::{LateContext, LateLintPass};
1212
use rustc_middle::hir::nested_filter;
1313
use rustc_middle::lint::in_external_macro;
1414
use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -51,59 +51,136 @@ impl ReturnVisitor {
5151
}
5252
}
5353

54-
impl<'ast> ast_visit::Visitor<'ast> for ReturnVisitor {
55-
fn visit_expr(&mut self, ex: &'ast ast::Expr) {
56-
if let ast::ExprKind::Ret(_) | ast::ExprKind::Try(_) = ex.kind {
54+
impl<'tcx> Visitor<'tcx> for ReturnVisitor {
55+
fn visit_expr(&mut self, ex: &'tcx hir::Expr<'tcx>) {
56+
if let hir::ExprKind::Ret(_) | hir::ExprKind::Match(.., hir::MatchSource::TryDesugar) = ex.kind {
5757
self.found_return = true;
5858
}
5959

60-
ast_visit::walk_expr(self, ex);
60+
hir_visit::walk_expr(self, ex);
6161
}
6262
}
6363

64-
impl EarlyLintPass for RedundantClosureCall {
65-
fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) {
64+
/// Checks if the body is owned by an async closure
65+
fn is_async_closure(body: &hir::Body<'_>) -> bool {
66+
if let hir::ExprKind::Closure(closure) = body.value.kind
67+
&& let [resume_ty] = closure.fn_decl.inputs
68+
&& let hir::TyKind::Path(hir::QPath::LangItem(hir::LangItem::ResumeTy, ..)) = resume_ty.kind
69+
{
70+
true
71+
} else {
72+
false
73+
}
74+
}
75+
76+
/// Tries to find the innermost closure:
77+
/// ```rust
78+
/// (|| || || || 42)()()()()
79+
/// ^^^^^^^^^^^^^^ given this nested closure expression
80+
/// ^^^^^ we want to return this closure
81+
/// ```
82+
/// It also has a parameter for how many steps to go in at most, so as to
83+
/// not take more closures than there are calls.
84+
fn find_innermost_closure<'tcx>(
85+
cx: &LateContext<'tcx>,
86+
mut expr: &'tcx hir::Expr<'tcx>,
87+
mut steps: usize,
88+
) -> Option<(&'tcx hir::Expr<'tcx>, &'tcx hir::FnDecl<'tcx>, hir::IsAsync)> {
89+
let mut data = None;
90+
91+
while let hir::ExprKind::Closure(closure) = expr.kind
92+
&& let body = cx.tcx.hir().body(closure.body)
93+
&& {
94+
let mut visitor = ReturnVisitor::new();
95+
visitor.visit_expr(body.value);
96+
!visitor.found_return
97+
}
98+
&& steps > 0
99+
{
100+
expr = body.value;
101+
data = Some((body.value, closure.fn_decl, if is_async_closure(body) {
102+
hir::IsAsync::Async
103+
} else {
104+
hir::IsAsync::NotAsync
105+
}));
106+
steps -= 1;
107+
}
108+
109+
data
110+
}
111+
112+
/// "Walks up" the chain of calls to find the outermost call expression, and returns the depth:
113+
/// ```rust
114+
/// (|| || || 3)()()()
115+
/// ^^ this is the call expression we were given
116+
/// ^^ this is what we want to return (and the depth is 3)
117+
/// ```
118+
fn get_parent_call_exprs<'tcx>(
119+
cx: &LateContext<'tcx>,
120+
mut expr: &'tcx hir::Expr<'tcx>,
121+
) -> (&'tcx hir::Expr<'tcx>, usize) {
122+
let mut depth = 1;
123+
while let Some(parent) = get_parent_expr(cx, expr)
124+
&& let hir::ExprKind::Call(recv, _) = parent.kind
125+
&& let hir::ExprKind::Call(..) = recv.kind
126+
{
127+
expr = parent;
128+
depth += 1;
129+
}
130+
(expr, depth)
131+
}
132+
133+
impl<'tcx> LateLintPass<'tcx> for RedundantClosureCall {
134+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
66135
if in_external_macro(cx.sess(), expr.span) {
67136
return;
68137
}
69-
if_chain! {
70-
if let ast::ExprKind::Call(ref paren, _) = expr.kind;
71-
if let ast::ExprKind::Paren(ref closure) = paren.kind;
72-
if let ast::ExprKind::Closure(box ast::Closure { ref asyncness, ref fn_decl, ref body, .. }) = closure.kind;
73-
then {
74-
let mut visitor = ReturnVisitor::new();
75-
visitor.visit_expr(body);
76-
if !visitor.found_return {
77-
span_lint_and_then(
78-
cx,
79-
REDUNDANT_CLOSURE_CALL,
80-
expr.span,
81-
"try not to call a closure in the expression where it is declared",
82-
|diag| {
83-
if fn_decl.inputs.is_empty() {
84-
let mut app = Applicability::MachineApplicable;
85-
let mut hint = Sugg::ast(cx, body, "..", closure.span.ctxt(), &mut app);
86-
87-
if asyncness.is_async() {
88-
// `async x` is a syntax error, so it becomes `async { x }`
89-
if !matches!(body.kind, ast::ExprKind::Block(_, _)) {
90-
hint = hint.blockify();
91-
}
92-
93-
hint = hint.asyncify();
94-
}
95-
96-
diag.span_suggestion(expr.span, "try doing something like", hint.to_string(), app);
138+
139+
if let hir::ExprKind::Call(recv, _) = expr.kind
140+
// don't lint if the receiver is a call, too.
141+
// we do this in order to prevent linting multiple times; consider:
142+
// `(|| || 1)()()`
143+
// ^^ we only want to lint for this call (but we walk up the calls to consider both calls).
144+
// without this check, we'd end up linting twice.
145+
&& !matches!(recv.kind, hir::ExprKind::Call(..))
146+
&& let (full_expr, call_depth) = get_parent_call_exprs(cx, expr)
147+
&& let Some((body, fn_decl, generator_kind)) = find_innermost_closure(cx, recv, call_depth)
148+
{
149+
span_lint_and_then(
150+
cx,
151+
REDUNDANT_CLOSURE_CALL,
152+
full_expr.span,
153+
"try not to call a closure in the expression where it is declared",
154+
|diag| {
155+
if fn_decl.inputs.is_empty() {
156+
let mut applicability = Applicability::MachineApplicable;
157+
let mut hint = Sugg::hir_with_context(cx, body, full_expr.span.ctxt(), "..", &mut applicability);
158+
159+
if generator_kind.is_async()
160+
&& let hir::ExprKind::Closure(closure) = body.kind
161+
{
162+
let async_closure_body = cx.tcx.hir().body(closure.body);
163+
164+
// `async x` is a syntax error, so it becomes `async { x }`
165+
if !matches!(async_closure_body.value.kind, hir::ExprKind::Block(_, _)) {
166+
hint = hint.blockify();
97167
}
98-
},
99-
);
168+
169+
hint = hint.asyncify();
170+
}
171+
172+
diag.span_suggestion(
173+
full_expr.span,
174+
"try doing something like",
175+
hint.maybe_par(),
176+
applicability
177+
);
178+
}
100179
}
101-
}
180+
);
102181
}
103182
}
104-
}
105183

106-
impl<'tcx> LateLintPass<'tcx> for RedundantClosureCall {
107184
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
108185
fn count_closure_usage<'tcx>(
109186
cx: &LateContext<'tcx>,

‎tests/ui/redundant_closure_call_fixable.fixed

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#![feature(async_closure)]
44
#![warn(clippy::redundant_closure_call)]
55
#![allow(clippy::redundant_async_block)]
6+
#![allow(clippy::type_complexity)]
67
#![allow(unused)]
78

89
async fn something() -> u32 {
@@ -38,4 +39,43 @@ fn main() {
3839
};
3940
}
4041
m2!();
42+
issue9956();
43+
}
44+
45+
fn issue9956() {
46+
assert_eq!(43, 42);
47+
48+
// ... and some more interesting cases I've found while implementing the fix
49+
50+
// not actually immediately calling the closure:
51+
let a = (|| 42);
52+
dbg!(a());
53+
54+
// immediately calling it inside of a macro
55+
dbg!(42);
56+
57+
// immediately calling only one closure, so we can't remove the other ones
58+
let a = (|| || 123);
59+
dbg!(a()());
60+
61+
// nested async closures
62+
let a = async { 1 };
63+
let h = async { a.await };
64+
65+
// macro expansion tests
66+
macro_rules! echo {
67+
($e:expr) => {
68+
$e
69+
};
70+
}
71+
let a = 1;
72+
assert_eq!(a, 1);
73+
let a = 123;
74+
assert_eq!(a, 123);
75+
76+
// chaining calls, but not closures
77+
fn x() -> fn() -> fn() -> fn() -> i32 {
78+
|| || || 42
79+
}
80+
let _ = x()()()();
4181
}

‎tests/ui/redundant_closure_call_fixable.rs

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#![feature(async_closure)]
44
#![warn(clippy::redundant_closure_call)]
55
#![allow(clippy::redundant_async_block)]
6+
#![allow(clippy::type_complexity)]
67
#![allow(unused)]
78

89
async fn something() -> u32 {
@@ -38,4 +39,43 @@ fn main() {
3839
};
3940
}
4041
m2!();
42+
issue9956();
43+
}
44+
45+
fn issue9956() {
46+
assert_eq!((|| || 43)()(), 42);
47+
48+
// ... and some more interesting cases I've found while implementing the fix
49+
50+
// not actually immediately calling the closure:
51+
let a = (|| 42);
52+
dbg!(a());
53+
54+
// immediately calling it inside of a macro
55+
dbg!((|| 42)());
56+
57+
// immediately calling only one closure, so we can't remove the other ones
58+
let a = (|| || || 123)();
59+
dbg!(a()());
60+
61+
// nested async closures
62+
let a = (|| || || || async || 1)()()()()();
63+
let h = async { a.await };
64+
65+
// macro expansion tests
66+
macro_rules! echo {
67+
($e:expr) => {
68+
$e
69+
};
70+
}
71+
let a = (|| echo!(|| echo!(|| 1)))()()();
72+
assert_eq!(a, 1);
73+
let a = (|| echo!((|| 123)))()();
74+
assert_eq!(a, 123);
75+
76+
// chaining calls, but not closures
77+
fn x() -> fn() -> fn() -> fn() -> i32 {
78+
|| || || 42
79+
}
80+
let _ = x()()()();
4181
}

‎tests/ui/redundant_closure_call_fixable.stderr

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
error: try not to call a closure in the expression where it is declared
2-
--> $DIR/redundant_closure_call_fixable.rs:17:13
2+
--> $DIR/redundant_closure_call_fixable.rs:18:13
33
|
44
LL | let a = (|| 42)();
55
| ^^^^^^^^^ help: try doing something like: `42`
66
|
77
= note: `-D clippy::redundant-closure-call` implied by `-D warnings`
88

99
error: try not to call a closure in the expression where it is declared
10-
--> $DIR/redundant_closure_call_fixable.rs:18:13
10+
--> $DIR/redundant_closure_call_fixable.rs:19:13
1111
|
1212
LL | let b = (async || {
1313
| _____________^
@@ -27,7 +27,7 @@ LL ~ };
2727
|
2828

2929
error: try not to call a closure in the expression where it is declared
30-
--> $DIR/redundant_closure_call_fixable.rs:23:13
30+
--> $DIR/redundant_closure_call_fixable.rs:24:13
3131
|
3232
LL | let c = (|| {
3333
| _____________^
@@ -47,13 +47,13 @@ LL ~ };
4747
|
4848

4949
error: try not to call a closure in the expression where it is declared
50-
--> $DIR/redundant_closure_call_fixable.rs:28:13
50+
--> $DIR/redundant_closure_call_fixable.rs:29:13
5151
|
5252
LL | let d = (async || something().await)();
5353
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try doing something like: `async { something().await }`
5454

5555
error: try not to call a closure in the expression where it is declared
56-
--> $DIR/redundant_closure_call_fixable.rs:37:13
56+
--> $DIR/redundant_closure_call_fixable.rs:38:13
5757
|
5858
LL | (|| m!())()
5959
| ^^^^^^^^^^^ help: try doing something like: `m!()`
@@ -64,7 +64,7 @@ LL | m2!();
6464
= note: this error originates in the macro `m2` (in Nightly builds, run with -Z macro-backtrace for more info)
6565

6666
error: try not to call a closure in the expression where it is declared
67-
--> $DIR/redundant_closure_call_fixable.rs:32:13
67+
--> $DIR/redundant_closure_call_fixable.rs:33:13
6868
|
6969
LL | (|| 0)()
7070
| ^^^^^^^^ help: try doing something like: `0`
@@ -74,5 +74,41 @@ LL | m2!();
7474
|
7575
= note: this error originates in the macro `m` which comes from the expansion of the macro `m2` (in Nightly builds, run with -Z macro-backtrace for more info)
7676

77-
error: aborting due to 6 previous errors
77+
error: try not to call a closure in the expression where it is declared
78+
--> $DIR/redundant_closure_call_fixable.rs:46:16
79+
|
80+
LL | assert_eq!((|| || 43)()(), 42);
81+
| ^^^^^^^^^^^^^^ help: try doing something like: `43`
82+
83+
error: try not to call a closure in the expression where it is declared
84+
--> $DIR/redundant_closure_call_fixable.rs:55:10
85+
|
86+
LL | dbg!((|| 42)());
87+
| ^^^^^^^^^ help: try doing something like: `42`
88+
89+
error: try not to call a closure in the expression where it is declared
90+
--> $DIR/redundant_closure_call_fixable.rs:58:13
91+
|
92+
LL | let a = (|| || || 123)();
93+
| ^^^^^^^^^^^^^^^^ help: try doing something like: `(|| || 123)`
94+
95+
error: try not to call a closure in the expression where it is declared
96+
--> $DIR/redundant_closure_call_fixable.rs:62:13
97+
|
98+
LL | let a = (|| || || || async || 1)()()()()();
99+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try doing something like: `async { 1 }`
100+
101+
error: try not to call a closure in the expression where it is declared
102+
--> $DIR/redundant_closure_call_fixable.rs:71:13
103+
|
104+
LL | let a = (|| echo!(|| echo!(|| 1)))()()();
105+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try doing something like: `1`
106+
107+
error: try not to call a closure in the expression where it is declared
108+
--> $DIR/redundant_closure_call_fixable.rs:73:13
109+
|
110+
LL | let a = (|| echo!((|| 123)))()();
111+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try doing something like: `123`
112+
113+
error: aborting due to 12 previous errors
78114

0 commit comments

Comments
 (0)