New lint: uninformative_asserts

CHANGELOG.md

@@ -1987,6 +1987,7 @@ Released 2018-09-13
 [`undropped_manually_drops`]: https://rust-lang.github.io/rust-clippy/master/index.html#undropped_manually_drops
 [`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
 [`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
+[`uninformative_asserts`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninformative_asserts
 [`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
 [`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
 [`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp

clippy_lints/src/doc.rs

@@ -291,7 +291,7 @@ pub fn strip_doc_comment_decoration(doc: &str, comment_kind: CommentKind, span:
     let mut contains_initial_stars = false;
     for line in doc.lines() {
         let offset = line.as_ptr() as usize - doc.as_ptr() as usize;
-        debug_assert_eq!(offset as u32 as usize, offset);
+        debug_assert_eq!(offset as u32 as usize, offset, "doc comment too large");
         contains_initial_stars |= line.trim_start().starts_with('*');
         // +1 adds the newline, +3 skips the opening delimiter
         sizes.push((line.len() + 1, span.with_lo(span.lo() + BytePos(3 + offset as u32))));

clippy_lints/src/enum_variants.rs

@@ -263,7 +263,7 @@ fn to_camel_case(item_name: &str) -> String {
 impl EarlyLintPass for EnumVariantNames {
     fn check_item_post(&mut self, _cx: &EarlyContext<'_>, _item: &Item) {
         let last = self.modules.pop();
-        assert!(last.is_some());
+        assert!(last.is_some(), "exited more items than entered");
     }
 
     #[allow(clippy::similar_names)]

clippy_lints/src/lib.rs

@@ -317,6 +317,7 @@ mod try_err;
 mod types;
 mod undropped_manually_drops;
 mod unicode;
+mod uninformative_asserts;
 mod unit_return_expecting_ord;
 mod unnamed_address;
 mod unnecessary_sort_by;
@@ -352,6 +353,7 @@ pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore) {
     store.register_pre_expansion_pass(|| box write::Write::default());
     store.register_pre_expansion_pass(|| box attrs::EarlyAttributes);
     store.register_pre_expansion_pass(|| box dbg_macro::DbgMacro);
+    store.register_pre_expansion_pass(|| box uninformative_asserts::UninformativeAsserts::default());
 }
 
 #[doc(hidden)]
@@ -874,6 +876,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &unicode::INVISIBLE_CHARACTERS,
         &unicode::NON_ASCII_LITERAL,
         &unicode::UNICODE_NOT_NFC,
+        &uninformative_asserts::UNINFORMATIVE_ASSERTS,
         &unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD,
         &unnamed_address::FN_ADDRESS_COMPARISONS,
         &unnamed_address::VTABLE_ADDRESS_COMPARISONS,
@@ -1271,6 +1274,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&types::OPTION_OPTION),
         LintId::of(&unicode::NON_ASCII_LITERAL),
         LintId::of(&unicode::UNICODE_NOT_NFC),
+        LintId::of(&uninformative_asserts::UNINFORMATIVE_ASSERTS),
         LintId::of(&unnested_or_patterns::UNNESTED_OR_PATTERNS),
         LintId::of(&unused_self::UNUSED_SELF),
         LintId::of(&wildcard_imports::ENUM_GLOB_USE),

clippy_lints/src/needless_borrow.rs

@@ -113,7 +113,10 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
 
     fn check_item(&mut self, _: &LateContext<'tcx>, item: &'tcx Item<'_>) {
         if item.attrs.iter().any(|a| a.has_name(sym!(automatically_derived))) {
-            debug_assert!(self.derived_item.is_none());
+            debug_assert!(
+                self.derived_item.is_none(),
+                "automatically-derived item contained another automatically-derived item"
+            );
             self.derived_item = Some(item.hir_id);
         }
     }

clippy_lints/src/non_expressive_names.rs

@@ -406,7 +406,10 @@ fn do_check(lint: &mut NonExpressiveNames, cx: &EarlyContext<'_>, attrs: &[Attri
 /// Precondition: `a_name.chars().count() < b_name.chars().count()`.
 #[must_use]
 fn levenstein_not_1(a_name: &str, b_name: &str) -> bool {
-    debug_assert!(a_name.chars().count() < b_name.chars().count());
+    debug_assert!(
+        a_name.chars().count() < b_name.chars().count(),
+        "levenstein_not_1 precondition failed"
+    );
     let mut a_chars = a_name.chars();
     let mut b_chars = b_name.chars();
     while let (Some(a), Some(b)) = (a_chars.next(), b_chars.next()) {

clippy_lints/src/regex.rs

@@ -84,7 +84,7 @@ fn str_span(base: Span, c: regex_syntax::ast::Span, offset: u16) -> Span {
     let offset = u32::from(offset);
     let end = base.lo() + BytePos(u32::try_from(c.end.offset).expect("offset too large") + offset);
     let start = base.lo() + BytePos(u32::try_from(c.start.offset).expect("offset too large") + offset);
-    assert!(start <= end);
+    assert!(start <= end, "span had negative length");
     Span::new(start, end, base.ctxt())
 }
 

clippy_lints/src/uninformative_asserts.rs

@@ -0,0 +1,109 @@
+use crate::utils::span_lint_and_help;
+use if_chain::if_chain;
+use rustc_ast::ast::{Item, MacArgs, MacCall};
+use rustc_ast::token;
+use rustc_ast::tokenstream::TokenTree;
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+
+declare_clippy_lint! {
+    /// **What it does:**
+    /// Lint {debug_}assert{_eq,_ne}! without a custom panic message.
+    ///
+    /// **Why is this bad?**
+    /// If the assertion fails, a custom message may make it easier to debug what went wrong.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// # fn some_condition(_x: u8) -> bool { true }
+    /// # fn bar(x: u8) -> u8 { x }
+    /// # let foo = 0u8;
+    /// # let a = 0u8;
+    /// # let b = 0u8;
+    /// assert!(some_condition(foo));
+    /// debug_assert_eq(a, bar(b));
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # fn some_condition(_x: u8) -> bool { true }
+    /// # fn bar(x: u8) -> u8 { x }
+    /// # let foo = 0u8;
+    /// # let a = 0u8;
+    /// # let b = 0u8;
+    /// assert!(some_condition(foo), "foo failed some condition: foo = {}", foo);
+    /// debug_assert_eq!(a, bar(b), "failed to find inverse of bar at {}", a);
+    /// ```
+    pub UNINFORMATIVE_ASSERTS,
+    pedantic,
+    "using `assert!` without custom panic message"
+}
+
+#[derive(Default)]
+pub struct UninformativeAsserts {
+    test_fns_deep: u32,
+}
+
+impl_lint_pass!(UninformativeAsserts => [UNINFORMATIVE_ASSERTS]);
+
+const ONE_ARG_ASSERT: [&str; 2] = ["assert", "debug_assert"];
+const TWO_ARG_ASSERT: [&str; 4] = ["assert_eq", "assert_ne", "debug_assert_eq", "debug_assert_ne"];
+
+impl EarlyLintPass for UninformativeAsserts {
+    fn check_mac(&mut self, cx: &EarlyContext<'_>, mac: &MacCall) {
+        if self.test_fns_deep > 0 {
+            return;
+        }
+        if let MacArgs::Delimited(_, _, ts) = &*mac.args {
+            let args_tts = ts.trees().collect::<Vec<_>>();
+            if let [seg] = &*mac.path.segments {
+                let mac_name = seg.ident.name.as_str();
+                let msg_arg_idx = if ONE_ARG_ASSERT.contains(&&*mac_name) {
+                    1
+                } else if TWO_ARG_ASSERT.contains(&&*mac_name) {
+                    2
+                } else {
+                    return;
+                };
+                // this is a call to an `assert!`-family macro
+                // check if it has a custom panic message argument
+                let opt_msg = args_tts.split(is_comma).nth(msg_arg_idx);
+                if let None | Some([]) = opt_msg {
+                    span_lint_and_help(
+                        cx,
+                        UNINFORMATIVE_ASSERTS,
+                        mac.span(),
+                        "`assert!` called without custom panic message",
+                        None,
+                        "consider adding a custom panic message",
+                    );
+                }
+            }
+        }
+    }
+
+    fn check_item(&mut self, _: &EarlyContext<'_>, item: &Item) {
+        if item.attrs.iter().any(|attr| attr.has_name(sym!(test))) {
+            self.test_fns_deep = self.test_fns_deep.saturating_add(1);
+        }
+    }
+
+    fn check_item_post(&mut self, _: &EarlyContext<'_>, item: &Item) {
+        if item.attrs.iter().any(|attr| attr.has_name(sym!(test))) {
+            self.test_fns_deep = self.test_fns_deep.saturating_sub(1);
+        }
+    }
+}
+
+fn is_comma(tt: &TokenTree) -> bool {
+    if_chain! {
+        if let TokenTree::Token(token) = tt;
+        if let token::TokenKind::Comma = token.kind;
+        then {
+            return true;
+        }
+    }
+    false
+}

clippy_lints/src/utils/attrs.rs

@@ -29,7 +29,7 @@ pub struct LimitStack {
 
 impl Drop for LimitStack {
     fn drop(&mut self) {
-        assert_eq!(self.stack.len(), 1);
+        assert_eq!(self.stack.len(), 1, "LimitStack was not empty when dropped");
     }
 }
 
@@ -47,7 +47,9 @@ impl LimitStack {
     }
     pub fn pop_attrs(&mut self, sess: &Session, attrs: &[ast::Attribute], name: &'static str) {
         let stack = &mut self.stack;
-        parse_attrs(sess, attrs, name, |val| assert_eq!(stack.pop(), Some(val)));
+        parse_attrs(sess, attrs, name, |val| {
+            assert_eq!(stack.pop(), Some(val), "popped non-matching attr value")
+        });
     }
 }
 

clippy_lints/src/utils/conf.rs

@@ -224,7 +224,10 @@ pub fn read(path: &Path) -> (Conf, Vec<Error>) {
         Err(err) => return default(vec![err.into()]),
     };
 
-    assert!(ERRORS.lock().expect("no threading -> mutex always safe").is_empty());
+    assert!(
+        ERRORS.lock().expect("no threading -> mutex always safe").is_empty(),
+        "ERRORS was not empty at the start of toml parsing"
+    );
     match toml::from_str(&content) {
         Ok(toml) => {
             let mut errors = ERRORS.lock().expect("no threading -> mutex always safe").split_off(0);

clippy_lints/src/utils/numeric_literal.rs

@@ -169,7 +169,7 @@ impl<'a> NumericLiteral<'a> {
     }
 
     pub fn group_digits(output: &mut String, input: &str, group_size: usize, partial_group_first: bool, pad: bool) {
-        debug_assert!(group_size > 0);
+        debug_assert!(group_size > 0, "cannot group into groups of 0");
 
         let mut digits = input.chars().filter(|&c| c != '_');
 
@@ -202,15 +202,21 @@ impl<'a> NumericLiteral<'a> {
 }
 
 fn split_suffix<'a>(src: &'a str, lit_kind: &LitKind) -> (&'a str, Option<&'a str>) {
-    debug_assert!(lit_kind.is_numeric());
+    debug_assert!(
+        lit_kind.is_numeric(),
+        "non-numeric literal does not have numeric suffix"
+    );
     lit_suffix_length(lit_kind).map_or((src, None), |suffix_length| {
         let (unsuffixed, suffix) = src.split_at(src.len() - suffix_length);
         (unsuffixed, Some(suffix))
     })
 }
 
 fn lit_suffix_length(lit_kind: &LitKind) -> Option<usize> {
-    debug_assert!(lit_kind.is_numeric());
+    debug_assert!(
+        lit_kind.is_numeric(),
+        "non-numeric literal does not have numeric suffix"
+    );
     let suffix = match lit_kind {
         LitKind::Int(_, int_lit_kind) => match int_lit_kind {
             LitIntType::Signed(int_ty) => Some(int_ty.name_str()),

src/lintlist/mod.rs

@@ -2468,6 +2468,13 @@ vec![
         deprecation: None,
         module: "panic_unimplemented",
     },
+    Lint {
+        name: "uninformative_asserts",
+        group: "pedantic",
+        desc: "using `assert!` without custom panic message",
+        deprecation: None,
+        module: "uninformative_asserts",
+    },
     Lint {
         name: "uninit_assumed_init",
         group: "correctness",

tests/integration.rs

@@ -25,7 +25,7 @@ fn integration_test() {
         ])
         .status()
         .expect("unable to run git");
-    assert!(st.success());
+    assert!(st.success(), "git failed");
 
     let root_dir = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR"));
     let target_dir = std::path::Path::new(&root_dir).join("target");

tests/ui/filter_map_next.rs

@@ -1,4 +1,5 @@
 #![warn(clippy::all, clippy::pedantic)]
+#![allow(clippy::uninformative_asserts)]
 
 fn main() {
     let a = ["1", "lol", "3", "NaN", "5"];

tests/ui/filter_map_next.stderr

@@ -1,5 +1,5 @@
 error: called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(p)` instead.
-  --> $DIR/filter_map_next.rs:6:32
+  --> $DIR/filter_map_next.rs:7:32
    |
 LL |     let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
    |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -8,7 +8,7 @@ LL |     let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next
    = note: replace `filter_map(|s| s.parse().ok()).next()` with `find_map(|s| s.parse().ok())`
 
 error: called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(p)` instead.
-  --> $DIR/filter_map_next.rs:10:26
+  --> $DIR/filter_map_next.rs:11:26
    |
 LL |       let _: Option<u32> = vec![1, 2, 3, 4, 5, 6]
    |  __________________________^

tests/ui/shadow.rs

@@ -10,7 +10,8 @@
     unused_variables,
     clippy::manual_unwrap_or,
     clippy::missing_docs_in_private_items,
-    clippy::single_match
+    clippy::single_match,
+    clippy::uninformative_asserts
 )]
 
 fn id<T>(x: T) -> T {

tests/ui/shadow.stderr

@@ -1,135 +1,135 @@
 error: `x` is shadowed by itself in `&mut x`
-  --> $DIR/shadow.rs:27:5
+  --> $DIR/shadow.rs:28:5
    |
 LL |     let x = &mut x;
    |     ^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::shadow-same` implied by `-D warnings`
 note: previous binding is here
-  --> $DIR/shadow.rs:26:13
+  --> $DIR/shadow.rs:27:13
    |
 LL |     let mut x = 1;
    |             ^
 
 error: `x` is shadowed by itself in `{ x }`
-  --> $DIR/shadow.rs:28:5
+  --> $DIR/shadow.rs:29:5
    |
 LL |     let x = { x };
    |     ^^^^^^^^^^^^^^
    |
 note: previous binding is here
-  --> $DIR/shadow.rs:27:9
+  --> $DIR/shadow.rs:28:9
    |
 LL |     let x = &mut x;
    |         ^
 
 error: `x` is shadowed by itself in `(&*x)`
-  --> $DIR/shadow.rs:29:5
+  --> $DIR/shadow.rs:30:5
    |
 LL |     let x = (&*x);
    |     ^^^^^^^^^^^^^^
    |
 note: previous binding is here
-  --> $DIR/shadow.rs:28:9
+  --> $DIR/shadow.rs:29:9
    |
 LL |     let x = { x };
    |         ^
 
 error: `x` is shadowed by `{ *x + 1 }` which reuses the original value
-  --> $DIR/shadow.rs:30:9
+  --> $DIR/shadow.rs:31:9
    |
 LL |     let x = { *x + 1 };
    |         ^
    |
    = note: `-D clippy::shadow-reuse` implied by `-D warnings`
 note: initialization happens here
-  --> $DIR/shadow.rs:30:13
+  --> $DIR/shadow.rs:31:13
    |
 LL |     let x = { *x + 1 };
    |             ^^^^^^^^^^
 note: previous binding is here
-  --> $DIR/shadow.rs:29:9
+  --> $DIR/shadow.rs:30:9
    |
 LL |     let x = (&*x);
    |         ^
 
 error: `x` is shadowed by `id(x)` which reuses the original value
-  --> $DIR/shadow.rs:31:9
+  --> $DIR/shadow.rs:32:9
    |
 LL |     let x = id(x);
    |         ^
    |
 note: initialization happens here
-  --> $DIR/shadow.rs:31:13
+  --> $DIR/shadow.rs:32:13
    |
 LL |     let x = id(x);
    |             ^^^^^
 note: previous binding is here
-  --> $DIR/shadow.rs:30:9
+  --> $DIR/shadow.rs:31:9
    |
 LL |     let x = { *x + 1 };
    |         ^
 
 error: `x` is shadowed by `(1, x)` which reuses the original value
-  --> $DIR/shadow.rs:32:9
+  --> $DIR/shadow.rs:33:9
    |
 LL |     let x = (1, x);
    |         ^
    |
 note: initialization happens here
-  --> $DIR/shadow.rs:32:13
+  --> $DIR/shadow.rs:33:13
    |
 LL |     let x = (1, x);
    |             ^^^^^^
 note: previous binding is here
-  --> $DIR/shadow.rs:31:9
+  --> $DIR/shadow.rs:32:9
    |
 LL |     let x = id(x);
    |         ^
 
 error: `x` is shadowed by `first(x)` which reuses the original value
-  --> $DIR/shadow.rs:33:9
+  --> $DIR/shadow.rs:34:9
    |
 LL |     let x = first(x);
    |         ^
    |
 note: initialization happens here
-  --> $DIR/shadow.rs:33:13
+  --> $DIR/shadow.rs:34:13
    |
 LL |     let x = first(x);
    |             ^^^^^^^^
 note: previous binding is here
-  --> $DIR/shadow.rs:32:9
+  --> $DIR/shadow.rs:33:9
    |
 LL |     let x = (1, x);
    |         ^
 
 error: `x` is being shadowed
-  --> $DIR/shadow.rs:35:9
+  --> $DIR/shadow.rs:36:9
    |
 LL |     let x = y;
    |         ^
    |
    = note: `-D clippy::shadow-unrelated` implied by `-D warnings`
 note: initialization happens here
-  --> $DIR/shadow.rs:35:13
+  --> $DIR/shadow.rs:36:13
    |
 LL |     let x = y;
    |             ^
 note: previous binding is here
-  --> $DIR/shadow.rs:33:9
+  --> $DIR/shadow.rs:34:9
    |
 LL |     let x = first(x);
    |         ^
 
 error: `x` shadows a previous declaration
-  --> $DIR/shadow.rs:37:5
+  --> $DIR/shadow.rs:38:5
    |
 LL |     let x;
    |     ^^^^^^
    |
 note: previous binding is here
-  --> $DIR/shadow.rs:35:9
+  --> $DIR/shadow.rs:36:9
    |
 LL |     let x = y;
    |         ^

tests/ui/uninformative_asserts.rs

@@ -0,0 +1,43 @@
+#![warn(clippy::uninformative_asserts)]
+#![allow(clippy::blacklisted_name)]
+
+fn main() {
+    let foo = 0u8;
+    let a = 0u8;
+    let b = 0u8;
+    // lint
+    {
+        assert!(some_condition(foo));
+        assert!(some_condition(foo),);
+        debug_assert!(some_condition(foo));
+        assert_eq!(a, bar(b));
+        assert_eq!(a, bar(b),);
+        assert_ne!(a, bar(b));
+        debug_assert_eq!(a, bar(b));
+        debug_assert_ne!(a, bar(b));
+    }
+
+    // ok
+    {
+        assert!(some_condition(foo), "_");
+        assert!(some_condition(foo), "{} {}", 1, 2);
+        debug_assert!(some_condition(foo), "_");
+        assert_eq!(a, bar(b), "_");
+        assert_eq!(a, bar(b), "{} {}", 1, 2);
+        assert_ne!(a, bar(b), "_");
+        debug_assert_eq!(a, bar(b), "_");
+        debug_assert_ne!(a, bar(b), "_");
+    }
+}
+
+#[test]
+fn test_something() {
+    assert_eq!(bar(0), 0);
+}
+
+fn some_condition(_x: u8) -> bool {
+    true
+}
+fn bar(x: u8) -> u8 {
+    x
+}

tests/ui/uninformative_asserts.stderr

@@ -0,0 +1,67 @@
+error: `assert!` called without custom panic message
+  --> $DIR/uninformative_asserts.rs:10:9
+   |
+LL |         assert!(some_condition(foo));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::uninformative-asserts` implied by `-D warnings`
+   = help: consider adding a custom panic message
+
+error: `assert!` called without custom panic message
+  --> $DIR/uninformative_asserts.rs:11:9
+   |
+LL |         assert!(some_condition(foo),);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a custom panic message
+
+error: `assert!` called without custom panic message
+  --> $DIR/uninformative_asserts.rs:12:9
+   |
+LL |         debug_assert!(some_condition(foo));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a custom panic message
+
+error: `assert!` called without custom panic message
+  --> $DIR/uninformative_asserts.rs:13:9
+   |
+LL |         assert_eq!(a, bar(b));
+   |         ^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a custom panic message
+
+error: `assert!` called without custom panic message
+  --> $DIR/uninformative_asserts.rs:14:9
+   |
+LL |         assert_eq!(a, bar(b),);
+   |         ^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a custom panic message
+
+error: `assert!` called without custom panic message
+  --> $DIR/uninformative_asserts.rs:15:9
+   |
+LL |         assert_ne!(a, bar(b));
+   |         ^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a custom panic message
+
+error: `assert!` called without custom panic message
+  --> $DIR/uninformative_asserts.rs:16:9
+   |
+LL |         debug_assert_eq!(a, bar(b));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a custom panic message
+
+error: `assert!` called without custom panic message
+  --> $DIR/uninformative_asserts.rs:17:9
+   |
+LL |         debug_assert_ne!(a, bar(b));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a custom panic message
+
+error: aborting due to 8 previous errors
+