Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new lint: [should_panic_without_expect] #11204

Merged
merged 2 commits into from
Aug 19, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
74 changes: 73 additions & 1 deletion clippy_lints/src/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ use clippy_utils::macros::{is_panic, macro_backtrace};
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,
Expand Down Expand Up @@ -339,6 +343,41 @@ declare_clippy_lint! {
"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.
Expand Down Expand Up @@ -395,6 +434,7 @@ declare_lint_pass!(Attributes => [
DEPRECATED_SEMVER,
USELESS_ATTRIBUTE,
BLANKET_CLIPPY_RESTRICTION_LINTS,
SHOULD_PANIC_WITHOUT_EXPECT,
]);

impl<'tcx> LateLintPass<'tcx> for Attributes {
Expand Down Expand Up @@ -442,6 +482,9 @@ impl<'tcx> LateLintPass<'tcx> for Attributes {
}
}
}
if attr.has_name(sym::should_panic) {
check_should_panic_reason(cx, attr);
}
}

fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
Expand Down Expand Up @@ -550,6 +593,35 @@ fn extract_clippy_lint(lint: &NestedMetaItem) -> Option<Symbol> {
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) {
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
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,
Expand Down
2 changes: 2 additions & 0 deletions clippy_utils/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,5 @@ pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"];
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", "<impl 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"];
Alexendoo marked this conversation as resolved.
Show resolved Hide resolved
21 changes: 21 additions & 0 deletions tests/ui/should_panic_without_expect.rs
Original file line number Diff line number Diff line change
@@ -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() {}
14 changes: 14 additions & 0 deletions tests/ui/should_panic_without_expect.stderr
Original file line number Diff line number Diff line change
@@ -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