From 80f0f280df446d5b4169d1cf1314b030f52c02c1 Mon Sep 17 00:00:00 2001 From: Michael Krasnitski Date: Wed, 3 Aug 2022 23:06:12 -0400 Subject: [PATCH 1/2] Extend `if_then_some_else_none` to also suggest `bool::then_some` --- clippy_lints/src/if_then_some_else_none.rs | 82 ++++++++++++---------- tests/ui/if_then_some_else_none.stderr | 8 +-- 2 files changed, 47 insertions(+), 43 deletions(-) diff --git a/clippy_lints/src/if_then_some_else_none.rs b/clippy_lints/src/if_then_some_else_none.rs index b8d227855d9..20fcba90773 100644 --- a/clippy_lints/src/if_then_some_else_none.rs +++ b/clippy_lints/src/if_then_some_else_none.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::eager_or_lazy::switch_to_eager_eval; use clippy_utils::source::snippet_with_macro_callsite; use clippy_utils::{contains_return, higher, is_else_clause, is_lang_ctor, meets_msrv, msrvs, peel_blocks}; -use if_chain::if_chain; use rustc_hir::LangItem::{OptionNone, OptionSome}; use rustc_hir::{Expr, ExprKind, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; @@ -56,7 +56,7 @@ impl IfThenSomeElseNone { impl_lint_pass!(IfThenSomeElseNone => [IF_THEN_SOME_ELSE_NONE]); impl<'tcx> LateLintPass<'tcx> for IfThenSomeElseNone { - fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'tcx Expr<'_>) { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { if !meets_msrv(self.msrv, msrvs::BOOL_THEN) { return; } @@ -70,43 +70,47 @@ impl<'tcx> LateLintPass<'tcx> for IfThenSomeElseNone { return; } - if_chain! { - if let Some(higher::If { cond, then, r#else: Some(els) }) = higher::If::hir(expr); - if let ExprKind::Block(then_block, _) = then.kind; - if let Some(then_expr) = then_block.expr; - if let ExprKind::Call(then_call, [then_arg]) = then_expr.kind; - if let ExprKind::Path(ref then_call_qpath) = then_call.kind; - if is_lang_ctor(cx, then_call_qpath, OptionSome); - if let ExprKind::Path(ref qpath) = peel_blocks(els).kind; - if is_lang_ctor(cx, qpath, OptionNone); - if !stmts_contains_early_return(then_block.stmts); - then { - let cond_snip = snippet_with_macro_callsite(cx, cond.span, "[condition]"); - let cond_snip = if matches!(cond.kind, ExprKind::Unary(_, _) | ExprKind::Binary(_, _, _)) { - format!("({})", cond_snip) - } else { - cond_snip.into_owned() - }; - let arg_snip = snippet_with_macro_callsite(cx, then_arg.span, ""); - let closure_body = if then_block.stmts.is_empty() { - arg_snip.into_owned() - } else { - format!("{{ /* snippet */ {} }}", arg_snip) - }; - let help = format!( - "consider using `bool::then` like: `{}.then(|| {})`", - cond_snip, - closure_body, - ); - span_lint_and_help( - cx, - IF_THEN_SOME_ELSE_NONE, - expr.span, - "this could be simplified with `bool::then`", - None, - &help, - ); - } + if let Some(higher::If { cond, then, r#else: Some(els) }) = higher::If::hir(expr) + && let ExprKind::Block(then_block, _) = then.kind + && let Some(then_expr) = then_block.expr + && let ExprKind::Call(then_call, [then_arg]) = then_expr.kind + && let ExprKind::Path(ref then_call_qpath) = then_call.kind + && is_lang_ctor(cx, then_call_qpath, OptionSome) + && let ExprKind::Path(ref qpath) = peel_blocks(els).kind + && is_lang_ctor(cx, qpath, OptionNone) + && !stmts_contains_early_return(then_block.stmts) + { + let cond_snip = snippet_with_macro_callsite(cx, cond.span, "[condition]"); + let cond_snip = if matches!(cond.kind, ExprKind::Unary(_, _) | ExprKind::Binary(_, _, _)) { + format!("({})", cond_snip) + } else { + cond_snip.into_owned() + }; + let arg_snip = snippet_with_macro_callsite(cx, then_arg.span, ""); + let mut method_body = if then_block.stmts.is_empty() { + arg_snip.into_owned() + } else { + format!("{{ /* snippet */ {} }}", arg_snip) + }; + let method_name = if switch_to_eager_eval(cx, expr) && meets_msrv(self.msrv, msrvs::BOOL_THEN_SOME) { + "then_some" + } else { + method_body.insert_str(0, "|| "); + "then" + }; + + let help = format!( + "consider using `bool::{}` like: `{}.{}({})`", + method_name, cond_snip, method_name, method_body, + ); + span_lint_and_help( + cx, + IF_THEN_SOME_ELSE_NONE, + expr.span, + &format!("this could be simplified with `bool::{}`", method_name), + None, + &help, + ); } } diff --git a/tests/ui/if_then_some_else_none.stderr b/tests/ui/if_then_some_else_none.stderr index 8cb22d569f4..c22ace30d2d 100644 --- a/tests/ui/if_then_some_else_none.stderr +++ b/tests/ui/if_then_some_else_none.stderr @@ -27,21 +27,21 @@ LL | | }; | = help: consider using `bool::then` like: `matches!(true, true).then(|| { /* snippet */ matches!(true, false) })` -error: this could be simplified with `bool::then` +error: this could be simplified with `bool::then_some` --> $DIR/if_then_some_else_none.rs:23:28 | LL | let _ = x.and_then(|o| if o < 32 { Some(o) } else { None }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: consider using `bool::then` like: `(o < 32).then(|| o)` + = help: consider using `bool::then_some` like: `(o < 32).then_some(o)` -error: this could be simplified with `bool::then` +error: this could be simplified with `bool::then_some` --> $DIR/if_then_some_else_none.rs:27:13 | LL | let _ = if !x { Some(0) } else { None }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: consider using `bool::then` like: `(!x).then(|| 0)` + = help: consider using `bool::then_some` like: `(!x).then_some(0)` error: this could be simplified with `bool::then` --> $DIR/if_then_some_else_none.rs:82:13 From f7f60b82e8d9ce281f67cef8b95019482ff47298 Mon Sep 17 00:00:00 2001 From: Michael Krasnitski Date: Thu, 11 Aug 2022 22:41:25 -0400 Subject: [PATCH 2/2] Adjust lint description for better clarity --- clippy_lints/src/if_then_some_else_none.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/if_then_some_else_none.rs b/clippy_lints/src/if_then_some_else_none.rs index 20fcba90773..11c43247868 100644 --- a/clippy_lints/src/if_then_some_else_none.rs +++ b/clippy_lints/src/if_then_some_else_none.rs @@ -11,10 +11,12 @@ use rustc_session::{declare_tool_lint, impl_lint_pass}; declare_clippy_lint! { /// ### What it does - /// Checks for if-else that could be written to `bool::then`. + /// Checks for if-else that could be written using either `bool::then` or `bool::then_some`. /// /// ### Why is this bad? - /// Looks a little redundant. Using `bool::then` helps it have less lines of code. + /// Looks a little redundant. Using `bool::then` is more concise and incurs no loss of clarity. + /// For simple calculations and known values, use `bool::then_some`, which is eagerly evaluated + /// in comparison to `bool::then`. /// /// ### Example /// ```rust @@ -39,7 +41,7 @@ declare_clippy_lint! { #[clippy::version = "1.53.0"] pub IF_THEN_SOME_ELSE_NONE, restriction, - "Finds if-else that could be written using `bool::then`" + "Finds if-else that could be written using either `bool::then` or `bool::then_some`" } pub struct IfThenSomeElseNone {