From da93ee86e5708cd991d7e21012ff2a2a980b0b2d Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Mon, 26 Jun 2023 00:20:31 -0500 Subject: [PATCH 1/2] Make `comparison_to_empty` work on `if let`/`let` chains refactor it --- clippy_lints/src/len_zero.rs | 30 +++++++++- clippy_utils/src/higher.rs | 92 +++++++++++++++++++++++++++++ tests/ui/comparison_to_empty.fixed | 12 +++- tests/ui/comparison_to_empty.rs | 12 +++- tests/ui/comparison_to_empty.stderr | 40 +++++++++++-- 5 files changed, 176 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index e64d46bd6..a707921ce 100644 --- 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 index 802adbd4d..312e6ea40 100644 --- 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>, + pub if_then: &'hir Expr<'hir>, + pub if_else: Option<&'hir Expr<'hir>>, +} + +impl<'hir> LetChain<'hir> { + pub fn hir(expr: &Expr<'hir>) -> Option { + 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 { + 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> for IfOrIfLetInChain<'hir> { + fn from(value: IfInChain<'hir>) -> Self { + Self::If(value) + } +} + +impl<'hir> From> 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 index c92dd509e..af219eed0 100644 --- 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 index b34897143..21e65184d 100644 --- 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 index cc09b17eb..f29782ed8 100644 --- a/tests/ui/comparison_to_empty.stderr +++ b/tests/ui/comparison_to_empty.stderr @@ -1,5 +1,5 @@ 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()` @@ -7,22 +7,52 @@ LL | let _ = s == ""; = 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 From ae5d391d21fb227da007da44286e43ce61f10f38 Mon Sep 17 00:00:00 2001 From: Catherine Flores Date: Sat, 22 Jul 2023 05:57:39 -0500 Subject: [PATCH 2/2] Remove `LetChain` --- clippy_lints/src/len_zero.rs | 2 +- clippy_utils/src/higher.rs | 92 ------------------------------------ 2 files changed, 1 insertion(+), 93 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index a707921ce..d4236926a 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -170,7 +170,7 @@ impl<'tcx> LateLintPass<'tcx> for LenZero { if let ExprKind::Let(lt) = expr.kind && has_is_empty(cx, lt.init) && match lt.pat.kind { - PatKind::Slice([], _, []) => true, + PatKind::Slice([], None, []) => true, PatKind::Lit(lit) if is_empty_string(lit) => true, _ => false, } diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 312e6ea40..802adbd4d 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -5,7 +5,6 @@ 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; @@ -138,97 +137,6 @@ impl<'hir> IfLet<'hir> { } } -/// A `let` chain, like `if true && let Some(true) = x {}` -#[derive(Debug)] -pub struct LetChain<'hir> { - pub conds: Vec>, - pub if_then: &'hir Expr<'hir>, - pub if_else: Option<&'hir Expr<'hir>>, -} - -impl<'hir> LetChain<'hir> { - pub fn hir(expr: &Expr<'hir>) -> Option { - 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 { - 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> for IfOrIfLetInChain<'hir> { - fn from(value: IfInChain<'hir>) -> Self { - Self::If(value) - } -} - -impl<'hir> From> 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> {