Add missing_assert_message lint

Co-authored-by: Weihang Lo <me@weihanglo.tw>
unexge · Feb 16, 2023 · f9e133a · f9e133a
1 parent 5b6795f
commit f9e133a
Show file tree

Hide file tree

Showing 8 changed files with 313 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4558,6 +4558,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

diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -414,6 +414,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,

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -190,6 +190,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;
@@ -911,6 +912,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(size_of_ref::SizeOfRef));
     store.register_late_pass(|_| Box::new(multiple_unsafe_ops_per_block::MultipleUnsafeOpsPerBlock));
     store.register_late_pass(|_| Box::new(extra_unused_type_parameters::ExtraUnusedTypeParameters));
+    store.register_pre_expansion_pass(|| Box::new(missing_assert_message::MissingAssertMessage));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

diff --git a/clippy_lints/src/missing_assert_message.rs b/clippy_lints/src/missing_assert_message.rs
@@ -0,0 +1,123 @@
+use clippy_utils::diagnostics::span_lint;
+use rustc_ast::ast;
+use rustc_ast::{
+    token::{Token, TokenKind},
+    tokenstream::TokenTree,
+};
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks assertions that doesn't have a custom panic message.
+    ///
+    /// ### Why is this bad?
+    /// If the assertion fails, a custom message may make it easier to debug what went wrong.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let threshold = 50;
+    /// let num = 42;
+    /// assert!(num < threshold);
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let threshold = 50;
+    /// let num = 42;
+    /// assert!(num < threshold, "{num} is lower than threshold ({threshold})");
+    /// ```
+    #[clippy::version = "1.69.0"]
+    pub MISSING_ASSERT_MESSAGE,
+    pedantic,
+    "checks assertions that doesn't have a custom panic message"
+}
+
+#[derive(Default, Clone, Debug)]
+pub struct MissingAssertMessage {
+    // This field will be greater than zero if we are inside a `#[test]` or `#[cfg(test)]`
+    test_deepnes: usize,
+}
+
+impl_lint_pass!(MissingAssertMessage => [MISSING_ASSERT_MESSAGE]);
+
+impl EarlyLintPass for MissingAssertMessage {
+    fn check_mac(&mut self, cx: &EarlyContext<'_>, mac_call: &ast::MacCall) {
+        if self.test_deepnes != 0 {
+            return;
+        }
+
+        let Some(last_segment) = mac_call.path.segments.last() else { return; };
+        let num_separators_needed = match last_segment.ident.as_str() {
+            "assert" | "debug_assert" => 1,
+            "assert_eq" | "assert_ne" | "debug_assert_eq" | "debug_assert_ne" => 2,
+            _ => return,
+        };
+        let num_separators = num_commas_on_arguments(mac_call);
+
+        if num_separators < num_separators_needed {
+            span_lint(
+                cx,
+                MISSING_ASSERT_MESSAGE,
+                mac_call.span(),
+                "assert without any message",
+            );
+        }
+    }
+
+    fn check_item(&mut self, _: &EarlyContext<'_>, item: &ast::Item) {
+        if item.attrs.iter().any(is_a_test_attribute) {
+            self.test_deepnes += 1;
+        }
+    }
+
+    fn check_item_post(&mut self, _: &EarlyContext<'_>, item: &ast::Item) {
+        if item.attrs.iter().any(is_a_test_attribute) {
+            self.test_deepnes -= 1;
+        }
+    }
+}
+
+// Returns number of commas (excluding trailing comma) from `MacCall`'s arguments.
+fn num_commas_on_arguments(mac_call: &ast::MacCall) -> usize {
+    let mut num_separators = 0;
+    let mut is_trailing = false;
+    for tt in mac_call.args.tokens.trees() {
+        match tt {
+            TokenTree::Token(
+                Token {
+                    kind: TokenKind::Comma,
+                    span: _,
+                },
+                _,
+            ) => {
+                num_separators += 1;
+                is_trailing = true;
+            },
+            _ => {
+                is_trailing = false;
+            },
+        }
+    }
+    if is_trailing {
+        num_separators -= 1;
+    }
+    num_separators
+}
+
+// Returns true if the attribute is either a `#[test]` or a `#[cfg(test)]`.
+fn is_a_test_attribute(attr: &ast::Attribute) -> bool {
+    if attr.has_name(sym::test) {
+        return true;
+    }
+
+    if attr.has_name(sym::cfg)
+        && let Some(items) = attr.meta_item_list()
+        && let [item] = &*items
+        && item.has_name(sym::test)
+    {
+        true
+    } else {
+        false
+    }
+}
diff --git a/tests/ui/filter_map_next_fixable.fixed b/tests/ui/filter_map_next_fixable.fixed
@@ -1,7 +1,7 @@
 // run-rustfix
 
 #![warn(clippy::all, clippy::pedantic)]
-#![allow(unused)]
+#![allow(unused, clippy::missing_assert_message)]
 
 fn main() {
     let a = ["1", "lol", "3", "NaN", "5"];

diff --git a/tests/ui/filter_map_next_fixable.rs b/tests/ui/filter_map_next_fixable.rs
@@ -1,7 +1,7 @@
 // run-rustfix
 
 #![warn(clippy::all, clippy::pedantic)]
-#![allow(unused)]
+#![allow(unused, clippy::missing_assert_message)]
 
 fn main() {
     let a = ["1", "lol", "3", "NaN", "5"];

diff --git a/tests/ui/missing_assert_message.rs b/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
+}
diff --git a/tests/ui/missing_assert_message.stderr b/tests/ui/missing_assert_message.stderr
@@ -0,0 +1,100 @@
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:14:5
+   |
+LL |     assert!(foo());
+   |     ^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::missing-assert-message` implied by `-D warnings`
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:15:5
+   |
+LL |     assert_eq!(foo(), foo());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:16:5
+   |
+LL |     assert_ne!(foo(), foo());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:17:5
+   |
+LL |     debug_assert!(foo());
+   |     ^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:18:5
+   |
+LL |     debug_assert_eq!(foo(), foo());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:19:5
+   |
+LL |     debug_assert_ne!(foo(), foo());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:24:5
+   |
+LL |     assert!(bar!(true));
+   |     ^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:25:5
+   |
+LL |     assert!(bar!(true, false));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:26:5
+   |
+LL |     assert_eq!(bar!(true), foo());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:27:5
+   |
+LL |     assert_ne!(bar!(true, true), bar!(true));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:32:5
+   |
+LL |     assert!(foo(),);
+   |     ^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:33:5
+   |
+LL |     assert_eq!(foo(), foo(),);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:34:5
+   |
+LL |     assert_ne!(foo(), foo(),);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:35:5
+   |
+LL |     debug_assert!(foo(),);
+   |     ^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:36:5
+   |
+LL |     debug_assert_eq!(foo(), foo(),);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: assert without any message
+  --> $DIR/missing_assert_message.rs:37:5
+   |
+LL |     debug_assert_ne!(foo(), foo(),);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 16 previous errors
+