Auto merge of #5800 - montrivo:bugfix/redundant_closure, r=matthiaskrgr

redundant_closure_call - don't lint when used more than once

Fixes #3354.

changelog: fix redundant_closure_call false positive when closure called more than once

clippy_lints/src/lib.rs

@@ -276,6 +276,7 @@ mod ptr_offset_with_cast;
 mod question_mark;
 mod ranges;
 mod redundant_clone;
+mod redundant_closure_call;
 mod redundant_field_names;
 mod redundant_pub_crate;
 mod redundant_static_lifetimes;
@@ -702,7 +703,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &misc_early::DOUBLE_NEG,
         &misc_early::DUPLICATE_UNDERSCORE_ARGUMENT,
         &misc_early::MIXED_CASE_HEX_LITERALS,
-        &misc_early::REDUNDANT_CLOSURE_CALL,
         &misc_early::REDUNDANT_PATTERN,
         &misc_early::UNNEEDED_FIELD_PATTERN,
         &misc_early::UNNEEDED_WILDCARD_PATTERN,
@@ -759,6 +759,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &ranges::RANGE_ZIP_WITH_LEN,
         &ranges::REVERSED_EMPTY_RANGES,
         &redundant_clone::REDUNDANT_CLONE,
+        &redundant_closure_call::REDUNDANT_CLOSURE_CALL,
         &redundant_field_names::REDUNDANT_FIELD_NAMES,
         &redundant_pub_crate::REDUNDANT_PUB_CRATE,
         &redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES,
@@ -1018,6 +1019,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(|| box int_plus_one::IntPlusOne);
     store.register_early_pass(|| box formatting::Formatting);
     store.register_early_pass(|| box misc_early::MiscEarlyLints);
+    store.register_early_pass(|| box redundant_closure_call::RedundantClosureCall);
+    store.register_late_pass(|| box redundant_closure_call::RedundantClosureCall);
     store.register_early_pass(|| box returns::Return);
     store.register_late_pass(|| box let_and_return::LetReturn);
     store.register_early_pass(|| box collapsible_if::CollapsibleIf);
@@ -1359,7 +1362,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&misc_early::DOUBLE_NEG),
         LintId::of(&misc_early::DUPLICATE_UNDERSCORE_ARGUMENT),
         LintId::of(&misc_early::MIXED_CASE_HEX_LITERALS),
-        LintId::of(&misc_early::REDUNDANT_CLOSURE_CALL),
         LintId::of(&misc_early::REDUNDANT_PATTERN),
         LintId::of(&misc_early::UNNEEDED_WILDCARD_PATTERN),
         LintId::of(&misc_early::ZERO_PREFIXED_LITERAL),
@@ -1393,6 +1395,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&ranges::RANGE_ZIP_WITH_LEN),
         LintId::of(&ranges::REVERSED_EMPTY_RANGES),
         LintId::of(&redundant_clone::REDUNDANT_CLONE),
+        LintId::of(&redundant_closure_call::REDUNDANT_CLOSURE_CALL),
         LintId::of(&redundant_field_names::REDUNDANT_FIELD_NAMES),
         LintId::of(&redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES),
         LintId::of(&reference::DEREF_ADDROF),
@@ -1593,7 +1596,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::UNNECESSARY_FILTER_MAP),
         LintId::of(&methods::USELESS_ASREF),
         LintId::of(&misc::SHORT_CIRCUIT_STATEMENT),
-        LintId::of(&misc_early::REDUNDANT_CLOSURE_CALL),
         LintId::of(&misc_early::UNNEEDED_WILDCARD_PATTERN),
         LintId::of(&misc_early::ZERO_PREFIXED_LITERAL),
         LintId::of(&needless_bool::BOOL_COMPARISON),
@@ -1608,6 +1610,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&precedence::PRECEDENCE),
         LintId::of(&ptr_offset_with_cast::PTR_OFFSET_WITH_CAST),
         LintId::of(&ranges::RANGE_ZIP_WITH_LEN),
+        LintId::of(&redundant_closure_call::REDUNDANT_CLOSURE_CALL),
         LintId::of(&reference::DEREF_ADDROF),
         LintId::of(&reference::REF_IN_DEREF),
         LintId::of(&repeat_once::REPEAT_ONCE),

clippy_lints/src/misc_early.rs

@@ -1,13 +1,9 @@
-use crate::utils::{
+use crate::utils::{constants, snippet_opt, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
-    constants, snippet_opt, snippet_with_applicability, span_lint, span_lint_and_help, span_lint_and_sugg,
-    span_lint_and_then,
-};
-use if_chain::if_chain;
 use rustc_ast::ast::{
-    BindingMode, Block, Expr, ExprKind, GenericParamKind, Generics, Lit, LitFloatType, LitIntType, LitKind, Mutability,
+    BindingMode, Expr, ExprKind, GenericParamKind, Generics, Lit, LitFloatType, LitIntType, LitKind, Mutability,
-    NodeId, Pat, PatKind, StmtKind, UnOp,
+    NodeId, Pat, PatKind, UnOp,
 };
-use rustc_ast::visit::{walk_expr, FnKind, Visitor};
+use rustc_ast::visit::FnKind;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::Applicability;
 use rustc_lint::{EarlyContext, EarlyLintPass, LintContext};
@@ -70,28 +66,6 @@ declare_clippy_lint! {
     "function arguments having names which only differ by an underscore"
 }
 
-declare_clippy_lint! {
-    /// **What it does:** Detects closures called in the same expression where they
-    /// are defined.
-    ///
-    /// **Why is this bad?** It is unnecessarily adding to the expression's
-    /// complexity.
-    ///
-    /// **Known problems:** None.
-    ///
-    /// **Example:**
-    /// ```rust,ignore
-    /// // Bad
-    /// let a = (|| 42)()
-    ///
-    /// // Good
-    /// let a = 42
-    /// ```
-    pub REDUNDANT_CLOSURE_CALL,
-    complexity,
-    "throwaway closures called in the expression they are defined"
-}
-
 declare_clippy_lint! {
     /// **What it does:** Detects expressions of the form `--x`.
     ///
@@ -278,7 +252,6 @@ declare_clippy_lint! {
 declare_lint_pass!(MiscEarlyLints => [
     UNNEEDED_FIELD_PATTERN,
     DUPLICATE_UNDERSCORE_ARGUMENT,
-    REDUNDANT_CLOSURE_CALL,
     DOUBLE_NEG,
     MIXED_CASE_HEX_LITERALS,
     UNSEPARATED_LITERAL_SUFFIX,
@@ -288,30 +261,6 @@ declare_lint_pass!(MiscEarlyLints => [
     UNNEEDED_WILDCARD_PATTERN,
 ]);
 
-// Used to find `return` statements or equivalents e.g., `?`
-struct ReturnVisitor {
-    found_return: bool,
-}
-
-impl ReturnVisitor {
-    #[must_use]
-    fn new() -> Self {
-        Self { found_return: false }
-    }
-}
-
-impl<'ast> Visitor<'ast> for ReturnVisitor {
-    fn visit_expr(&mut self, ex: &'ast Expr) {
-        if let ExprKind::Ret(_) = ex.kind {
-            self.found_return = true;
-        } else if let ExprKind::Try(_) = ex.kind {
-            self.found_return = true;
-        }
-
-        walk_expr(self, ex)
-    }
-}
-
 impl EarlyLintPass for MiscEarlyLints {
     fn check_generics(&mut self, cx: &EarlyContext<'_>, gen: &Generics) {
         for param in &gen.params {
@@ -453,30 +402,6 @@ impl EarlyLintPass for MiscEarlyLints {
             return;
         }
         match expr.kind {
-            ExprKind::Call(ref paren, _) => {
-                if let ExprKind::Paren(ref closure) = paren.kind {
-                    if let ExprKind::Closure(_, _, _, ref decl, ref block, _) = closure.kind {
-                        let mut visitor = ReturnVisitor::new();
-                        visitor.visit_expr(block);
-                        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 decl.inputs.is_empty() {
-                                        let mut app = Applicability::MachineApplicable;
-                                        let hint =
-                                            snippet_with_applicability(cx, block.span, "..", &mut app).into_owned();
-                                        diag.span_suggestion(expr.span, "Try doing something like: ", hint, app);
-                                    }
-                                },
-                            );
-                        }
-                    }
-                }
-            },
             ExprKind::Unary(UnOp::Neg, ref inner) => {
                 if let ExprKind::Unary(UnOp::Neg, _) = inner.kind {
                     span_lint(
@@ -491,31 +416,6 @@ impl EarlyLintPass for MiscEarlyLints {
             _ => (),
         }
     }
-
-    fn check_block(&mut self, cx: &EarlyContext<'_>, block: &Block) {
-        for w in block.stmts.windows(2) {
-            if_chain! {
-                if let StmtKind::Local(ref local) = w[0].kind;
-                if let Option::Some(ref t) = local.init;
-                if let ExprKind::Closure(..) = t.kind;
-                if let PatKind::Ident(_, ident, _) = local.pat.kind;
-                if let StmtKind::Semi(ref second) = w[1].kind;
-                if let ExprKind::Assign(_, ref call, _) = second.kind;
-                if let ExprKind::Call(ref closure, _) = call.kind;
-                if let ExprKind::Path(_, ref path) = closure.kind;
-                then {
-                    if ident == path.segments[0].ident {
-                        span_lint(
-                            cx,
-                            REDUNDANT_CLOSURE_CALL,
-                            second.span,
-                            "Closure called just once immediately after it was declared",
-                        );
-                    }
-                }
-            }
-        }
-    }
 }
 
 impl MiscEarlyLints {

clippy_lints/src/redundant_closure_call.rs

@@ -0,0 +1,151 @@
+use crate::utils::{snippet_with_applicability, span_lint, span_lint_and_then};
+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_middle::hir::map::Map;
+use rustc_middle::lint::in_external_macro;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// **What it does:** Detects closures called in the same expression where they
+    /// are defined.
+    ///
+    /// **Why is this bad?** It is unnecessarily adding to the expression's
+    /// complexity.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust,ignore
+    /// // Bad
+    /// let a = (|| 42)()
+    ///
+    /// // Good
+    /// let a = 42
+    /// ```
+    pub REDUNDANT_CLOSURE_CALL,
+    complexity,
+    "throwaway closures called in the expression they are defined"
+}
+
+declare_lint_pass!(RedundantClosureCall => [REDUNDANT_CLOSURE_CALL]);
+
+// Used to find `return` statements or equivalents e.g., `?`
+struct ReturnVisitor {
+    found_return: bool,
+}
+
+impl ReturnVisitor {
+    #[must_use]
+    fn new() -> Self {
+        Self { found_return: false }
+    }
+}
+
+impl<'ast> ast_visit::Visitor<'ast> for ReturnVisitor {
+    fn visit_expr(&mut self, ex: &'ast ast::Expr) {
+        if let ast::ExprKind::Ret(_) = ex.kind {
+            self.found_return = true;
+        } else if let ast::ExprKind::Try(_) = ex.kind {
+            self.found_return = true;
+        }
+
+        ast_visit::walk_expr(self, ex)
+    }
+}
+
+impl EarlyLintPass for RedundantClosureCall {
+    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) {
+        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(_, _, _, ref decl, ref block, _) = closure.kind;
+            then {
+                let mut visitor = ReturnVisitor::new();
+                visitor.visit_expr(block);
+                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 decl.inputs.is_empty() {
+                                let mut app = Applicability::MachineApplicable;
+                                let hint =
+                                    snippet_with_applicability(cx, block.span, "..", &mut app).into_owned();
+                                diag.span_suggestion(expr.span, "try doing something like", hint, app);
+                            }
+                        },
+                    );
+                }
+            }
+        }
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for RedundantClosureCall {
+    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
+        fn count_closure_usage<'tcx>(block: &'tcx hir::Block<'_>, path: &'tcx hir::Path<'tcx>) -> usize {
+            struct ClosureUsageCount<'tcx> {
+                path: &'tcx hir::Path<'tcx>,
+                count: usize,
+            };
+            impl<'tcx> hir_visit::Visitor<'tcx> for ClosureUsageCount<'tcx> {
+                type Map = Map<'tcx>;
+
+                fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) {
+                    if_chain! {
+                        if let hir::ExprKind::Call(ref closure, _) = expr.kind;
+                        if let hir::ExprKind::Path(hir::QPath::Resolved(_, ref path)) = closure.kind;
+                        if self.path.segments[0].ident == path.segments[0].ident
+                            && self.path.res == path.res;
+                        then {
+                            self.count += 1;
+                        }
+                    }
+                    hir_visit::walk_expr(self, expr);
+                }
+
+                fn nested_visit_map(&mut self) -> hir_visit::NestedVisitorMap<Self::Map> {
+                    hir_visit::NestedVisitorMap::None
+                }
+            };
+            let mut closure_usage_count = ClosureUsageCount { path, count: 0 };
+            closure_usage_count.visit_block(block);
+            closure_usage_count.count
+        }
+
+        for w in block.stmts.windows(2) {
+            if_chain! {
+                if let hir::StmtKind::Local(ref local) = w[0].kind;
+                if let Option::Some(ref t) = local.init;
+                if let hir::ExprKind::Closure(..) = t.kind;
+                if let hir::PatKind::Binding(_, _, ident, _) = local.pat.kind;
+                if let hir::StmtKind::Semi(ref second) = w[1].kind;
+                if let hir::ExprKind::Assign(_, ref call, _) = second.kind;
+                if let hir::ExprKind::Call(ref closure, _) = call.kind;
+                if let hir::ExprKind::Path(hir::QPath::Resolved(_, ref path)) = closure.kind;
+                if ident == path.segments[0].ident;
+                if count_closure_usage(block, path) == 1;
+                then {
+                    span_lint(
+                        cx,
+                        REDUNDANT_CLOSURE_CALL,
+                        second.span,
+                        "closure called just once immediately after it was declared",
+                    );
+                }
+            }
+        }
+    }
+}

src/lintlist/mod.rs

@@ -1835,7 +1835,7 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         group: "complexity",
         desc: "throwaway closures called in the expression they are defined",
         deprecation: None,
-        module: "misc_early",
+        module: "redundant_closure_call",
     },
     Lint {
         name: "redundant_closure_for_method_calls",

tests/ui/redundant_closure_call.stderr

@@ -1,28 +0,0 @@
-error: Closure called just once immediately after it was declared
-  --> $DIR/redundant_closure_call.rs:12:5
-   |
-LL |     i = closure();
-   |     ^^^^^^^^^^^^^
-   |
-   = note: `-D clippy::redundant-closure-call` implied by `-D warnings`
-
-error: Closure called just once immediately after it was declared
-  --> $DIR/redundant_closure_call.rs:15:5
-   |
-LL |     i = closure(3);
-   |     ^^^^^^^^^^^^^^
-
-error: Try not to call a closure in the expression where it is declared.
-  --> $DIR/redundant_closure_call.rs:7:17
-   |
-LL |     let mut k = (|m| m + 1)(i);
-   |                 ^^^^^^^^^^^^^^
-
-error: Try not to call a closure in the expression where it is declared.
-  --> $DIR/redundant_closure_call.rs:9:9
-   |
-LL |     k = (|a, b| a * b)(1, 5);
-   |         ^^^^^^^^^^^^^^^^^^^^
-
-error: aborting due to 4 previous errors
-

tests/ui/redundant_closure_call_early.rs

@@ -4,18 +4,14 @@
 
 fn main() {
     let mut i = 1;
+
+    // lint here
     let mut k = (|m| m + 1)(i);
 
+    // lint here
     k = (|a, b| a * b)(1, 5);
 
-    let closure = || 32;
+    // don't lint these
-    i = closure();
-
-    let closure = |i| i + 1;
-    i = closure(3);
-
-    i = closure(4);
-
     #[allow(clippy::needless_return)]
     (|| return 2)();
     (|| -> Option<i32> { None? })();

tests/ui/redundant_closure_call_early.stderr

@@ -0,0 +1,16 @@
+error: try not to call a closure in the expression where it is declared.
+  --> $DIR/redundant_closure_call_early.rs:9:17
+   |
+LL |     let mut k = (|m| m + 1)(i);
+   |                 ^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::redundant-closure-call` implied by `-D warnings`
+
+error: try not to call a closure in the expression where it is declared.
+  --> $DIR/redundant_closure_call_early.rs:12:9
+   |
+LL |     k = (|a, b| a * b)(1, 5);
+   |         ^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+

tests/ui/redundant_closure_call_fixable.stderr

@@ -1,8 +1,8 @@
-error: Try not to call a closure in the expression where it is declared.
+error: try not to call a closure in the expression where it is declared.
   --> $DIR/redundant_closure_call_fixable.rs:7:13
    |
 LL |     let a = (|| 42)();
-   |             ^^^^^^^^^ help: Try doing something like: : `42`
+   |             ^^^^^^^^^ help: try doing something like: `42`
    |
    = note: `-D clippy::redundant-closure-call` implied by `-D warnings`
 

tests/ui/redundant_closure_call_late.rs

@@ -0,0 +1,27 @@
+// non rustfixable, see redundant_closure_call_fixable.rs
+
+#![warn(clippy::redundant_closure_call)]
+
+fn main() {
+    let mut i = 1;
+
+    // don't lint here, the closure is used more than once
+    let closure = |i| i + 1;
+    i = closure(3);
+    i = closure(4);
+
+    // lint here
+    let redun_closure = || 1;
+    i = redun_closure();
+
+    // shadowed closures are supported, lint here
+    let shadowed_closure = || 1;
+    i = shadowed_closure();
+    let shadowed_closure = || 2;
+    i = shadowed_closure();
+
+    // don't lint here
+    let shadowed_closure = || 2;
+    i = shadowed_closure();
+    i = shadowed_closure();
+}

tests/ui/redundant_closure_call_late.stderr

@@ -0,0 +1,22 @@
+error: closure called just once immediately after it was declared
+  --> $DIR/redundant_closure_call_late.rs:15:5
+   |
+LL |     i = redun_closure();
+   |     ^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::redundant-closure-call` implied by `-D warnings`
+
+error: closure called just once immediately after it was declared
+  --> $DIR/redundant_closure_call_late.rs:19:5
+   |
+LL |     i = shadowed_closure();
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: closure called just once immediately after it was declared
+  --> $DIR/redundant_closure_call_late.rs:21:5
+   |
+LL |     i = shadowed_closure();
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 3 previous errors
+