Auto merge of #6167 - ThibsG:IdenticalArgumentsAssertEq3574, r=ebroto

bors · bors · commit eaffd0e41837 · 2020-10-19T19:50:12.000Z
Identical arguments on assert macro family Lint when identical args are used on `assert_eq!`, `debug_assert_eq!`, `assert_ne!` and `debug_assert_ne!` macros. Added to the lint `eq_op`. Common functions added to `utils/higher.rs` Fixes: #3574 Fixes: #4694 changelog: Lint on identical args when calling `assert_eq!`, `debug_assert_eq!`, `assert_ne!` and `debug_assert_ne!` macros
diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs
@@ -1,8 +1,10 @@
 use crate::utils::{
-    eq_expr_value, implements_trait, in_macro, is_copy, multispan_sugg, snippet, span_lint, span_lint_and_then,
+    eq_expr_value, higher, implements_trait, in_macro, is_copy, is_expn_of, multispan_sugg, snippet, span_lint,
+    span_lint_and_then,
 };
+use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::{BinOp, BinOpKind, BorrowKind, Expr, ExprKind};
+use rustc_hir::{BinOp, BinOpKind, BorrowKind, Expr, ExprKind, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
@@ -23,6 +25,12 @@ declare_clippy_lint! {
     /// # let x = 1;
     /// if x + 1 == x + 1 {}
     /// ```
+    /// or
+    /// ```rust
+    /// # let a = 3;
+    /// # let b = 4;
+    /// assert_eq!(a, a);
+    /// ```
     pub EQ_OP,
     correctness,
     "equal operands on both sides of a comparison or bitwise combination (e.g., `x == x`)"
@@ -52,9 +60,34 @@ declare_clippy_lint! {
 
 declare_lint_pass!(EqOp => [EQ_OP, OP_REF]);
 
+const ASSERT_MACRO_NAMES: [&str; 4] = ["assert_eq", "assert_ne", "debug_assert_eq", "debug_assert_ne"];
+
 impl<'tcx> LateLintPass<'tcx> for EqOp {
     #[allow(clippy::similar_names, clippy::too_many_lines)]
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
+        if let ExprKind::Block(ref block, _) = e.kind {
+            for stmt in block.stmts {
+                for amn in &ASSERT_MACRO_NAMES {
+                    if_chain! {
+                        if is_expn_of(stmt.span, amn).is_some();
+                        if let StmtKind::Semi(ref matchexpr) = stmt.kind;
+                        if let Some(macro_args) = higher::extract_assert_macro_args(matchexpr);
+                        if macro_args.len() == 2;
+                        let (lhs, rhs) = (macro_args[0], macro_args[1]);
+                        if eq_expr_value(cx, lhs, rhs);
+
+                        then {
+                            span_lint(
+                                cx,
+                                EQ_OP,
+                                lhs.span.to(rhs.span),
+                                &format!("identical args used in this `{}!` macro call", amn),
+                            );
+                        }
+                    }
+                }
+            }
+        }
         if let ExprKind::Binary(op, ref left, ref right) = e.kind {
             if e.span.from_expansion() {
                 return;
diff --git a/clippy_lints/src/mutable_debug_assertion.rs b/clippy_lints/src/mutable_debug_assertion.rs
@@ -1,7 +1,6 @@
-use crate::utils::{is_direct_expn_of, span_lint};
-use if_chain::if_chain;
+use crate::utils::{higher, is_direct_expn_of, span_lint};
 use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
-use rustc_hir::{BorrowKind, Expr, ExprKind, MatchSource, Mutability, StmtKind, UnOp};
+use rustc_hir::{BorrowKind, Expr, ExprKind, MatchSource, Mutability};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::Map;
 use rustc_middle::ty;
@@ -39,66 +38,23 @@ impl<'tcx> LateLintPass<'tcx> for DebugAssertWithMutCall {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
         for dmn in &DEBUG_MACRO_NAMES {
             if is_direct_expn_of(e.span, dmn).is_some() {
-                if let Some(span) = extract_call(cx, e) {
-                    span_lint(
-                        cx,
-                        DEBUG_ASSERT_WITH_MUT_CALL,
-                        span,
-                        &format!("do not call a function with mutable arguments inside of `{}!`", dmn),
-                    );
-                }
-            }
-        }
-    }
-}
-
-//HACK(hellow554): remove this when #4694 is implemented
-fn extract_call<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> Option<Span> {
-    if_chain! {
-        if let ExprKind::Block(ref block, _) = e.kind;
-        if block.stmts.len() == 1;
-        if let StmtKind::Semi(ref matchexpr) = block.stmts[0].kind;
-        then {
-            // debug_assert
-            if_chain! {
-                if let ExprKind::Match(ref ifclause, _, _) = matchexpr.kind;
-                if let ExprKind::DropTemps(ref droptmp) = ifclause.kind;
-                if let ExprKind::Unary(UnOp::UnNot, ref condition) = droptmp.kind;
-                then {
-                    let mut visitor = MutArgVisitor::new(cx);
-                    visitor.visit_expr(condition);
-                    return visitor.expr_span();
-                }
-            }
-
-            // debug_assert_{eq,ne}
-            if_chain! {
-                if let ExprKind::Block(ref matchblock, _) = matchexpr.kind;
-                if let Some(ref matchheader) = matchblock.expr;
-                if let ExprKind::Match(ref headerexpr, _, _) = matchheader.kind;
-                if let ExprKind::Tup(ref conditions) = headerexpr.kind;
-                if conditions.len() == 2;
-                then {
-                    if let ExprKind::AddrOf(BorrowKind::Ref, _, ref lhs) = conditions[0].kind {
+                if let Some(macro_args) = higher::extract_assert_macro_args(e) {
+                    for arg in macro_args {
                         let mut visitor = MutArgVisitor::new(cx);
-                        visitor.visit_expr(lhs);
+                        visitor.visit_expr(arg);
                         if let Some(span) = visitor.expr_span() {
-                            return Some(span);
-                        }
-                    }
-                    if let ExprKind::AddrOf(BorrowKind::Ref, _, ref rhs) = conditions[1].kind {
-                        let mut visitor = MutArgVisitor::new(cx);
-                        visitor.visit_expr(rhs);
-                        if let Some(span) = visitor.expr_span() {
-                            return Some(span);
+                            span_lint(
+                                cx,
+                                DEBUG_ASSERT_WITH_MUT_CALL,
+                                span,
+                                &format!("do not call a function with mutable arguments inside of `{}!`", dmn),
+                            );
                         }
                     }
                 }
             }
         }
     }
-
-    None
 }
 
 struct MutArgVisitor<'a, 'tcx> {
diff --git a/clippy_lints/src/utils/higher.rs b/clippy_lints/src/utils/higher.rs
@@ -7,6 +7,7 @@ use crate::utils::{is_expn_of, match_def_path, paths};
 use if_chain::if_chain;
 use rustc_ast::ast;
 use rustc_hir as hir;
+use rustc_hir::{BorrowKind, Expr, ExprKind, StmtKind, UnOp};
 use rustc_lint::LateContext;
 
 /// Converts a hir binary operator to the corresponding `ast` type.
@@ -241,3 +242,56 @@ pub fn vec_macro<'e>(cx: &LateContext<'_>, expr: &'e hir::Expr<'_>) -> Option<Ve
 
     None
 }
+
+/// Extract args from an assert-like macro.
+/// Currently working with:
+/// - `assert!`, `assert_eq!` and `assert_ne!`
+/// - `debug_assert!`, `debug_assert_eq!` and `debug_assert_ne!`
+/// For example:
+/// `assert!(expr)` will return Some([expr])
+/// `debug_assert_eq!(a, b)` will return Some([a, b])
+pub fn extract_assert_macro_args<'tcx>(e: &'tcx Expr<'tcx>) -> Option<Vec<&'tcx Expr<'tcx>>> {
+    /// Try to match the AST for a pattern that contains a match, for example when two args are
+    /// compared
+    fn ast_matchblock(matchblock_expr: &'tcx Expr<'tcx>) -> Option<Vec<&Expr<'_>>> {
+        if_chain! {
+            if let ExprKind::Match(ref headerexpr, _, _) = &matchblock_expr.kind;
+            if let ExprKind::Tup([lhs, rhs]) = &headerexpr.kind;
+            if let ExprKind::AddrOf(BorrowKind::Ref, _, lhs) = lhs.kind;
+            if let ExprKind::AddrOf(BorrowKind::Ref, _, rhs) = rhs.kind;
+            then {
+                return Some(vec![lhs, rhs]);
+            }
+        }
+        None
+    }
+
+    if let ExprKind::Block(ref block, _) = e.kind {
+        if block.stmts.len() == 1 {
+            if let StmtKind::Semi(ref matchexpr) = block.stmts[0].kind {
+                // macros with unique arg: `{debug_}assert!` (e.g., `debug_assert!(some_condition)`)
+                if_chain! {
+                    if let ExprKind::Match(ref ifclause, _, _) = matchexpr.kind;
+                    if let ExprKind::DropTemps(ref droptmp) = ifclause.kind;
+                    if let ExprKind::Unary(UnOp::UnNot, condition) = droptmp.kind;
+                    then {
+                        return Some(vec![condition]);
+                    }
+                }
+
+                // debug macros with two args: `debug_assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
+                if_chain! {
+                    if let ExprKind::Block(ref matchblock,_) = matchexpr.kind;
+                    if let Some(ref matchblock_expr) = matchblock.expr;
+                    then {
+                        return ast_matchblock(matchblock_expr);
+                    }
+                }
+            }
+        } else if let Some(matchblock_expr) = block.expr {
+            // macros with two args: `assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
+            return ast_matchblock(&matchblock_expr);
+        }
+    }
+    None
+}
diff --git a/tests/ui/auxiliary/proc_macro_derive.rs b/tests/ui/auxiliary/proc_macro_derive.rs
@@ -3,6 +3,7 @@
 
 #![crate_type = "proc-macro"]
 #![feature(repr128, proc_macro_quote)]
+#![allow(clippy::eq_op)]
 
 extern crate proc_macro;
 
diff --git a/tests/ui/double_parens.rs b/tests/ui/double_parens.rs
@@ -1,5 +1,5 @@
 #![warn(clippy::double_parens)]
-#![allow(dead_code)]
+#![allow(dead_code, clippy::eq_op)]
 #![feature(custom_inner_attributes)]
 #![rustfmt::skip]
 
diff --git a/tests/ui/eq_op_macros.rs b/tests/ui/eq_op_macros.rs
@@ -0,0 +1,56 @@
+#![warn(clippy::eq_op)]
+
+// lint also in macro definition
+macro_rules! assert_in_macro_def {
+    () => {
+        let a = 42;
+        assert_eq!(a, a);
+        assert_ne!(a, a);
+        debug_assert_eq!(a, a);
+        debug_assert_ne!(a, a);
+    };
+}
+
+// lint identical args in assert-like macro invocations (see #3574)
+fn main() {
+    assert_in_macro_def!();
+
+    let a = 1;
+    let b = 2;
+
+    // lint identical args in `assert_eq!`
+    assert_eq!(a, a);
+    assert_eq!(a + 1, a + 1);
+    // ok
+    assert_eq!(a, b);
+    assert_eq!(a, a + 1);
+    assert_eq!(a + 1, b + 1);
+
+    // lint identical args in `assert_ne!`
+    assert_ne!(a, a);
+    assert_ne!(a + 1, a + 1);
+    // ok
+    assert_ne!(a, b);
+    assert_ne!(a, a + 1);
+    assert_ne!(a + 1, b + 1);
+
+    // lint identical args in `debug_assert_eq!`
+    debug_assert_eq!(a, a);
+    debug_assert_eq!(a + 1, a + 1);
+    // ok
+    debug_assert_eq!(a, b);
+    debug_assert_eq!(a, a + 1);
+    debug_assert_eq!(a + 1, b + 1);
+
+    // lint identical args in `debug_assert_ne!`
+    debug_assert_ne!(a, a);
+    debug_assert_ne!(a + 1, a + 1);
+    // ok
+    debug_assert_ne!(a, b);
+    debug_assert_ne!(a, a + 1);
+    debug_assert_ne!(a + 1, b + 1);
+
+    let my_vec = vec![1; 5];
+    let mut my_iter = my_vec.iter();
+    assert_ne!(my_iter.next(), my_iter.next());
+}
diff --git a/tests/ui/eq_op_macros.stderr b/tests/ui/eq_op_macros.stderr
@@ -0,0 +1,95 @@
+error: identical args used in this `assert_eq!` macro call
+  --> $DIR/eq_op_macros.rs:7:20
+   |
+LL |         assert_eq!(a, a);
+   |                    ^^^^
+...
+LL |     assert_in_macro_def!();
+   |     ----------------------- in this macro invocation
+   |
+   = note: `-D clippy::eq-op` implied by `-D warnings`
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: identical args used in this `assert_ne!` macro call
+  --> $DIR/eq_op_macros.rs:8:20
+   |
+LL |         assert_ne!(a, a);
+   |                    ^^^^
+...
+LL |     assert_in_macro_def!();
+   |     ----------------------- in this macro invocation
+   |
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: identical args used in this `assert_eq!` macro call
+  --> $DIR/eq_op_macros.rs:22:16
+   |
+LL |     assert_eq!(a, a);
+   |                ^^^^
+
+error: identical args used in this `assert_eq!` macro call
+  --> $DIR/eq_op_macros.rs:23:16
+   |
+LL |     assert_eq!(a + 1, a + 1);
+   |                ^^^^^^^^^^^^
+
+error: identical args used in this `assert_ne!` macro call
+  --> $DIR/eq_op_macros.rs:30:16
+   |
+LL |     assert_ne!(a, a);
+   |                ^^^^
+
+error: identical args used in this `assert_ne!` macro call
+  --> $DIR/eq_op_macros.rs:31:16
+   |
+LL |     assert_ne!(a + 1, a + 1);
+   |                ^^^^^^^^^^^^
+
+error: identical args used in this `debug_assert_eq!` macro call
+  --> $DIR/eq_op_macros.rs:9:26
+   |
+LL |         debug_assert_eq!(a, a);
+   |                          ^^^^
+...
+LL |     assert_in_macro_def!();
+   |     ----------------------- in this macro invocation
+   |
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: identical args used in this `debug_assert_ne!` macro call
+  --> $DIR/eq_op_macros.rs:10:26
+   |
+LL |         debug_assert_ne!(a, a);
+   |                          ^^^^
+...
+LL |     assert_in_macro_def!();
+   |     ----------------------- in this macro invocation
+   |
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: identical args used in this `debug_assert_eq!` macro call
+  --> $DIR/eq_op_macros.rs:38:22
+   |
+LL |     debug_assert_eq!(a, a);
+   |                      ^^^^
+
+error: identical args used in this `debug_assert_eq!` macro call
+  --> $DIR/eq_op_macros.rs:39:22
+   |
+LL |     debug_assert_eq!(a + 1, a + 1);
+   |                      ^^^^^^^^^^^^
+
+error: identical args used in this `debug_assert_ne!` macro call
+  --> $DIR/eq_op_macros.rs:46:22
+   |
+LL |     debug_assert_ne!(a, a);
+   |                      ^^^^
+
+error: identical args used in this `debug_assert_ne!` macro call
+  --> $DIR/eq_op_macros.rs:47:22
+   |
+LL |     debug_assert_ne!(a + 1, a + 1);
+   |                      ^^^^^^^^^^^^
+
+error: aborting due to 12 previous errors
+
diff --git a/tests/ui/used_underscore_binding.rs b/tests/ui/used_underscore_binding.rs
@@ -3,7 +3,7 @@
 
 #![feature(rustc_private)]
 #![warn(clippy::all)]
-#![allow(clippy::blacklisted_name)]
+#![allow(clippy::blacklisted_name, clippy::eq_op)]
 #![warn(clippy::used_underscore_binding)]
 
 #[macro_use]
