From 59bca098f906ecf7bef2f0cab95afbcc2d2e66ca Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Sun, 11 Jun 2023 06:50:03 -0500 Subject: [PATCH] don't lint on `if let` don't lint on `if let` --- clippy_lints/src/needless_if.rs | 51 ++++++++++++++++++++++++++------- tests/ui/needless_if.fixed | 30 +++++++++++++++---- tests/ui/needless_if.rs | 29 +++++++++++++++++-- tests/ui/needless_if.stderr | 24 ++++++++++++---- 4 files changed, 109 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/needless_if.rs b/clippy_lints/src/needless_if.rs index 8e27d3a40..b118516e0 100644 --- a/clippy_lints/src/needless_if.rs +++ b/clippy_lints/src/needless_if.rs @@ -1,20 +1,23 @@ use clippy_utils::{diagnostics::span_lint_and_sugg, is_from_proc_macro, source::snippet_with_applicability}; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, Node}; +use rustc_hir::{ + intravisit::{walk_expr, Visitor}, + Expr, ExprKind, Node, +}; 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 empty `if` statements with no else branch. + /// Checks for empty `if` branches with no else branch. /// /// ### Why is this bad? /// It can be entirely omitted, and often the condition too. /// /// ### Known issues - /// This will only suggest to remove the `if` statement, not the condition. Other lints such as - /// `no_effect` will take care of removing the condition if it's unnecessary. + /// This will usually only suggest to remove the `if` statement, not the condition. Other lints + /// such as `no_effect` will take care of removing the condition if it's unnecessary. /// /// ### Example /// ```rust,ignore @@ -42,9 +45,6 @@ impl LateLintPass<'_> for NeedlessIf { && else_expr.is_none() && !in_external_macro(cx.sess(), expr.span) { - let mut app = Applicability::MachineApplicable; - let snippet = snippet_with_applicability(cx, if_expr.span, "{ ... }", &mut app); - // Ignore `else if` if let Some(parent_id) = cx.tcx.hir().opt_parent_id(expr.hir_id) && let Some(Node::Expr(Expr { @@ -56,19 +56,48 @@ impl LateLintPass<'_> for NeedlessIf { return; } - if is_from_proc_macro(cx, expr) { + 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, - "this if branch is empty", + "this `if` branch is empty", "you can remove it", - format!("{snippet};"), - Applicability::MachineApplicable, + if if_expr.can_have_side_effects() { + format!("{snippet};") + } else { + String::new() + }, + app, ); } } } + +/// 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; + } + + walk_expr(self, expr); + } +} diff --git a/tests/ui/needless_if.fixed b/tests/ui/needless_if.fixed index 7587a397d..2abde2ad4 100644 --- a/tests/ui/needless_if.fixed +++ b/tests/ui/needless_if.fixed @@ -1,11 +1,13 @@ //@run-rustfix //@aux-build:proc_macros.rs +#![feature(let_chains)] #![allow( clippy::blocks_in_if_conditions, clippy::if_same_then_else, clippy::ifs_same_cond, clippy::needless_else, clippy::no_effect, + clippy::nonminimal_bool, unused )] #![warn(clippy::needless_if)] @@ -14,21 +16,39 @@ 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; + true +} + fn main() { // Lint - (true); + + // Do not remove the condition + no_side_effects(); + let mut x = 0; + has_side_effects(&mut x); + assert_eq!(x, 1); // Do not lint if (true) { } else { } + { + return; + }; // Do not lint if `else if` is present if (true) { } else if (true) { } - // Ensure clippy does not bork this up, other cases should be added - { - return; - }; + // Do not lint if any `let` is present + if let true = true {} + if let true = true && true {} + if true && let true = true {} + if { if let true = true && true { true } else { false } } && true {} external! { if (true) {} } with_span! { span diff --git a/tests/ui/needless_if.rs b/tests/ui/needless_if.rs index 3006995e0..1608d261a 100644 --- a/tests/ui/needless_if.rs +++ b/tests/ui/needless_if.rs @@ -1,11 +1,13 @@ //@run-rustfix //@aux-build:proc_macros.rs +#![feature(let_chains)] #![allow( clippy::blocks_in_if_conditions, clippy::if_same_then_else, clippy::ifs_same_cond, clippy::needless_else, clippy::no_effect, + clippy::nonminimal_bool, unused )] #![warn(clippy::needless_if)] @@ -14,21 +16,42 @@ 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; + true +} + 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); // Do not lint if (true) { } else { } + if { + return; + } {} // Do not lint if `else if` is present if (true) { } else if (true) { } - // Ensure clippy does not bork this up, other cases should be added + // Do not lint if any `let` is present + if let true = true {} + if let true = true && true {} + if true && let true = true {} if { - return; - } {} + if let true = true && true { true } else { false } + } && true + {} external! { if (true) {} } with_span! { span diff --git a/tests/ui/needless_if.stderr b/tests/ui/needless_if.stderr index bbb2a0356..4236b0053 100644 --- a/tests/ui/needless_if.stderr +++ b/tests/ui/needless_if.stderr @@ -1,13 +1,25 @@ -error: this if branch is empty - --> $DIR/needless_if.rs:19:5 +error: this `if` branch is empty + --> $DIR/needless_if.rs:30:5 | LL | if (true) {} - | ^^^^^^^^^^^^ help: you can remove it: `(true);` + | ^^^^^^^^^^^^ help: you can remove it | = note: `-D clippy::needless-if` implied by `-D warnings` -error: this if branch is empty - --> $DIR/needless_if.rs:29:5 +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 + | +LL | if has_side_effects(&mut x) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `has_side_effects(&mut x);` + +error: this `if` branch is empty + --> $DIR/needless_if.rs:40:5 | LL | / if { LL | | return; @@ -21,5 +33,5 @@ LL + return; LL + }; | -error: aborting due to 2 previous errors +error: aborting due to 4 previous errors