From ea2547b8c63ce4e410c53e7fa55b127c81721454 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Burak=20Varl=C4=B1?= Date: Thu, 16 Feb 2023 14:50:51 +0000 Subject: [PATCH 01/10] Add `missing_assert_message` lint Co-authored-by: Weihang Lo --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/missing_assert_message.rs | 123 +++++++++++++++++++++ tests/ui/filter_map_next_fixable.fixed | 2 +- tests/ui/filter_map_next_fixable.rs | 2 +- tests/ui/missing_assert_message.rs | 84 ++++++++++++++ tests/ui/missing_assert_message.stderr | 100 +++++++++++++++++ 8 files changed, 313 insertions(+), 2 deletions(-) create mode 100644 clippy_lints/src/missing_assert_message.rs create mode 100644 tests/ui/missing_assert_message.rs create mode 100644 tests/ui/missing_assert_message.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 9508ab57cb87..62cc7437b824 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 470a2e79e479..462fcb6483de 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ae7fdd6be26b..fe0229a814ff 100644 --- a/clippy_lints/src/lib.rs +++ b/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_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 new file mode 100644 index 000000000000..a884b22370d5 --- /dev/null +++ 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 index 462d46169fcb..0568eff41b52 100644 --- 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 index 2ea00cf73072..b0722ee8258f 100644 --- 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 new file mode 100644 index 000000000000..89404ca88271 --- /dev/null +++ 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 new file mode 100644 index 000000000000..900966500c88 --- /dev/null +++ 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 + From 8f3ac65227b4744924fef56d13de991acc04395b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Burak=20Varl=C4=B1?= Date: Thu, 16 Feb 2023 16:58:51 +0000 Subject: [PATCH 02/10] Dogfood `missing_assert_message` on Clippy Co-authored-by: Weihang Lo --- clippy_lints/src/doc.rs | 2 +- clippy_lints/src/duplicate_mod.rs | 6 +++++- clippy_lints/src/enum_variants.rs | 2 +- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/non_expressive_names.rs | 5 ++++- clippy_utils/src/attrs.rs | 6 ++++-- clippy_utils/src/lib.rs | 11 +++++++---- clippy_utils/src/numeric_literal.rs | 6 +++--- clippy_utils/src/ty.rs | 3 +-- lintcheck/src/main.rs | 3 ++- tests/compile-test.rs | 5 ++++- tests/integration.rs | 1 + tests/test_utils/mod.rs | 2 +- 13 files changed, 35 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 384aca7feadd..b1c2a51daa0e 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -467,7 +467,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, "`offset` shouldn't overflow `u32`"); 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)))); diff --git a/clippy_lints/src/duplicate_mod.rs b/clippy_lints/src/duplicate_mod.rs index 7ff7068f0b05..9135af40979d 100644 --- a/clippy_lints/src/duplicate_mod.rs +++ b/clippy_lints/src/duplicate_mod.rs @@ -90,7 +90,11 @@ impl EarlyLintPass for DuplicateMod { } // At this point the lint would be emitted - assert_eq!(spans.len(), lint_levels.len()); + assert_eq!( + spans.len(), + lint_levels.len(), + "`spans` and `lint_levels` should have equal lengths" + ); let spans: Vec<_> = spans .iter() .zip(lint_levels) diff --git a/clippy_lints/src/enum_variants.rs b/clippy_lints/src/enum_variants.rs index 4c69dacf381a..68fb88bcf7e3 100644 --- a/clippy_lints/src/enum_variants.rs +++ b/clippy_lints/src/enum_variants.rs @@ -242,7 +242,7 @@ fn to_camel_case(item_name: &str) -> String { impl LateLintPass<'_> for EnumVariantNames { fn check_item_post(&mut self, _cx: &LateContext<'_>, _item: &Item<'_>) { let last = self.modules.pop(); - assert!(last.is_some()); + assert!(last.is_some(), "`modules` should not be empty"); } #[expect(clippy::similar_names)] diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fe0229a814ff..e3be798f30b0 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -927,7 +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_pre_expansion_pass(|| Box::new(missing_assert_message::MissingAssertMessage)); + store.register_pre_expansion_pass(|| Box::::default()); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs index 9f6917c146f6..e24f3aa567a7 100644 --- a/clippy_lints/src/non_expressive_names.rs +++ b/clippy_lints/src/non_expressive_names.rs @@ -408,7 +408,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(), + "Precondition: `a_name.chars().count() < b_name.chars().count()` does not meet" + ); 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()) { diff --git a/clippy_utils/src/attrs.rs b/clippy_utils/src/attrs.rs index 7987a233bdc1..75f7b5cf98ed 100644 --- a/clippy_utils/src/attrs.rs +++ b/clippy_utils/src/attrs.rs @@ -31,7 +31,7 @@ pub struct LimitStack { impl Drop for LimitStack { fn drop(&mut self) { - assert_eq!(self.stack.len(), 1); + assert_eq!(self.stack.len(), 1, "stack should only have one element"); } } @@ -49,7 +49,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), "incorrect last element"); + }); } } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 213e5b33503e..1c453b87f8d7 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1011,10 +1011,13 @@ pub fn capture_local_usage(cx: &LateContext<'_>, e: &Expr<'_>) -> CaptureKind { capture } - debug_assert!(matches!( - e.kind, - ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(_), .. })) - )); + debug_assert!( + matches!( + e.kind, + ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(_), .. })) + ), + "`e.kind` should be a resolved local path" + ); let mut child_id = e.hir_id; let mut capture = CaptureKind::Value; diff --git a/clippy_utils/src/numeric_literal.rs b/clippy_utils/src/numeric_literal.rs index c225398ad2a8..7d8f31e1dfbc 100644 --- a/clippy_utils/src/numeric_literal.rs +++ b/clippy_utils/src/numeric_literal.rs @@ -179,7 +179,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, "group size should be greater than zero"); let mut digits = input.chars().filter(|&c| c != '_'); @@ -219,7 +219,7 @@ 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(), "`lit_kind` should be numeric"); lit_suffix_length(lit_kind) .and_then(|suffix_length| src.len().checked_sub(suffix_length)) .map_or((src, None), |split_pos| { @@ -229,7 +229,7 @@ fn split_suffix<'a>(src: &'a str, lit_kind: &LitKind) -> (&'a str, Option<&'a st } fn lit_suffix_length(lit_kind: &LitKind) -> Option { - debug_assert!(lit_kind.is_numeric()); + debug_assert!(lit_kind.is_numeric(), "`lit_kind` should be numeric"); let suffix = match lit_kind { LitKind::Int(_, int_lit_kind) => match int_lit_kind { LitIntType::Signed(int_ty) => Some(int_ty.name_str()), diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 7cbb77ea2a8b..2b3c781477fb 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -225,8 +225,7 @@ pub fn implements_trait_with_env<'tcx>( trait_id: DefId, ty_params: impl IntoIterator>>, ) -> bool { - // Clippy shouldn't have infer types - assert!(!ty.needs_infer()); + assert!(!ty.needs_infer(), "Clippy shouldn't have infer types"); let ty = tcx.erase_regions(ty); if ty.has_escaping_bound_vars() { diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index 23c852980275..6e39c8d42432 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -383,7 +383,7 @@ impl Crate { .status() .expect("failed to run cargo"); - assert_eq!(status.code(), Some(0)); + assert_eq!(status.code(), Some(0), "`cargo check` exited with non-zero code"); return Vec::new(); } @@ -741,6 +741,7 @@ fn print_stats(old_stats: HashMap, new_stats: HashMap<&String, us let mut new_stats_deduped = new_stats; // remove duplicates from both hashmaps + #[allow(clippy::missing_assert_message)] for (k, v) in &same_in_both_hashmaps { assert!(old_stats_deduped.remove(k) == Some(*v)); assert!(new_stats_deduped.remove(k) == Some(*v)); diff --git a/tests/compile-test.rs b/tests/compile-test.rs index c10ee969c014..2f2d305f54ba 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -410,7 +410,10 @@ fn check_rustfix_coverage() { }; if let Ok(missing_coverage_contents) = std::fs::read_to_string(missing_coverage_path) { - assert!(RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS.iter().is_sorted_by_key(Path::new)); + assert!( + RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS.iter().is_sorted_by_key(Path::new), + "`RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS` should be sorted" + ); for rs_file in missing_coverage_contents.lines() { let rs_path = Path::new(rs_file); diff --git a/tests/integration.rs b/tests/integration.rs index a771d8b87c81..2d2d6e6739ee 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -21,6 +21,7 @@ const CARGO_CLIPPY: &str = "cargo-clippy"; const CARGO_CLIPPY: &str = "cargo-clippy.exe"; #[cfg_attr(feature = "integration", test)] +#[allow(clippy::missing_assert_message)] fn integration_test() { let repo_name = env::var("INTEGRATION").expect("`INTEGRATION` var not set"); let repo_url = format!("https://github.com/{repo_name}"); diff --git a/tests/test_utils/mod.rs b/tests/test_utils/mod.rs index ea8c54e08b33..3081bf2d8ccc 100644 --- a/tests/test_utils/mod.rs +++ b/tests/test_utils/mod.rs @@ -5,7 +5,7 @@ use std::sync::LazyLock; pub static CARGO_CLIPPY_PATH: LazyLock = LazyLock::new(|| { let mut path = std::env::current_exe().unwrap(); - assert!(path.pop()); // deps + assert!(path.pop(), "current running executable path shouldn't be empty"); // deps path.set_file_name("cargo-clippy"); path }); From 099d610640a5ab35a25218af6cedfc4ece765aa0 Mon Sep 17 00:00:00 2001 From: Burak Date: Sat, 18 Feb 2023 18:40:33 +0100 Subject: [PATCH 03/10] Apply suggestions from code review Co-authored-by: llogiq --- clippy_lints/src/missing_assert_message.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/missing_assert_message.rs b/clippy_lints/src/missing_assert_message.rs index a884b22370d5..07424eb46a48 100644 --- a/clippy_lints/src/missing_assert_message.rs +++ b/clippy_lints/src/missing_assert_message.rs @@ -10,10 +10,10 @@ use rustc_span::sym; declare_clippy_lint! { /// ### What it does - /// Checks assertions that doesn't have a custom panic message. + /// Checks assertions 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. + /// If the assertion fails, the custom message may make it easier to understand what went wrong. /// /// ### Example /// ```rust @@ -30,7 +30,7 @@ declare_clippy_lint! { #[clippy::version = "1.69.0"] pub MISSING_ASSERT_MESSAGE, pedantic, - "checks assertions that doesn't have a custom panic message" + "checks assertions without a custom panic message" } #[derive(Default, Clone, Debug)] From 4eb6ccc9731095ed014bd5c047bcd7dd75bdf335 Mon Sep 17 00:00:00 2001 From: unexge Date: Sat, 18 Feb 2023 19:09:31 +0000 Subject: [PATCH 04/10] Update lint description and add help section Co-authored-by: Weihang Lo --- clippy_lints/src/missing_assert_message.rs | 24 ++++++++++------- tests/ui/missing_assert_message.stderr | 31 ++++++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/missing_assert_message.rs b/clippy_lints/src/missing_assert_message.rs index 07424eb46a48..3b73332215e1 100644 --- a/clippy_lints/src/missing_assert_message.rs +++ b/clippy_lints/src/missing_assert_message.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::span_lint; +use clippy_utils::diagnostics::span_lint_and_help; use rustc_ast::ast; use rustc_ast::{ token::{Token, TokenKind}, @@ -13,19 +13,23 @@ declare_clippy_lint! { /// Checks assertions without a custom panic message. /// /// ### Why is this bad? - /// If the assertion fails, the custom message may make it easier to understand what went wrong. + /// 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. /// /// ### Example /// ```rust - /// let threshold = 50; - /// let num = 42; - /// assert!(num < threshold); + /// # struct Service { ready: bool } + /// fn call(service: Service) { + /// assert!(service.ready); + /// } /// ``` /// Use instead: /// ```rust - /// let threshold = 50; - /// let num = 42; - /// assert!(num < threshold, "{num} is lower than threshold ({threshold})"); + /// # 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, @@ -56,11 +60,13 @@ impl EarlyLintPass for MissingAssertMessage { let num_separators = num_commas_on_arguments(mac_call); if num_separators < num_separators_needed { - span_lint( + span_lint_and_help( cx, MISSING_ASSERT_MESSAGE, mac_call.span(), "assert without any message", + None, + "consider describing why the failing assert is problematic", ); } } diff --git a/tests/ui/missing_assert_message.stderr b/tests/ui/missing_assert_message.stderr index 900966500c88..ecd038012779 100644 --- a/tests/ui/missing_assert_message.stderr +++ b/tests/ui/missing_assert_message.stderr @@ -4,6 +4,7 @@ error: assert without any message LL | assert!(foo()); | ^^^^^^^^^^^^^^ | + = help: consider describing why the failing assert is problematic = note: `-D clippy::missing-assert-message` implied by `-D warnings` error: assert without any message @@ -11,90 +12,120 @@ error: assert without any message | LL | assert_eq!(foo(), foo()); | ^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:16:5 | LL | assert_ne!(foo(), foo()); | ^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:17:5 | LL | debug_assert!(foo()); | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:18:5 | LL | debug_assert_eq!(foo(), foo()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:19:5 | LL | debug_assert_ne!(foo(), foo()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:24:5 | LL | assert!(bar!(true)); | ^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:25:5 | LL | assert!(bar!(true, false)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:26:5 | LL | assert_eq!(bar!(true), foo()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:27:5 | LL | assert_ne!(bar!(true, true), bar!(true)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:32:5 | LL | assert!(foo(),); | ^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:33:5 | LL | assert_eq!(foo(), foo(),); | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:34:5 | LL | assert_ne!(foo(), foo(),); | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:35:5 | LL | debug_assert!(foo(),); | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:36:5 | LL | debug_assert_eq!(foo(), foo(),); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: assert without any message --> $DIR/missing_assert_message.rs:37:5 | LL | debug_assert_ne!(foo(), foo(),); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider describing why the failing assert is problematic error: aborting due to 16 previous errors From 6fac73b9874ea79fbac5f0fa9429c726c196d9a6 Mon Sep 17 00:00:00 2001 From: unexge Date: Sat, 18 Feb 2023 19:14:50 +0000 Subject: [PATCH 05/10] Move lint to `restriction` category Co-authored-by: Weihang Lo --- clippy_lints/src/missing_assert_message.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/missing_assert_message.rs b/clippy_lints/src/missing_assert_message.rs index 3b73332215e1..f499b3e50ca5 100644 --- a/clippy_lints/src/missing_assert_message.rs +++ b/clippy_lints/src/missing_assert_message.rs @@ -33,7 +33,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.69.0"] pub MISSING_ASSERT_MESSAGE, - pedantic, + restriction, "checks assertions without a custom panic message" } From b4b2b1235a07aeb5f6d6e345e23d8fa4873cda08 Mon Sep 17 00:00:00 2001 From: unexge Date: Sat, 18 Feb 2023 19:18:52 +0000 Subject: [PATCH 06/10] Revert "Dogfood `missing_assert_message` on Clippy" This reverts commit ec653570ad50d11ecc3b5649dd28e29ed96199d3. --- clippy_lints/src/doc.rs | 2 +- clippy_lints/src/duplicate_mod.rs | 6 +----- clippy_lints/src/enum_variants.rs | 2 +- clippy_lints/src/non_expressive_names.rs | 5 +---- clippy_utils/src/attrs.rs | 6 ++---- clippy_utils/src/lib.rs | 11 ++++------- clippy_utils/src/numeric_literal.rs | 6 +++--- clippy_utils/src/ty.rs | 3 ++- lintcheck/src/main.rs | 3 +-- tests/compile-test.rs | 5 +---- tests/integration.rs | 1 - tests/test_utils/mod.rs | 2 +- 12 files changed, 18 insertions(+), 34 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index b1c2a51daa0e..384aca7feadd 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -467,7 +467,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, "`offset` shouldn't overflow `u32`"); + debug_assert_eq!(offset as u32 as usize, offset); 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)))); diff --git a/clippy_lints/src/duplicate_mod.rs b/clippy_lints/src/duplicate_mod.rs index 9135af40979d..7ff7068f0b05 100644 --- a/clippy_lints/src/duplicate_mod.rs +++ b/clippy_lints/src/duplicate_mod.rs @@ -90,11 +90,7 @@ impl EarlyLintPass for DuplicateMod { } // At this point the lint would be emitted - assert_eq!( - spans.len(), - lint_levels.len(), - "`spans` and `lint_levels` should have equal lengths" - ); + assert_eq!(spans.len(), lint_levels.len()); let spans: Vec<_> = spans .iter() .zip(lint_levels) diff --git a/clippy_lints/src/enum_variants.rs b/clippy_lints/src/enum_variants.rs index 68fb88bcf7e3..4c69dacf381a 100644 --- a/clippy_lints/src/enum_variants.rs +++ b/clippy_lints/src/enum_variants.rs @@ -242,7 +242,7 @@ fn to_camel_case(item_name: &str) -> String { impl LateLintPass<'_> for EnumVariantNames { fn check_item_post(&mut self, _cx: &LateContext<'_>, _item: &Item<'_>) { let last = self.modules.pop(); - assert!(last.is_some(), "`modules` should not be empty"); + assert!(last.is_some()); } #[expect(clippy::similar_names)] diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs index e24f3aa567a7..9f6917c146f6 100644 --- a/clippy_lints/src/non_expressive_names.rs +++ b/clippy_lints/src/non_expressive_names.rs @@ -408,10 +408,7 @@ 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(), - "Precondition: `a_name.chars().count() < b_name.chars().count()` does not meet" - ); + debug_assert!(a_name.chars().count() < b_name.chars().count()); 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()) { diff --git a/clippy_utils/src/attrs.rs b/clippy_utils/src/attrs.rs index 75f7b5cf98ed..7987a233bdc1 100644 --- a/clippy_utils/src/attrs.rs +++ b/clippy_utils/src/attrs.rs @@ -31,7 +31,7 @@ pub struct LimitStack { impl Drop for LimitStack { fn drop(&mut self) { - assert_eq!(self.stack.len(), 1, "stack should only have one element"); + assert_eq!(self.stack.len(), 1); } } @@ -49,9 +49,7 @@ 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), "incorrect last element"); - }); + parse_attrs(sess, attrs, name, |val| assert_eq!(stack.pop(), Some(val))); } } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 1c453b87f8d7..213e5b33503e 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1011,13 +1011,10 @@ pub fn capture_local_usage(cx: &LateContext<'_>, e: &Expr<'_>) -> CaptureKind { capture } - debug_assert!( - matches!( - e.kind, - ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(_), .. })) - ), - "`e.kind` should be a resolved local path" - ); + debug_assert!(matches!( + e.kind, + ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(_), .. })) + )); let mut child_id = e.hir_id; let mut capture = CaptureKind::Value; diff --git a/clippy_utils/src/numeric_literal.rs b/clippy_utils/src/numeric_literal.rs index 7d8f31e1dfbc..c225398ad2a8 100644 --- a/clippy_utils/src/numeric_literal.rs +++ b/clippy_utils/src/numeric_literal.rs @@ -179,7 +179,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, "group size should be greater than zero"); + debug_assert!(group_size > 0); let mut digits = input.chars().filter(|&c| c != '_'); @@ -219,7 +219,7 @@ 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(), "`lit_kind` should be numeric"); + debug_assert!(lit_kind.is_numeric()); lit_suffix_length(lit_kind) .and_then(|suffix_length| src.len().checked_sub(suffix_length)) .map_or((src, None), |split_pos| { @@ -229,7 +229,7 @@ fn split_suffix<'a>(src: &'a str, lit_kind: &LitKind) -> (&'a str, Option<&'a st } fn lit_suffix_length(lit_kind: &LitKind) -> Option { - debug_assert!(lit_kind.is_numeric(), "`lit_kind` should be numeric"); + debug_assert!(lit_kind.is_numeric()); let suffix = match lit_kind { LitKind::Int(_, int_lit_kind) => match int_lit_kind { LitIntType::Signed(int_ty) => Some(int_ty.name_str()), diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 2b3c781477fb..7cbb77ea2a8b 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -225,7 +225,8 @@ pub fn implements_trait_with_env<'tcx>( trait_id: DefId, ty_params: impl IntoIterator>>, ) -> bool { - assert!(!ty.needs_infer(), "Clippy shouldn't have infer types"); + // Clippy shouldn't have infer types + assert!(!ty.needs_infer()); let ty = tcx.erase_regions(ty); if ty.has_escaping_bound_vars() { diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index 6e39c8d42432..23c852980275 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -383,7 +383,7 @@ impl Crate { .status() .expect("failed to run cargo"); - assert_eq!(status.code(), Some(0), "`cargo check` exited with non-zero code"); + assert_eq!(status.code(), Some(0)); return Vec::new(); } @@ -741,7 +741,6 @@ fn print_stats(old_stats: HashMap, new_stats: HashMap<&String, us let mut new_stats_deduped = new_stats; // remove duplicates from both hashmaps - #[allow(clippy::missing_assert_message)] for (k, v) in &same_in_both_hashmaps { assert!(old_stats_deduped.remove(k) == Some(*v)); assert!(new_stats_deduped.remove(k) == Some(*v)); diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 2f2d305f54ba..c10ee969c014 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -410,10 +410,7 @@ fn check_rustfix_coverage() { }; if let Ok(missing_coverage_contents) = std::fs::read_to_string(missing_coverage_path) { - assert!( - RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS.iter().is_sorted_by_key(Path::new), - "`RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS` should be sorted" - ); + assert!(RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS.iter().is_sorted_by_key(Path::new)); for rs_file in missing_coverage_contents.lines() { let rs_path = Path::new(rs_file); diff --git a/tests/integration.rs b/tests/integration.rs index 2d2d6e6739ee..a771d8b87c81 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -21,7 +21,6 @@ const CARGO_CLIPPY: &str = "cargo-clippy"; const CARGO_CLIPPY: &str = "cargo-clippy.exe"; #[cfg_attr(feature = "integration", test)] -#[allow(clippy::missing_assert_message)] fn integration_test() { let repo_name = env::var("INTEGRATION").expect("`INTEGRATION` var not set"); let repo_url = format!("https://github.com/{repo_name}"); diff --git a/tests/test_utils/mod.rs b/tests/test_utils/mod.rs index 3081bf2d8ccc..ea8c54e08b33 100644 --- a/tests/test_utils/mod.rs +++ b/tests/test_utils/mod.rs @@ -5,7 +5,7 @@ use std::sync::LazyLock; pub static CARGO_CLIPPY_PATH: LazyLock = LazyLock::new(|| { let mut path = std::env::current_exe().unwrap(); - assert!(path.pop(), "current running executable path shouldn't be empty"); // deps + assert!(path.pop()); // deps path.set_file_name("cargo-clippy"); path }); From e7065efc764316dae08376ca2edbb8617c425911 Mon Sep 17 00:00:00 2001 From: unexge Date: Sat, 18 Feb 2023 19:42:09 +0000 Subject: [PATCH 07/10] Revert `tests/ui/filter_map_next_fixable.rs` Co-authored-by: Weihang Lo --- tests/ui/filter_map_next_fixable.fixed | 2 +- tests/ui/filter_map_next_fixable.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ui/filter_map_next_fixable.fixed b/tests/ui/filter_map_next_fixable.fixed index 0568eff41b52..462d46169fcb 100644 --- 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, clippy::missing_assert_message)] +#![allow(unused)] 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 index b0722ee8258f..2ea00cf73072 100644 --- 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, clippy::missing_assert_message)] +#![allow(unused)] fn main() { let a = ["1", "lol", "3", "NaN", "5"]; From 682d52cf7c83b95a73cb64c1f938bfab37d528ab Mon Sep 17 00:00:00 2001 From: unexge Date: Fri, 24 Feb 2023 00:18:59 +0000 Subject: [PATCH 08/10] Update assertion macro parsing logic for Rust 1.52 changes Co-authored-by: Weihang Lo --- clippy_utils/src/macros.rs | 42 +++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/clippy_utils/src/macros.rs b/clippy_utils/src/macros.rs index be6133d32024..16a5ee766456 100644 --- a/clippy_utils/src/macros.rs +++ b/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 { - 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) From 87f58a1a4fb0a95973add9ff8c0c8e3439599e8a Mon Sep 17 00:00:00 2001 From: unexge Date: Fri, 24 Feb 2023 00:20:47 +0000 Subject: [PATCH 09/10] Use late lint pass for `missing_assert_message` lint Co-authored-by: Weihang Lo --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/missing_assert_message.rs | 106 +++++---------------- 2 files changed, 27 insertions(+), 81 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index e3be798f30b0..6d4ec5277567 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -927,7 +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_pre_expansion_pass(|| Box::::default()); + store.register_late_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 index f499b3e50ca5..2ff0cee2925c 100644 --- a/clippy_lints/src/missing_assert_message.rs +++ b/clippy_lints/src/missing_assert_message.rs @@ -1,11 +1,9 @@ use clippy_utils::diagnostics::span_lint_and_help; -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 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! { @@ -37,93 +35,41 @@ declare_clippy_lint! { "checks assertions without 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, -} +declare_lint_pass!(MissingAssertMessage => [MISSING_ASSERT_MESSAGE]); -impl_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, + }; -impl EarlyLintPass for MissingAssertMessage { - fn check_mac(&mut self, cx: &EarlyContext<'_>, mac_call: &ast::MacCall) { - if self.test_deepnes != 0 { + // 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 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 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 }; - let num_separators = num_commas_on_arguments(mac_call); - if num_separators < num_separators_needed { + if let PanicExpn::Empty = panic_expn { span_lint_and_help( cx, MISSING_ASSERT_MESSAGE, - mac_call.span(), + macro_call.span, "assert without any message", None, "consider describing why the failing assert is problematic", ); } } - - 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 - } } From b554ff4cd833a60e753050703dc5d1384607d1da Mon Sep 17 00:00:00 2001 From: unexge Date: Sun, 5 Mar 2023 14:25:22 +0000 Subject: [PATCH 10/10] Add `Known problems` section Co-authored-by: Weihang Lo --- clippy_lints/src/missing_assert_message.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/clippy_lints/src/missing_assert_message.rs b/clippy_lints/src/missing_assert_message.rs index 2ff0cee2925c..2214a568d9c6 100644 --- a/clippy_lints/src/missing_assert_message.rs +++ b/clippy_lints/src/missing_assert_message.rs @@ -15,6 +15,13 @@ declare_clippy_lint! { /// 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 }