Auto merge of #10362 - unexge:missing-assert-message-lint, r=llogiq

Add `missing_assert_message` lint

Fixes #6207.

changelog: new lint: [`missing_assert_message`]: A new lint for checking assertions that doesn't have a custom panic message.
[#10362](#10362)
<!-- changelog_checked -->

r? `@llogiq`

Author: bors

CHANGELOG.md

@@ -4706,6 +4706,7 @@ Released 2018-09-13
 [`mismatching_type_param_order`]: https://rust-lang.github.io/rust-clippy/master/index.html#mismatching_type_param_order
 [`misnamed_getters`]: https://rust-lang.github.io/rust-clippy/master/index.html#misnamed_getters
 [`misrefactored_assign_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#misrefactored_assign_op
+[`missing_assert_message`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_assert_message
 [`missing_const_for_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn
 [`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items
 [`missing_enforced_import_renames`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_enforced_import_renames

clippy_lints/src/declared_lints.rs

@@ -417,6 +417,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::misc_early::UNSEPARATED_LITERAL_SUFFIX_INFO,
     crate::misc_early::ZERO_PREFIXED_LITERAL_INFO,
     crate::mismatching_type_param_order::MISMATCHING_TYPE_PARAM_ORDER_INFO,
+    crate::missing_assert_message::MISSING_ASSERT_MESSAGE_INFO,
     crate::missing_const_for_fn::MISSING_CONST_FOR_FN_INFO,
     crate::missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS_INFO,
     crate::missing_enforced_import_rename::MISSING_ENFORCED_IMPORT_RENAMES_INFO,

clippy_lints/src/lib.rs

@@ -193,6 +193,7 @@ mod minmax;
 mod misc;
 mod misc_early;
 mod mismatching_type_param_order;
+mod missing_assert_message;
 mod missing_const_for_fn;
 mod missing_doc;
 mod missing_enforced_import_rename;
@@ -926,6 +927,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     });
     store.register_late_pass(|_| Box::new(no_mangle_with_rust_abi::NoMangleWithRustAbi));
     store.register_late_pass(|_| Box::new(collection_is_never_read::CollectionIsNeverRead));
+    store.register_late_pass(|_| Box::new(missing_assert_message::MissingAssertMessage));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/missing_assert_message.rs

@@ -0,0 +1,82 @@
+use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::macros::{find_assert_args, find_assert_eq_args, root_macro_call_first_node, PanicExpn};
+use clippy_utils::{is_in_cfg_test, is_in_test_function};
+use rustc_hir::Expr;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks assertions without a custom panic message.
+    ///
+    /// ### Why is this bad?
+    /// Without a good custom message, it'd be hard to understand what went wrong when the assertion fails.
+    /// A good custom message should be more about why the failure of the assertion is problematic
+    /// and not what is failed because the assertion already conveys that.
+    ///
+    /// ### Known problems
+    /// This lint cannot check the quality of the custom panic messages.
+    /// Hence, you can suppress this lint simply by adding placeholder messages
+    /// like "assertion failed". However, we recommend coming up with good messages
+    /// that provide useful information instead of placeholder messages that
+    /// don't provide any extra information.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # struct Service { ready: bool }
+    /// fn call(service: Service) {
+    ///     assert!(service.ready);
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # struct Service { ready: bool }
+    /// fn call(service: Service) {
+    ///     assert!(service.ready, "`service.poll_ready()` must be called first to ensure that service is ready to receive requests");
+    /// }
+    /// ```
+    #[clippy::version = "1.69.0"]
+    pub MISSING_ASSERT_MESSAGE,
+    restriction,
+    "checks assertions without a custom panic message"
+}
+
+declare_lint_pass!(MissingAssertMessage => [MISSING_ASSERT_MESSAGE]);
+
+impl<'tcx> LateLintPass<'tcx> for MissingAssertMessage {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        let Some(macro_call) = root_macro_call_first_node(cx, expr) else { return };
+        let single_argument = match cx.tcx.get_diagnostic_name(macro_call.def_id) {
+            Some(sym::assert_macro | sym::debug_assert_macro) => true,
+            Some(
+                sym::assert_eq_macro | sym::assert_ne_macro | sym::debug_assert_eq_macro | sym::debug_assert_ne_macro,
+            ) => false,
+            _ => return,
+        };
+
+        // This lint would be very noisy in tests, so just ignore if we're in test context
+        if is_in_test_function(cx.tcx, expr.hir_id) || is_in_cfg_test(cx.tcx, expr.hir_id) {
+            return;
+        }
+
+        let panic_expn = if single_argument {
+            let Some((_, panic_expn)) = find_assert_args(cx, expr, macro_call.expn) else { return };
+            panic_expn
+        } else {
+            let Some((_, _, panic_expn)) = find_assert_eq_args(cx, expr, macro_call.expn) else { return };
+            panic_expn
+        };
+
+        if let PanicExpn::Empty = panic_expn {
+            span_lint_and_help(
+                cx,
+                MISSING_ASSERT_MESSAGE,
+                macro_call.span,
+                "assert without any message",
+                None,
+                "consider describing why the failing assert is problematic",
+            );
+        }
+    }
+}

clippy_utils/src/macros.rs

@@ -213,6 +213,7 @@ pub fn is_assert_macro(cx: &LateContext<'_>, def_id: DefId) -> bool {
     matches!(name, sym::assert_macro | sym::debug_assert_macro)
 }
 
+#[derive(Debug)]
 pub enum PanicExpn<'a> {
     /// No arguments - `panic!()`
     Empty,
@@ -226,10 +227,7 @@ pub enum PanicExpn<'a> {
 
 impl<'a> PanicExpn<'a> {
     pub fn parse(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option<Self> {
-        if !macro_backtrace(expr.span).any(|macro_call| is_panic(cx, macro_call.def_id)) {
-            return None;
-        }
-        let ExprKind::Call(callee, [arg]) = &expr.kind else { return None };
+        let ExprKind::Call(callee, [arg, rest @ ..]) = &expr.kind else { return None };
         let ExprKind::Path(QPath::Resolved(_, path)) = &callee.kind else { return None };
         let result = match path.segments.last().unwrap().ident.as_str() {
             "panic" if arg.span.ctxt() == expr.span.ctxt() => Self::Empty,
@@ -239,6 +237,21 @@ impl<'a> PanicExpn<'a> {
                 Self::Display(e)
             },
             "panic_fmt" => Self::Format(FormatArgsExpn::parse(cx, arg)?),
+            // Since Rust 1.52, `assert_{eq,ne}` macros expand to use:
+            // `core::panicking::assert_failed(.., left_val, right_val, None | Some(format_args!(..)));`
+            "assert_failed" => {
+                // It should have 4 arguments in total (we already matched with the first argument,
+                // so we're just checking for 3)
+                if rest.len() != 3 {
+                    return None;
+                }
+                // `msg_arg` is either `None` (no custom message) or `Some(format_args!(..))` (custom message)
+                let msg_arg = &rest[2];
+                match msg_arg.kind {
+                    ExprKind::Call(_, [fmt_arg]) => Self::Format(FormatArgsExpn::parse(cx, fmt_arg)?),
+                    _ => Self::Empty,
+                }
+            },
             _ => return None,
         };
         Some(result)
@@ -251,7 +264,17 @@ pub fn find_assert_args<'a>(
     expr: &'a Expr<'a>,
     expn: ExpnId,
 ) -> Option<(&'a Expr<'a>, PanicExpn<'a>)> {
-    find_assert_args_inner(cx, expr, expn).map(|([e], p)| (e, p))
+    find_assert_args_inner(cx, expr, expn).map(|([e], mut p)| {
+        // `assert!(..)` expands to `core::panicking::panic("assertion failed: ...")` (which we map to
+        // `PanicExpn::Str(..)`) and `assert!(.., "..")` expands to
+        // `core::panicking::panic_fmt(format_args!(".."))` (which we map to `PanicExpn::Format(..)`).
+        // So even we got `PanicExpn::Str(..)` that means there is no custom message provided
+        if let PanicExpn::Str(_) = p {
+            p = PanicExpn::Empty;
+        }
+
+        (e, p)
+    })
 }
 
 /// Finds the arguments of an `assert_eq!` or `debug_assert_eq!` macro call within the macro
@@ -275,13 +298,12 @@ fn find_assert_args_inner<'a, const N: usize>(
         Some(inner_name) => find_assert_within_debug_assert(cx, expr, expn, Symbol::intern(inner_name))?,
     };
     let mut args = ArrayVec::new();
-    let mut panic_expn = None;
-    let _: Option<!> = for_each_expr(expr, |e| {
+    let panic_expn = for_each_expr(expr, |e| {
         if args.is_full() {
-            if panic_expn.is_none() && e.span.ctxt() != expr.span.ctxt() {
-                panic_expn = PanicExpn::parse(cx, e);
+            match PanicExpn::parse(cx, e) {
+                Some(expn) => ControlFlow::Break(expn),
+                None => ControlFlow::Continue(Descend::Yes),
             }
-            ControlFlow::Continue(Descend::from(panic_expn.is_none()))
         } else if is_assert_arg(cx, e, expn) {
             args.push(e);
             ControlFlow::Continue(Descend::No)

tests/ui/missing_assert_message.rs

@@ -0,0 +1,84 @@
+#![allow(unused)]
+#![warn(clippy::missing_assert_message)]
+
+macro_rules! bar {
+    ($( $x:expr ),*) => {
+        foo()
+    };
+}
+
+fn main() {}
+
+// Should trigger warning
+fn asserts_without_message() {
+    assert!(foo());
+    assert_eq!(foo(), foo());
+    assert_ne!(foo(), foo());
+    debug_assert!(foo());
+    debug_assert_eq!(foo(), foo());
+    debug_assert_ne!(foo(), foo());
+}
+
+// Should trigger warning
+fn asserts_without_message_but_with_macro_calls() {
+    assert!(bar!(true));
+    assert!(bar!(true, false));
+    assert_eq!(bar!(true), foo());
+    assert_ne!(bar!(true, true), bar!(true));
+}
+
+// Should trigger warning
+fn asserts_with_trailing_commas() {
+    assert!(foo(),);
+    assert_eq!(foo(), foo(),);
+    assert_ne!(foo(), foo(),);
+    debug_assert!(foo(),);
+    debug_assert_eq!(foo(), foo(),);
+    debug_assert_ne!(foo(), foo(),);
+}
+
+// Should not trigger warning
+fn asserts_with_message_and_with_macro_calls() {
+    assert!(bar!(true), "msg");
+    assert!(bar!(true, false), "msg");
+    assert_eq!(bar!(true), foo(), "msg");
+    assert_ne!(bar!(true, true), bar!(true), "msg");
+}
+
+// Should not trigger warning
+fn asserts_with_message() {
+    assert!(foo(), "msg");
+    assert_eq!(foo(), foo(), "msg");
+    assert_ne!(foo(), foo(), "msg");
+    debug_assert!(foo(), "msg");
+    debug_assert_eq!(foo(), foo(), "msg");
+    debug_assert_ne!(foo(), foo(), "msg");
+}
+
+// Should not trigger warning
+#[test]
+fn asserts_without_message_but_inside_a_test_function() {
+    assert!(foo());
+    assert_eq!(foo(), foo());
+    assert_ne!(foo(), foo());
+    debug_assert!(foo());
+    debug_assert_eq!(foo(), foo());
+    debug_assert_ne!(foo(), foo());
+}
+
+// Should not trigger warning
+#[cfg(test)]
+mod tests {
+    fn asserts_without_message_but_inside_a_test_module() {
+        assert!(foo());
+        assert_eq!(foo(), foo());
+        assert_ne!(foo(), foo());
+        debug_assert!(foo());
+        debug_assert_eq!(foo(), foo());
+        debug_assert_ne!(foo(), foo());
+    }
+}
+
+fn foo() -> bool {
+    true
+}