From 493498b65fbd1a7a5bc9c0c9cfc906083ca5439f Mon Sep 17 00:00:00 2001 From: Alex Macleod Date: Tue, 23 Jul 2024 20:57:35 +0000 Subject: [PATCH] Make `BindInsteadOfMap` a struct Makes it codegen once instead of three times --- clippy.toml | 1 - .../src/methods/bind_instead_of_map.rs | 109 +++++++++++------- clippy_lints/src/methods/mod.rs | 7 +- 3 files changed, 68 insertions(+), 49 deletions(-) diff --git a/clippy.toml b/clippy.toml index 319b72e8c..a7b0cc56e 100644 --- a/clippy.toml +++ b/clippy.toml @@ -8,7 +8,6 @@ reason = "this function does not add a link to our documentation, please use the path = "rustc_lint::context::LintContext::span_lint" reason = "this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint*` functions instead" - [[disallowed-methods]] path = "rustc_middle::ty::context::TyCtxt::node_span_lint" reason = "this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint_hir*` functions instead" diff --git a/clippy_lints/src/methods/bind_instead_of_map.rs b/clippy_lints/src/methods/bind_instead_of_map.rs index fb440ce65..20f3722e1 100644 --- a/clippy_lints/src/methods/bind_instead_of_map.rs +++ b/clippy_lints/src/methods/bind_instead_of_map.rs @@ -10,59 +10,80 @@ use rustc_hir::{LangItem, QPath}; use rustc_lint::LateContext; use rustc_span::Span; -pub(crate) struct OptionAndThenSome; - -impl BindInsteadOfMap for OptionAndThenSome { - const VARIANT_LANG_ITEM: LangItem = LangItem::OptionSome; - const BAD_METHOD_NAME: &'static str = "and_then"; - const GOOD_METHOD_NAME: &'static str = "map"; +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) } -pub(crate) struct ResultAndThenOk; - -impl BindInsteadOfMap for ResultAndThenOk { - const VARIANT_LANG_ITEM: LangItem = LangItem::ResultOk; - const BAD_METHOD_NAME: &'static str = "and_then"; - const GOOD_METHOD_NAME: &'static str = "map"; +pub(super) fn check_and_then_ok( + cx: &LateContext<'_>, + expr: &hir::Expr<'_>, + recv: &hir::Expr<'_>, + arg: &hir::Expr<'_>, +) -> bool { + BindInsteadOfMap { + variant_lang_item: LangItem::ResultOk, + bad_method_name: "and_then", + good_method_name: "map", + } + .check(cx, expr, recv, arg) } -pub(crate) struct ResultOrElseErrInfo; - -impl BindInsteadOfMap for ResultOrElseErrInfo { - const VARIANT_LANG_ITEM: LangItem = LangItem::ResultErr; - const BAD_METHOD_NAME: &'static str = "or_else"; - const GOOD_METHOD_NAME: &'static str = "map_err"; +pub(super) fn check_or_else_err( + cx: &LateContext<'_>, + expr: &hir::Expr<'_>, + recv: &hir::Expr<'_>, + arg: &hir::Expr<'_>, +) -> bool { + BindInsteadOfMap { + variant_lang_item: LangItem::ResultErr, + bad_method_name: "or_else", + good_method_name: "map_err", + } + .check(cx, expr, recv, arg) } -pub(crate) trait BindInsteadOfMap { - const VARIANT_LANG_ITEM: LangItem; - const BAD_METHOD_NAME: &'static str; - const GOOD_METHOD_NAME: &'static str; +struct BindInsteadOfMap { + variant_lang_item: LangItem, + bad_method_name: &'static str, + good_method_name: &'static str, +} - fn no_op_msg(cx: &LateContext<'_>) -> Option { - let variant_id = cx.tcx.lang_items().get(Self::VARIANT_LANG_ITEM)?; +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, + self.bad_method_name, cx.tcx.item_name(variant_id), )) } - fn lint_msg(cx: &LateContext<'_>) -> Option { - let variant_id = cx.tcx.lang_items().get(Self::VARIANT_LANG_ITEM)?; + 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!( "using `{}.{}(|x| {}(y))`, which is more succinctly expressed as `{}(|x| y)`", cx.tcx.item_name(item_id), - Self::BAD_METHOD_NAME, + self.bad_method_name, cx.tcx.item_name(variant_id), - Self::GOOD_METHOD_NAME + self.good_method_name, )) } fn lint_closure_autofixable( + &self, cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, @@ -71,9 +92,9 @@ pub(crate) trait BindInsteadOfMap { ) -> 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) + && self.is_variant(cx, path.res) && !contains_return(inner_expr) - && let Some(msg) = Self::lint_msg(cx) + && 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; @@ -82,7 +103,7 @@ pub(crate) trait BindInsteadOfMap { let option_snip = snippet(cx, recv.span, ".."); let note = format!( "{option_snip}.{}({closure_args_snip} {some_inner_snip})", - Self::GOOD_METHOD_NAME + self.good_method_name ); span_lint_and_sugg(cx, BIND_INSTEAD_OF_MAP, expr.span, msg, "try", note, app); true @@ -91,13 +112,13 @@ pub(crate) trait BindInsteadOfMap { } } - fn lint_closure(cx: &LateContext<'_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>) -> bool { + fn lint_closure(&self, cx: &LateContext<'_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>) -> bool { let mut suggs = Vec::new(); let can_sugg: bool = find_all_ret_expressions(cx, closure_expr, |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_variant(cx, path.res) && !contains_return(arg) { suggs.push((ret_expr.span, arg.span.source_callsite())); @@ -108,7 +129,7 @@ pub(crate) trait BindInsteadOfMap { }); let (span, msg) = if can_sugg && let hir::ExprKind::MethodCall(segment, ..) = expr.kind - && let Some(msg) = Self::lint_msg(cx) + && let Some(msg) = self.lint_msg(cx) { (segment.ident.span, msg) } else { @@ -119,7 +140,7 @@ pub(crate) trait BindInsteadOfMap { diag, "try", Applicability::MachineApplicable, - std::iter::once((span, Self::GOOD_METHOD_NAME.into())).chain( + std::iter::once((span, self.good_method_name.into())).chain( suggs .into_iter() .map(|(span1, span2)| (span1, snippet(cx, span2, "_").into())), @@ -130,9 +151,9 @@ pub(crate) trait BindInsteadOfMap { } /// Lint use of `_.and_then(|x| Some(y))` for `Option`s - fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>) -> bool { + 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) + && let Some(vid) = cx.tcx.lang_items().get(self.variant_lang_item) && adt.did() == cx.tcx.parent(vid) { } else { @@ -144,15 +165,15 @@ pub(crate) trait BindInsteadOfMap { let closure_body = cx.tcx.hir().body(body); let closure_expr = peel_blocks(closure_body.value); - if Self::lint_closure_autofixable(cx, expr, recv, closure_expr, fn_decl_span) { + 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(cx, expr, closure_expr) } }, // `_.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) { + 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, @@ -169,9 +190,9 @@ pub(crate) trait BindInsteadOfMap { } } - fn is_variant(cx: &LateContext<'_>, res: Res) -> bool { + fn is_variant(&self, cx: &LateContext<'_>, res: Res) -> bool { if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res { - if let Some(variant_id) = cx.tcx.lang_items().get(Self::VARIANT_LANG_ITEM) { + if let Some(variant_id) = cx.tcx.lang_items().get(self.variant_lang_item) { return cx.tcx.parent(id) == variant_id; } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 12a3a36e8..aa4cad934 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -131,7 +131,6 @@ mod waker_clone_wake; mod wrong_self_convention; mod zst_offset; -use bind_instead_of_map::BindInsteadOfMap; use clippy_config::msrvs::{self, Msrv}; use clippy_config::Conf; use clippy_utils::consts::{constant, Constant}; @@ -4506,8 +4505,8 @@ impl Methods { } }, ("and_then", [arg]) => { - let biom_option_linted = bind_instead_of_map::OptionAndThenSome::check(cx, expr, recv, arg); - let biom_result_linted = bind_instead_of_map::ResultAndThenOk::check(cx, expr, recv, arg); + 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); if !biom_option_linted && !biom_result_linted { unnecessary_lazy_eval::check(cx, expr, recv, arg, "and"); } @@ -4847,7 +4846,7 @@ impl Methods { open_options::check(cx, expr, recv); }, ("or_else", [arg]) => { - if !bind_instead_of_map::ResultOrElseErrInfo::check(cx, expr, recv, arg) { + if !bind_instead_of_map::check_or_else_err(cx, expr, recv, arg) { unnecessary_lazy_eval::check(cx, expr, recv, arg, "or"); } },