[bind_instead_of_map]: also lint on AtomicT::fetch_update / AtomicT::try_update

clippy_config/src/conf.rs

@@ -758,6 +758,7 @@ define_Conf! {
         almost_complete_range,
         approx_constant,
         assigning_clones,
+        bind_instead_of_map,
         borrow_as_ptr,
         cast_abs_to_unsigned,
         checked_conversions,

clippy_lints/src/methods/bind_instead_of_map.rs

@@ -1,202 +1,222 @@
 use super::{BIND_INSTEAD_OF_MAP, contains_return};
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
-use clippy_utils::peel_blocks;
+use clippy_utils::res::MaybeDef;
 use clippy_utils::source::{snippet, snippet_with_context};
 use clippy_utils::visitors::find_all_ret_expressions;
+use clippy_utils::{peel_blocks, sym};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
+use rustc_hir::def_id::DefId;
 use rustc_hir::{LangItem, QPath};
 use rustc_lint::LateContext;
+use rustc_middle::ty::TyCtxt;
 use rustc_span::Span;
 
-pub(super) fn check_and_then_some(
-    cx: &LateContext<'_>,
-    expr: &hir::Expr<'_>,
-    recv: &hir::Expr<'_>,
-    arg: &hir::Expr<'_>,
-) -> bool {
-    BindInsteadOfMap {
-        variant_lang_item: LangItem::OptionSome,
-        bad_method_name: "and_then",
-        good_method_name: "map",
-    }
-    .check(cx, expr, recv, arg)
+#[derive(Clone, Copy)]
+pub(super) struct CheckSite<'a, 'ctx> {
+    pub cx: &'a LateContext<'ctx>,
+    pub expr: Span,
+    pub recv: &'ctx hir::Expr<'ctx>,
+    pub method_name: Span,
+    pub arg: &'ctx hir::Expr<'ctx>,
+}
+
+pub(super) fn check_and_then_some(site: CheckSite<'_, '_>) -> bool {
+    check(site, "and_then", LangItem::OptionSome, "map")
+}
+
+pub(super) fn check_and_then_ok(site: CheckSite<'_, '_>) -> bool {
+    check(site, "and_then", LangItem::ResultOk, "map")
 }
 
-pub(super) fn check_and_then_ok(
-    cx: &LateContext<'_>,
-    expr: &hir::Expr<'_>,
-    recv: &hir::Expr<'_>,
-    arg: &hir::Expr<'_>,
+pub(super) fn check_or_else_err(site: CheckSite<'_, '_>) -> bool {
+    check(site, "or_else", LangItem::ResultErr, "map_err")
+}
+
+pub(super) fn check_fetch_update(
+    bad_method_name: &'static str,
+    CheckSite {
+        cx,
+        expr,
+        recv,
+        method_name,
+        arg,
+    }: CheckSite<'_, '_>,
 ) -> bool {
-    BindInsteadOfMap {
-        variant_lang_item: LangItem::ResultOk,
-        bad_method_name: "and_then",
-        good_method_name: "map",
+    let rec_t = cx.typeck_results().expr_ty_adjusted(recv).peel_refs();
+    if dbg!(dbg!(rec_t).is_diag_item(cx, sym::Atomic))
+        && let Some(wrapper_variant) = cx.tcx.lang_items().get(LangItem::OptionSome)
+    {
+        BindInsteadOfMap {
+            receiver_ty: rec_t.ty_adt_def().unwrap().did(),
+            bad_method_name,
+            wrapper_variant,
+            good_method_name: "update",
+        }
+        .check(cx, expr, recv, method_name, arg)
+    } else {
+        false
     }
-    .check(cx, expr, recv, arg)
 }
 
-pub(super) fn check_or_else_err(
-    cx: &LateContext<'_>,
-    expr: &hir::Expr<'_>,
-    recv: &hir::Expr<'_>,
-    arg: &hir::Expr<'_>,
+fn check(
+    CheckSite {
+        cx,
+        expr,
+        recv,
+        method_name,
+        arg,
+    }: CheckSite<'_, '_>,
+    bad_method_name: &'static str,
+    wrapper_variant: LangItem,
+    good_method_name: &'static str,
 ) -> bool {
-    BindInsteadOfMap {
-        variant_lang_item: LangItem::ResultErr,
-        bad_method_name: "or_else",
-        good_method_name: "map_err",
+    if let Some(wrapper_variant) = cx.tcx.lang_items().get(wrapper_variant)
+        && let receiver_ty = cx.tcx.parent(wrapper_variant)
+        && let Some(adt) = cx.typeck_results().expr_ty(recv).ty_adt_def()
+        && adt.did() == receiver_ty
+    {
+        BindInsteadOfMap {
+            receiver_ty,
+            bad_method_name,
+            wrapper_variant,
+            good_method_name,
+        }
+        .check(cx, expr, recv, method_name, arg)
+    } else {
+        false
     }
-    .check(cx, expr, recv, arg)
 }
 
 struct BindInsteadOfMap {
-    variant_lang_item: LangItem,
+    receiver_ty: DefId,
     bad_method_name: &'static str,
+    wrapper_variant: DefId,
     good_method_name: &'static str,
 }
 
 impl BindInsteadOfMap {
-    fn no_op_msg(&self, cx: &LateContext<'_>) -> Option<String> {
-        let variant_id = cx.tcx.lang_items().get(self.variant_lang_item)?;
-        let item_id = cx.tcx.parent(variant_id);
-        Some(format!(
-            "using `{}.{}({})`, which is a no-op",
-            cx.tcx.item_name(item_id),
-            self.bad_method_name,
-            cx.tcx.item_name(variant_id),
-        ))
-    }
-
-    fn lint_msg(&self, cx: &LateContext<'_>) -> Option<String> {
-        let variant_id = cx.tcx.lang_items().get(self.variant_lang_item)?;
-        let item_id = cx.tcx.parent(variant_id);
-        Some(format!(
+    fn lint_msg(&self, tcx: TyCtxt<'_>) -> String {
+        format!(
             "using `{}.{}(|x| {}(y))`, which is more succinctly expressed as `{}(|x| y)`",
-            cx.tcx.item_name(item_id),
+            tcx.item_name(self.receiver_ty),
             self.bad_method_name,
-            cx.tcx.item_name(variant_id),
+            tcx.item_name(self.wrapper_variant),
             self.good_method_name,
-        ))
+        )
     }
 
-    fn lint_closure_autofixable(
+    // Fast path for when the closure is `|x| Some(..)` / `|x| Ok(..)` / `|x| Err(..)`.
+    fn lint_closure_simple(
         &self,
         cx: &LateContext<'_>,
-        expr: &hir::Expr<'_>,
-        recv: &hir::Expr<'_>,
-        closure_expr: &hir::Expr<'_>,
+        expr: Span,
+        recv: Span,
+        closure_body: &hir::Expr<'_>,
         closure_args_span: Span,
     ) -> bool {
-        if let hir::ExprKind::Call(some_expr, [inner_expr]) = closure_expr.kind
-            && let hir::ExprKind::Path(QPath::Resolved(_, path)) = some_expr.kind
-            && self.is_variant(cx, path.res)
+        if let hir::ExprKind::Call(callee, [inner_expr]) = closure_body.kind
+            && let hir::ExprKind::Path(QPath::Resolved(_, path)) = callee.kind
+            && self.is_wrapper_variant_ctor(cx, path.res)
             && !contains_return(inner_expr)
-            && let Some(msg) = self.lint_msg(cx)
         {
-            let mut app = Applicability::MachineApplicable;
-            let some_inner_snip = snippet_with_context(cx, inner_expr.span, closure_expr.span.ctxt(), "_", &mut app).0;
+            let msg = self.lint_msg(cx.tcx);
+            let mut applicability = Applicability::MachineApplicable;
+            let (some_inner_snip, _is_macro_call) =
+                snippet_with_context(cx, inner_expr.span, closure_body.span.ctxt(), "_", &mut applicability);
 
             let closure_args_snip = snippet(cx, closure_args_span, "..");
-            let option_snip = snippet(cx, recv.span, "..");
-            let note = format!(
+            let option_snip = snippet(cx, recv, "..");
+            let sugg = format!(
                 "{option_snip}.{}({closure_args_snip} {some_inner_snip})",
                 self.good_method_name
             );
-            span_lint_and_sugg(cx, BIND_INSTEAD_OF_MAP, expr.span, msg, "try", note, app);
+            span_lint_and_sugg(cx, BIND_INSTEAD_OF_MAP, expr, msg, "try", sugg, applicability);
             true
         } else {
             false
         }
     }
 
-    fn lint_closure(&self, cx: &LateContext<'_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>) -> bool {
+    fn lint_closure(&self, cx: &LateContext<'_>, expr: Span, method_name: Span, closure_body: &hir::Expr<'_>) -> bool {
         let mut suggs = Vec::new();
-        let can_sugg: bool = find_all_ret_expressions(cx, closure_expr, |ret_expr| {
+
+        if find_all_ret_expressions(cx, closure_body, |ret_expr| {
             if !ret_expr.span.from_expansion()
                 && let hir::ExprKind::Call(func_path, [arg]) = ret_expr.kind
                 && let hir::ExprKind::Path(QPath::Resolved(_, path)) = func_path.kind
-                && self.is_variant(cx, path.res)
+                && self.is_wrapper_variant_ctor(cx, path.res)
                 && !contains_return(arg)
             {
                 suggs.push((ret_expr.span, arg.span.source_callsite()));
-                true
+                Ok(())
             } else {
-                false
+                Err(())
             }
-        });
-        let (span, msg) = if can_sugg
-            && let hir::ExprKind::MethodCall(segment, ..) = expr.kind
-            && let Some(msg) = self.lint_msg(cx)
-        {
-            (segment.ident.span, msg)
-        } else {
-            return false;
-        };
-        span_lint_and_then(cx, BIND_INSTEAD_OF_MAP, expr.span, msg, |diag| {
-            diag.multipart_suggestion(
-                format!("use `{}` instead", self.good_method_name),
-                std::iter::once((span, self.good_method_name.into()))
-                    .chain(
-                        suggs
-                            .into_iter()
-                            .map(|(span1, span2)| (span1, snippet(cx, span2, "_").into())),
-                    )
-                    .collect(),
-                Applicability::MachineApplicable,
-            );
-        });
-        true
-    }
-
-    /// Lint use of `_.and_then(|x| Some(y))` for `Option`s
-    fn check(&self, cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>) -> bool {
-        if let Some(adt) = cx.typeck_results().expr_ty(recv).ty_adt_def()
-            && let Some(vid) = cx.tcx.lang_items().get(self.variant_lang_item)
-            && adt.did() == cx.tcx.parent(vid)
+        })
+        .is_ok()
         {
+            span_lint_and_then(cx, BIND_INSTEAD_OF_MAP, expr, self.lint_msg(cx.tcx), |diag| {
+                diag.multipart_suggestion(
+                    format!("use `{}` instead", self.good_method_name),
+                    std::iter::once((method_name, self.good_method_name.into()))
+                        .chain(
+                            suggs
+                                .into_iter()
+                                .map(|(span1, span2)| (span1, snippet(cx, span2, "_").into())),
+                        )
+                        .collect(),
+                    Applicability::MachineApplicable,
+                );
+            });
+            true
         } else {
-            return false;
+            false
         }
+    }
 
+    fn check(
+        &self,
+        cx: &LateContext<'_>,
+        expr: Span,
+        recv: &hir::Expr<'_>,
+        method_name: Span,
+        arg: &hir::Expr<'_>,
+    ) -> bool {
         match arg.kind {
             hir::ExprKind::Closure(&hir::Closure { body, fn_decl_span, .. }) => {
-                let closure_body = cx.tcx.hir_body(body);
-                let closure_expr = peel_blocks(closure_body.value);
+                let closure_body = peel_blocks(cx.tcx.hir_body(body).value);
 
-                if self.lint_closure_autofixable(cx, expr, recv, closure_expr, fn_decl_span) {
-                    true
-                } else {
-                    self.lint_closure(cx, expr, closure_expr)
-                }
+                self.lint_closure_simple(cx, expr, recv.span, closure_body, fn_decl_span)
+                    || self.lint_closure(cx, expr, method_name, closure_body)
             },
-            // `_.and_then(Some)` case, which is no-op.
-            hir::ExprKind::Path(QPath::Resolved(_, path)) if self.is_variant(cx, path.res) => {
-                if let Some(msg) = self.no_op_msg(cx) {
-                    span_lint_and_sugg(
-                        cx,
-                        BIND_INSTEAD_OF_MAP,
-                        expr.span,
-                        msg,
-                        "use the expression directly",
-                        snippet(cx, recv.span, "..").into(),
-                        Applicability::MachineApplicable,
-                    );
-                }
+            // `.and_then(Some)` / `.and_then(Ok)` / `.or_else(Err)`, which is no-op
+            hir::ExprKind::Path(QPath::Resolved(_, path)) if self.is_wrapper_variant_ctor(cx, path.res) => {
+                span_lint_and_sugg(
+                    cx,
+                    BIND_INSTEAD_OF_MAP,
+                    expr,
+                    format!(
+                        "using `{}.{}({})`, which is a no-op",
+                        cx.tcx.item_name(self.receiver_ty),
+                        self.bad_method_name,
+                        cx.tcx.item_name(self.wrapper_variant),
+                    ),
+                    "use the expression directly",
+                    snippet(cx, recv.span, "..").into(),
+                    Applicability::MachineApplicable,
+                );
                 true
             },
             _ => false,
         }
     }
 
-    fn is_variant(&self, cx: &LateContext<'_>, res: Res) -> bool {
-        if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res
-            && let Some(variant_id) = cx.tcx.lang_items().get(self.variant_lang_item)
-        {
-            return cx.tcx.parent(id) == variant_id;
-        }
-        false
+    fn is_wrapper_variant_ctor(&self, cx: &LateContext<'_>, res: Res) -> bool {
+        let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res else {
+            return false;
+        };
+        cx.tcx.parent(id) == self.wrapper_variant
     }
 }