feat(manual_assert_eq): new lint

Author: ada4a

CHANGELOG.md

@@ -6526,6 +6526,7 @@ Released 2018-09-13
 [`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion
 [`manual_abs_diff`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_abs_diff
 [`manual_assert`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert
+[`manual_assert_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert_eq
 [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
 [`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits
 [`manual_c_str_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_c_str_literals

clippy_lints/src/declared_lints.rs

@@ -292,6 +292,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::main_recursion::MAIN_RECURSION_INFO,
     crate::manual_abs_diff::MANUAL_ABS_DIFF_INFO,
     crate::manual_assert::MANUAL_ASSERT_INFO,
+    crate::manual_assert_eq::MANUAL_ASSERT_EQ_INFO,
     crate::manual_async_fn::MANUAL_ASYNC_FN_INFO,
     crate::manual_bits::MANUAL_BITS_INFO,
     crate::manual_clamp::MANUAL_CLAMP_INFO,

clippy_lints/src/lib.rs

@@ -195,6 +195,7 @@ mod macro_use;
 mod main_recursion;
 mod manual_abs_diff;
 mod manual_assert;
+mod manual_assert_eq;
 mod manual_async_fn;
 mod manual_bits;
 mod manual_clamp;
@@ -819,5 +820,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg));
     store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites));
     store.register_late_pass(|_| Box::new(replace_box::ReplaceBox::default()));
+    store.register_late_pass(|_| Box::new(manual_assert_eq::ManualAssertEq));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/manual_assert_eq.rs

@@ -0,0 +1,78 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::macros::{find_assert_args, root_macro_call_first_node};
+use clippy_utils::source::walk_span_to_context;
+use clippy_utils::{is_in_const_context, sym};
+use rustc_errors::Applicability;
+use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_session::declare_lint_pass;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `assert!` and `debug_assert!` that consist of only an (in)equality check
+    ///
+    /// ### Why is this bad?
+    /// `assert_{eq,ne}!` and `debug_assert_{eq,ne}!` achieves the same goal, and provides some
+    /// additional debug information
+    ///
+    /// ### Example
+    /// ```no_run
+    /// assert!(2 * 2 == 4);
+    /// assert!(2 * 2 != 5);
+    /// debug_assert!(2 * 2 == 4);
+    /// debug_assert!(2 * 2 != 5);
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// assert_eq!(2 * 2, 4);
+    /// assert_ne!(2 * 2, 5);
+    /// debug_assert_eq!(2 * 2, 4);
+    /// debug_assert_ne!(2 * 2, 5);
+    /// ```
+    #[clippy::version = "1.92.0"]
+    pub MANUAL_ASSERT_EQ,
+    complexity,
+    "checks for assertions consisting of an (in)equality check"
+}
+declare_lint_pass!(ManualAssertEq => [MANUAL_ASSERT_EQ]);
+
+impl LateLintPass<'_> for ManualAssertEq {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
+        if let Some(macro_call) = root_macro_call_first_node(cx, expr)
+            && let macro_name = cx.tcx.item_name(macro_call.def_id)
+            && matches!(macro_name, sym::assert | sym::debug_assert)
+            // `assert_eq` isn't allowed in const context because it calls non-const `core::panicking::assert_failed`
+            // XXX: this might change in the future, so might want to relax this restriction
+            && !is_in_const_context(cx)
+            && let Some((cond, _)) = find_assert_args(cx, expr, macro_call.expn)
+            && let ExprKind::Binary(op, lhs, rhs) = cond.kind
+            && matches!(op.node, BinOpKind::Eq | BinOpKind::Ne)
+            && !cond.span.from_expansion()
+        {
+            let macro_name = macro_name.as_str();
+            span_lint_and_then(
+                cx,
+                MANUAL_ASSERT_EQ,
+                macro_call.span,
+                format!("used `{macro_name}!` with an equality comparison"),
+                |diag| {
+                    let new_name = format!("{macro_name}_{}", if op.node == BinOpKind::Eq { "eq" } else { "ne" });
+                    let msg = format!("replace it with `{new_name}!(..)`");
+
+                    let ctxt = cond.span.ctxt();
+                    if let Some(lhs_span) = walk_span_to_context(lhs.span, ctxt)
+                        && let Some(rhs_span) = walk_span_to_context(rhs.span, ctxt)
+                    {
+                        let macro_name_span = cx.sess().source_map().span_until_char(macro_call.span, '!');
+                        let eq_span = cond.span.with_lo(lhs_span.hi()).with_hi(rhs_span.lo());
+                        let suggestions = vec![(macro_name_span, new_name), (eq_span, ", ".to_string())];
+
+                        diag.multipart_suggestion(msg, suggestions, Applicability::MachineApplicable);
+                    } else {
+                        diag.span_help(expr.span, msg);
+                    }
+                },
+            );
+        }
+    }
+}

clippy_utils/src/sym.rs

@@ -133,6 +133,7 @@ generate! {
     cycle,
     cyclomatic_complexity,
     de,
+    debug_assert,
     deprecated_in_future,
     diagnostics,
     disallowed_types,

tests/ui/manual_assert_eq.fixed

@@ -0,0 +1,49 @@
+//@aux-build:proc_macros.rs
+#![warn(clippy::manual_assert_eq)]
+#![allow(clippy::manual_ignore_case_cmp)] // only raised before the fix
+#![expect(clippy::eq_op, clippy::assertions_on_constants)]
+
+fn main() {
+    assert_eq!("a", "a".to_ascii_lowercase());
+    //~^ manual_assert_eq
+    assert_eq!("a", "a".to_ascii_lowercase(), "a==a");
+    //~^ manual_assert_eq
+    assert_ne!("A", "A".to_ascii_lowercase());
+    //~^ manual_assert_eq
+    assert_ne!("A", "A".to_ascii_lowercase(), "A!=a");
+    //~^ manual_assert_eq
+
+    let v = vec![];
+    assert_eq!(v, vec![1, 2, 3], "v!=vec");
+    //~^ manual_assert_eq
+    assert_eq!(vec![1, 2, 3], v, "v!=vec");
+    //~^ manual_assert_eq
+    assert_eq!(vec![1], vec![1, 2, 3], "v!=vec");
+    //~^ manual_assert_eq
+
+    // Don't lint: in const context
+    const {
+        assert!(5 == 2 + 3);
+    }
+
+    // Don't lint: in external macro
+    {
+        // NOTE: this only works because `root_macro_call_first_node` returns `external!`,
+        // which then gets rejected by the macro name check
+        proc_macros::external!(assert!('a' == 'b'));
+        proc_macros::external!({
+            let some_padding_before = 'a';
+            assert!('a' == 'b');
+            let some_padding_after = 'b';
+        });
+
+        // .. which also means that the following is _technically_ a FN -- but surely no one would write
+        // code like this (diverging/unit expression as a child expression of a macro call)
+        vec![(), assert!('a' == 'b'), ()];
+    }
+}
+
+// Don't lint: in const context
+const _: () = {
+    assert!(8 == (7 + 1));
+};

tests/ui/manual_assert_eq.rs

@@ -0,0 +1,49 @@
+//@aux-build:proc_macros.rs
+#![warn(clippy::manual_assert_eq)]
+#![allow(clippy::manual_ignore_case_cmp)] // only raised before the fix
+#![expect(clippy::eq_op, clippy::assertions_on_constants)]
+
+fn main() {
+    assert!("a" == "a".to_ascii_lowercase());
+    //~^ manual_assert_eq
+    assert!("a" == "a".to_ascii_lowercase(), "a==a");
+    //~^ manual_assert_eq
+    assert!("A" != "A".to_ascii_lowercase());
+    //~^ manual_assert_eq
+    assert!("A" != "A".to_ascii_lowercase(), "A!=a");
+    //~^ manual_assert_eq
+
+    let v = vec![];
+    assert!(v == vec![1, 2, 3], "v!=vec");
+    //~^ manual_assert_eq
+    assert!(vec![1, 2, 3] == v, "v!=vec");
+    //~^ manual_assert_eq
+    assert!(vec![1] == vec![1, 2, 3], "v!=vec");
+    //~^ manual_assert_eq
+
+    // Don't lint: in const context
+    const {
+        assert!(5 == 2 + 3);
+    }
+
+    // Don't lint: in external macro
+    {
+        // NOTE: this only works because `root_macro_call_first_node` returns `external!`,
+        // which then gets rejected by the macro name check
+        proc_macros::external!(assert!('a' == 'b'));
+        proc_macros::external!({
+            let some_padding_before = 'a';
+            assert!('a' == 'b');
+            let some_padding_after = 'b';
+        });
+
+        // .. which also means that the following is _technically_ a FN -- but surely no one would write
+        // code like this (diverging/unit expression as a child expression of a macro call)
+        vec![(), assert!('a' == 'b'), ()];
+    }
+}
+
+// Don't lint: in const context
+const _: () = {
+    assert!(8 == (7 + 1));
+};

tests/ui/manual_assert_eq.stderr

@@ -0,0 +1,88 @@
+error: used `assert!` with an equality comparison
+  --> tests/ui/manual_assert_eq.rs:7:5
+   |
+LL |     assert!("a" == "a".to_ascii_lowercase());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::manual-assert-eq` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::manual_assert_eq)]`
+help: replace it with `assert_eq!(..)`
+   |
+LL -     assert!("a" == "a".to_ascii_lowercase());
+LL +     assert_eq!("a", "a".to_ascii_lowercase());
+   |
+
+error: used `assert!` with an equality comparison
+  --> tests/ui/manual_assert_eq.rs:9:5
+   |
+LL |     assert!("a" == "a".to_ascii_lowercase(), "a==a");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace it with `assert_eq!(..)`
+   |
+LL -     assert!("a" == "a".to_ascii_lowercase(), "a==a");
+LL +     assert_eq!("a", "a".to_ascii_lowercase(), "a==a");
+   |
+
+error: used `assert!` with an equality comparison
+  --> tests/ui/manual_assert_eq.rs:11:5
+   |
+LL |     assert!("A" != "A".to_ascii_lowercase());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace it with `assert_ne!(..)`
+   |
+LL -     assert!("A" != "A".to_ascii_lowercase());
+LL +     assert_ne!("A", "A".to_ascii_lowercase());
+   |
+
+error: used `assert!` with an equality comparison
+  --> tests/ui/manual_assert_eq.rs:13:5
+   |
+LL |     assert!("A" != "A".to_ascii_lowercase(), "A!=a");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace it with `assert_ne!(..)`
+   |
+LL -     assert!("A" != "A".to_ascii_lowercase(), "A!=a");
+LL +     assert_ne!("A", "A".to_ascii_lowercase(), "A!=a");
+   |
+
+error: used `assert!` with an equality comparison
+  --> tests/ui/manual_assert_eq.rs:17:5
+   |
+LL |     assert!(v == vec![1, 2, 3], "v!=vec");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace it with `assert_eq!(..)`
+   |
+LL -     assert!(v == vec![1, 2, 3], "v!=vec");
+LL +     assert_eq!(v, vec![1, 2, 3], "v!=vec");
+   |
+
+error: used `assert!` with an equality comparison
+  --> tests/ui/manual_assert_eq.rs:19:5
+   |
+LL |     assert!(vec![1, 2, 3] == v, "v!=vec");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace it with `assert_eq!(..)`
+   |
+LL -     assert!(vec![1, 2, 3] == v, "v!=vec");
+LL +     assert_eq!(vec![1, 2, 3], v, "v!=vec");
+   |
+
+error: used `assert!` with an equality comparison
+  --> tests/ui/manual_assert_eq.rs:21:5
+   |
+LL |     assert!(vec![1] == vec![1, 2, 3], "v!=vec");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace it with `assert_eq!(..)`
+   |
+LL -     assert!(vec![1] == vec![1, 2, 3], "v!=vec");
+LL +     assert_eq!(vec![1], vec![1, 2, 3], "v!=vec");
+   |
+
+error: aborting due to 7 previous errors
+

tests/ui/unnecessary_map_or.fixed

@@ -132,8 +132,12 @@ fn issue14201(a: Option<String>, b: Option<String>, s: &String) -> bool {
 }
 
 fn issue14714() {
-    assert!(Some("test") == Some("test"));
-    //~^ unnecessary_map_or
+    #[allow(clippy::manual_assert_eq, reason = "will fire after the fix")]
+    // blocks are needed because attributes (here, the `allow`) on macro calls are ignored
+    {
+        assert!(Some("test") == Some("test"));
+        //~^ unnecessary_map_or
+    };
 
     // even though we're in a macro context, we still need to parenthesise because of the `then`
     assert!((Some("test") == Some("test")).then(|| 1).is_some());

tests/ui/unnecessary_map_or.rs

@@ -136,8 +136,12 @@ fn issue14201(a: Option<String>, b: Option<String>, s: &String) -> bool {
 }
 
 fn issue14714() {
-    assert!(Some("test").map_or(false, |x| x == "test"));
-    //~^ unnecessary_map_or
+    #[allow(clippy::manual_assert_eq, reason = "will fire after the fix")]
+    // blocks are needed because attributes (here, the `allow`) on macro calls are ignored
+    {
+        assert!(Some("test").map_or(false, |x| x == "test"));
+        //~^ unnecessary_map_or
+    };
 
     // even though we're in a macro context, we still need to parenthesise because of the `then`
     assert!(Some("test").map_or(false, |x| x == "test").then(|| 1).is_some());