diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 41099f94b044..5ac82dc2df4e 100644 --- a/clippy_config/src/conf.rs +++ b/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, diff --git a/clippy_lints/src/methods/bind_instead_of_map.rs b/clippy_lints/src/methods/bind_instead_of_map.rs index f8520c23ea50..b42ac60c3e2f 100644 --- a/clippy_lints/src/methods/bind_instead_of_map.rs +++ b/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 { - 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 { - 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 } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index b39aec6e521c..468f5ded304f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -5146,8 +5146,15 @@ impl Methods { }, (sym::and_then, [arg]) => { manual_option_zip::check(cx, expr, recv, arg, self.msrv); - let biom_option_linted = bind_instead_of_map::check_and_then_some(cx, expr, recv, arg); - let biom_result_linted = bind_instead_of_map::check_and_then_ok(cx, expr, recv, arg); + let site = bind_instead_of_map::CheckSite { + cx, + expr: expr.span, + recv, + method_name: span, + arg, + }; + let biom_option_linted = bind_instead_of_map::check_and_then_some(site); + let biom_result_linted = bind_instead_of_map::check_and_then_ok(site); if !biom_option_linted && !biom_result_linted { let ule_and_linted = unnecessary_lazy_eval::check(cx, expr, recv, arg, "and"); if !ule_and_linted { @@ -5285,6 +5292,24 @@ impl Methods { string_extend_chars::check(cx, expr, recv, arg); extend_with_drain::check(cx, expr, recv, arg); }, + (bad_method_name @ (sym::fetch_update | sym::try_update), [_, _, arg]) + if self.msrv.meets(cx, msrvs::ATOMIC_TRY_UPDATE) => + { + bind_instead_of_map::check_fetch_update( + match bad_method_name { + sym::fetch_update => "fetch_update", + sym::try_update => "try_update", + _ => unreachable!(), + }, + bind_instead_of_map::CheckSite { + cx, + expr: expr.span, + recv, + method_name: span, + arg, + }, + ); + }, (sym::filter, [arg]) => { if let Some((sym::cloned, recv2, [], _span2, _)) = method_call(recv) { // if `arg` has side-effect, the semantic will change @@ -5542,10 +5567,15 @@ impl Methods { (sym::open, [_]) => { open_options::check(cx, expr, recv); }, - (sym::or_else, [arg]) => - { + (sym::or_else, [arg]) => { #[expect(clippy::collapsible_match)] - if !bind_instead_of_map::check_or_else_err(cx, expr, recv, arg) { + if !bind_instead_of_map::check_or_else_err(bind_instead_of_map::CheckSite { + cx, + expr: expr.span, + recv, + method_name: span, + arg, + }) { unnecessary_lazy_eval::check(cx, expr, recv, arg, "or"); } }, diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs index af5498c353d4..a2f41d7baea4 100644 --- a/clippy_lints/src/methods/unnecessary_to_owned.rs +++ b/clippy_lints/src/methods/unnecessary_to_owned.rs @@ -513,8 +513,9 @@ fn can_change_type<'a>(cx: &LateContext<'a>, mut expr: &'a Expr<'a>, mut ty: Ty< let mut count = 0; return find_all_ret_expressions(cx, body_expr, |_| { count += 1; - count <= 1 - }); + if count <= 1 { Ok(()) } else { Err(()) } + }) + .is_ok(); } }, Node::Expr(parent_expr) => { diff --git a/clippy_lints/src/unnecessary_wraps.rs b/clippy_lints/src/unnecessary_wraps.rs index 17e05db644b0..8c29445d67ef 100644 --- a/clippy_lints/src/unnecessary_wraps.rs +++ b/clippy_lints/src/unnecessary_wraps.rs @@ -135,13 +135,13 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { snippet(cx, arg.span.source_callsite(), "..").to_string() }, )); - true + Ok(()) } else { - false + Err(()) } }); - if can_sugg && !suggs.is_empty() { + if can_sugg.is_ok() && !suggs.is_empty() { let (lint_msg, return_type_sugg_msg, return_type_sugg, body_sugg_msg) = if inner_type.is_unit() { ( "this function's return value is unnecessary".to_string(), diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 4f9a064bf7a6..71bc6eabc371 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -23,6 +23,7 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { + 1,95,0 { ATOMIC_TRY_UPDATE } 1,93,0 { VEC_DEQUE_POP_BACK_IF, VEC_DEQUE_POP_FRONT_IF } 1,91,0 { DURATION_FROM_MINUTES_HOURS } 1,88,0 { LET_CHAINS } diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs index 6ac979d595f4..de5d4ec0c583 100644 --- a/clippy_utils/src/visitors.rs +++ b/clippy_utils/src/visitors.rs @@ -204,13 +204,19 @@ fn contains_try(expr: &Expr<'_>) -> bool { .is_some() } -pub fn find_all_ret_expressions<'hir, F>(_cx: &LateContext<'_>, expr: &'hir Expr<'hir>, callback: F) -> bool -where - F: FnMut(&'hir Expr<'hir>) -> bool, -{ +pub enum FindAllRetError { + ContainsTry, + CallbackFailed, +} + +pub fn find_all_ret_expressions<'hir>( + _cx: &LateContext<'_>, + expr: &'hir Expr<'hir>, + callback: impl FnMut(&'hir Expr<'hir>) -> Result<(), ()>, +) -> Result<(), FindAllRetError> { struct RetFinder { in_stmt: bool, - failed: bool, + status: Result<(), ()>, cb: F, } @@ -249,22 +255,19 @@ where } } - impl<'hir, F: FnMut(&'hir Expr<'hir>) -> bool> Visitor<'hir> for RetFinder { + impl<'hir, F: FnMut(&'hir Expr<'hir>) -> Result<(), ()>> Visitor<'hir> for RetFinder { fn visit_stmt(&mut self, stmt: &'hir Stmt<'_>) { intravisit::walk_stmt(&mut *self.inside_stmt(true), stmt); } fn visit_expr(&mut self, expr: &'hir Expr<'_>) { - if self.failed { - return; - } - if self.in_stmt { - match expr.kind { + match self.status { + Err(()) => {}, + Ok(()) if self.in_stmt => match expr.kind { ExprKind::Ret(Some(expr)) => self.inside_stmt(false).visit_expr(expr), _ => walk_expr(self, expr), - } - } else { - match expr.kind { + }, + Ok(()) => match expr.kind { ExprKind::If(cond, then, else_opt) => { self.inside_stmt(true).visit_expr(cond); self.visit_expr(then); @@ -280,20 +283,22 @@ where }, ExprKind::Block(..) => walk_expr(self, expr), ExprKind::Ret(Some(expr)) => self.visit_expr(expr), - _ => self.failed |= !(self.cb)(expr), - } + _ => self.status = (self.cb)(expr), + }, } } } - !contains_try(expr) && { + if contains_try(expr) { + Err(FindAllRetError::ContainsTry) + } else { let mut ret_finder = RetFinder { in_stmt: false, - failed: false, + status: Ok(()), cb: callback, }; ret_finder.visit_expr(expr); - !ret_finder.failed + ret_finder.status.map_err(|()| FindAllRetError::CallbackFailed) } } diff --git a/tests/ui/bind_instead_of_map_fetch_update.rs b/tests/ui/bind_instead_of_map_fetch_update.rs new file mode 100644 index 000000000000..876e6a6e5315 --- /dev/null +++ b/tests/ui/bind_instead_of_map_fetch_update.rs @@ -0,0 +1,41 @@ +#![deny(clippy::bind_instead_of_map)] +#![allow(unused_must_use)] +#![allow( + deprecated, + reason = "`fetch_update` will be a deprecated alias to `try_update` starting in 1.99, + but we still want to lint both" +)] + +use std::sync::atomic::*; + +#[clippy::msrv = "1.94"] +fn msrv_1_94() { + let x = AtomicBool::new(true); + x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, |old| Some(!old)); + x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, Some); + let x = AtomicUsize::new(0); + x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, |old| { + if old == 0 { Some(0) } else { Some(old - 1) } + }); +} + +#[clippy::msrv = "1.95"] +fn msrv_1_95() { + let x = AtomicBool::new(true); + x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, |old| Some(!old)); + x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, Some); + x.try_update(Ordering::Relaxed, Ordering::SeqCst, |old| Some(!old)); + x.try_update(Ordering::Relaxed, Ordering::SeqCst, Some); + let x = AtomicUsize::new(0); + x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, |old| { + if old == 0 { Some(0) } else { Some(old - 1) } + }); + x.try_update(Ordering::Relaxed, Ordering::SeqCst, |old| { + if old == 0 { Some(0) } else { Some(old - 1) } + }); +} + +fn main() { + msrv_1_94(); + msrv_1_95(); +}