Skip to content

Conversation

@hcbarker
Copy link
Contributor

@hcbarker hcbarker commented Oct 28, 2025

Checks for calls to functions marked with #[clippy::may_panic] or configured in may-panic-functions that lack a // Panic: comment documenting why the panic is acceptable.

fixes #15861

changelog: new_lint: [undocumented_may_panic_call]

@rustbot rustbot added needs-fcp S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2025

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot

This comment has been minimized.

@hcbarker
Copy link
Contributor Author

r? blyxyas

@rustbot rustbot assigned blyxyas and unassigned Jarcho Oct 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2025

blyxyas is not on the review rotation at the moment.
They may take a while to respond.

@hcbarker hcbarker force-pushed the may-panic-attribute branch from bab87c9 to 4bb7fdc Compare October 28, 2025 08:02
@blyxyas blyxyas added the G-Rust-for-Linux Issues related to Rust-for-Linux wants and bugfixes label Oct 28, 2025
@rustbot

This comment has been minimized.

/// #[clippy::may_panic]
/// fn my_panicable_func(n: u32) {
/// if n % 2 == 0 {
/// panic!("even number not allowed")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little typo in the documentation =^w^=

Suggested change
/// panic!("even number not allowed")
/// panic!("even numbers are not allowed")

/// #[clippy::may_panic]
/// fn my_panicable_func(n: u32) {
/// if n % 2 == 0 {
/// panic!("even number not allowed")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// panic!("even number not allowed")
/// panic!("even numbers are not allowed")

Comment on lines 86 to 90
if get_unique_attr(cx.sess(), cx.tcx.get_all_attrs(def_id), sym::may_panic).is_some() {
return true;
}

self.may_panic_def_ids.contains(&def_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if get_unique_attr(cx.sess(), cx.tcx.get_all_attrs(def_id), sym::may_panic).is_some() {
return true;
}
self.may_panic_def_ids.contains(&def_id)
get_unique_attr(cx.sess(), cx.tcx.get_all_attrs(def_id), sym::may_panic).is_some() || self.may_panic_def_ids.contains(&def_id)

Comment on lines +129 to +164
&& call_line.line > 0
&& let Some(src) = call_line.sf.src.as_deref()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to handle macros (the most difficult portion in the art of lint-making), I've been thinking about this and I think that the best option is to permit this two scenarios:

  1. The macro handles the comment, this is what's going on currently.
  2. The callsite of the macro handles the comment.

So, for macro calls that are not justified in-macro, the user will need to write a PANIC comment.

So, we'll need to recurse through the HIR parents, check if we're in a macro, and if we're not, check for the comment.

The functions to check if we're in a macro are is_from_proc_macro, in_external_macro and from_expansion


// Panic: This is safe, just trust me.
let _ = t.trait_panic_method();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more testing would be much appreciated, note that tests also test that every other lint is working correctly, so doing some extremely weird syntax (and, if it's too weird, explaining where it should lint) is very beneficial for our test suite.

Many of the bugs we receive are from extremely niche use cases, so testing for those before a lint goes into a release is very beneficial.

Suggested change
}
#[allow(clippy::needless_bool, clippy::unnecessary_operation)]
fn weird_exprs(t: &dyn MyTrait) {
// The Panic comment should be in the second If statement
if if if true { true } else { false } {
t.trait_panic_method() == 0
//~^ undocumented_may_panic_call
} else {
false
} {
true
} else {
false
};
if if if true { true } else { false } {
// PANIC: This is safe!
t.trait_panic_method() == 0
} else {
false
} {
true
} else {
false
};
#[clippy::may_panic]
fn panic<T>(val: T) -> T {val}
macro_rules! mac {
($t:tt) => ($t)
}
// Panic comment should go on the line before the panic() function
async {{};({(u8::max)(
// PANIC: Safe!
panic(1)+mac!(2),mac!(2))});
};
}
macro_rules! may_panic {
() => {dangerous(1)}
}
fn on_macro() {
// PANIC: Hi
may_panic!();
// REVIEWER: ^^^^^^^ This currently doesn't work! See other comments
}

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great first patch! Sorry for the delayed response, I took a vacation right after starting to review this (in fact, if you check the comment date for those first review comments, you'll see that I made them 2 weeks ago)

View changes since this review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 11, 2025
@hcbarker
Copy link
Contributor Author

@blyxyas Thanks for the review! It was my turn to be on vacation when I got it haha, but I'll be working on an update here over the next few days :)

Checks for calls to functions marked with `#[clippy::may_panic]` or
configured in `may-panic-functions` that lack a `// Panic:` comment
documenting why the panic is acceptable.

changelog: new_lint: [`undocumented_may_panic_call`]
- Fixing typos
- Code cleanup and more tests
- Adding handling and tests for all kinds of macros
@hcbarker hcbarker force-pushed the may-panic-attribute branch from 4bb7fdc to ac41ba9 Compare November 25, 2025 08:46
@rustbot
Copy link
Collaborator

rustbot commented Nov 25, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@hcbarker
Copy link
Contributor Author

@blyxyas I had to rebase and push --force-with-lease to fix some conflicts, and I responded to your comments and added handling and tests for macros!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

G-Rust-for-Linux Issues related to Rust-for-Linux wants and bugfixes needs-fcp S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

// PANIC comment on panic-able calls

4 participants