Make comparison_to_empty work on if let/let chains

Centri3 · Centri3 · commit da93ee86e570 · 2023-07-22T05:59:01.000-05:00
refactor it
diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs
@@ -7,11 +7,10 @@ use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
 use rustc_hir::def_id::{DefId, DefIdSet};
-use rustc_hir::lang_items::LangItem;
 use rustc_hir::{
     AssocItemKind, BinOpKind, Expr, ExprKind, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind,
-    ImplicitSelfKind, Item, ItemKind, Mutability, Node, PathSegment, PrimTy, QPath, TraitItemRef, TyKind,
-    TypeBindingKind,
+    ImplicitSelfKind, Item, ItemKind, LangItem, Mutability, Node, PatKind, PathSegment, PrimTy, QPath, TraitItemRef,
+    TyKind, TypeBindingKind,
 };
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::{self, AssocKind, FnSig, Ty};
@@ -168,6 +167,31 @@ impl<'tcx> LateLintPass<'tcx> for LenZero {
             return;
         }
 
+        if let ExprKind::Let(lt) = expr.kind
+            && has_is_empty(cx, lt.init)
+            && match lt.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, lt.init);
+            let lit_str =
+                Sugg::hir_with_context(cx, lit1, lt.span.ctxt(), "_", &mut applicability).maybe_par();
+
+            span_lint_and_sugg(
+                cx,
+                COMPARISON_TO_EMPTY,
+                lt.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.
 #[derive(Debug)]
 pub enum IfLetOrMatch<'hir> {
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
 
