unnecessary_option_map_or_else also uses is_expr_identity_function

Author: teofr

clippy_lints/src/matches/match_single_binding.rs

@@ -82,7 +82,7 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e
                         snippet_with_context(cx, pat_span, ctxt, "..", &mut app).0
                     ),
                 ),
-                None if is_expr_identity_of_pat(cx, arms[0].pat, ex, false) => {
+                None if is_expr_identity_of_pat(cx, arms[0].pat, ex, cx.enclosing_body.unwrap(), false) => {
                     span_lint_and_sugg(
                         cx,
                         MATCH_SINGLE_BINDING,

clippy_lints/src/methods/unnecessary_option_map_or_else.rs

@@ -1,15 +1,13 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_expr_identity_function;
 use clippy_utils::res::MaybeDef;
 use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::{expr_or_init, find_binding_init, peel_blocks};
 use rustc_errors::Applicability;
-use rustc_hir::def::{DefKind, Res};
-use rustc_hir::{Body, BodyId, Closure, Expr, ExprKind, HirId, QPath};
+use rustc_hir::Expr;
 use rustc_lint::LateContext;
 use rustc_span::symbol::sym;
 
 use super::UNNECESSARY_OPTION_MAP_OR_ELSE;
-use super::utils::get_last_chain_binding_hir_id;
 
 fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>) {
     let msg = "unused \"map closure\" when calling `Option::map_or_else` value";
@@ -27,85 +25,13 @@ fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &E
     );
 }
 
-fn handle_qpath(
-    cx: &LateContext<'_>,
-    expr: &Expr<'_>,
-    recv: &Expr<'_>,
-    def_arg: &Expr<'_>,
-    expected_hir_id: HirId,
-    qpath: QPath<'_>,
-) {
-    if let QPath::Resolved(_, path) = qpath
-        && let Res::Local(hir_id) = path.res
-        && expected_hir_id == hir_id
-    {
-        emit_lint(cx, expr, recv, def_arg);
-    }
-}
-
-fn handle_closure(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, body_id: BodyId) {
-    let body = cx.tcx.hir_body(body_id);
-    handle_fn_body(cx, expr, recv, def_arg, body);
-}
-
-fn handle_fn_body(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, body: &Body<'_>) {
-    if let Some(first_param) = body.params.first() {
-        let body_expr = peel_blocks(body.value);
-        match body_expr.kind {
-            ExprKind::Path(qpath) => {
-                handle_qpath(cx, expr, recv, def_arg, first_param.pat.hir_id, qpath);
-            },
-            // If this is a block (that wasn't peeled off), then it means there are statements.
-            ExprKind::Block(block, _) => {
-                if let Some(block_expr) = block.expr
-                        // First we ensure that this is a "binding chain" (each statement is a binding
-                        // of the previous one) and that it is a binding of the closure argument.
-                        && let Some(last_chain_binding_id) =
-                            get_last_chain_binding_hir_id(first_param.pat.hir_id, block.stmts)
-                        && let ExprKind::Path(qpath) = block_expr.kind
-                {
-                    handle_qpath(cx, expr, recv, def_arg, last_chain_binding_id, qpath);
-                }
-            },
-            _ => {},
-        }
-    }
-}
-
 /// lint use of `_.map_or_else(|err| err, |n| n)` for `Option`s.
 pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, map_arg: &Expr<'_>) {
     // lint if the caller of `map_or_else()` is an `Option`
     if !cx.typeck_results().expr_ty(recv).is_diag_item(cx, sym::Option) {
         return;
     }
-    match map_arg.kind {
-        // If the second argument is a closure, we can check its body.
-        ExprKind::Closure(&Closure { body, .. }) => {
-            handle_closure(cx, expr, recv, def_arg, body);
-        },
-        ExprKind::Path(qpath) => {
-            let res = cx.qpath_res(&qpath, map_arg.hir_id);
-            match res {
-                // Case 1: Local variable (could be a closure)
-                Res::Local(hir_id) => {
-                    if let Some(init_expr) = find_binding_init(cx, hir_id) {
-                        let origin = expr_or_init(cx, init_expr);
-                        if let ExprKind::Closure(&Closure { body, .. }) = origin.kind {
-                            handle_closure(cx, expr, recv, def_arg, body);
-                        }
-                    }
-                },
-                // Case 2: Function definition
-                Res::Def(DefKind::Fn, def_id) => {
-                    if let Some(local_def_id) = def_id.as_local()
-                        && let Some(body) = cx.tcx.hir_maybe_body_owned_by(local_def_id)
-                    {
-                        handle_fn_body(cx, expr, recv, def_arg, body);
-                    }
-                },
-                _ => (),
-            }
-        },
-        _ => (),
+    if is_expr_identity_function(cx, map_arg) {
+        emit_lint(cx, expr, recv, def_arg);
     }
 }

clippy_utils/src/lib.rs

@@ -103,11 +103,11 @@ use rustc_hir::definitions::{DefPath, DefPathData};
 use rustc_hir::hir_id::{HirIdMap, HirIdSet};
 use rustc_hir::intravisit::{Visitor, walk_expr};
 use rustc_hir::{
-    self as hir, Arm, BindingMode, Block, BlockCheckMode, Body, ByRef, Closure, ConstArgKind, CoroutineDesugaring,
-    CoroutineKind, CoroutineSource, Destination, Expr, ExprField, ExprKind, FnDecl, FnRetTy, GenericArg, GenericArgs,
-    HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, LetStmt, MatchSource, Mutability, Node, OwnerId,
-    OwnerNode, Param, Pat, PatExpr, PatExprKind, PatKind, Path, PathSegment, QPath, Stmt, StmtKind, TraitFn, TraitItem,
-    TraitItemKind, TraitRef, TyKind, UnOp, def, find_attr,
+    self as hir, Arm, BindingMode, Block, BlockCheckMode, Body, BodyId, ByRef, Closure, ConstArgKind,
+    CoroutineDesugaring, CoroutineKind, CoroutineSource, Destination, Expr, ExprField, ExprKind, FnDecl, FnRetTy,
+    GenericArg, GenericArgs, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, LetStmt, MatchSource,
+    Mutability, Node, OwnerId, OwnerNode, Param, Pat, PatExpr, PatExprKind, PatKind, Path, PathSegment, QPath, Stmt,
+    StmtKind, TraitFn, TraitItem, TraitItemKind, TraitRef, TyKind, UnOp, def, find_attr,
 };
 use rustc_lexer::{FrontmatterAllowed, TokenKind, tokenize};
 use rustc_lint::{LateContext, Level, Lint, LintContext};
@@ -1824,7 +1824,7 @@ fn is_body_identity_function(cx: &LateContext<'_>, func: &Body<'_>) -> bool {
                 for stmt in stmts {
                     if let StmtKind::Let(local) = stmt.kind
                         && let Some(init) = local.init
-                        && is_expr_identity_of_pat(cx, param_pat, init, true)
+                        && is_expr_identity_of_pat(cx, param_pat, init, func.id(), true)
                     {
                         param_pat = local.pat;
                     } else {
@@ -1833,7 +1833,7 @@ fn is_body_identity_function(cx: &LateContext<'_>, func: &Body<'_>) -> bool {
                 }
                 expr = e;
             },
-            _ => return is_expr_identity_of_pat(cx, param_pat, expr, true),
+            _ => return is_expr_identity_of_pat(cx, param_pat, expr, func.id(), true),
         }
     }
 }
@@ -1847,9 +1847,16 @@ fn is_body_identity_function(cx: &LateContext<'_>, func: &Body<'_>) -> bool {
 ///
 /// Note that `by_hir` is used to determine bindings are checked by their `HirId` or by their name.
 /// This can be useful when checking patterns in `let` bindings or `match` arms.
-pub fn is_expr_identity_of_pat(cx: &LateContext<'_>, pat: &Pat<'_>, expr: &Expr<'_>, by_hir: bool) -> bool {
+pub fn is_expr_identity_of_pat(
+    cx: &LateContext<'_>,
+    pat: &Pat<'_>,
+    expr: &Expr<'_>,
+    func_id: BodyId,
+    by_hir: bool,
+) -> bool {
     if cx
-        .typeck_results()
+        .tcx
+        .typeck_body(func_id)
         .pat_binding_modes()
         .get(pat.hir_id)
         .is_some_and(|mode| matches!(mode.0, ByRef::Yes(_)))
@@ -1861,22 +1868,25 @@ pub fn is_expr_identity_of_pat(cx: &LateContext<'_>, pat: &Pat<'_>, expr: &Expr<
     }
 
     // NOTE: we're inside a (function) body, so this won't ICE
-    let qpath_res = |qpath, hir| cx.typeck_results().qpath_res(qpath, hir);
+    let qpath_res = |qpath, hir| cx.tcx.typeck_body(func_id).qpath_res(qpath, hir);
 
     match (pat.kind, expr.kind) {
         (PatKind::Binding(_, id, _, _), _) if by_hir => {
-            expr.res_local_id() == Some(id) && cx.typeck_results().expr_adjustments(expr).is_empty()
+            expr.res_local_id() == Some(id) && cx.tcx.typeck_body(func_id).expr_adjustments(expr).is_empty()
         },
         (PatKind::Binding(_, _, ident, _), ExprKind::Path(QPath::Resolved(_, path))) => {
             matches!(path.segments, [ segment] if segment.ident.name == ident.name)
         },
         (PatKind::Tuple(pats, dotdot), ExprKind::Tup(tup))
             if dotdot.as_opt_usize().is_none() && pats.len() == tup.len() =>
         {
-            over(pats, tup, |pat, expr| is_expr_identity_of_pat(cx, pat, expr, by_hir))
+            over(pats, tup, |pat, expr| {
+                is_expr_identity_of_pat(cx, pat, expr, func_id, by_hir)
+            })
         },
         (PatKind::Slice(before, None, after), ExprKind::Array(arr)) if before.len() + after.len() == arr.len() => {
-            zip(before.iter().chain(after), arr).all(|(pat, expr)| is_expr_identity_of_pat(cx, pat, expr, by_hir))
+            zip(before.iter().chain(after), arr)
+                .all(|(pat, expr)| is_expr_identity_of_pat(cx, pat, expr, func_id, by_hir))
         },
         (PatKind::TupleStruct(pat_ident, field_pats, dotdot), ExprKind::Call(ident, fields))
             if dotdot.as_opt_usize().is_none() && field_pats.len() == fields.len() =>
@@ -1885,7 +1895,7 @@ pub fn is_expr_identity_of_pat(cx: &LateContext<'_>, pat: &Pat<'_>, expr: &Expr<
             if let ExprKind::Path(ident) = &ident.kind
                 && qpath_res(&pat_ident, pat.hir_id) == qpath_res(ident, expr.hir_id)
                 // check fields
-                && over(field_pats, fields, |pat, expr| is_expr_identity_of_pat(cx, pat, expr,by_hir))
+                && over(field_pats, fields, |pat, expr| is_expr_identity_of_pat(cx, pat, expr,func_id, by_hir))
             {
                 true
             } else {
@@ -1899,7 +1909,8 @@ pub fn is_expr_identity_of_pat(cx: &LateContext<'_>, pat: &Pat<'_>, expr: &Expr<
             qpath_res(&pat_ident, pat.hir_id) == qpath_res(ident, expr.hir_id)
                 // check fields
                 && unordered_over(field_pats, fields, |field_pat, field| {
-                    field_pat.ident == field.ident && is_expr_identity_of_pat(cx, field_pat.pat, field.expr, by_hir)
+                    field_pat.ident == field.ident &&
+                    is_expr_identity_of_pat(cx, field_pat.pat, field.expr, func_id, by_hir)
                 })
         },
         _ => false,
@@ -1938,7 +1949,38 @@ pub fn is_expr_untyped_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>)
 pub fn is_expr_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     match expr.kind {
         ExprKind::Closure(&Closure { body, .. }) => is_body_identity_function(cx, cx.tcx.hir_body(body)),
-        _ => expr.basic_res().is_diag_item(cx, sym::convert_identity),
+        _ if expr.basic_res().is_diag_item(cx, sym::convert_identity) => true,
+
+        ExprKind::Path(qpath) => {
+            let res = cx.qpath_res(&qpath, expr.hir_id);
+            match res {
+                // Case 1: Local variable (could be a closure)
+                Res::Local(hir_id) => {
+                    if let Some(init_expr) = find_binding_init(cx, hir_id) {
+                        let origin = expr_or_init(cx, init_expr);
+                        if let ExprKind::Closure(&Closure { body, .. }) = origin.kind {
+                            is_body_identity_function(cx, cx.tcx.hir_body(body))
+                        } else {
+                            false
+                        }
+                    } else {
+                        false
+                    }
+                },
+                // Case 2: Function definition
+                Res::Def(DefKind::Fn, def_id) => {
+                    if let Some(local_def_id) = def_id.as_local()
+                        && let Some(body) = cx.tcx.hir_maybe_body_owned_by(local_def_id)
+                    {
+                        is_body_identity_function(cx, body)
+                    } else {
+                        false
+                    }
+                },
+                _ => false,
+            }
+        },
+        _ => false,
     }
 }
 

tests/ui/eta.fixed

@@ -13,7 +13,8 @@
     clippy::uninlined_format_args,
     clippy::useless_vec,
     clippy::unnecessary_map_on_constructor,
-    clippy::needless_lifetimes
+    clippy::needless_lifetimes,
+    clippy::unnecessary_option_map_or_else
 )]
 
 use std::path::{Path, PathBuf};

tests/ui/eta.rs

@@ -13,7 +13,8 @@
     clippy::uninlined_format_args,
     clippy::useless_vec,
     clippy::unnecessary_map_on_constructor,
-    clippy::needless_lifetimes
+    clippy::needless_lifetimes,
+    clippy::unnecessary_option_map_or_else
 )]
 
 use std::path::{Path, PathBuf};