Auto merge of rust-lang#10930 - y21:issue9956, r=blyxyas,xFrednet

[`redundant_closure_call`]: handle nested closures

Fixes rust-lang#9956.

This ended up being a much larger change than I'd thought, and I ended up having to pretty much rewrite it as a late lint pass, because it needs access to certain things that I don't think are available in early lint passes (e.g. getting the parent expr). I think this'll be required to fi-x rust-lang#10922 anyway, so this is probably fine.
(edit: had to write "fi-x" because "fix" makes github think that this PR fixes it, which it doesn't 😅 )

Previously, it would suggest changing `(|| || 42)()()` to `|| 42()`, which is a type error (it needs parens: `(|| 42)()`). In my opinion, though, the suggested fix should have really been `42`, so that's what this PR changes.

changelog: [`redundant_closure_call`]: handle nested closures and rewrite as a late lint pass

clippy_lints/src/lib.rs

@@ -804,7 +804,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(|| Box::new(int_plus_one::IntPlusOne));
     store.register_early_pass(|| Box::new(formatting::Formatting));
     store.register_early_pass(|| Box::new(misc_early::MiscEarlyLints));
-    store.register_early_pass(|| Box::new(redundant_closure_call::RedundantClosureCall));
     store.register_late_pass(|_| Box::new(redundant_closure_call::RedundantClosureCall));
     store.register_early_pass(|| Box::new(unused_unit::UnusedUnit));
     store.register_late_pass(|_| Box::new(returns::Return));

clippy_lints/src/redundant_closure_call.rs

@@ -1,14 +1,14 @@
+use crate::rustc_lint::LintContext;
 use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
+use clippy_utils::get_parent_expr;
 use clippy_utils::sugg::Sugg;
 use if_chain::if_chain;
-use rustc_ast::ast;
-use rustc_ast::visit as ast_visit;
-use rustc_ast::visit::Visitor as AstVisitor;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_hir::intravisit as hir_visit;
 use rustc_hir::intravisit::Visitor as HirVisitor;
-use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};
+use rustc_hir::intravisit::Visitor;
+use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::nested_filter;
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -51,59 +51,136 @@ impl ReturnVisitor {
     }
 }
 
-impl<'ast> ast_visit::Visitor<'ast> for ReturnVisitor {
-    fn visit_expr(&mut self, ex: &'ast ast::Expr) {
-        if let ast::ExprKind::Ret(_) | ast::ExprKind::Try(_) = ex.kind {
+impl<'tcx> Visitor<'tcx> for ReturnVisitor {
+    fn visit_expr(&mut self, ex: &'tcx hir::Expr<'tcx>) {
+        if let hir::ExprKind::Ret(_) | hir::ExprKind::Match(.., hir::MatchSource::TryDesugar) = ex.kind {
             self.found_return = true;
+        } else {
+            hir_visit::walk_expr(self, ex);
         }
+    }
+}
 
-        ast_visit::walk_expr(self, ex);
+/// Checks if the body is owned by an async closure
+fn is_async_closure(body: &hir::Body<'_>) -> bool {
+    if let hir::ExprKind::Closure(closure) = body.value.kind
+        && let [resume_ty] = closure.fn_decl.inputs
+        && let hir::TyKind::Path(hir::QPath::LangItem(hir::LangItem::ResumeTy, ..)) = resume_ty.kind
+    {
+        true
+    } else {
+        false
     }
 }
 
-impl EarlyLintPass for RedundantClosureCall {
-    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) {
+/// Tries to find the innermost closure:
+/// ```rust,ignore
+/// (|| || || || 42)()()()()
+///  ^^^^^^^^^^^^^^          given this nested closure expression
+///           ^^^^^          we want to return this closure
+/// ```
+/// It also has a parameter for how many steps to go in at most, so as to
+/// not take more closures than there are calls.
+fn find_innermost_closure<'tcx>(
+    cx: &LateContext<'tcx>,
+    mut expr: &'tcx hir::Expr<'tcx>,
+    mut steps: usize,
+) -> Option<(&'tcx hir::Expr<'tcx>, &'tcx hir::FnDecl<'tcx>, hir::IsAsync)> {
+    let mut data = None;
+
+    while let hir::ExprKind::Closure(closure) = expr.kind
+        && let body = cx.tcx.hir().body(closure.body)
+        && {
+            let mut visitor = ReturnVisitor::new();
+            visitor.visit_expr(body.value);
+            !visitor.found_return
+        }
+        && steps > 0
+    {
+        expr = body.value;
+        data = Some((body.value, closure.fn_decl, if is_async_closure(body) {
+            hir::IsAsync::Async
+        } else {
+            hir::IsAsync::NotAsync
+        }));
+        steps -= 1;
+    }
+
+    data
+}
+
+/// "Walks up" the chain of calls to find the outermost call expression, and returns the depth:
+/// ```rust,ignore
+/// (|| || || 3)()()()
+///             ^^      this is the call expression we were given
+///                 ^^  this is what we want to return (and the depth is 3)
+/// ```
+fn get_parent_call_exprs<'tcx>(
+    cx: &LateContext<'tcx>,
+    mut expr: &'tcx hir::Expr<'tcx>,
+) -> (&'tcx hir::Expr<'tcx>, usize) {
+    let mut depth = 1;
+    while let Some(parent) = get_parent_expr(cx, expr)
+        && let hir::ExprKind::Call(recv, _) = parent.kind
+        && expr.span == recv.span
+    {
+        expr = parent;
+        depth += 1;
+    }
+    (expr, depth)
+}
+
+impl<'tcx> LateLintPass<'tcx> for RedundantClosureCall {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
         if in_external_macro(cx.sess(), expr.span) {
             return;
         }
-        if_chain! {
-            if let ast::ExprKind::Call(ref paren, _) = expr.kind;
-            if let ast::ExprKind::Paren(ref closure) = paren.kind;
-            if let ast::ExprKind::Closure(box ast::Closure { ref asyncness, ref fn_decl, ref body, .. }) = closure.kind;
-            then {
-                let mut visitor = ReturnVisitor::new();
-                visitor.visit_expr(body);
-                if !visitor.found_return {
-                    span_lint_and_then(
-                        cx,
-                        REDUNDANT_CLOSURE_CALL,
-                        expr.span,
-                        "try not to call a closure in the expression where it is declared",
-                        |diag| {
-                            if fn_decl.inputs.is_empty() {
-                                let mut app = Applicability::MachineApplicable;
-                                let mut hint = Sugg::ast(cx, body, "..", closure.span.ctxt(), &mut app);
-
-                                if asyncness.is_async() {
-                                    // `async x` is a syntax error, so it becomes `async { x }`
-                                    if !matches!(body.kind, ast::ExprKind::Block(_, _)) {
-                                        hint = hint.blockify();
-                                    }
-
-                                    hint = hint.asyncify();
-                                }
-
-                                diag.span_suggestion(expr.span, "try doing something like", hint.to_string(), app);
+
+        if let hir::ExprKind::Call(recv, _) = expr.kind
+            // don't lint if the receiver is a call, too. 
+            // we do this in order to prevent linting multiple times; consider:
+            // `(|| || 1)()()`
+            //           ^^  we only want to lint for this call (but we walk up the calls to consider both calls).
+            // without this check, we'd end up linting twice.
+            && !matches!(recv.kind, hir::ExprKind::Call(..))
+            && let (full_expr, call_depth) = get_parent_call_exprs(cx, expr)
+            && let Some((body, fn_decl, generator_kind)) = find_innermost_closure(cx, recv, call_depth)
+        {
+            span_lint_and_then(
+                cx,
+                REDUNDANT_CLOSURE_CALL,
+                full_expr.span,
+                "try not to call a closure in the expression where it is declared",
+                |diag| {
+                    if fn_decl.inputs.is_empty() {
+                        let mut applicability = Applicability::MachineApplicable;
+                        let mut hint = Sugg::hir_with_context(cx, body, full_expr.span.ctxt(), "..", &mut applicability);
+
+                        if generator_kind.is_async()
+                            && let hir::ExprKind::Closure(closure) = body.kind
+                        {
+                            let async_closure_body = cx.tcx.hir().body(closure.body);
+
+                            // `async x` is a syntax error, so it becomes `async { x }`
+                            if !matches!(async_closure_body.value.kind, hir::ExprKind::Block(_, _)) {
+                                hint = hint.blockify();
                             }
-                        },
-                    );
+
+                            hint = hint.asyncify();
+                        }
+
+                        diag.span_suggestion(
+                            full_expr.span,
+                            "try doing something like",
+                            hint.maybe_par(),
+                            applicability
+                        );
+                    }
                 }
-            }
+            );
         }
     }
-}
 
-impl<'tcx> LateLintPass<'tcx> for RedundantClosureCall {
     fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
         fn count_closure_usage<'tcx>(
             cx: &LateContext<'tcx>,

tests/ui/redundant_closure_call_fixable.fixed

@@ -3,6 +3,7 @@
 #![feature(async_closure)]
 #![warn(clippy::redundant_closure_call)]
 #![allow(clippy::redundant_async_block)]
+#![allow(clippy::type_complexity)]
 #![allow(unused)]
 
 async fn something() -> u32 {
@@ -38,4 +39,50 @@ fn main() {
         };
     }
     m2!();
+    issue9956();
+}
+
+fn issue9956() {
+    assert_eq!(43, 42);
+
+    // ... and some more interesting cases I've found while implementing the fix
+
+    // not actually immediately calling the closure:
+    let a = (|| 42);
+    dbg!(a());
+
+    // immediately calling it inside of a macro
+    dbg!(42);
+
+    // immediately calling only one closure, so we can't remove the other ones
+    let a = (|| || 123);
+    dbg!(a()());
+
+    // nested async closures
+    let a = async { 1 };
+    let h = async { a.await };
+
+    // macro expansion tests
+    macro_rules! echo {
+        ($e:expr) => {
+            $e
+        };
+    }
+    let a = 1;
+    assert_eq!(a, 1);
+    let a = 123;
+    assert_eq!(a, 123);
+
+    // chaining calls, but not closures
+    fn x() -> fn() -> fn() -> fn() -> i32 {
+        || || || 42
+    }
+    let _ = x()()()();
+
+    fn bar() -> fn(i32, i32) {
+        foo
+    }
+    fn foo(_: i32, _: i32) {}
+    bar()(42, 5);
+    foo(42, 5);
 }

tests/ui/redundant_closure_call_fixable.rs

@@ -3,6 +3,7 @@
 #![feature(async_closure)]
 #![warn(clippy::redundant_closure_call)]
 #![allow(clippy::redundant_async_block)]
+#![allow(clippy::type_complexity)]
 #![allow(unused)]
 
 async fn something() -> u32 {
@@ -38,4 +39,50 @@ fn main() {
         };
     }
     m2!();
+    issue9956();
+}
+
+fn issue9956() {
+    assert_eq!((|| || 43)()(), 42);
+
+    // ... and some more interesting cases I've found while implementing the fix
+
+    // not actually immediately calling the closure:
+    let a = (|| 42);
+    dbg!(a());
+
+    // immediately calling it inside of a macro
+    dbg!((|| 42)());
+
+    // immediately calling only one closure, so we can't remove the other ones
+    let a = (|| || || 123)();
+    dbg!(a()());
+
+    // nested async closures
+    let a = (|| || || || async || 1)()()()()();
+    let h = async { a.await };
+
+    // macro expansion tests
+    macro_rules! echo {
+        ($e:expr) => {
+            $e
+        };
+    }
+    let a = (|| echo!(|| echo!(|| 1)))()()();
+    assert_eq!(a, 1);
+    let a = (|| echo!((|| 123)))()();
+    assert_eq!(a, 123);
+
+    // chaining calls, but not closures
+    fn x() -> fn() -> fn() -> fn() -> i32 {
+        || || || 42
+    }
+    let _ = x()()()();
+
+    fn bar() -> fn(i32, i32) {
+        foo
+    }
+    fn foo(_: i32, _: i32) {}
+    bar()((|| || 42)()(), 5);
+    foo((|| || 42)()(), 5);
 }