From 32fdb8fb0c15ddc202eed70b82babca8d529e39b Mon Sep 17 00:00:00 2001 From: ThibsG Date: Thu, 8 Oct 2020 23:02:16 +0200 Subject: [PATCH 1/6] Lint on identical variable used as args in `assert_eq!` macro call --- clippy_lints/src/eq_op.rs | 37 ++++++++++++++++++++++++- clippy_lints/src/lib.rs | 2 ++ tests/ui/auxiliary/proc_macro_derive.rs | 1 + tests/ui/double_parens.rs | 2 +- tests/ui/eq_op_early.rs | 15 ++++++++++ tests/ui/eq_op_early.stderr | 16 +++++++++++ tests/ui/used_underscore_binding.rs | 2 +- 7 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 tests/ui/eq_op_early.rs create mode 100644 tests/ui/eq_op_early.stderr diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index e16ec783fab7..7126c98a0b43 100644 --- a/clippy_lints/src/eq_op.rs +++ b/clippy_lints/src/eq_op.rs @@ -1,9 +1,13 @@ +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, }; +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::{LateContext, LateLintPass}; +use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass}; +use rustc_parse::parser; use rustc_session::{declare_lint_pass, declare_tool_lint}; declare_clippy_lint! { @@ -23,6 +27,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,6 +62,31 @@ 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) { + if_chain! { + if mac.path == sym!(assert_eq); + 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), + "identical args used in this `assert_eq!` macro call", + ); + } + } + } +} + 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<'_>) { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fc4afde9d9e6..dd99b6b9040c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -348,6 +348,7 @@ 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)] @@ -910,6 +911,7 @@ 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/auxiliary/proc_macro_derive.rs b/tests/ui/auxiliary/proc_macro_derive.rs index 3df8be6c2323..e369f62f8bfe 100644 --- 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 index 9c7590c7dd63..ff1dc76ab63b 100644 --- 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_early.rs b/tests/ui/eq_op_early.rs new file mode 100644 index 000000000000..cf5660ea98da --- /dev/null +++ b/tests/ui/eq_op_early.rs @@ -0,0 +1,15 @@ +#![warn(clippy::eq_op)] + +fn main() { + let a = 1; + let b = 2; + + // lint identical args in `assert_eq!` (see #3574) + 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); +} diff --git a/tests/ui/eq_op_early.stderr b/tests/ui/eq_op_early.stderr new file mode 100644 index 000000000000..9206e9026e95 --- /dev/null +++ b/tests/ui/eq_op_early.stderr @@ -0,0 +1,16 @@ +error: identical args used in this `assert_eq!` macro call + --> $DIR/eq_op_early.rs:8:16 + | +LL | assert_eq!(a, a); + | ^^^^ + | + = note: `-D clippy::eq-op` implied by `-D warnings` + +error: identical args used in this `assert_eq!` macro call + --> $DIR/eq_op_early.rs:9:16 + | +LL | assert_eq!(a + 1, a + 1); + | ^^^^^^^^^^^^ + +error: aborting due to 2 previous errors + diff --git a/tests/ui/used_underscore_binding.rs b/tests/ui/used_underscore_binding.rs index 8e0243c49aaa..d8bda7e8f48a 100644 --- 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] From a3e0446afe0ebd7a420f65cd6aec1c56687f0ef5 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 13 Oct 2020 09:31:53 +0200 Subject: [PATCH 2/6] Extend to the `assert` macro family --- clippy_lints/src/eq_op.rs | 17 ++++++++++++++--- tests/ui/eq_op_early.rs | 25 +++++++++++++++++++++++- tests/ui/eq_op_early.stderr | 38 ++++++++++++++++++++++++++++++++++++- 3 files changed, 75 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index 7126c98a0b43..a95d71042ee2 100644 --- a/clippy_lints/src/eq_op.rs +++ b/clippy_lints/src/eq_op.rs @@ -7,6 +7,7 @@ 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_session::{declare_lint_pass, declare_tool_lint}; @@ -64,10 +65,20 @@ 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 mac.path == sym!(assert_eq); + 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(¯o_name); let tokens = mac.args.inner_tokens(); - let mut parser = parser::Parser::new(&cx.sess.parse_sess, tokens, false, None); + 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(); @@ -80,7 +91,7 @@ impl EarlyLintPass for EqOp { cx, EQ_OP, left_expr.span.to(right_expr.span), - "identical args used in this `assert_eq!` macro call", + &format!("identical args used in this `{}!` macro call", macro_name), ); } } diff --git a/tests/ui/eq_op_early.rs b/tests/ui/eq_op_early.rs index cf5660ea98da..25e1c6ac6b75 100644 --- a/tests/ui/eq_op_early.rs +++ b/tests/ui/eq_op_early.rs @@ -7,9 +7,32 @@ fn main() { // lint identical args in `assert_eq!` (see #3574) 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_early.stderr b/tests/ui/eq_op_early.stderr index 9206e9026e95..1df094fae180 100644 --- a/tests/ui/eq_op_early.stderr +++ b/tests/ui/eq_op_early.stderr @@ -12,5 +12,41 @@ error: identical args used in this `assert_eq!` macro call LL | assert_eq!(a + 1, a + 1); | ^^^^^^^^^^^^ -error: aborting due to 2 previous errors +error: identical args used in this `assert_ne!` macro call + --> $DIR/eq_op_early.rs:16:16 + | +LL | assert_ne!(a, a); + | ^^^^ + +error: identical args used in this `assert_ne!` macro call + --> $DIR/eq_op_early.rs:17:16 + | +LL | assert_ne!(a + 1, a + 1); + | ^^^^^^^^^^^^ + +error: identical args used in this `debug_assert_eq!` macro call + --> $DIR/eq_op_early.rs:24:22 + | +LL | debug_assert_eq!(a, a); + | ^^^^ + +error: identical args used in this `debug_assert_eq!` macro call + --> $DIR/eq_op_early.rs:25:22 + | +LL | debug_assert_eq!(a + 1, a + 1); + | ^^^^^^^^^^^^ + +error: identical args used in this `debug_assert_ne!` macro call + --> $DIR/eq_op_early.rs:32:22 + | +LL | debug_assert_ne!(a, a); + | ^^^^ + +error: identical args used in this `debug_assert_ne!` macro call + --> $DIR/eq_op_early.rs:33:22 + | +LL | debug_assert_ne!(a + 1, a + 1); + | ^^^^^^^^^^^^ + +error: aborting due to 8 previous errors From 121a047645270d5e9ac965d57c324301ea1f21c0 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 13 Oct 2020 23:46:23 +0200 Subject: [PATCH 3/6] Move linting of `assert` macros from early to late pass --- clippy_lints/src/eq_op.rs | 73 +++++++++++++--------------- clippy_lints/src/lib.rs | 2 - tests/ui/eq_op.rs | 53 +++++++++++++++++++++ tests/ui/eq_op.stderr | 95 ++++++++++++++++++++++++++++++++++++- tests/ui/eq_op_early.rs | 38 --------------- tests/ui/eq_op_early.stderr | 52 -------------------- 6 files changed, 179 insertions(+), 134 deletions(-) delete mode 100644 tests/ui/eq_op_early.rs delete mode 100644 tests/ui/eq_op_early.stderr diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index a95d71042ee2..9653e62cad07 100644 --- 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(¯o_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 index dd99b6b9040c..fc4afde9d9e6 100644 --- 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 index 272b0900a31c..3ab4dfc439bc 100644 --- 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 index 5b80e6078eed..21a63aec7a17 100644 --- 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 deleted file mode 100644 index 25e1c6ac6b75..000000000000 --- a/tests/ui/eq_op_early.rs +++ /dev/null @@ -1,38 +0,0 @@ -#![warn(clippy::eq_op)] - -fn main() { - let a = 1; - let b = 2; - - // lint identical args in `assert_eq!` (see #3574) - 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_early.stderr b/tests/ui/eq_op_early.stderr deleted file mode 100644 index 1df094fae180..000000000000 --- a/tests/ui/eq_op_early.stderr +++ /dev/null @@ -1,52 +0,0 @@ -error: identical args used in this `assert_eq!` macro call - --> $DIR/eq_op_early.rs:8:16 - | -LL | assert_eq!(a, a); - | ^^^^ - | - = note: `-D clippy::eq-op` implied by `-D warnings` - -error: identical args used in this `assert_eq!` macro call - --> $DIR/eq_op_early.rs:9:16 - | -LL | assert_eq!(a + 1, a + 1); - | ^^^^^^^^^^^^ - -error: identical args used in this `assert_ne!` macro call - --> $DIR/eq_op_early.rs:16:16 - | -LL | assert_ne!(a, a); - | ^^^^ - -error: identical args used in this `assert_ne!` macro call - --> $DIR/eq_op_early.rs:17:16 - | -LL | assert_ne!(a + 1, a + 1); - | ^^^^^^^^^^^^ - -error: identical args used in this `debug_assert_eq!` macro call - --> $DIR/eq_op_early.rs:24:22 - | -LL | debug_assert_eq!(a, a); - | ^^^^ - -error: identical args used in this `debug_assert_eq!` macro call - --> $DIR/eq_op_early.rs:25:22 - | -LL | debug_assert_eq!(a + 1, a + 1); - | ^^^^^^^^^^^^ - -error: identical args used in this `debug_assert_ne!` macro call - --> $DIR/eq_op_early.rs:32:22 - | -LL | debug_assert_ne!(a, a); - | ^^^^ - -error: identical args used in this `debug_assert_ne!` macro call - --> $DIR/eq_op_early.rs:33:22 - | -LL | debug_assert_ne!(a + 1, a + 1); - | ^^^^^^^^^^^^ - -error: aborting due to 8 previous errors - From 71c29b5be8526562c3de8d3b7dc94611647ee120 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Wed, 14 Oct 2020 21:29:53 +0200 Subject: [PATCH 4/6] Add iterator test case for `eq_op` lint --- tests/ui/eq_op.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/ui/eq_op.rs b/tests/ui/eq_op.rs index 3ab4dfc439bc..20613ac6afe9 100644 --- a/tests/ui/eq_op.rs +++ b/tests/ui/eq_op.rs @@ -137,4 +137,8 @@ fn check_assert_identical_args() { 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()); } From 5a13217ea9c07121e7d3cdcfb0ddd2aa52b90f12 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Fri, 16 Oct 2020 17:58:26 +0200 Subject: [PATCH 5/6] Assert macro args extractor as a common function in higher --- clippy_lints/src/eq_op.rs | 12 ++-- clippy_lints/src/mutable_debug_assertion.rs | 66 ++++----------------- clippy_lints/src/utils/higher.rs | 54 +++++++++++++++++ 3 files changed, 69 insertions(+), 63 deletions(-) diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index 9653e62cad07..3201adbf9a0b 100644 --- a/clippy_lints/src/eq_op.rs +++ b/clippy_lints/src/eq_op.rs @@ -1,5 +1,5 @@ use crate::utils::{ - eq_expr_value, implements_trait, in_macro, is_copy, is_expn_of, multispan_sugg, snippet, span_lint, + 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; @@ -71,13 +71,9 @@ impl<'tcx> LateLintPass<'tcx> for EqOp { 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 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 { diff --git a/clippy_lints/src/mutable_debug_assertion.rs b/clippy_lints/src/mutable_debug_assertion.rs index cc635c2a202f..76417aa7ed09 100644 --- 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 { - 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 index 8563b469a30d..6d7c5058b4f3 100644 --- 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(e: &'tcx Expr<'tcx>) -> Option>> { + /// 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>> { + 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 +} From 16b5f37b5a23f475d0d94efea764c57e4572f63f Mon Sep 17 00:00:00 2001 From: ThibsG Date: Mon, 19 Oct 2020 17:31:41 +0200 Subject: [PATCH 6/6] Split `eq_op` ui tests to avoid file limit error in CI --- tests/ui/eq_op.rs | 57 ---------------------- tests/ui/eq_op.stderr | 95 +----------------------------------- tests/ui/eq_op_macros.rs | 56 +++++++++++++++++++++ tests/ui/eq_op_macros.stderr | 95 ++++++++++++++++++++++++++++++++++++ 4 files changed, 152 insertions(+), 151 deletions(-) create mode 100644 tests/ui/eq_op_macros.rs create mode 100644 tests/ui/eq_op_macros.stderr diff --git a/tests/ui/eq_op.rs b/tests/ui/eq_op.rs index 20613ac6afe9..272b0900a31c 100644 --- a/tests/ui/eq_op.rs +++ b/tests/ui/eq_op.rs @@ -60,8 +60,6 @@ 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] @@ -87,58 +85,3 @@ 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); - - 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.stderr b/tests/ui/eq_op.stderr index 21a63aec7a17..5b80e6078eed 100644 --- a/tests/ui/eq_op.stderr +++ b/tests/ui/eq_op.stderr @@ -162,98 +162,5 @@ error: equal expressions as operands to `/` LL | const D: u32 = A / A; | ^^^^^ -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 +error: aborting due to 27 previous errors diff --git a/tests/ui/eq_op_macros.rs b/tests/ui/eq_op_macros.rs new file mode 100644 index 000000000000..6b5b31a1a2ef --- /dev/null +++ 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 new file mode 100644 index 000000000000..fb9378108b98 --- /dev/null +++ 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 +