diff --git a/CHANGELOG.md b/CHANGELOG.md index f53143e564b6..2490722b614f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/assert_multiple.rs b/clippy_lints/src/assert_multiple.rs new file mode 100644 index 000000000000..96c84e277234 --- /dev/null +++ b/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, + 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, + ); + } + } + } +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 4218d1fdc290..b2fc3f0ed126 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index af25b1f7f1aa..8c4835dc9f94 100644 --- a/clippy_lints/src/lib.rs +++ b/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; @@ -871,6 +872,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); diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index f194103ae2e8..09220103f117 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -451,7 +451,7 @@ pub fn has_enclosing_paren(sugg: impl AsRef) -> 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]), diff --git a/tests/ui/assert_multiple.fixed b/tests/ui/assert_multiple.fixed new file mode 100644 index 000000000000..7e58a280a0ae --- /dev/null +++ b/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); +} diff --git a/tests/ui/assert_multiple.rs b/tests/ui/assert_multiple.rs new file mode 100644 index 000000000000..0fa7a151b589 --- /dev/null +++ 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); +} diff --git a/tests/ui/assert_multiple.stderr b/tests/ui/assert_multiple.stderr new file mode 100644 index 000000000000..65faef0e1f4f --- /dev/null +++ b/tests/ui/assert_multiple.stderr @@ -0,0 +1,124 @@ +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:30:5 + | +LL | assert!(myfunc1(1, "foo".to_string()) && b == Vals::Borrowed); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::assert-multiple` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::assert_multiple)]` +help: consider writing + | +LL ~ assert!(myfunc1(1, "foo".to_string())); +LL ~ assert_eq!(b, Vals::Borrowed); + | + +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:33:5 + | +LL | assert!(ms.myfunc(1, "foo".to_string()) && myfunc1(2, "bar".to_string())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider writing + | +LL ~ assert!(ms.myfunc(1, "foo".to_string())); +LL ~ assert!(myfunc1(2, "bar".to_string())); + | + +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:36:5 + | +LL | assert!(o == Vals::Owned && b == Vals::Other); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider writing + | +LL ~ assert_eq!(o, Vals::Owned); +LL ~ assert_eq!(b, Vals::Other); + | + +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:39:5 + | +LL | debug_assert!(o == b && other == Vals::Other); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider writing + | +LL ~ debug_assert_eq!(o, b); +LL ~ debug_assert_eq!(other, Vals::Other); + | + +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:42:5 + | +LL | assert!(o == b && (o == Vals::Owned || b == Vals::Other)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider writing + | +LL ~ assert_eq!(o, b); +LL ~ assert!(o == Vals::Owned || b == Vals::Other); + | + +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:44:5 + | +LL | assert!(o == b && is_bool); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider writing + | +LL ~ assert_eq!(o, b); +LL ~ assert!(is_bool); + | + +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:46:5 + | +LL | assert!(!is_bool && o == b); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider writing + | +LL ~ assert!(!is_bool); +LL ~ assert_eq!(o, b); + | + +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:48:5 + | +LL | assert!(o == b && !is_bool); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider writing + | +LL ~ assert_eq!(o, b); +LL ~ assert!(!is_bool); + | + +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:50:5 + | +LL | assert!(o == b && !(is_bool && o == Vals::Owned)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider writing + | +LL ~ assert_eq!(o, b); +LL ~ assert!(!(is_bool && o == Vals::Owned)); + | + +error: multiple asserts combined into one + --> tests/ui/assert_multiple.rs:53:5 + | +LL | assert!(v == vec![] && is_bool); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider writing + | +LL ~ assert_eq!(v, vec![]); +LL ~ assert!(is_bool); + | + +error: aborting due to 10 previous errors +