diff --git a/CHANGELOG.md b/CHANGELOG.md index 41c334c68169..f7916511edfe 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 000000000000..a527f51b1fdc --- /dev/null +++ b/clippy_lints/src/if_then_some_else_none.rs @@ -0,0 +1,115 @@ +use crate::utils; +use if_chain::if_chain; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_semver::RustcVersion; +use rustc_session::{declare_tool_lint, impl_lint_pass}; + +const IF_THEN_SOME_ELSE_NONE_MSRV: RustcVersion = RustcVersion::new(1, 50, 0); + +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`" +} + +pub struct IfThenSomeElseNone { + msrv: Option, +} + +impl IfThenSomeElseNone { + #[must_use] + pub fn new(msrv: Option) -> Self { + Self { msrv } + } +} + +impl_lint_pass!(IfThenSomeElseNone => [IF_THEN_SOME_ELSE_NONE]); + +impl LateLintPass<'_> for IfThenSomeElseNone { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'tcx Expr<'_>) { + if !utils::meets_msrv(self.msrv.as_ref(), &IF_THEN_SOME_ELSE_NONE_MSRV) { + return; + } + + 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 cond_snip = utils::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 = utils::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, + ); + utils::span_lint_and_help( + cx, + IF_THEN_SOME_ELSE_NONE, + expr.span, + "this could be simplified with `bool::then`", + None, + &help, + ); + } + } + } + + extract_msrv_attr!(LateContext); +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8259fd3c320b..2e4c1e3bdf8e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -230,6 +230,7 @@ mod identity_op; 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(move || box if_then_some_else_none::IfThenSomeElseNone::new(msrv)); 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 000000000000..54789bf93209 --- /dev/null +++ b/tests/ui/if_then_some_else_none.rs @@ -0,0 +1,104 @@ +#![warn(clippy::if_then_some_else_none)] +#![feature(custom_inner_attributes)] + +fn main() { + // Should issue an error. + let _ = if foo() { + println!("true!"); + Some("foo") + } else { + None + }; + + // Should issue an error when macros are used. + let _ = if matches!(true, true) { + println!("true!"); + Some(matches!(true, false)) + } else { + None + }; + + // Should issue an error. Binary expression `o < 32` should be parenthesized. + let x = Some(5); + let _ = x.and_then(|o| if o < 32 { Some(o) } else { None }); + + // Should issue an error. Unary expression `!x` should be parenthesized. + let x = true; + let _ = if !x { Some(0) } 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 _msrv_1_49() { + #![clippy::msrv = "1.49"] + // `bool::then` was stabilized in 1.50. Do not lint this + let _ = if foo() { + println!("true!"); + Some(149) + } else { + None + }; +} + +fn _msrv_1_50() { + #![clippy::msrv = "1.50"] + let _ = if foo() { + println!("true!"); + Some(150) + } 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 000000000000..8cb22d569f4c --- /dev/null +++ b/tests/ui/if_then_some_else_none.stderr @@ -0,0 +1,61 @@ +error: this could be simplified with `bool::then` + --> $DIR/if_then_some_else_none.rs:6:13 + | +LL | let _ = if foo() { + | _____________^ +LL | | println!("true!"); +LL | | Some("foo") +LL | | } else { +LL | | None +LL | | }; + | |_____^ + | + = note: `-D clippy::if-then-some-else-none` implied by `-D warnings` + = help: consider using `bool::then` like: `foo().then(|| { /* snippet */ "foo" })` + +error: this could be simplified with `bool::then` + --> $DIR/if_then_some_else_none.rs:14:13 + | +LL | let _ = if matches!(true, true) { + | _____________^ +LL | | println!("true!"); +LL | | Some(matches!(true, false)) +LL | | } else { +LL | | None +LL | | }; + | |_____^ + | + = help: consider using `bool::then` like: `matches!(true, true).then(|| { /* snippet */ matches!(true, false) })` + +error: this could be simplified with `bool::then` + --> $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)` + +error: this could be simplified with `bool::then` + --> $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)` + +error: this could be simplified with `bool::then` + --> $DIR/if_then_some_else_none.rs:82:13 + | +LL | let _ = if foo() { + | _____________^ +LL | | println!("true!"); +LL | | Some(150) +LL | | } else { +LL | | None +LL | | }; + | |_____^ + | + = help: consider using `bool::then` like: `foo().then(|| { /* snippet */ 150 })` + +error: aborting due to 5 previous errors +