diff --git a/CHANGELOG.md b/CHANGELOG.md index 87bffe7f74d6..c19547a9752a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5704,6 +5704,7 @@ Released 2018-09-13 [`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next [`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization [`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive +[`stacked_ifs`]: https://rust-lang.github.io/rust-clippy/master/index.html#stacked_ifs [`std_instead_of_alloc`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_alloc [`std_instead_of_core`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_core [`str_split_at_newline`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_split_at_newline diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index eba048ad90be..c6d03c6acca6 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -648,6 +648,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT_INFO, crate::size_of_ref::SIZE_OF_REF_INFO, crate::slow_vector_initialization::SLOW_VECTOR_INITIALIZATION_INFO, + crate::stacked_ifs::STACKED_IFS_INFO, crate::std_instead_of_core::ALLOC_INSTEAD_OF_CORE_INFO, crate::std_instead_of_core::STD_INSTEAD_OF_ALLOC_INFO, crate::std_instead_of_core::STD_INSTEAD_OF_CORE_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 169e51e2cb93..a0ed0739d28f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -326,6 +326,7 @@ mod single_range_in_vec_init; mod size_of_in_element_count; mod size_of_ref; mod slow_vector_initialization; +mod stacked_ifs; mod std_instead_of_core; mod strings; mod strlen_on_c_strings; @@ -1129,6 +1130,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(zero_repeat_side_effects::ZeroRepeatSideEffects)); store.register_late_pass(|_| Box::new(manual_unwrap_or_default::ManualUnwrapOrDefault)); store.register_late_pass(|_| Box::new(integer_division_remainder_used::IntegerDivisionRemainderUsed)); + store.register_late_pass(|_| Box::new(stacked_ifs::StackedIfs)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/stacked_ifs.rs b/clippy_lints/src/stacked_ifs.rs new file mode 100644 index 000000000000..9f3e7f621b51 --- /dev/null +++ b/clippy_lints/src/stacked_ifs.rs @@ -0,0 +1,94 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Detects `if` statements where the condition is another `if` statement. + /// ### Why is this bad? + /// This makes code hard to read. + /// ### Example + /// ```no_run + /// let a = 3; + /// let b = 4; + /// let c = 5; + /// if if a == b { + /// 4 + /// } else { + /// 5 + /// } == c { + /// // Do something. + /// } + /// ``` + /// Use instead: + /// ```no_run + /// let a = 3; + /// let b = 4; + /// let c = 5; + /// let value = if a == b { + /// 4 + /// } else { + /// 5 + /// }; + /// if value == c { + /// // Do something. + /// } + /// ``` + #[clippy::version = "1.79.0"] + pub STACKED_IFS, + style, + "finds if statements with another if statement as the condition" +} + +declare_lint_pass!(StackedIfs => [STACKED_IFS]); + +impl LateLintPass<'_> for StackedIfs { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + // Ensure that the expression isn't a part of a constant or macro expansion. + if !expr.span.from_expansion() { + stacked_ifs(cx, expr); + } + } +} + +fn stacked_ifs(cx: &LateContext<'_>, expr: &Expr<'_>) { + // Check for if statements where the condition is another if statement. + let ExprKind::If(condition, _, _) = expr.kind else { + return; + }; + + let condition = condition.peel_drop_temps(); + + if condition.span.from_expansion() { + return; + } + + if let ExprKind::If(..) = condition.kind { + emit_lint(cx, condition); + } + + if let ExprKind::Binary(_, lhs, rhs) = condition.kind { + if let ExprKind::If(..) = lhs.kind + && !lhs.span.from_expansion() + { + emit_lint(cx, lhs); + } + if let ExprKind::If(..) = rhs.kind + && !rhs.span.from_expansion() + { + emit_lint(cx, rhs); + } + } +} + +fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>) { + span_lint_and_help( + cx, + STACKED_IFS, + expr.span, + "stacked `if` found", + None, + "avoid using an `if` statement as a condition for another `if` statement", + ); +} diff --git a/tests/ui/stacked_ifs.rs b/tests/ui/stacked_ifs.rs new file mode 100644 index 000000000000..2cab4e28b73f --- /dev/null +++ b/tests/ui/stacked_ifs.rs @@ -0,0 +1,92 @@ +#![warn(clippy::stacked_ifs)] +#![allow(clippy::manual_unwrap_or_default)] + +macro_rules! m { + () => { + let a = true; + let b = false; + if if a { b } else { true } { + println!("True"); + } + }; +} + +fn main() { + let a = 3; + let b = 4; + if a == b { // No error + // Do something. + } else { + // Do something else. + } + + if if a % 2 == 0 { + //~^ ERROR: stacked `if` found + a - 1 + } else { + a + 1 + } == b + { + // Do something. + } + + if if a % 2 == 0 { + //~^ ERROR: stacked `if` found + a - 1 + } else { + a + 1 + } == b + { + // Do something. + } else { + // Do something else. + } + + if b == if a % 2 == 0 { + //~^ ERROR: stacked `if` found + a - 1 + } else { + a + 1 + } { + // Do something. + } else { + // Do something else. + } + + let c = 5; + let d = 6; + + // More complex statements (example from issue 12483): + if if if a == b { + //~^ ERROR: stacked `if` found + //~^^ ERROR: stacked `if` found + b == c + } else { + a == c + } { + a == d + } else { + c == d + } { + println!("True!"); + } else { + println!("False!"); + } + + let a = true; + let b = false; + + if if a { b } else { true } { + //~^ ERROR: stacked `if` found + // Do something. + } + + let x = Some(10); + + if if let Some(num) = x { num } else { 0 } == 0 { + //~^ ERROR: stacked `if` found + // Do something. + } + + m!(); // No error +} diff --git a/tests/ui/stacked_ifs.stderr b/tests/ui/stacked_ifs.stderr new file mode 100644 index 000000000000..a035daf81372 --- /dev/null +++ b/tests/ui/stacked_ifs.stderr @@ -0,0 +1,92 @@ +error: stacked `if` found + --> tests/ui/stacked_ifs.rs:23:8 + | +LL | if if a % 2 == 0 { + | ________^ +LL | | +LL | | a - 1 +LL | | } else { +LL | | a + 1 +LL | | } == b + | |_____^ + | + = help: avoid using an `if` statement as a condition for another `if` statement + = note: `-D clippy::stacked-ifs` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::stacked_ifs)]` + +error: stacked `if` found + --> tests/ui/stacked_ifs.rs:33:8 + | +LL | if if a % 2 == 0 { + | ________^ +LL | | +LL | | a - 1 +LL | | } else { +LL | | a + 1 +LL | | } == b + | |_____^ + | + = help: avoid using an `if` statement as a condition for another `if` statement + +error: stacked `if` found + --> tests/ui/stacked_ifs.rs:45:13 + | +LL | if b == if a % 2 == 0 { + | _____________^ +LL | | +LL | | a - 1 +LL | | } else { +LL | | a + 1 +LL | | } { + | |_____^ + | + = help: avoid using an `if` statement as a condition for another `if` statement + +error: stacked `if` found + --> tests/ui/stacked_ifs.rs:60:8 + | +LL | if if if a == b { + | ________^ +LL | | +LL | | +LL | | b == c +... | +LL | | c == d +LL | | } { + | |_____^ + | + = help: avoid using an `if` statement as a condition for another `if` statement + +error: stacked `if` found + --> tests/ui/stacked_ifs.rs:60:11 + | +LL | if if if a == b { + | ___________^ +LL | | +LL | | +LL | | b == c +LL | | } else { +LL | | a == c +LL | | } { + | |_____^ + | + = help: avoid using an `if` statement as a condition for another `if` statement + +error: stacked `if` found + --> tests/ui/stacked_ifs.rs:79:8 + | +LL | if if a { b } else { true } { + | ^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: avoid using an `if` statement as a condition for another `if` statement + +error: stacked `if` found + --> tests/ui/stacked_ifs.rs:86:8 + | +LL | if if let Some(num) = x { num } else { 0 } == 0 { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: avoid using an `if` statement as a condition for another `if` statement + +error: aborting due to 7 previous errors +