Rework redundant_closure

* Better track when a early-bound region appears when a late-bound region is required
* Don't lint when the closure gives explicit types.

Author: Jarcho

clippy_lints/src/eta_reduction.rs

@@ -1,20 +1,18 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::higher::VecArgs;
 use clippy_utils::source::snippet_opt;
-use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::ty::type_diagnostic_name;
 use clippy_utils::usage::local_used_after_expr;
 use clippy_utils::{get_enclosing_loop_or_closure, higher, path_to_local, path_to_local_id};
-use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::def_id::DefId;
-use rustc_hir::{Expr, ExprKind, Param, PatKind, Unsafety};
+use rustc_hir::{BindingAnnotation, Expr, ExprKind, FnRetTy, Param, PatKind, TyKind, Unsafety};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
-use rustc_middle::ty::binding::BindingMode;
-use rustc_middle::ty::subst::Subst;
-use rustc_middle::ty::{self, ClosureKind, Ty, TypeFoldable};
+use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
+use rustc_middle::ty::{self, ClosureKind, ClosureSubsts, FnSig, Region, RegionKind, Ty, TypeFoldable, TypeckResults};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::symbol::sym;
+use rustc_target::spec::abi::Abi;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -73,14 +71,18 @@ declare_clippy_lint! {
 declare_lint_pass!(EtaReduction => [REDUNDANT_CLOSURE, REDUNDANT_CLOSURE_FOR_METHOD_CALLS]);
 
 impl<'tcx> LateLintPass<'tcx> for EtaReduction {
+    #[allow(clippy::too_many_lines)]
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if expr.span.from_expansion() {
-            return;
-        }
-        let body = match expr.kind {
-            ExprKind::Closure(_, _, id, _, _) => cx.tcx.hir().body(id),
-            _ => return,
+        let body = if let ExprKind::Closure(_, decl, id, _, _) = expr.kind
+            && decl.inputs.iter().all(|ty| matches!(ty.kind, TyKind::Infer))
+            && matches!(decl.output, FnRetTy::DefaultReturn(_))
+            && !expr.span.from_expansion()
+        {
+            cx.tcx.hir().body(id)
+        } else {
+            return
         };
+
         if body.value.span.from_expansion() {
             if body.params.is_empty() {
                 if let Some(VecArgs::Vec(&[])) = higher::VecArgs::hir(cx, &body.value) {
@@ -100,122 +102,187 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
             return;
         }
 
-        let closure_ty = cx.typeck_results().expr_ty(expr);
-
-        if_chain!(
-            if let ExprKind::Call(callee, args) = body.value.kind;
-            if let ExprKind::Path(_) = callee.kind;
-            if check_inputs(cx, body.params, args);
-            let callee_ty = cx.typeck_results().expr_ty_adjusted(callee);
-            let call_ty = cx.typeck_results().type_dependent_def_id(body.value.hir_id)
-                .map_or(callee_ty, |id| cx.tcx.type_of(id));
-            if check_sig(cx, closure_ty, call_ty);
-            let substs = cx.typeck_results().node_substs(callee.hir_id);
-            // This fixes some false positives that I don't entirely understand
-            if substs.is_empty() || !cx.typeck_results().expr_ty(expr).has_late_bound_regions();
-            // A type param function ref like `T::f` is not 'static, however
-            // it is if cast like `T::f as fn()`. This seems like a rustc bug.
-            if !substs.types().any(|t| matches!(t.kind(), ty::Param(_)));
-            let callee_ty_unadjusted = cx.typeck_results().expr_ty(callee).peel_refs();
-            if !is_type_diagnostic_item(cx, callee_ty_unadjusted, sym::Arc);
-            if !is_type_diagnostic_item(cx, callee_ty_unadjusted, sym::Rc);
-            then {
-                span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
-                    if let Some(mut snippet) = snippet_opt(cx, callee.span) {
-                        if_chain! {
-                            if let ty::Closure(_, substs) = callee_ty.peel_refs().kind();
-                            if substs.as_closure().kind() == ClosureKind::FnMut;
-                            if get_enclosing_loop_or_closure(cx.tcx, expr).is_some()
-                                || path_to_local(callee).map_or(false, |l| local_used_after_expr(cx, l, callee));
+        let typeck = cx.typeck_results();
+        let closure = if let ty::Closure(_, closure_subs) = typeck.expr_ty(expr).kind() {
+            closure_subs.as_closure()
+        } else {
+            return;
+        };
 
-                            then {
-                                // Mutable closure is used after current expr; we cannot consume it.
-                                snippet = format!("&mut {}", snippet);
+        match body.value.kind {
+            ExprKind::Call(callee, args)
+                if matches!(callee.kind, ExprKind::Path(..))
+                    && !matches!(
+                        type_diagnostic_name(cx, typeck.expr_ty(callee).peel_refs()),
+                        Some(sym::Arc | sym::Rc)
+                    )
+                    && check_inputs(typeck, body.params, args) =>
+            {
+                let sig = match *typeck.expr_ty_adjusted(callee).kind() {
+                    ty::FnDef(def, _) => cx.tcx.fn_sig(def).skip_binder(),
+                    ty::FnPtr(sig) => sig.skip_binder(),
+                    ty::Closure(_, subs) => cx
+                        .tcx
+                        .signature_unclosure(subs.as_closure().sig(), Unsafety::Normal)
+                        .skip_binder(),
+                    _ => {
+                        if typeck.type_dependent_def_id(body.value.hir_id).is_some()
+                            && let subs = typeck.node_substs(body.value.hir_id)
+                            && let output = typeck.expr_ty(&body.value)
+                            && let ty::Tuple(tys) = *subs.type_at(1).kind()
+                        {
+                            cx.tcx.mk_fn_sig(tys.into_iter(), output, false, Unsafety::Normal, Abi::Rust)
+                        } else {
+                            return;
+                        }
+                    },
+                };
+                if check_sig(cx, closure, sig)
+                    // Given some trait fn `fn f() -> ()` and some type `T: Trait`, `T::f` is not
+                    // `'static` unless `T: 'static`. The cast `T::f as fn()` will, however, result
+                    // in a type which is `'static`.
+                    // For now ignore all callee types which reference a type parameter.
+                    && !typeck.node_substs(callee.hir_id).types().any(|t| matches!(t.kind(), ty::Param(_)))
+                {
+                    span_lint_and_then(
+                        cx,
+                        REDUNDANT_CLOSURE,
+                        expr.span,
+                        "redundant closure",
+                        |diag| {
+                            if let Some(mut snippet) = snippet_opt(cx, callee.span) {
+                                if let ty::Closure(_, substs) = *typeck.expr_ty(callee).peel_refs().kind()
+                                    && substs.as_closure().kind() == ClosureKind::FnMut
+                                    && (
+                                        get_enclosing_loop_or_closure(cx.tcx, expr).is_some()
+                                            || path_to_local(callee)
+                                                .map_or(false, |l| local_used_after_expr(cx, l, callee))
+                                    )
+                                {
+                                    // Mutable closure is used after current expr; we cannot consume it.
+                                    snippet = format!("&mut {}", snippet);
+                                }
+                                diag.span_suggestion(
+                                    expr.span,
+                                    "replace the closure with the function itself",
+                                    snippet,
+                                    Applicability::MachineApplicable,
+                                );
                             }
                         }
-                        diag.span_suggestion(
-                            expr.span,
-                            "replace the closure with the function itself",
-                            snippet,
-                            Applicability::MachineApplicable,
-                        );
-                    }
-                });
-            }
-        );
-
-        if_chain!(
-            if let ExprKind::MethodCall(path, args, _) = body.value.kind;
-            if check_inputs(cx, body.params, args);
-            let method_def_id = cx.typeck_results().type_dependent_def_id(body.value.hir_id).unwrap();
-            let substs = cx.typeck_results().node_substs(body.value.hir_id);
-            let call_ty = cx.tcx.type_of(method_def_id).subst(cx.tcx, substs);
-            if check_sig(cx, closure_ty, call_ty);
-            then {
-                span_lint_and_then(cx, REDUNDANT_CLOSURE_FOR_METHOD_CALLS, expr.span, "redundant closure", |diag| {
-                    let name = get_ufcs_type_name(cx, method_def_id);
-                    diag.span_suggestion(
+                    );
+                }
+            },
+            ExprKind::MethodCall(path, args, _) if check_inputs(typeck, body.params, args) => {
+                if let Some(method_def_id) = typeck.type_dependent_def_id(body.value.hir_id)
+                    && check_sig(cx, closure, cx.tcx.fn_sig(method_def_id).skip_binder())
+                {
+                    span_lint_and_then(
+                        cx,
+                        REDUNDANT_CLOSURE_FOR_METHOD_CALLS,
                         expr.span,
-                        "replace the closure with the method itself",
-                        format!("{}::{}", name, path.ident.name),
-                        Applicability::MachineApplicable,
+                        "redundant closure",
+                        |diag| {
+                            let name = get_ufcs_type_name(cx, method_def_id);
+                            diag.span_suggestion(
+                                expr.span,
+                                "replace the closure with the method itself",
+                                format!("{}::{}", name, path.ident.name),
+                                Applicability::MachineApplicable,
+                            );
+                        },
                     );
-                })
-            }
-        );
+                }
+            },
+            _ => (),
+        }
     }
 }
 
-fn check_inputs(cx: &LateContext<'_>, params: &[Param<'_>], call_args: &[Expr<'_>]) -> bool {
-    if params.len() != call_args.len() {
-        return false;
+fn check_inputs(typeck: &TypeckResults<'_>, params: &[Param<'_>], call_args: &[Expr<'_>]) -> bool {
+    params.len() == call_args.len()
+        && params.iter().zip(call_args).all(|(p, arg)| {
+            matches!(
+                p.pat.kind,PatKind::Binding(BindingAnnotation::Unannotated, id, _, None)
+                if path_to_local_id(arg, id)
+            )
+            // Only allow adjustments which change regions (i.e. re-borrowing).
+            && typeck
+                .expr_adjustments(arg)
+                .last()
+                .map_or(true, |a| a.target == typeck.expr_ty(arg))
+        })
+}
+
+fn check_sig<'tcx>(cx: &LateContext<'tcx>, closure: ClosureSubsts<'tcx>, call_sig: FnSig<'_>) -> bool {
+    call_sig.unsafety == Unsafety::Normal
+        && !has_late_bound_to_non_late_bound_regions(
+            cx.tcx
+                .signature_unclosure(closure.sig(), Unsafety::Normal)
+                .skip_binder(),
+            call_sig,
+        )
+}
+
+/// This walks through both signatures and checks for any time a late-bound region is expected by an
+/// `impl Fn` type, but the target signature does not have a late-bound region in the same position.
+///
+/// This is needed because rustc is unable to late bind early-bound regions in a function signature.
+fn has_late_bound_to_non_late_bound_regions(from_sig: FnSig<'_>, to_sig: FnSig<'_>) -> bool {
+    fn check_region(from_region: Region<'_>, to_region: Region<'_>) -> bool {
+        matches!(from_region.kind(), RegionKind::ReLateBound(..))
+            && !matches!(to_region.kind(), RegionKind::ReLateBound(..))
     }
-    let binding_modes = cx.typeck_results().pat_binding_modes();
-    std::iter::zip(params, call_args).all(|(param, arg)| {
-        match param.pat.kind {
-            PatKind::Binding(_, id, ..) if path_to_local_id(arg, id) => {},
-            _ => return false,
-        }
-        // checks that parameters are not bound as `ref` or `ref mut`
-        if let Some(BindingMode::BindByReference(_)) = binding_modes.get(param.pat.hir_id) {
-            return false;
-        }
 
-        match *cx.typeck_results().expr_adjustments(arg) {
-            [] => true,
-            [
-                Adjustment {
-                    kind: Adjust::Deref(None),
-                    ..
+    fn check_subs(from_subs: &[GenericArg<'_>], to_subs: &[GenericArg<'_>]) -> bool {
+        if from_subs.len() != to_subs.len() {
+            return true;
+        }
+        for (from_arg, to_arg) in to_subs.iter().zip(from_subs) {
+            match (from_arg.unpack(), to_arg.unpack()) {
+                (GenericArgKind::Lifetime(from_region), GenericArgKind::Lifetime(to_region)) => {
+                    if check_region(from_region, to_region) {
+                        return true;
+                    }
                 },
-                Adjustment {
-                    kind: Adjust::Borrow(AutoBorrow::Ref(_, mu2)),
-                    ..
+                (GenericArgKind::Type(from_ty), GenericArgKind::Type(to_ty)) => {
+                    if check_ty(from_ty, to_ty) {
+                        return true;
+                    }
                 },
-            ] => {
-                // re-borrow with the same mutability is allowed
-                let ty = cx.typeck_results().expr_ty(arg);
-                matches!(*ty.kind(), ty::Ref(.., mu1) if mu1 == mu2.into())
-            },
-            _ => false,
+                (GenericArgKind::Const(_), GenericArgKind::Const(_)) => (),
+                _ => return true,
+            }
         }
-    })
-}
-
-fn check_sig<'tcx>(cx: &LateContext<'tcx>, closure_ty: Ty<'tcx>, call_ty: Ty<'tcx>) -> bool {
-    let call_sig = call_ty.fn_sig(cx.tcx);
-    if call_sig.unsafety() == Unsafety::Unsafe {
-        return false;
+        false
     }
-    if !closure_ty.has_late_bound_regions() {
-        return true;
+
+    fn check_ty(from_ty: Ty<'_>, to_ty: Ty<'_>) -> bool {
+        match (from_ty.kind(), to_ty.kind()) {
+            (&ty::Adt(_, from_subs), &ty::Adt(_, to_subs)) => check_subs(from_subs, to_subs),
+            (&ty::Array(from_ty, _), &ty::Array(to_ty, _)) | (&ty::Slice(from_ty), &ty::Slice(to_ty)) => {
+                check_ty(from_ty, to_ty)
+            },
+            (&ty::Ref(from_region, from_ty, _), &ty::Ref(to_region, to_ty, _)) => {
+                check_region(from_region, to_region) || check_ty(from_ty, to_ty)
+            },
+            (&ty::Tuple(from_tys), &ty::Tuple(to_tys)) => {
+                from_tys.len() != to_tys.len()
+                    || from_tys
+                        .iter()
+                        .zip(to_tys)
+                        .any(|(from_ty, to_ty)| check_ty(from_ty, to_ty))
+            },
+            _ => from_ty.has_late_bound_regions(),
+        }
     }
-    let substs = match closure_ty.kind() {
-        ty::Closure(_, substs) => substs,
-        _ => return false,
-    };
-    let closure_sig = cx.tcx.signature_unclosure(substs.as_closure().sig(), Unsafety::Normal);
-    cx.tcx.erase_late_bound_regions(closure_sig) == cx.tcx.erase_late_bound_regions(call_sig)
+
+    assert!(from_sig.inputs_and_output.len() == to_sig.inputs_and_output.len());
+    from_sig
+        .inputs_and_output
+        .iter()
+        .zip(to_sig.inputs_and_output)
+        .any(|(from_ty, to_ty)| check_ty(from_ty, to_ty))
 }
 
 fn get_ufcs_type_name(cx: &LateContext<'_>, method_def_id: DefId) -> String {

clippy_utils/src/ty.rs

@@ -303,6 +303,11 @@ pub fn is_type_lang_item(cx: &LateContext<'_>, ty: Ty<'_>, lang_item: hir::LangI
     }
 }
 
+/// Gets the diagnostic name of the type, if it has one
+pub fn type_diagnostic_name(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<Symbol> {
+    ty.ty_adt_def().and_then(|adt| cx.tcx.get_diagnostic_name(adt.did()))
+}
+
 /// Return `true` if the passed `typ` is `isize` or `usize`.
 pub fn is_isize_or_usize(typ: Ty<'_>) -> bool {
     matches!(typ.kind(), ty::Int(IntTy::Isize) | ty::Uint(UintTy::Usize))