Add undocumented_may_panic_call lint - #15969
Conversation
This comment has been minimized.
This comment has been minimized.
|
r? blyxyas |
|
|
bab87c9 to
4bb7fdc
Compare
This comment has been minimized.
This comment has been minimized.
| /// #[clippy::may_panic] | ||
| /// fn my_panicable_func(n: u32) { | ||
| /// if n % 2 == 0 { | ||
| /// panic!("even number not allowed") |
There was a problem hiding this comment.
Little typo in the documentation =^w^=
| /// 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") |
There was a problem hiding this comment.
| /// panic!("even number not allowed") | |
| /// panic!("even numbers are not allowed") |
| 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) |
There was a problem hiding this comment.
| 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) |
| && call_line.line > 0 | ||
| && let Some(src) = call_line.sf.src.as_deref() |
There was a problem hiding this comment.
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:
- The macro handles the comment, this is what's going on currently.
- 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(); | ||
| } |
There was a problem hiding this comment.
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.
| } | |
| #[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 | |
| } |
|
@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 :) |
4bb7fdc to
ac41ba9
Compare
This comment has been minimized.
This comment has been minimized.
|
@blyxyas I had to rebase and |
This comment has been minimized.
This comment has been minimized.
| get_builtin_attr(cx.sess(), cx.tcx.get_all_attrs(def_id), sym::may_panic).count() > 0 | ||
| || self.may_panic_def_ids.contains(&def_id) |
There was a problem hiding this comment.
Even if less frequent, cheaper operations should go earlier ଲ( ⓛ ω ⓛ *)ଲ
| get_builtin_attr(cx.sess(), cx.tcx.get_all_attrs(def_id), sym::may_panic).count() > 0 | |
| || self.may_panic_def_ids.contains(&def_id) | |
| self.may_panic_def_ids.contains(&def_id) || | |
| get_builtin_attr(cx.sess(), cx.tcx.get_all_attrs(def_id), sym::may_panic).count() > 0 |
| hir::ExprKind::MethodCall(_path, _receiver, _args, _span) => { | ||
| cx.typeck_results().type_dependent_def_id(expr.hir_id) | ||
| }, | ||
| _ => None, |
There was a problem hiding this comment.
Instead of None, what about a return?
| _ => None, | |
| _ => return, |
| let mut lint_span = None; | ||
| for macro_call in macro_backtrace(call_span) { | ||
| if has_panic_comment_above_span(cx, macro_call.span) { | ||
| return None; | ||
| } | ||
| lint_span = Some(macro_call.span); | ||
| } |
|
@hcbarker How are you doing with this? I think that the lint would still be great to have |
|
@blyxyas Ah! thanks for the bump, this totally fell off my radar. I'll get back to it and finish it up |
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
ac41ba9 to
0c13765
Compare
|
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. |
|
r? blyxyas thanks for the comments! |
|
r? blyxyas |
|
☔ The latest upstream changes (possibly #16687) made this pull request unmergeable. Please resolve the merge conflicts. |
Checks for calls to functions marked with
#[clippy::may_panic]or configured inmay-panic-functionsthat lack a// Panic:comment documenting why the panic is acceptable.fixes #15861
changelog: new_lint: [
undocumented_may_panic_call]