Make comparison_to_empty work on if let/let chains

Centri3 · Centri3 · commit b81442b83c03 · 2023-06-26T00:20:31.000-05:00
diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs
@@ -1,10 +1,12 @@
 use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then};
+use clippy_utils::higher::{IfOrIfLetInChain, LetChain};
 use clippy_utils::source::snippet_with_context;
 use clippy_utils::{get_item_name, get_parent_as_impl, is_lint_allowed, peel_ref_operators, sugg::Sugg};
 use if_chain::if_chain;
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::def_id::DefIdSet;
+use rustc_hir::PatKind;
 use rustc_hir::{
     def::Res, def_id::DefId, lang_items::LangItem, AssocItemKind, BinOpKind, Expr, ExprKind, FnRetTy, GenericArg,
     GenericBound, ImplItem, ImplItemKind, ImplicitSelfKind, Item, ItemKind, Mutability, Node, PathSegment, PrimTy,
@@ -167,6 +169,39 @@ impl<'tcx> LateLintPass<'tcx> for LenZero {
             return;
         }
 
+        if let Some(let_chain) = LetChain::hir(expr) {
+            for if_let in let_chain.conds.iter().filter_map(|cond| {
+                if let IfOrIfLetInChain::IfLet(if_let) = cond {
+                    return Some(if_let);
+                }
+                None
+            }) {
+                if has_is_empty(cx, if_let.let_expr)
+                    && match if_let.let_pat.kind {
+                        PatKind::Slice([], _, []) => true,
+                        PatKind::Lit(lit) if is_empty_string(lit) => true,
+                        _ => false,
+                    }
+                {
+                    let mut applicability = Applicability::MachineApplicable;
+
+                    let lit1 = peel_ref_operators(cx, if_let.let_expr);
+                    let lit_str =
+                        Sugg::hir_with_context(cx, lit1, if_let.let_span.ctxt(), "_", &mut applicability).maybe_par();
+
+                    span_lint_and_sugg(
+                        cx,
+                        COMPARISON_TO_EMPTY,
+                        if_let.let_span,
+                        "comparison to empty slice using `if let`",
+                        "using `is_empty` is clearer and more explicit",
+                        format!("{lit_str}.is_empty()"),
+                        applicability,
+                    );
+                }
+            }
+        }
+
         if let ExprKind::Binary(Spanned { node: cmp, .. }, left, right) = expr.kind {
             // expr.span might contains parenthesis, see issue #10529
             let actual_span = left.span.with_hi(right.span.hi());
diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs
@@ -5,6 +5,7 @@
 use crate::consts::{constant_simple, Constant};
 use crate::ty::is_type_diagnostic_item;
 use crate::{is_expn_of, match_def_path, paths};
+use hir::BinOpKind;
 use if_chain::if_chain;
 use rustc_ast::ast;
 use rustc_hir as hir;
@@ -137,6 +138,97 @@ impl<'hir> IfLet<'hir> {
     }
 }
 
+/// A `let` chain, like `if true && let Some(true) = x {}`
+#[derive(Debug)]
+pub struct LetChain<'hir> {
+    pub conds: Vec<IfOrIfLetInChain<'hir>>,
+    pub if_then: &'hir Expr<'hir>,
+    pub if_else: Option<&'hir Expr<'hir>>,
+}
+
+impl<'hir> LetChain<'hir> {
+    pub fn hir(expr: &Expr<'hir>) -> Option<Self> {
+        if let ExprKind::If(cond, if_then, if_else) = expr.kind {
+            let mut conds = vec![];
+            let mut cursor = cond;
+            while let ExprKind::Binary(binop, lhs, rhs) = cursor.kind
+                && let BinOpKind::And = binop.node
+            {
+                cursor = lhs;
+                conds.push(IfOrIfLetInChain::hir(rhs)?);
+            }
+
+            // The final lhs cannot be `&&`
+            conds.push(IfOrIfLetInChain::hir(cursor)?);
+
+            return Some(Self {
+                conds,
+                if_then,
+                if_else,
+            });
+        }
+
+        None
+    }
+}
+
+/// An `if let` or `if` expression in a let chain.
+#[derive(Debug)]
+pub enum IfOrIfLetInChain<'hir> {
+    If(IfInChain<'hir>),
+    IfLet(IfLetInChain<'hir>),
+}
+
+impl<'hir> IfOrIfLetInChain<'hir> {
+    pub fn hir(expr: &Expr<'hir>) -> Option<Self> {
+        match expr.kind {
+            ExprKind::DropTemps(cond) => Some(IfInChain { cond }.into()),
+            ExprKind::Let(hir::Let {
+                pat: let_pat,
+                init: let_expr,
+                span: let_span,
+                ..
+            }) => Some(
+                IfLetInChain {
+                    let_pat,
+                    let_expr,
+                    let_span: *let_span,
+                }
+                .into(),
+            ),
+            _ => None,
+        }
+    }
+}
+
+impl<'hir> From<IfInChain<'hir>> for IfOrIfLetInChain<'hir> {
+    fn from(value: IfInChain<'hir>) -> Self {
+        Self::If(value)
+    }
+}
+
+impl<'hir> From<IfLetInChain<'hir>> for IfOrIfLetInChain<'hir> {
+    fn from(value: IfLetInChain<'hir>) -> Self {
+        Self::IfLet(value)
+    }
+}
+
+/// An `if` expression in a let chain.
+#[derive(Debug)]
+pub struct IfInChain<'hir> {
+    pub cond: &'hir Expr<'hir>,
+}
+
+/// An `if let` expression in a let chain.
+#[derive(Debug)]
+pub struct IfLetInChain<'hir> {
+    pub let_span: Span,
+    /// `if let` pattern
+    pub let_pat: &'hir Pat<'hir>,
+    /// `if let` scrutinee
+    pub let_expr: &'hir Expr<'hir>,
+}
+
 /// An `if let` or `match` expression. Useful for lints that trigger on one or the other.
 pub enum IfLetOrMatch<'hir> {
     /// Any `match` expression
diff --git a/tests/ui/comparison_to_empty.fixed b/tests/ui/comparison_to_empty.fixed
@@ -1,7 +1,8 @@
 //@run-rustfix
 
 #![warn(clippy::comparison_to_empty)]
-#![allow(clippy::useless_vec)]
+#![allow(clippy::borrow_deref_ref, clippy::needless_if, clippy::useless_vec)]
+#![feature(let_chains)]
 
 fn main() {
     // Disallow comparisons to empty
@@ -12,6 +13,11 @@ fn main() {
     let v = vec![0];
     let _ = v.is_empty();
     let _ = !v.is_empty();
+    if (*v).is_empty() {}
+    let s = [0].as_slice();
+    if s.is_empty() {}
+    if s.is_empty() {}
+    if s.is_empty() && s.is_empty() {}
 
     // Allow comparisons to non-empty
     let s = String::new();
@@ -21,4 +27,8 @@ fn main() {
     let v = vec![0];
     let _ = v == [0];
     let _ = v != [0];
+    if let [0] = &*v {}
+    let s = [0].as_slice();
+    if let [0] = s {}
+    if let [0] = &*s && s == [0] {}
 }
diff --git a/tests/ui/comparison_to_empty.rs b/tests/ui/comparison_to_empty.rs
@@ -1,7 +1,8 @@
 //@run-rustfix
 
 #![warn(clippy::comparison_to_empty)]
-#![allow(clippy::useless_vec)]
+#![allow(clippy::borrow_deref_ref, clippy::needless_if, clippy::useless_vec)]
+#![feature(let_chains)]
 
 fn main() {
     // Disallow comparisons to empty
@@ -12,6 +13,11 @@ fn main() {
     let v = vec![0];
     let _ = v == [];
     let _ = v != [];
+    if let [] = &*v {}
+    let s = [0].as_slice();
+    if let [] = s {}
+    if let [] = &*s {}
+    if let [] = &*s && s == [] {}
 
     // Allow comparisons to non-empty
     let s = String::new();
@@ -21,4 +27,8 @@ fn main() {
     let v = vec![0];
     let _ = v == [0];
     let _ = v != [0];
+    if let [0] = &*v {}
+    let s = [0].as_slice();
+    if let [0] = s {}
+    if let [0] = &*s && s == [0] {}
 }
diff --git a/tests/ui/comparison_to_empty.stderr b/tests/ui/comparison_to_empty.stderr
@@ -1,28 +1,58 @@
 error: comparison to empty slice
-  --> $DIR/comparison_to_empty.rs:9:13
+  --> $DIR/comparison_to_empty.rs:10:13
    |
 LL |     let _ = s == "";
    |             ^^^^^^^ help: using `is_empty` is clearer and more explicit: `s.is_empty()`
    |
    = note: `-D clippy::comparison-to-empty` implied by `-D warnings`
 
 error: comparison to empty slice
-  --> $DIR/comparison_to_empty.rs:10:13
+  --> $DIR/comparison_to_empty.rs:11:13
    |
 LL |     let _ = s != "";
    |             ^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!s.is_empty()`
 
 error: comparison to empty slice
-  --> $DIR/comparison_to_empty.rs:13:13
+  --> $DIR/comparison_to_empty.rs:14:13
    |
 LL |     let _ = v == [];
    |             ^^^^^^^ help: using `is_empty` is clearer and more explicit: `v.is_empty()`
 
 error: comparison to empty slice
-  --> $DIR/comparison_to_empty.rs:14:13
+  --> $DIR/comparison_to_empty.rs:15:13
    |
 LL |     let _ = v != [];
    |             ^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!v.is_empty()`
 
-error: aborting due to 4 previous errors
+error: comparison to empty slice using `if let`
+  --> $DIR/comparison_to_empty.rs:16:8
+   |
+LL |     if let [] = &*v {}
+   |        ^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `(*v).is_empty()`
+
+error: comparison to empty slice using `if let`
+  --> $DIR/comparison_to_empty.rs:18:8
+   |
+LL |     if let [] = s {}
+   |        ^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `s.is_empty()`
+
+error: comparison to empty slice using `if let`
+  --> $DIR/comparison_to_empty.rs:19:8
+   |
+LL |     if let [] = &*s {}
+   |        ^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `s.is_empty()`
+
+error: comparison to empty slice using `if let`
+  --> $DIR/comparison_to_empty.rs:20:8
+   |
+LL |     if let [] = &*s && s == [] {}
+   |        ^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `s.is_empty()`
+
+error: comparison to empty slice
+  --> $DIR/comparison_to_empty.rs:20:24
+   |
+LL |     if let [] = &*s && s == [] {}
+   |                        ^^^^^^^ help: using `is_empty` is clearer and more explicit: `s.is_empty()`
+
+error: aborting due to 9 previous errors
 
