From a672d335a248034369cd41d4e08e02cf378c0e4b Mon Sep 17 00:00:00 2001 From: Yusuke Tanaka Date: Sun, 7 Mar 2021 02:08:46 +0900 Subject: [PATCH 1/4] 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 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..0bd393f89968 --- /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 8259fd3c320b..9a7dedf416e4 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(|| 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 000000000000..b19e2a50010f --- /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 000000000000..7ad9c4ce79df --- /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 + From f2a85cb42ae64bc5a82eaee49d92b6f9d93153fe Mon Sep 17 00:00:00 2001 From: Yusuke Tanaka Date: Mon, 8 Mar 2021 22:52:03 +0900 Subject: [PATCH 2/4] Set 1.50 as msrv of if_then_some_else_none --- clippy_lints/src/if_then_some_else_none.rs | 24 ++++++++++++++++++++-- clippy_lints/src/lib.rs | 2 +- tests/ui/if_then_some_else_none.rs | 22 ++++++++++++++++++++ tests/ui/if_then_some_else_none.stderr | 16 +++++++++++++-- 4 files changed, 59 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/if_then_some_else_none.rs b/clippy_lints/src/if_then_some_else_none.rs index 0bd393f89968..aadadd0d934a 100644 --- a/clippy_lints/src/if_then_some_else_none.rs +++ b/clippy_lints/src/if_then_some_else_none.rs @@ -4,7 +4,10 @@ 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}; +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`. @@ -39,10 +42,25 @@ declare_clippy_lint! { "Finds if-else that could be written using `bool::then`" } -declare_lint_pass!(IfThenSomeElseNone => [IF_THEN_SOME_ELSE_NONE]); +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; } @@ -85,4 +103,6 @@ impl LateLintPass<'_> for IfThenSomeElseNone { } } } + + extract_msrv_attr!(LateContext); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9a7dedf416e4..2e4c1e3bdf8e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1284,7 +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_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), diff --git a/tests/ui/if_then_some_else_none.rs b/tests/ui/if_then_some_else_none.rs index b19e2a50010f..337292fd9b3d 100644 --- a/tests/ui/if_then_some_else_none.rs +++ b/tests/ui/if_then_some_else_none.rs @@ -1,4 +1,5 @@ #![warn(clippy::if_then_some_else_none)] +#![feature(custom_inner_attributes)] fn main() { // Should issue an error. @@ -49,6 +50,27 @@ fn main() { 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("foo") + } else { + None + }; +} + +fn _msrv_1_50() { + #![clippy::msrv = "1.50"] + let _ = if foo() { + println!("true!"); + Some("foo") + } else { + None + }; +} + fn foo() -> bool { unimplemented!() } diff --git a/tests/ui/if_then_some_else_none.stderr b/tests/ui/if_then_some_else_none.stderr index 7ad9c4ce79df..19c96f900a30 100644 --- a/tests/ui/if_then_some_else_none.stderr +++ b/tests/ui/if_then_some_else_none.stderr @@ -1,5 +1,5 @@ error: this could be simplified with `bool::then` - --> $DIR/if_then_some_else_none.rs:5:13 + --> $DIR/if_then_some_else_none.rs:6:13 | LL | let _ = if foo() { | _____________^ @@ -12,5 +12,17 @@ LL | | }; | = note: `-D clippy::if-then-some-else-none` implied by `-D warnings` -error: aborting due to previous error +error: this could be simplified with `bool::then` + --> $DIR/if_then_some_else_none.rs:66:13 + | +LL | let _ = if foo() { + | _____________^ +LL | | println!("true!"); +LL | | Some("foo") +LL | | } else { +LL | | None +LL | | }; + | |_____^ help: try this: `foo().then(|| { /* snippet */ "foo" })` + +error: aborting due to 2 previous errors From 0327c2e04106e258949dbf96e58bb582e2960c6b Mon Sep 17 00:00:00 2001 From: Yusuke Tanaka Date: Mon, 8 Mar 2021 23:19:57 +0900 Subject: [PATCH 3/4] Output help instead of suggestion in `if_then_some_else_none` diagnose --- clippy_lints/src/if_then_some_else_none.rs | 17 +++++++---------- tests/ui/if_then_some_else_none.rs | 4 ++-- tests/ui/if_then_some_else_none.stderr | 9 ++++++--- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/if_then_some_else_none.rs b/clippy_lints/src/if_then_some_else_none.rs index aadadd0d934a..569a7f06f95b 100644 --- a/clippy_lints/src/if_then_some_else_none.rs +++ b/clippy_lints/src/if_then_some_else_none.rs @@ -1,6 +1,5 @@ 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; @@ -83,22 +82,20 @@ impl LateLintPass<'_> for IfThenSomeElseNone { 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 */ {} }})", + let cond_snip = utils::snippet(cx, cond.span, "[condition]"); + let arg_snip = utils::snippet(cx, then_arg.span, ""); + let help = format!( + "consider using `bool::then` like: `{}.then(|| {{ /* snippet */ {} }})`", cond_snip, arg_snip, ); - utils::span_lint_and_sugg( + utils::span_lint_and_help( cx, IF_THEN_SOME_ELSE_NONE, expr.span, "this could be simplified with `bool::then`", - "try this", - sugg, - applicability, + None, + &help, ); } } diff --git a/tests/ui/if_then_some_else_none.rs b/tests/ui/if_then_some_else_none.rs index 337292fd9b3d..14a5fe76245a 100644 --- a/tests/ui/if_then_some_else_none.rs +++ b/tests/ui/if_then_some_else_none.rs @@ -55,7 +55,7 @@ fn _msrv_1_49() { // `bool::then` was stabilized in 1.50. Do not lint this let _ = if foo() { println!("true!"); - Some("foo") + Some(149) } else { None }; @@ -65,7 +65,7 @@ fn _msrv_1_50() { #![clippy::msrv = "1.50"] let _ = if foo() { println!("true!"); - Some("foo") + Some(150) } else { None }; diff --git a/tests/ui/if_then_some_else_none.stderr b/tests/ui/if_then_some_else_none.stderr index 19c96f900a30..722c52b1cb43 100644 --- a/tests/ui/if_then_some_else_none.stderr +++ b/tests/ui/if_then_some_else_none.stderr @@ -8,9 +8,10 @@ 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` + = 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:66:13 @@ -18,11 +19,13 @@ error: this could be simplified with `bool::then` LL | let _ = if foo() { | _____________^ LL | | println!("true!"); -LL | | Some("foo") +LL | | Some(150) LL | | } else { LL | | None LL | | }; - | |_____^ help: try this: `foo().then(|| { /* snippet */ "foo" })` + | |_____^ + | + = help: consider using `bool::then` like: `foo().then(|| { /* snippet */ 150 })` error: aborting due to 2 previous errors From 11d2af7e9694e8e63000e33f77d4926e936e3822 Mon Sep 17 00:00:00 2001 From: Yusuke Tanaka Date: Fri, 12 Mar 2021 20:46:46 +0900 Subject: [PATCH 4/4] Improve suggestion and make it work for macros --- clippy_lints/src/if_then_some_else_none.rs | 18 +++++++++--- tests/ui/if_then_some_else_none.rs | 16 ++++++++++ tests/ui/if_then_some_else_none.stderr | 34 ++++++++++++++++++++-- 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/if_then_some_else_none.rs b/clippy_lints/src/if_then_some_else_none.rs index 569a7f06f95b..a527f51b1fdc 100644 --- a/clippy_lints/src/if_then_some_else_none.rs +++ b/clippy_lints/src/if_then_some_else_none.rs @@ -82,12 +82,22 @@ impl LateLintPass<'_> for IfThenSomeElseNone { 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(cx, cond.span, "[condition]"); - let arg_snip = utils::snippet(cx, then_arg.span, ""); + 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(|| {{ /* snippet */ {} }})`", + "consider using `bool::then` like: `{}.then(|| {})`", cond_snip, - arg_snip, + closure_body, ); utils::span_lint_and_help( cx, diff --git a/tests/ui/if_then_some_else_none.rs b/tests/ui/if_then_some_else_none.rs index 14a5fe76245a..54789bf93209 100644 --- a/tests/ui/if_then_some_else_none.rs +++ b/tests/ui/if_then_some_else_none.rs @@ -10,6 +10,22 @@ fn main() { 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!"); diff --git a/tests/ui/if_then_some_else_none.stderr b/tests/ui/if_then_some_else_none.stderr index 722c52b1cb43..8cb22d569f4c 100644 --- a/tests/ui/if_then_some_else_none.stderr +++ b/tests/ui/if_then_some_else_none.stderr @@ -14,7 +14,37 @@ LL | | }; = 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:66:13 + --> $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() { | _____________^ @@ -27,5 +57,5 @@ LL | | }; | = help: consider using `bool::then` like: `foo().then(|| { /* snippet */ 150 })` -error: aborting due to 2 previous errors +error: aborting due to 5 previous errors