diff options
| -rw-r--r-- | clippy_lints/src/needless_if.rs | 78 | ||||
| -rw-r--r-- | tests/ui/needless_if.fixed | 65 | ||||
| -rw-r--r-- | tests/ui/needless_if.rs | 56 | ||||
| -rw-r--r-- | tests/ui/needless_if.stderr | 56 |
4 files changed, 161 insertions, 94 deletions
diff --git a/clippy_lints/src/needless_if.rs b/clippy_lints/src/needless_if.rs index dbffbd6f0d3..ad5c3e1dc82 100644 --- a/clippy_lints/src/needless_if.rs +++ b/clippy_lints/src/needless_if.rs @@ -1,9 +1,6 @@ -use clippy_utils::{diagnostics::span_lint_and_sugg, is_from_proc_macro, source::snippet_with_applicability}; +use clippy_utils::{diagnostics::span_lint_and_sugg, higher::If, is_from_proc_macro, source::snippet_opt}; use rustc_errors::Applicability; -use rustc_hir::{ - intravisit::{walk_expr, Visitor}, - Expr, ExprKind, Node, -}; +use rustc_hir::{ExprKind, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -37,67 +34,42 @@ declare_clippy_lint! { declare_lint_pass!(NeedlessIf => [NEEDLESS_IF]); impl LateLintPass<'_> for NeedlessIf { - fn check_expr<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { - if let ExprKind::If(if_expr, block, else_expr) = &expr.kind - && let ExprKind::Block(block, ..) = block.kind + fn check_stmt<'tcx>(&mut self, cx: &LateContext<'tcx>, stmt: &Stmt<'tcx>) { + if let StmtKind::Expr(expr) = stmt.kind + && let Some(If {cond, then, r#else: None }) = If::hir(expr) + && let ExprKind::Block(block, ..) = then.kind && block.stmts.is_empty() && block.expr.is_none() - && else_expr.is_none() && !in_external_macro(cx.sess(), expr.span) + && !is_from_proc_macro(cx, expr) + && let Some(then_snippet) = snippet_opt(cx, then.span) + // Ignore + // - empty macro expansions + // - empty reptitions in macro expansions + // - comments + // - #[cfg]'d out code + && then_snippet.chars().all(|ch| matches!(ch, '{' | '}') || ch.is_ascii_whitespace()) + && let Some(cond_snippet) = snippet_opt(cx, cond.span) { - // Ignore `else if` - if let Some(parent_id) = cx.tcx.hir().opt_parent_id(expr.hir_id) - && let Some(Node::Expr(Expr { - kind: ExprKind::If(_, _, Some(else_expr)), - .. - })) = cx.tcx.hir().find(parent_id) - && else_expr.hir_id == expr.hir_id - { - return; - } - - if is_any_if_let(if_expr) || is_from_proc_macro(cx, expr) { - return; - } - - let mut app = Applicability::MachineApplicable; - let snippet = snippet_with_applicability(cx, if_expr.span, "{ ... }", &mut app); - span_lint_and_sugg( cx, NEEDLESS_IF, - expr.span, + stmt.span, "this `if` branch is empty", "you can remove it", - if if_expr.can_have_side_effects() { - format!("{snippet};") + if cond.can_have_side_effects() || !cx.tcx.hir().attrs(stmt.hir_id).is_empty() { + // `{ foo }` or `{ foo } && bar` placed into a statement position would be + // interpreted as a block statement, force it to be an expression + if cond_snippet.starts_with('{') { + format!("({cond_snippet});") + } else { + format!("{cond_snippet};") + } } else { String::new() }, - app, + Applicability::MachineApplicable, ); } } } - -/// Returns true if any `Expr` contained within this `Expr` is a `Let`, else false. -/// -/// Really wish `Expr` had a `walk` method... -fn is_any_if_let(expr: &Expr<'_>) -> bool { - let mut v = IsAnyLetVisitor(false); - - v.visit_expr(expr); - v.0 -} - -struct IsAnyLetVisitor(bool); - -impl Visitor<'_> for IsAnyLetVisitor { - fn visit_expr(&mut self, expr: &Expr<'_>) { - if matches!(expr.kind, ExprKind::Let(..)) { - self.0 = true; - } else { - walk_expr(self, expr); - } - } -} diff --git a/tests/ui/needless_if.fixed b/tests/ui/needless_if.fixed index bee579be1bf..8fd945f1a54 100644 --- a/tests/ui/needless_if.fixed +++ b/tests/ui/needless_if.fixed @@ -5,9 +5,12 @@ clippy::blocks_in_if_conditions, clippy::if_same_then_else, clippy::ifs_same_cond, + clippy::let_unit_value, clippy::needless_else, clippy::no_effect, clippy::nonminimal_bool, + clippy::short_circuit_statement, + clippy::unnecessary_operation, unused )] #![warn(clippy::needless_if)] @@ -16,12 +19,7 @@ extern crate proc_macros; use proc_macros::external; use proc_macros::with_span; -fn no_side_effects() -> bool { - true -} - -fn has_side_effects(a: &mut u32) -> bool { - *a = 1; +fn maybe_side_effect() -> bool { true } @@ -29,32 +27,67 @@ fn main() { // Lint // Do not remove the condition - no_side_effects(); - let mut x = 0; - has_side_effects(&mut x); - assert_eq!(x, 1); + maybe_side_effect(); // Do not lint if (true) { } else { } - { + ({ return; - }; + }); // Do not lint if `else if` is present if (true) { } else if (true) { } - // Do not lint if any `let` is present + // Do not lint `if let` or let chains if let true = true {} if let true = true && true {} if true && let true = true {} - if { + // Can lint nested `if let`s + ({ if let true = true && true { true } else { false } - } && true - {} + } && true); external! { if (true) {} } with_span! { span if (true) {} } + + if true { + // comment + } + + if true { + #[cfg(any())] + foo; + } + + macro_rules! empty_expansion { + () => {}; + } + + if true { + empty_expansion!(); + } + + macro_rules! empty_repetition { + ($($t:tt)*) => { + if true { + $($t)* + } + } + } + + empty_repetition!(); + + // Must be placed into an expression context to not be interpreted as a block + ({ maybe_side_effect() }); + // Would be a block followed by `&&true` - a double reference to `true` + ({ maybe_side_effect() } && true); + + // Don't leave trailing attributes + #[allow(unused)] + true; + + let () = if maybe_side_effect() {}; } diff --git a/tests/ui/needless_if.rs b/tests/ui/needless_if.rs index 1608d261a36..04868299a54 100644 --- a/tests/ui/needless_if.rs +++ b/tests/ui/needless_if.rs @@ -5,9 +5,12 @@ clippy::blocks_in_if_conditions, clippy::if_same_then_else, clippy::ifs_same_cond, + clippy::let_unit_value, clippy::needless_else, clippy::no_effect, clippy::nonminimal_bool, + clippy::short_circuit_statement, + clippy::unnecessary_operation, unused )] #![warn(clippy::needless_if)] @@ -16,12 +19,7 @@ extern crate proc_macros; use proc_macros::external; use proc_macros::with_span; -fn no_side_effects() -> bool { - true -} - -fn has_side_effects(a: &mut u32) -> bool { - *a = 1; +fn maybe_side_effect() -> bool { true } @@ -29,10 +27,7 @@ fn main() { // Lint if (true) {} // Do not remove the condition - if no_side_effects() {} - let mut x = 0; - if has_side_effects(&mut x) {} - assert_eq!(x, 1); + if maybe_side_effect() {} // Do not lint if (true) { } else { @@ -44,10 +39,11 @@ fn main() { if (true) { } else if (true) { } - // Do not lint if any `let` is present + // Do not lint `if let` or let chains if let true = true {} if let true = true && true {} if true && let true = true {} + // Can lint nested `if let`s if { if let true = true && true { true } else { false } } && true @@ -57,4 +53,42 @@ fn main() { span if (true) {} } + + if true { + // comment + } + + if true { + #[cfg(any())] + foo; + } + + macro_rules! empty_expansion { + () => {}; + } + + if true { + empty_expansion!(); + } + + macro_rules! empty_repetition { + ($($t:tt)*) => { + if true { + $($t)* + } + } + } + + empty_repetition!(); + + // Must be placed into an expression context to not be interpreted as a block + if { maybe_side_effect() } {} + // Would be a block followed by `&&true` - a double reference to `true` + if { maybe_side_effect() } && true {} + + // Don't leave trailing attributes + #[allow(unused)] + if true {} + + let () = if maybe_side_effect() {}; } diff --git a/tests/ui/needless_if.stderr b/tests/ui/needless_if.stderr index 4236b0053eb..5cb42c36921 100644 --- a/tests/ui/needless_if.stderr +++ b/tests/ui/needless_if.stderr @@ -1,5 +1,5 @@ error: this `if` branch is empty - --> $DIR/needless_if.rs:30:5 + --> $DIR/needless_if.rs:28:5 | LL | if (true) {} | ^^^^^^^^^^^^ help: you can remove it @@ -7,19 +7,13 @@ LL | if (true) {} = note: `-D clippy::needless-if` implied by `-D warnings` error: this `if` branch is empty - --> $DIR/needless_if.rs:32:5 - | -LL | if no_side_effects() {} - | ^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `no_side_effects();` - -error: this `if` branch is empty - --> $DIR/needless_if.rs:34:5 + --> $DIR/needless_if.rs:30:5 | -LL | if has_side_effects(&mut x) {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `has_side_effects(&mut x);` +LL | if maybe_side_effect() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `maybe_side_effect();` error: this `if` branch is empty - --> $DIR/needless_if.rs:40:5 + --> $DIR/needless_if.rs:35:5 | LL | / if { LL | | return; @@ -28,10 +22,44 @@ LL | | } {} | help: you can remove it | -LL ~ { +LL ~ ({ LL + return; -LL + }; +LL + }); + | + +error: this `if` branch is empty + --> $DIR/needless_if.rs:47:5 + | +LL | / if { +LL | | if let true = true && true { true } else { false } +LL | | } && true +LL | | {} + | |______^ + | +help: you can remove it + | +LL ~ ({ +LL + if let true = true && true { true } else { false } +LL + } && true); + | + +error: this `if` branch is empty + --> $DIR/needless_if.rs:85:5 + | +LL | if { maybe_side_effect() } {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `({ maybe_side_effect() });` + +error: this `if` branch is empty + --> $DIR/needless_if.rs:87:5 + | +LL | if { maybe_side_effect() } && true {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `({ maybe_side_effect() } && true);` + +error: this `if` branch is empty + --> $DIR/needless_if.rs:91:5 | +LL | if true {} + | ^^^^^^^^^^ help: you can remove it: `true;` -error: aborting due to 4 previous errors +error: aborting due to 7 previous errors |