diff options
| -rw-r--r-- | clippy_lints/src/eq_op.rs | 37 | ||||
| -rw-r--r-- | clippy_lints/src/mutable_debug_assertion.rs | 66 | ||||
| -rw-r--r-- | clippy_lints/src/utils/higher.rs | 54 | ||||
| -rw-r--r-- | tests/ui/auxiliary/proc_macro_derive.rs | 1 | ||||
| -rw-r--r-- | tests/ui/double_parens.rs | 2 | ||||
| -rw-r--r-- | tests/ui/eq_op_macros.rs | 56 | ||||
| -rw-r--r-- | tests/ui/eq_op_macros.stderr | 95 | ||||
| -rw-r--r-- | tests/ui/used_underscore_binding.rs | 2 |
8 files changed, 254 insertions, 59 deletions
diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index e16ec783fab..3201adbf9a0 100644 --- 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 index cc635c2a202..76417aa7ed0 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<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 index 8563b469a30..6d7c5058b4f 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<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 index 3df8be6c232..e369f62f8bf 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 9c7590c7dd6..ff1dc76ab63 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_macros.rs b/tests/ui/eq_op_macros.rs new file mode 100644 index 00000000000..6b5b31a1a2e --- /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 00000000000..fb9378108b9 --- /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 + diff --git a/tests/ui/used_underscore_binding.rs b/tests/ui/used_underscore_binding.rs index 8e0243c49aa..d8bda7e8f48 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] |