diff --git a/CHANGELOG.md b/CHANGELOG.md index 786df2ae9ab..fe64283462d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5329,6 +5329,7 @@ Released 2018-09-13 [`short_circuit_statement`]: https://rust-lang.github.io/rust-clippy/master/index.html#short_circuit_statement [`should_assert_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_assert_eq [`should_implement_trait`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait +[`should_panic_without_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_panic_without_expect [`significant_drop_in_scrutinee`]: https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee [`significant_drop_tightening`]: https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_tightening [`similar_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#similar_names diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index 2a5be275615..a88f2b51c82 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -6,7 +6,11 @@ use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::{first_line_of_span, is_present_in_source, snippet_opt, without_block_comments}; use if_chain::if_chain; -use rustc_ast::{AttrKind, AttrStyle, Attribute, LitKind, MetaItemKind, MetaItemLit, NestedMetaItem}; +use rustc_ast::token::{Token, TokenKind}; +use rustc_ast::tokenstream::TokenTree; +use rustc_ast::{ + AttrArgs, AttrArgsEq, AttrKind, AttrStyle, Attribute, LitKind, MetaItemKind, MetaItemLit, NestedMetaItem, +}; use rustc_errors::Applicability; use rustc_hir::{ Block, Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, StmtKind, TraitFn, TraitItem, TraitItemKind, @@ -339,6 +343,41 @@ "ensures that all `allow` and `expect` attributes have a reason" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `#[should_panic]` attributes without specifying the expected panic message. + /// + /// ### Why is this bad? + /// The expected panic message should be specified to ensure that the test is actually + /// panicking with the expected message, and not another unrelated panic. + /// + /// ### Example + /// ```rust + /// fn random() -> i32 { 0 } + /// + /// #[should_panic] + /// #[test] + /// fn my_test() { + /// let _ = 1 / random(); + /// } + /// ``` + /// + /// Use instead: + /// ```rust + /// fn random() -> i32 { 0 } + /// + /// #[should_panic = "attempt to divide by zero"] + /// #[test] + /// fn my_test() { + /// let _ = 1 / random(); + /// } + /// ``` + #[clippy::version = "1.73.0"] + pub SHOULD_PANIC_WITHOUT_EXPECT, + pedantic, + "ensures that all `should_panic` attributes specify its expected panic message" +} + declare_clippy_lint! { /// ### What it does /// Checks for `any` and `all` combinators in `cfg` with only one condition. @@ -395,6 +434,7 @@ DEPRECATED_SEMVER, USELESS_ATTRIBUTE, BLANKET_CLIPPY_RESTRICTION_LINTS, + SHOULD_PANIC_WITHOUT_EXPECT, ]); impl<'tcx> LateLintPass<'tcx> for Attributes { @@ -442,6 +482,9 @@ fn check_attribute(&mut self, cx: &LateContext<'tcx>, attr: &'tcx Attribute) { } } } + if attr.has_name(sym::should_panic) { + check_should_panic_reason(cx, attr); + } } fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { @@ -550,6 +593,35 @@ fn extract_clippy_lint(lint: &NestedMetaItem) -> Option { None } +fn check_should_panic_reason(cx: &LateContext<'_>, attr: &Attribute) { + if let AttrKind::Normal(normal_attr) = &attr.kind { + if let AttrArgs::Eq(_, AttrArgsEq::Hir(_)) = &normal_attr.item.args { + // `#[should_panic = ".."]` found, good + return; + } + + if let AttrArgs::Delimited(args) = &normal_attr.item.args + && let mut tt_iter = args.tokens.trees() + && let Some(TokenTree::Token(Token { kind: TokenKind::Ident(sym::expected, _), .. }, _)) = tt_iter.next() + && let Some(TokenTree::Token(Token { kind: TokenKind::Eq, .. }, _)) = tt_iter.next() + && let Some(TokenTree::Token(Token { kind: TokenKind::Literal(_), .. }, _)) = tt_iter.next() + { + // `#[should_panic(expected = "..")]` found, good + return; + } + + span_lint_and_sugg( + cx, + SHOULD_PANIC_WITHOUT_EXPECT, + attr.span, + "#[should_panic] attribute without a reason", + "consider specifying the expected panic", + r#"#[should_panic(expected = /* panic message */)]"#.into(), + Applicability::HasPlaceholders, + ); + } +} + fn check_clippy_lint_names(cx: &LateContext<'_>, name: Symbol, items: &[NestedMetaItem]) { for lint in items { if let Some(lint_name) = extract_clippy_lint(lint) { diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index db114abfc86..bab77f91269 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -58,6 +58,7 @@ crate::attrs::MAYBE_MISUSED_CFG_INFO, crate::attrs::MISMATCHED_TARGET_OS_INFO, crate::attrs::NON_MINIMAL_CFG_INFO, + crate::attrs::SHOULD_PANIC_WITHOUT_EXPECT_INFO, crate::attrs::USELESS_ATTRIBUTE_INFO, crate::await_holding_invalid::AWAIT_HOLDING_INVALID_TYPE_INFO, crate::await_holding_invalid::AWAIT_HOLDING_LOCK_INFO, diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index e72d063cfd9..6d5754eee78 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -166,3 +166,5 @@ pub const ORD_CMP: [&str; 4] = ["core", "cmp", "Ord", "cmp"]; #[expect(clippy::invalid_paths)] // not sure why it thinks this, it works so pub const BOOL_THEN: [&str; 4] = ["core", "bool", "", "then"]; +#[allow(clippy::invalid_paths, reason = "internal lints do not always know about ::test")] +pub const TEST_DESC_AND_FN: [&str; 3] = ["test", "types", "TestDescAndFn"]; diff --git a/tests/ui/should_panic_without_expect.rs b/tests/ui/should_panic_without_expect.rs new file mode 100644 index 00000000000..b554fdaf224 --- /dev/null +++ b/tests/ui/should_panic_without_expect.rs @@ -0,0 +1,21 @@ +//@no-rustfix +#![deny(clippy::should_panic_without_expect)] + +#[test] +#[should_panic] +fn no_message() {} + +#[test] +#[should_panic] +#[cfg(not(test))] +fn no_message_cfg_false() {} + +#[test] +#[should_panic = "message"] +fn metastr() {} + +#[test] +#[should_panic(expected = "message")] +fn metalist() {} + +fn main() {} diff --git a/tests/ui/should_panic_without_expect.stderr b/tests/ui/should_panic_without_expect.stderr new file mode 100644 index 00000000000..dfcef52a9f5 --- /dev/null +++ b/tests/ui/should_panic_without_expect.stderr @@ -0,0 +1,14 @@ +error: #[should_panic] attribute without a reason + --> $DIR/should_panic_without_expect.rs:5:1 + | +LL | #[should_panic] + | ^^^^^^^^^^^^^^^ help: consider specifying the expected panic: `#[should_panic(expected = /* panic message */)]` + | +note: the lint level is defined here + --> $DIR/should_panic_without_expect.rs:2:9 + | +LL | #![deny(clippy::should_panic_without_expect)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error +