Split assert

CHANGELOG.md

@@ -6545,6 +6545,7 @@ Released 2018-09-13
 [`as_pointer_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_pointer_underscore
 [`as_ptr_cast_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_ptr_cast_mut
 [`as_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_underscore
+[`assert_multiple`]: https://rust-lang.github.io/rust-clippy/master/index.html#assert_multiple
 [`assertions_on_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants
 [`assertions_on_result_states`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_result_states
 [`assign_op_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern

clippy_lints/src/assert_multiple.rs

@@ -0,0 +1,129 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::macros::{find_assert_args, root_macro_call_first_node};
+use clippy_utils::source::{snippet_indent, snippet_with_context};
+use clippy_utils::sugg::strip_enclosing_paren;
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::Visitor;
+use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::declare_lint_pass;
+use rustc_span::{SyntaxContext, sym};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Looks for cases of `assert!(a == b && c == d)`
+    ///
+    /// ### Why is this bad?
+    /// When such an assertion fails, there isn't any information about which particular sub-expression caused the failure.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let a = true;
+    /// let b = true;
+    /// let c = true;
+    /// let d = true;
+    /// assert!(a == b && c != d /* &&  ... */)
+    /// ```
+    ///
+    /// Use instead:
+    /// ```no_run
+    /// let a = true;
+    /// let b = true;
+    /// let c = true;
+    /// let d = true;
+    /// assert_eq!(a, b);
+    /// assert_ne!(c, d);
+    /// /* ... */
+    /// ```
+    #[clippy::version = "1.97.0"]
+    pub ASSERT_MULTIPLE,
+    nursery,
+    "Splitting an assert using `&&` into separate asserts makes it clearer which is failing."
+}
+
+declare_lint_pass!(AssertMultiple => [ASSERT_MULTIPLE]);
+
+/// This visitor is a convenient place to hold the session context, as well as the collection of
+/// replacement strings and the type of assert to use.
+struct AssertVisitor<'tcx, 'v> {
+    cx: &'v LateContext<'tcx>,
+    suggests: Vec<String>,
+    assert_string: &'static str,
+    outerctxt: SyntaxContext,
+}
+
+impl<'tcx> Visitor<'tcx> for AssertVisitor<'tcx, '_> {
+    fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+        let mut app = Applicability::MaybeIncorrect;
+        if let ExprKind::Binary(op, lhs, rhs) = e.kind {
+            let lhs_name = snippet_with_context(self.cx, lhs.span, self.outerctxt, "..", &mut app).0;
+            let rhs_name = snippet_with_context(self.cx, rhs.span, self.outerctxt, "..", &mut app).0;
+
+            match op.node {
+                BinOpKind::And => {
+                    // For And, turn each of the rhs and lhs expressions into their own assert.
+                    rustc_hir::intravisit::walk_expr(self, lhs);
+                    rustc_hir::intravisit::walk_expr(self, rhs);
+                },
+                BinOpKind::Eq => {
+                    let tmpstr = format!("{}_eq!({lhs_name}, {rhs_name});", self.assert_string);
+                    self.suggests.push(tmpstr);
+                },
+                BinOpKind::Ne => {
+                    let tmpstr = format!("{}_ne!({lhs_name}, {rhs_name});", self.assert_string);
+                    self.suggests.push(tmpstr);
+                },
+                BinOpKind::Or | BinOpKind::Ge | BinOpKind::Gt | BinOpKind::Le | BinOpKind::Lt => {
+                    let snip = snippet_with_context(self.cx, e.span, self.outerctxt, "..", &mut app).0;
+                    let stripped = strip_enclosing_paren(snip);
+                    let tmpstr = format!("{}!({stripped});", self.assert_string);
+                    self.suggests.push(tmpstr);
+                },
+                _ => {},
+            }
+        } else {
+            let snip = snippet_with_context(self.cx, e.span, self.outerctxt, "..", &mut app).0;
+            let tmpstr = format!("{}!({snip});", self.assert_string);
+            self.suggests.push(tmpstr);
+        }
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for AssertMultiple {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) {
+        if let Some(macro_call) = root_macro_call_first_node(cx, e)
+            && let assert_string = match cx.tcx.get_diagnostic_name(macro_call.def_id) {
+                Some(sym::assert_macro) => "assert",
+                Some(sym::debug_assert_macro) => "debug_assert",
+                _ => return,
+            }
+            && let Some((condition, _)) = find_assert_args(cx, e, macro_call.expn)
+            && matches!(condition.kind, ExprKind::Binary(binop, _lhs, _rhs) if binop.node == BinOpKind::And)
+        {
+            // We only get here on assert/debug_assert macro calls whose arguments have an And expression
+            // on the top of the tree.
+            let mut am_visitor = AssertVisitor {
+                cx,
+                suggests: Vec::new(),
+                assert_string,
+                outerctxt: condition.span.ctxt(),
+            };
+            rustc_hir::intravisit::walk_expr(&mut am_visitor, condition);
+
+            if !am_visitor.suggests.is_empty() {
+                let indent = snippet_indent(cx, macro_call.span).unwrap_or_default();
+                let jointext = format!("\n{indent}");
+                let suggs = am_visitor.suggests.join(&jointext).trim_end_matches(';').to_string();
+                span_lint_and_sugg(
+                    cx,
+                    ASSERT_MULTIPLE,
+                    macro_call.span,
+                    "multiple asserts combined into one",
+                    "consider writing",
+                    suggs,
+                    Applicability::MaybeIncorrect,
+                );
+            }
+        }
+    }
+}

clippy_lints/src/declared_lints.rs

@@ -11,6 +11,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::as_conversions::AS_CONVERSIONS_INFO,
     crate::asm_syntax::INLINE_ASM_X86_ATT_SYNTAX_INFO,
     crate::asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX_INFO,
+    crate::assert_multiple::ASSERT_MULTIPLE_INFO,
     crate::assertions_on_constants::ASSERTIONS_ON_CONSTANTS_INFO,
     crate::assertions_on_result_states::ASSERTIONS_ON_RESULT_STATES_INFO,
     crate::assigning_clones::ASSIGNING_CLONES_INFO,

clippy_lints/src/lib.rs

@@ -66,6 +66,7 @@ mod arbitrary_source_item_ordering;
 mod arc_with_non_send_sync;
 mod as_conversions;
 mod asm_syntax;
+mod assert_multiple;
 mod assertions_on_constants;
 mod assertions_on_result_states;
 mod assigning_clones;
@@ -869,6 +870,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
         Box::new(move |_| Box::new(manual_noop_waker::ManualNoopWaker::new(conf))),
         Box::new(|_| Box::new(byte_char_slices::ByteCharSlice)),
         Box::new(|_| Box::new(manual_assert_eq::ManualAssertEq)),
+        Box::new(|_| Box::new(assert_multiple::AssertMultiple)),
         // add late passes here, used by `cargo dev new_lint`
     ];
     store.late_passes.extend(late_lints);

clippy_utils/src/sugg.rs

@@ -450,7 +450,7 @@ pub fn has_enclosing_paren(sugg: impl AsRef<str>) -> bool {
 }
 
 /// Strip enclosing parentheses from a snippet if present.
-fn strip_enclosing_paren(snippet: Cow<'_, str>) -> Cow<'_, str> {
+pub fn strip_enclosing_paren(snippet: Cow<'_, str>) -> Cow<'_, str> {
     if has_enclosing_paren(&snippet) {
         match snippet {
             Cow::Borrowed(s) => Cow::Borrowed(&s[1..s.len() - 1]),

tests/ui/assert_multiple.fixed

@@ -0,0 +1,70 @@
+#![warn(clippy::assert_multiple)]
+#![allow(unused)]
+use std::thread::sleep;
+use std::time::{Duration, SystemTime};
+
+fn myfunc1(_a: u32, _b: String) -> bool {
+    todo!()
+}
+
+struct MyStruct {}
+
+impl MyStruct {
+    fn myfunc(&self, a: u32, b: String) -> bool {
+        todo!()
+    }
+}
+
+fn main() {
+    #[derive(PartialEq, Debug)]
+    enum Vals {
+        Owned,
+        Borrowed,
+        Other,
+    }
+    let o = Vals::Owned;
+    let b = Vals::Borrowed;
+    let other = Vals::Other;
+    let is_bool = true;
+
+    assert!(myfunc1(1, "foo".to_string()));
+    assert_eq!(b, Vals::Borrowed);
+    //~^ assert_multiple
+    let ms = MyStruct {};
+    assert!(ms.myfunc(1, "foo".to_string()));
+    assert!(myfunc1(2, "bar".to_string()));
+    //~^ assert_multiple
+
+    assert_eq!(o, Vals::Owned);
+    assert_eq!(b, Vals::Other);
+    //~^ assert_multiple
+
+    debug_assert_eq!(o, b);
+    debug_assert_eq!(other, Vals::Other);
+    //~^ assert_multiple
+
+    assert_eq!(o, b);
+    assert!(o == Vals::Owned || b == Vals::Other);
+    //~^ assert_multiple
+    assert_eq!(o, b);
+    assert!(is_bool);
+    //~^ assert_multiple
+    assert!(!is_bool);
+    assert_eq!(o, b);
+    //~^ assert_multiple
+    assert_eq!(o, b);
+    assert!(!is_bool);
+    //~^ assert_multiple
+    assert_eq!(o, b);
+    assert!(!(is_bool && o == Vals::Owned));
+    //~^ assert_multiple
+    let v = vec![1, 2, 3];
+    assert_eq!(v, vec![]);
+    assert!(is_bool);
+    //~^ assert_multiple
+
+    // Next ones we cannot split.
+    assert!((o == b && o == Vals::Owned) || b == Vals::Other);
+    assert!(o == Vals::Owned || b == Vals::Other);
+    debug_assert!(o == b);
+}

tests/ui/assert_multiple.rs

@@ -0,0 +1,60 @@
+#![warn(clippy::assert_multiple)]
+#![allow(unused)]
+use std::thread::sleep;
+use std::time::{Duration, SystemTime};
+
+fn myfunc1(_a: u32, _b: String) -> bool {
+    todo!()
+}
+
+struct MyStruct {}
+
+impl MyStruct {
+    fn myfunc(&self, a: u32, b: String) -> bool {
+        todo!()
+    }
+}
+
+fn main() {
+    #[derive(PartialEq, Debug)]
+    enum Vals {
+        Owned,
+        Borrowed,
+        Other,
+    }
+    let o = Vals::Owned;
+    let b = Vals::Borrowed;
+    let other = Vals::Other;
+    let is_bool = true;
+
+    assert!(myfunc1(1, "foo".to_string()) && b == Vals::Borrowed);
+    //~^ assert_multiple
+    let ms = MyStruct {};
+    assert!(ms.myfunc(1, "foo".to_string()) && myfunc1(2, "bar".to_string()));
+    //~^ assert_multiple
+
+    assert!(o == Vals::Owned && b == Vals::Other);
+    //~^ assert_multiple
+
+    debug_assert!(o == b && other == Vals::Other);
+    //~^ assert_multiple
+
+    assert!(o == b && (o == Vals::Owned || b == Vals::Other));
+    //~^ assert_multiple
+    assert!(o == b && is_bool);
+    //~^ assert_multiple
+    assert!(!is_bool && o == b);
+    //~^ assert_multiple
+    assert!(o == b && !is_bool);
+    //~^ assert_multiple
+    assert!(o == b && !(is_bool && o == Vals::Owned));
+    //~^ assert_multiple
+    let v = vec![1, 2, 3];
+    assert!(v == vec![] && is_bool);
+    //~^ assert_multiple
+
+    // Next ones we cannot split.
+    assert!((o == b && o == Vals::Owned) || b == Vals::Other);
+    assert!(o == Vals::Owned || b == Vals::Other);
+    debug_assert!(o == b);
+}