Move linting of assert macros from early to late pass

ThibsG · ThibsG · commit 121a04764527 · 2020-10-13T23:57:22.000+02:00
diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs
@@ -1,14 +1,11 @@
-use crate::utils::ast_utils::eq_expr;
 use crate::utils::{
-    eq_expr_value, implements_trait, in_macro, is_copy, multispan_sugg, snippet, span_lint, span_lint_and_then,
+    eq_expr_value, 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_ast::{ast, token};
 use rustc_errors::Applicability;
-use rustc_hir::{BinOp, BinOpKind, BorrowKind, Expr, ExprKind};
-use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass};
-use rustc_middle::lint::in_external_macro;
-use rustc_parse::parser;
+use rustc_hir::{BinOp, BinOpKind, BorrowKind, Expr, ExprKind, StmtKind};
+use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
@@ -63,44 +60,38 @@ declare_clippy_lint! {
 
 declare_lint_pass!(EqOp => [EQ_OP, OP_REF]);
 
-impl EarlyLintPass for EqOp {
-    fn check_mac(&mut self, cx: &EarlyContext<'_>, mac: &ast::MacCall) {
-        let macro_list = [
-            sym!(assert_eq),
-            sym!(assert_ne),
-            sym!(debug_assert_eq),
-            sym!(debug_assert_ne),
-        ];
-        if_chain! {
-            if !in_external_macro(cx.sess, mac.span());
-            if mac.path.segments.len() == 1;
-            let macro_name = mac.path.segments[0].ident.name;
-            if macro_list.contains(&macro_name);
-            let tokens = mac.args.inner_tokens();
-            let mut parser = parser::Parser::new(
-                &cx.sess.parse_sess, tokens, false, None);
-            if let Ok(left) = parser.parse_expr();
-            if parser.eat(&token::Comma);
-            if let Ok(right) = parser.parse_expr();
-            let left_expr = left.into_inner();
-            let right_expr = right.into_inner();
-            if eq_expr(&left_expr, &right_expr);
-
-            then {
-                span_lint(
-                    cx,
-                    EQ_OP,
-                    left_expr.span.to(right_expr.span),
-                    &format!("identical args used in this `{}!` macro call", macro_name),
-                );
-            }
-        }
-    }
-}
+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 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;
+                        if let ExprKind::AddrOf(BorrowKind::Ref, _, ref lhs) = conditions[0].kind;
+                        if let ExprKind::AddrOf(BorrowKind::Ref, _, ref rhs) = conditions[1].kind;
+                        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/lib.rs b/clippy_lints/src/lib.rs
@@ -348,7 +348,6 @@ pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore) {
     store.register_pre_expansion_pass(|| box write::Write::default());
     store.register_pre_expansion_pass(|| box attrs::EarlyAttributes);
     store.register_pre_expansion_pass(|| box dbg_macro::DbgMacro);
-    store.register_pre_expansion_pass(|| box eq_op::EqOp);
 }
 
 #[doc(hidden)]
@@ -911,7 +910,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     let vec_box_size_threshold = conf.vec_box_size_threshold;
     store.register_late_pass(move || box types::Types::new(vec_box_size_threshold));
     store.register_late_pass(|| box booleans::NonminimalBool);
-    store.register_early_pass(|| box eq_op::EqOp);
     store.register_late_pass(|| box eq_op::EqOp);
     store.register_late_pass(|| box enum_clike::UnportableVariant);
     store.register_late_pass(|| box float_literal::FloatLiteral);
diff --git a/tests/ui/eq_op.rs b/tests/ui/eq_op.rs
@@ -60,6 +60,8 @@ fn main() {
     const B: u32 = 10;
     const C: u32 = A / B; // ok, different named constants
     const D: u32 = A / A;
+
+    check_assert_identical_args();
 }
 
 #[rustfmt::skip]
@@ -85,3 +87,54 @@ fn check_ignore_macro() {
     // checks if the lint ignores macros with `!` operator
     !bool_macro!(1) && !bool_macro!("");
 }
+
+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 check_assert_identical_args() {
+    // lint also in macro definition
+    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);
+}
diff --git a/tests/ui/eq_op.stderr b/tests/ui/eq_op.stderr
@@ -162,5 +162,98 @@ error: equal expressions as operands to `/`
 LL |     const D: u32 = A / A;
    |                    ^^^^^
 
-error: aborting due to 27 previous errors
+error: identical args used in this `assert_eq!` macro call
+  --> $DIR/eq_op.rs:94:20
+   |
+LL |         assert_eq!(a, a);
+   |                    ^^^^
+...
+LL |     assert_in_macro_def!();
+   |     ----------------------- in this macro invocation
+   |
+   = note: `#[deny(clippy::eq_op)]` on by default
+   = 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.rs:95: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.rs:110:16
+   |
+LL |     assert_eq!(a, a);
+   |                ^^^^
+
+error: identical args used in this `assert_eq!` macro call
+  --> $DIR/eq_op.rs:111:16
+   |
+LL |     assert_eq!(a + 1, a + 1);
+   |                ^^^^^^^^^^^^
+
+error: identical args used in this `assert_ne!` macro call
+  --> $DIR/eq_op.rs:118:16
+   |
+LL |     assert_ne!(a, a);
+   |                ^^^^
+
+error: identical args used in this `assert_ne!` macro call
+  --> $DIR/eq_op.rs:119:16
+   |
+LL |     assert_ne!(a + 1, a + 1);
+   |                ^^^^^^^^^^^^
+
+error: identical args used in this `debug_assert_eq!` macro call
+  --> $DIR/eq_op.rs:96: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.rs:97: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.rs:126:22
+   |
+LL |     debug_assert_eq!(a, a);
+   |                      ^^^^
+
+error: identical args used in this `debug_assert_eq!` macro call
+  --> $DIR/eq_op.rs:127:22
+   |
+LL |     debug_assert_eq!(a + 1, a + 1);
+   |                      ^^^^^^^^^^^^
+
+error: identical args used in this `debug_assert_ne!` macro call
+  --> $DIR/eq_op.rs:134:22
+   |
+LL |     debug_assert_ne!(a, a);
+   |                      ^^^^
+
+error: identical args used in this `debug_assert_ne!` macro call
+  --> $DIR/eq_op.rs:135:22
+   |
+LL |     debug_assert_ne!(a + 1, a + 1);
+   |                      ^^^^^^^^^^^^
+
+error: aborting due to 39 previous errors
 
diff --git a/tests/ui/eq_op_early.rs b/tests/ui/eq_op_early.rs
diff --git a/tests/ui/eq_op_early.stderr b/tests/ui/eq_op_early.stderr
