From a672d335a248034369cd41d4e08e02cf378c0e4b Mon Sep 17 00:00:00 2001 From: Yusuke Tanaka Date: Sun, 7 Mar 2021 02:08:46 +0900 Subject: [PATCH] Implement new lint: if_then_some_else_none --- CHANGELOG.md | 1 + clippy_lints/src/if_then_some_else_none.rs | 88 ++++++++++++++++++++++ clippy_lints/src/lib.rs | 4 + tests/ui/if_then_some_else_none.rs | 66 ++++++++++++++++ tests/ui/if_then_some_else_none.stderr | 16 ++++ 5 files changed, 175 insertions(+) create mode 100644 clippy_lints/src/if_then_some_else_none.rs create mode 100644 tests/ui/if_then_some_else_none.rs create mode 100644 tests/ui/if_then_some_else_none.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 41c334c6816..f7916511edf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2103,6 +2103,7 @@ Released 2018-09-13 [`if_let_some_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_some_result [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else +[`if_then_some_else_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_then_some_else_none [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond [`implicit_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_clone [`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher diff --git a/clippy_lints/src/if_then_some_else_none.rs b/clippy_lints/src/if_then_some_else_none.rs new file mode 100644 index 00000000000..0bd393f8996 --- /dev/null +++ b/clippy_lints/src/if_then_some_else_none.rs @@ -0,0 +1,88 @@ +use crate::utils; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Checks for if-else that could be written to `bool::then`. + /// + /// **Why is this bad?** Looks a little redundant. Using `bool::then` helps it have less lines of code. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// # let v = vec![0]; + /// let a = if v.is_empty() { + /// println!("true!"); + /// Some(42) + /// } else { + /// None + /// }; + /// ``` + /// + /// Could be written: + /// + /// ```rust + /// # let v = vec![0]; + /// let a = v.is_empty().then(|| { + /// println!("true!"); + /// 42 + /// }); + /// ``` + pub IF_THEN_SOME_ELSE_NONE, + restriction, + "Finds if-else that could be written using `bool::then`" +} + +declare_lint_pass!(IfThenSomeElseNone => [IF_THEN_SOME_ELSE_NONE]); + +impl LateLintPass<'_> for IfThenSomeElseNone { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'tcx Expr<'_>) { + if in_external_macro(cx.sess(), expr.span) { + return; + } + + // We only care about the top-most `if` in the chain + if utils::parent_node_is_if_expr(expr, cx) { + return; + } + + if_chain! { + if let ExprKind::If(ref cond, ref then, Some(ref els)) = expr.kind; + if let ExprKind::Block(ref then_block, _) = then.kind; + if let Some(ref then_expr) = then_block.expr; + if let ExprKind::Call(ref then_call, [then_arg]) = then_expr.kind; + if let ExprKind::Path(ref then_call_qpath) = then_call.kind; + if utils::match_qpath(then_call_qpath, &utils::paths::OPTION_SOME); + if let ExprKind::Block(ref els_block, _) = els.kind; + if els_block.stmts.is_empty(); + if let Some(ref els_expr) = els_block.expr; + if let ExprKind::Path(ref els_call_qpath) = els_expr.kind; + if utils::match_qpath(els_call_qpath, &utils::paths::OPTION_NONE); + then { + let mut applicability = Applicability::MachineApplicable; + let cond_snip = utils::snippet_with_applicability(cx, cond.span, "[condition]", &mut applicability); + let arg_snip = utils::snippet_with_applicability(cx, then_arg.span, "", &mut applicability); + let sugg = format!( + "{}.then(|| {{ /* snippet */ {} }})", + cond_snip, + arg_snip, + ); + utils::span_lint_and_sugg( + cx, + IF_THEN_SOME_ELSE_NONE, + expr.span, + "this could be simplified with `bool::then`", + "try this", + sugg, + applicability, + ); + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8259fd3c320..9a7dedf416e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -230,6 +230,7 @@ macro_rules! extract_msrv_attr { mod if_let_mutex; mod if_let_some_result; mod if_not_else; +mod if_then_some_else_none; mod implicit_return; mod implicit_saturating_sub; mod inconsistent_struct_constructor; @@ -667,6 +668,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &if_let_mutex::IF_LET_MUTEX, &if_let_some_result::IF_LET_SOME_RESULT, &if_not_else::IF_NOT_ELSE, + &if_then_some_else_none::IF_THEN_SOME_ELSE_NONE, &implicit_return::IMPLICIT_RETURN, &implicit_saturating_sub::IMPLICIT_SATURATING_SUB, &inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR, @@ -1282,6 +1284,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box redundant_slicing::RedundantSlicing); store.register_late_pass(|| box from_str_radix_10::FromStrRadix10); store.register_late_pass(|| box manual_map::ManualMap); + store.register_late_pass(|| box if_then_some_else_none::IfThenSomeElseNone); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1297,6 +1300,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&exhaustive_items::EXHAUSTIVE_STRUCTS), LintId::of(&exit::EXIT), LintId::of(&float_literal::LOSSY_FLOAT_LITERAL), + LintId::of(&if_then_some_else_none::IF_THEN_SOME_ELSE_NONE), LintId::of(&implicit_return::IMPLICIT_RETURN), LintId::of(&indexing_slicing::INDEXING_SLICING), LintId::of(&inherent_impl::MULTIPLE_INHERENT_IMPL), diff --git a/tests/ui/if_then_some_else_none.rs b/tests/ui/if_then_some_else_none.rs new file mode 100644 index 00000000000..b19e2a50010 --- /dev/null +++ b/tests/ui/if_then_some_else_none.rs @@ -0,0 +1,66 @@ +#![warn(clippy::if_then_some_else_none)] + +fn main() { + // Should issue an error. + let _ = if foo() { + println!("true!"); + Some("foo") + } else { + None + }; + + // Should not issue an error since the `else` block has a statement besides `None`. + let _ = if foo() { + println!("true!"); + Some("foo") + } else { + eprintln!("false..."); + None + }; + + // Should not issue an error since there are more than 2 blocks in the if-else chain. + let _ = if foo() { + println!("foo true!"); + Some("foo") + } else if bar() { + println!("bar true!"); + Some("bar") + } else { + None + }; + + let _ = if foo() { + println!("foo true!"); + Some("foo") + } else { + bar().then(|| { + println!("bar true!"); + "bar" + }) + }; + + // Should not issue an error since the `then` block has `None`, not `Some`. + let _ = if foo() { None } else { Some("foo is false") }; + + // Should not issue an error since the `else` block doesn't use `None` directly. + let _ = if foo() { Some("foo is true") } else { into_none() }; + + // Should not issue an error since the `then` block doesn't use `Some` directly. + let _ = if foo() { into_some("foo") } else { None }; +} + +fn foo() -> bool { + unimplemented!() +} + +fn bar() -> bool { + unimplemented!() +} + +fn into_some(v: T) -> Option { + Some(v) +} + +fn into_none() -> Option { + None +} diff --git a/tests/ui/if_then_some_else_none.stderr b/tests/ui/if_then_some_else_none.stderr new file mode 100644 index 00000000000..7ad9c4ce79d --- /dev/null +++ b/tests/ui/if_then_some_else_none.stderr @@ -0,0 +1,16 @@ +error: this could be simplified with `bool::then` + --> $DIR/if_then_some_else_none.rs:5:13 + | +LL | let _ = if foo() { + | _____________^ +LL | | println!("true!"); +LL | | Some("foo") +LL | | } else { +LL | | None +LL | | }; + | |_____^ help: try this: `foo().then(|| { /* snippet */ "foo" })` + | + = note: `-D clippy::if-then-some-else-none` implied by `-D warnings` + +error: aborting due to previous error +