Auto merge of #7762 - HKalbasi:master, r=Manishearth

Add lint `equatable_if_let` This is my attempt for #1716. There is a major false positive, which is people may implement `PartialEq` in a different way. It is unactionable at the moment so I put it into `nursery`. There is a trait `StructuralPartialEq` for solving this problem which is promising but it has several problems currently: * Integers and tuples doesn't implement it. * Some types wrongly implement it, like `Option<T>` when `T` doesn't implement it. I consider them bugs and against the propose of `StructuralPartialEq`. When they become fixed, this lint can become a useful lint with a single line change. changelog: New lint: [`equatable_if_let`]
rust-lang · Oct 4, 2021 · abe551e · abe551e
2 parents 3311b36 + 388a3d0
commit abe551e
Show file tree

Hide file tree

Showing 72 changed files with 572 additions and 253 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2695,6 +2695,7 @@ Released 2018-09-13
 [`enum_glob_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#enum_glob_use
 [`enum_variant_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#enum_variant_names
 [`eq_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#eq_op
+[`equatable_if_let`]: https://rust-lang.github.io/rust-clippy/master/index.html#equatable_if_let
 [`erasing_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#erasing_op
 [`eval_order_dependence`]: https://rust-lang.github.io/rust-clippy/master/index.html#eval_order_dependence
 [`excessive_precision`]: https://rust-lang.github.io/rust-clippy/master/index.html#excessive_precision

diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs
@@ -563,7 +563,7 @@ fn check_deprecated_cfg_attr(cx: &EarlyContext<'_>, attr: &Attribute) {
             skip_item.path.segments.last().expect("empty path in attribute").ident.name == sym::skip;
         // Only lint outer attributes, because custom inner attributes are unstable
         // Tracking issue: https://github.com/rust-lang/rust/issues/54726
-        if let AttrStyle::Outer = attr.style;
+        if attr.style == AttrStyle::Outer;
         then {
             span_lint_and_sugg(
                 cx,

diff --git a/clippy_lints/src/casts/cast_precision_loss.rs b/clippy_lints/src/casts/cast_precision_loss.rs
@@ -12,7 +12,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_from: Ty<'_>, ca
     }
 
     let from_nbits = utils::int_ty_to_nbits(cast_from, cx.tcx);
-    let to_nbits = if let ty::Float(FloatTy::F32) = cast_to.kind() {
+    let to_nbits = if cast_to.kind() == &ty::Float(FloatTy::F32) {
         32
     } else {
         64

diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs
@@ -393,7 +393,7 @@ impl<'tcx> Visitor<'tcx> for UnsafeVisitor<'_, 'tcx> {
 
         if_chain! {
             if let Some(header) = kind.header();
-            if let Unsafety::Unsafe = header.unsafety;
+            if header.unsafety == Unsafety::Unsafe;
             then {
                 self.has_unsafe = true;
             }
@@ -408,7 +408,7 @@ impl<'tcx> Visitor<'tcx> for UnsafeVisitor<'_, 'tcx> {
         }
 
         if let ExprKind::Block(block, _) = expr.kind {
-            if let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules {
+            if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) {
                 self.has_unsafe = true;
             }
         }

diff --git a/clippy_lints/src/equatable_if_let.rs b/clippy_lints/src/equatable_if_let.rs
@@ -0,0 +1,100 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::ty::implements_trait;
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, Pat, PatKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::Ty;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for pattern matchings that can be expressed using equality.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// * It reads better and has less cognitive load because equality won't cause binding.
+    /// * It is a [Yoda condition](https://en.wikipedia.org/wiki/Yoda_conditions). Yoda conditions are widely
+    /// criticized for increasing the cognitive load of reading the code.
+    /// * Equality is a simple bool expression and can be merged with `&&` and `||` and
+    /// reuse if blocks
+    ///
+    /// ### Example
+    /// ```rust,ignore
+    /// if let Some(2) = x {
+    ///     do_thing();
+    /// }
+    /// ```
+    /// Should be written
+    /// ```rust,ignore
+    /// if x == Some(2) {
+    ///     do_thing();
+    /// }
+    /// ```
+    pub EQUATABLE_IF_LET,
+    nursery,
+    "using pattern matching instead of equality"
+}
+
+declare_lint_pass!(PatternEquality => [EQUATABLE_IF_LET]);
+
+/// detects if pattern matches just one thing
+fn unary_pattern(pat: &Pat<'_>) -> bool {
+    fn array_rec(pats: &[Pat<'_>]) -> bool {
+        pats.iter().all(unary_pattern)
+    }
+    match &pat.kind {
+        PatKind::Slice(_, _, _) | PatKind::Range(_, _, _) | PatKind::Binding(..) | PatKind::Wild | PatKind::Or(_) => {
+            false
+        },
+        PatKind::Struct(_, a, etc) => !etc && a.iter().all(|x| unary_pattern(x.pat)),
+        PatKind::Tuple(a, etc) | PatKind::TupleStruct(_, a, etc) => !etc.is_some() && array_rec(a),
+        PatKind::Ref(x, _) | PatKind::Box(x) => unary_pattern(x),
+        PatKind::Path(_) | PatKind::Lit(_) => true,
+    }
+}
+
+fn is_structural_partial_eq(cx: &LateContext<'tcx>, ty: Ty<'tcx>, other: Ty<'tcx>) -> bool {
+    if let Some(def_id) = cx.tcx.lang_items().eq_trait() {
+        implements_trait(cx, ty, def_id, &[other.into()])
+    } else {
+        false
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for PatternEquality {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        if_chain! {
+            if let ExprKind::Let(pat, exp, _) = expr.kind;
+            if unary_pattern(pat);
+            let exp_ty = cx.typeck_results().expr_ty(exp);
+            let pat_ty = cx.typeck_results().pat_ty(pat);
+            if is_structural_partial_eq(cx, exp_ty, pat_ty);
+            then {
+
+                let mut applicability = Applicability::MachineApplicable;
+                let pat_str = match pat.kind {
+                    PatKind::Struct(..) => format!(
+                        "({})",
+                        snippet_with_applicability(cx, pat.span, "..", &mut applicability),
+                    ),
+                    _ => snippet_with_applicability(cx, pat.span, "..", &mut applicability).to_string(),
+                };
+                span_lint_and_sugg(
+                    cx,
+                    EQUATABLE_IF_LET,
+                    expr.span,
+                    "this pattern matching can be expressed using equality",
+                    "try",
+                    format!(
+                        "{} == {}",
+                        snippet_with_applicability(cx, exp.span, "..", &mut applicability),
+                        pat_str,
+                    ),
+                    applicability,
+                );
+            }
+        }
+    }
+}
diff --git a/clippy_lints/src/erasing_op.rs b/clippy_lints/src/erasing_op.rs
@@ -47,7 +47,7 @@ impl<'tcx> LateLintPass<'tcx> for ErasingOp {
 }
 
 fn check(cx: &LateContext<'_>, e: &Expr<'_>, span: Span) {
-    if let Some(Constant::Int(0)) = constant_simple(cx, cx.typeck_results(), e) {
+    if constant_simple(cx, cx.typeck_results(), e) == Some(Constant::Int(0)) {
         span_lint(
             cx,
             ERASING_OP,

diff --git a/clippy_lints/src/escape.rs b/clippy_lints/src/escape.rs
@@ -90,7 +90,7 @@ impl<'tcx> LateLintPass<'tcx> for BoxedLocal {
                 for trait_item in items {
                     if trait_item.id.hir_id() == hir_id {
                         // be sure we have `self` parameter in this function
-                        if let AssocItemKind::Fn { has_self: true } = trait_item.kind {
+                        if trait_item.kind == (AssocItemKind::Fn { has_self: true }) {
                             trait_self_ty = Some(
                                 TraitRef::identity(cx.tcx, trait_item.id.def_id.to_def_id())
                                     .self_ty()

diff --git a/clippy_lints/src/eta_reduction.rs b/clippy_lints/src/eta_reduction.rs
@@ -116,7 +116,7 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
                     if let Some(mut snippet) = snippet_opt(cx, callee.span) {
                         if_chain! {
                             if let ty::Closure(_, substs) = callee_ty.peel_refs().kind();
-                            if let ClosureKind::FnMut = substs.as_closure().kind();
+                            if substs.as_closure().kind() == ClosureKind::FnMut;
                             if get_enclosing_loop_or_closure(cx.tcx, expr).is_some()
                                 || UsedAfterExprVisitor::is_found(cx, callee);
 

diff --git a/clippy_lints/src/eval_order_dependence.rs b/clippy_lints/src/eval_order_dependence.rs
@@ -141,7 +141,7 @@ impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> {
                 match typ.kind() {
                     ty::FnDef(..) | ty::FnPtr(_) => {
                         let sig = typ.fn_sig(self.cx.tcx);
-                        if let ty::Never = self.cx.tcx.erase_late_bound_regions(sig).output().kind() {
+                        if self.cx.tcx.erase_late_bound_regions(sig).output().kind() == &ty::Never {
                             self.report_diverging_sub_expr(e);
                         }
                     },

diff --git a/clippy_lints/src/identity_op.rs b/clippy_lints/src/identity_op.rs
@@ -1,5 +1,4 @@
 use clippy_utils::source::snippet;
-use if_chain::if_chain;
 use rustc_hir::{BinOp, BinOpKind, Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty;
@@ -62,16 +61,9 @@ impl<'tcx> LateLintPass<'tcx> for IdentityOp {
 
 fn is_allowed(cx: &LateContext<'_>, cmp: BinOp, left: &Expr<'_>, right: &Expr<'_>) -> bool {
     // `1 << 0` is a common pattern in bit manipulation code
-    if_chain! {
-        if let BinOpKind::Shl = cmp.node;
-        if let Some(Constant::Int(0)) = constant_simple(cx, cx.typeck_results(), right);
-        if let Some(Constant::Int(1)) = constant_simple(cx, cx.typeck_results(), left);
-        then {
-            return true;
-        }
-    }
-
-    false
+    cmp.node == BinOpKind::Shl
+        && constant_simple(cx, cx.typeck_results(), right) == Some(Constant::Int(0))
+        && constant_simple(cx, cx.typeck_results(), left) == Some(Constant::Int(1))
 }
 
 #[allow(clippy::cast_possible_wrap)]

diff --git a/clippy_lints/src/integer_division.rs b/clippy_lints/src/integer_division.rs
@@ -48,7 +48,7 @@ impl<'tcx> LateLintPass<'tcx> for IntegerDivision {
 fn is_integer_division<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) -> bool {
     if_chain! {
         if let hir::ExprKind::Binary(binop, left, right) = &expr.kind;
-        if let hir::BinOpKind::Div = &binop.node;
+        if binop.node == hir::BinOpKind::Div;
         then {
             let (left_ty, right_ty) = (cx.typeck_results().expr_ty(left), cx.typeck_results().expr_ty(right));
             return left_ty.is_integral() && right_ty.is_integral();

diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs
@@ -455,14 +455,10 @@ fn is_empty_array(expr: &Expr<'_>) -> bool {
 fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     /// Gets an `AssocItem` and return true if it matches `is_empty(self)`.
     fn is_is_empty(cx: &LateContext<'_>, item: &ty::AssocItem) -> bool {
-        if let ty::AssocKind::Fn = item.kind {
-            if item.ident.name.as_str() == "is_empty" {
-                let sig = cx.tcx.fn_sig(item.def_id);
-                let ty = sig.skip_binder();
-                ty.inputs().len() == 1
-            } else {
-                false
-            }
+        if item.kind == ty::AssocKind::Fn && item.ident.name.as_str() == "is_empty" {
+            let sig = cx.tcx.fn_sig(item.def_id);
+            let ty = sig.skip_binder();
+            ty.inputs().len() == 1
         } else {
             false
         }

diff --git a/clippy_lints/src/lib.mods.rs b/clippy_lints/src/lib.mods.rs
@@ -49,6 +49,7 @@ mod entry;
 mod enum_clike;
 mod enum_variants;
 mod eq_op;
+mod equatable_if_let;
 mod erasing_op;
 mod escape;
 mod eta_reduction;

diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
@@ -116,6 +116,7 @@ store.register_lints(&[
     enum_variants::MODULE_NAME_REPETITIONS,
     eq_op::EQ_OP,
     eq_op::OP_REF,
+    equatable_if_let::EQUATABLE_IF_LET,
     erasing_op::ERASING_OP,
     escape::BOXED_LOCAL,
     eta_reduction::REDUNDANT_CLOSURE,

diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs
@@ -8,6 +8,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![
     LintId::of(copies::BRANCHES_SHARING_CODE),
     LintId::of(disallowed_method::DISALLOWED_METHOD),
     LintId::of(disallowed_type::DISALLOWED_TYPE),
+    LintId::of(equatable_if_let::EQUATABLE_IF_LET),
     LintId::of(fallible_impl_from::FALLIBLE_IMPL_FROM),
     LintId::of(floating_point_arithmetic::IMPRECISE_FLOPS),
     LintId::of(floating_point_arithmetic::SUBOPTIMAL_FLOPS),

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -485,6 +485,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(option_if_let_else::OptionIfLetElse));
     store.register_late_pass(|| Box::new(future_not_send::FutureNotSend));
     store.register_late_pass(|| Box::new(if_let_mutex::IfLetMutex));
+    store.register_late_pass(|| Box::new(equatable_if_let::PatternEquality));
     store.register_late_pass(|| Box::new(mut_mutex_lock::MutMutexLock));
     store.register_late_pass(|| Box::new(match_on_vec_items::MatchOnVecItems));
     store.register_late_pass(|| Box::new(manual_async_fn::ManualAsyncFn));

diff --git a/clippy_lints/src/loops/mut_range_bound.rs b/clippy_lints/src/loops/mut_range_bound.rs
@@ -92,7 +92,7 @@ impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, 'tcx> {
     fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
 
     fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) {
-        if let ty::BorrowKind::MutBorrow = bk {
+        if bk == ty::BorrowKind::MutBorrow {
             if let PlaceBase::Local(id) = cmt.place.base {
                 if Some(id) == self.hir_id_low && !BreakAfterExprVisitor::is_found(self.cx, diag_expr_id) {
                     self.span_low = Some(self.cx.tcx.hir().span(diag_expr_id));

diff --git a/clippy_lints/src/loops/needless_range_loop.rs b/clippy_lints/src/loops/needless_range_loop.rs
@@ -95,7 +95,7 @@ pub(super) fn check<'tcx>(
                     let mut take_expr = end;
 
                     if let ExprKind::Binary(ref op, left, right) = end.kind {
-                        if let BinOpKind::Add = op.node {
+                        if op.node == BinOpKind::Add {
                             let start_equal_left = SpanlessEq::new(cx).eq_expr(start, left);
                             let start_equal_right = SpanlessEq::new(cx).eq_expr(start, right);
 

diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs
@@ -14,7 +14,7 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
             NeverLoopResult::AlwaysBreak => {
                 span_lint_and_then(cx, NEVER_LOOP, expr.span, "this loop never actually loops", |diag| {
                     if_chain! {
-                        if let LoopSource::ForLoop = source;
+                        if source == LoopSource::ForLoop;
                         if let Some((_, Node::Expr(parent_match))) = cx.tcx.hir().parent_iter(expr.hir_id).nth(1);
                         if let Some(ForLoop { arg: iterator, pat, span: for_span, .. }) = ForLoop::hir(parent_match);
                         then {

diff --git a/clippy_lints/src/manual_async_fn.rs b/clippy_lints/src/manual_async_fn.rs
@@ -49,7 +49,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn {
     ) {
         if_chain! {
             if let Some(header) = kind.header();
-            if let IsAsync::NotAsync = header.asyncness;
+            if header.asyncness == IsAsync::NotAsync;
             // Check that this function returns `impl Future`
             if let FnRetTy::Return(ret_ty) = decl.output;
             if let Some((trait_ref, output_lifetimes)) = future_trait_ref(cx, ret_ty);
@@ -178,7 +178,7 @@ fn desugared_async_block<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>)
         if args.len() == 1;
         if let Expr{kind: ExprKind::Closure(_, _, body_id, ..), ..} = args[0];
         let closure_body = cx.tcx.hir().body(body_id);
-        if let Some(GeneratorKind::Async(AsyncGeneratorKind::Block)) = closure_body.generator_kind;
+        if closure_body.generator_kind == Some(GeneratorKind::Async(AsyncGeneratorKind::Block));
         then {
             return Some(closure_body);
         }

diff --git a/clippy_lints/src/methods/or_fun_call.rs b/clippy_lints/src/methods/or_fun_call.rs
@@ -178,15 +178,15 @@ pub(super) fn check<'tcx>(
             hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => {
                 check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, None);
             },
-            hir::ExprKind::Block(block, _) => {
-                if let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules {
-                    if let Some(block_expr) = block.expr {
-                        if let hir::ExprKind::MethodCall(..) = block_expr.kind {
-                            check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, None);
-                        }
+            hir::ExprKind::Block(block, _)
+                if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) =>
+            {
+                if let Some(block_expr) = block.expr {
+                    if let hir::ExprKind::MethodCall(..) = block_expr.kind {
+                        check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, None);
                     }
                 }
-            },
+            }
             _ => (),
         }
     }

diff --git a/clippy_lints/src/modulo_arithmetic.rs b/clippy_lints/src/modulo_arithmetic.rs
@@ -128,7 +128,7 @@ impl<'tcx> LateLintPass<'tcx> for ModuloArithmetic {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         match &expr.kind {
             ExprKind::Binary(op, lhs, rhs) | ExprKind::AssignOp(op, lhs, rhs) => {
-                if let BinOpKind::Rem = op.node {
+                if op.node == BinOpKind::Rem {
                     let lhs_operand = analyze_operand(lhs, cx, expr);
                     let rhs_operand = analyze_operand(rhs, cx, expr);
                     if_chain! {

diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs
@@ -248,7 +248,7 @@ fn check_comparison<'a, 'tcx>(
         if l_ty.is_bool() && r_ty.is_bool() {
             let mut applicability = Applicability::MachineApplicable;
 
-            if let BinOpKind::Eq = op.node {
+            if op.node == BinOpKind::Eq {
                 let expression_info = one_side_is_unary_not(left_side, right_side);
                 if expression_info.one_side_is_unary_not {
                     span_lint_and_sugg(

diff --git a/clippy_lints/src/neg_multiply.rs b/clippy_lints/src/neg_multiply.rs
@@ -46,7 +46,7 @@ impl<'tcx> LateLintPass<'tcx> for NegMultiply {
 fn check_mul(cx: &LateContext<'_>, span: Span, lit: &Expr<'_>, exp: &Expr<'_>) {
     if_chain! {
         if let ExprKind::Lit(ref l) = lit.kind;
-        if let Constant::Int(1) = consts::lit_to_constant(&l.node, cx.typeck_results().expr_ty_opt(lit));
+        if consts::lit_to_constant(&l.node, cx.typeck_results().expr_ty_opt(lit)) == Constant::Int(1);
         if cx.typeck_results().expr_ty(exp).is_integral();
         then {
             span_lint(cx, NEG_MULTIPLY, span, "negation by multiplying with `-1`");

diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs
@@ -69,7 +69,7 @@ impl<'tcx> LateLintPass<'tcx> for NewWithoutDefault {
         }) = item.kind
         {
             for assoc_item in items {
-                if let hir::AssocItemKind::Fn { has_self: false } = assoc_item.kind {
+                if assoc_item.kind == (hir::AssocItemKind::Fn { has_self: false }) {
                     let impl_item = cx.tcx.hir().impl_item(assoc_item.id);
                     if in_external_macro(cx.sess(), impl_item.span) {
                         return;

diff --git a/clippy_lints/src/non_send_fields_in_send_ty.rs b/clippy_lints/src/non_send_fields_in_send_ty.rs
@@ -81,7 +81,7 @@ impl<'tcx> LateLintPass<'tcx> for NonSendFieldInSendTy {
             if let Some(trait_ref) = &hir_impl.of_trait;
             if let Some(trait_id) = trait_ref.trait_def_id();
             if send_trait == trait_id;
-            if let ImplPolarity::Positive = hir_impl.polarity;
+            if hir_impl.polarity == ImplPolarity::Positive;
             if let Some(ty_trait_ref) = cx.tcx.impl_trait_ref(item.def_id);
             if let self_ty = ty_trait_ref.self_ty();
             if let ty::Adt(adt_def, impl_trait_substs) = self_ty.kind();