Responding to PR comments

- Fixing typos
- Code cleanup and more tests
- Adding handling and tests for all kinds of macros

Author: hcbarker

clippy_lints/src/lib.rs

@@ -849,7 +849,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
         Box::new(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)),
         Box::new(|_| Box::new(volatile_composites::VolatileComposites)),
         Box::new(|_| Box::<replace_box::ReplaceBox>::default()),
-        Box::new(move |tcx| Box::new(undocumented_may_panic_call::UndocumentedMayPanicCall::new(tcx, conf)))
+        Box::new(move |tcx| Box::new(undocumented_may_panic_call::UndocumentedMayPanicCall::new(tcx, conf))),
         // add late passes here, used by `cargo dev new_lint`
     ];
     store.late_passes.extend(late_lints);

clippy_lints/src/undocumented_may_panic_call.rs

@@ -1,7 +1,8 @@
 use clippy_config::Conf;
 use clippy_utils::diagnostics::span_lint;
+use clippy_utils::macros::macro_backtrace;
 use clippy_utils::paths::{PathNS, lookup_path_str};
-use clippy_utils::{get_unique_attr, sym};
+use clippy_utils::{get_builtin_attr, is_from_proc_macro, sym};
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
@@ -24,7 +25,7 @@ declare_clippy_lint! {
     /// #[clippy::may_panic]
     /// fn my_panicable_func(n: u32) {
     ///     if n % 2 == 0 {
-    ///         panic!("even number not allowed")
+    ///         panic!("even numbers are not allowed")
     ///     }
     /// }
     ///
@@ -38,7 +39,7 @@ declare_clippy_lint! {
     /// #[clippy::may_panic]
     /// fn my_panicable_func(n: u32) {
     ///     if n % 2 == 0 {
-    ///         panic!("even number not allowed")
+    ///         panic!("even numbers are not allowed")
     ///     }
     /// }
     ///
@@ -83,16 +84,13 @@ impl UndocumentedMayPanicCall {
     // A function is a may_panic_function if it has the may_panic attribute
     // or is in the may-panic-functions configuration
     fn is_may_panic_function(&self, cx: &LateContext<'_>, def_id: DefId) -> bool {
-        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_builtin_attr(cx.sess(), cx.tcx.get_all_attrs(def_id), sym::may_panic).count() > 0
+            || self.may_panic_def_ids.contains(&def_id)
     }
 }
 
-impl LateLintPass<'_> for UndocumentedMayPanicCall {
-    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ rustc_hir::Expr<'_>) {
+impl<'tcx> LateLintPass<'tcx> for UndocumentedMayPanicCall {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
         let def_id = match &expr.kind {
             hir::ExprKind::Call(func, _args) => {
                 if let hir::ExprKind::Path(qpath) = &func.kind {
@@ -109,23 +107,59 @@ impl LateLintPass<'_> for UndocumentedMayPanicCall {
 
         if let Some(def_id) = def_id
             && self.is_may_panic_function(cx, def_id)
-            && !has_panic_comment_above(cx, expr.span)
+            && let Some(lint_span) = check_for_missing_panic_comment(cx, expr)
         {
             span_lint(
                 cx,
                 UNDOCUMENTED_MAY_PANIC_CALL,
-                expr.span,
+                lint_span,
                 "call to a function that may panic is not documented with a `// Panic:` comment",
             );
         }
     }
 }
 
-/// Checks if the lines immediately preceding the call contain a "Panic:" comment.
-fn has_panic_comment_above(cx: &LateContext<'_>, call_span: rustc_span::Span) -> bool {
+/// Checks if a panic comment is missing and returns the span to lint at
+/// Returns `None` if a panic comment exists
+/// Returns `Some(span)` if a panic comment is missing
+fn check_for_missing_panic_comment<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx hir::Expr<'tcx>,
+) -> Option<rustc_span::Span> {
+    let call_span = expr.span;
+
+    if call_span.from_expansion() {
+        // For external macros or proc macros, the user cannot modify the macro body,
+        // so we only check callsites
+        let is_external_or_proc_macro =
+            call_span.in_external_macro(cx.sess().source_map()) || is_from_proc_macro(cx, expr);
+
+        // For locally defined macros, check the macro body first before checking the callsite
+        if !is_external_or_proc_macro && has_panic_comment_above_span(cx, call_span) {
+            return None;
+        }
+
+        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);
+        }
+
+        lint_span
+    } else if has_panic_comment_above_span(cx, call_span) {
+        None
+    } else {
+        Some(call_span)
+    }
+}
+
+/// Checks if the lines immediately preceding a span contain a "Panic:" comment
+fn has_panic_comment_above_span(cx: &LateContext<'_>, span: rustc_span::Span) -> bool {
     let source_map = cx.sess().source_map();
 
-    if let Ok(call_line) = source_map.lookup_line(call_span.lo())
+    if let Ok(call_line) = source_map.lookup_line(span.lo())
         && call_line.line > 0
         && let Some(src) = call_line.sf.src.as_deref()
     {

clippy_utils/src/attrs.rs

@@ -34,7 +34,7 @@ pub fn get_builtin_attr<'a, A: AttributeExt + 'a>(
                 // The following attributes are for the 3rd party crate authors.
                 // See book/src/attribs.md
                 | sym::has_significant_drop
-                | sym::format_args 
+                | sym::format_args
                 | sym::may_panic => None,
                 _ => {
                     sess.dcx().span_err(segment2.span, "usage of unknown attribute");

tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.rs

@@ -1,5 +1,9 @@
+//@aux-build:../../ui/auxiliary/proc_macro_derive.rs
 #![warn(clippy::undocumented_may_panic_call)]
 
+extern crate proc_macro_derive;
+use proc_macro_derive::DeriveMayPanic;
+
 fn main() {
     let mut v = vec![1, 2, 3];
 
@@ -9,3 +13,13 @@ fn main() {
     // Panic: The capaticy is less than isize::MAX, so we are safe!
     v.push(5);
 }
+
+// Test proc macro derives with configured may-panic function (Vec::push)
+
+#[derive(DeriveMayPanic)]
+//~^ undocumented_may_panic_call
+struct TestProcMacroNoComment;
+
+// Panic: This derive macro is totally safe for this struct!
+#[derive(DeriveMayPanic)]
+struct TestProcMacroWithComment;

tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.stderr

@@ -1,11 +1,17 @@
 error: call to a function that may panic is not documented with a `// Panic:` comment
-  --> tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.rs:6:5
+  --> tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.rs:10:5
    |
 LL |     v.push(4);
    |     ^^^^^^^^^
    |
    = note: `-D clippy::undocumented-may-panic-call` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::undocumented_may_panic_call)]`
 
-error: aborting due to 1 previous error
+error: call to a function that may panic is not documented with a `// Panic:` comment
+  --> tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.rs:19:10
+   |
+LL | #[derive(DeriveMayPanic)]
+   |          ^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
 

tests/ui/auxiliary/macro_rules.rs

@@ -69,3 +69,11 @@ macro_rules! double_parens {
         InterruptMask((($a.union($b).union($c).union($d)).into_bits()) as u32)
     }};
 }
+
+/// Used for testing undocumented_may_panic_call
+#[macro_export]
+macro_rules! external_macro_may_panic {
+    ($func:path) => {
+        $func()
+    };
+}

tests/ui/auxiliary/proc_macro_derive.rs

@@ -241,3 +241,14 @@ pub fn derive_double_parens(_: TokenStream) -> TokenStream {
         }
     }
 }
+
+#[proc_macro_derive(DeriveMayPanic)]
+pub fn derive_may_panic(_: TokenStream) -> TokenStream {
+    // Used for testing the `undocumented_may_panic_call` lint's handling of proc macros
+    quote! {
+        fn _derived_may_panic_fn() {
+            let mut v = Vec::new();
+            v.push(1);
+        }
+    }
+}

tests/ui/undocumented_may_panic_call.rs

@@ -1,7 +1,10 @@
+//@aux-build:macro_rules.rs
 #![warn(clippy::undocumented_may_panic_call)]
 #![allow(clippy::short_circuit_statement)]
 #![allow(clippy::manual_is_multiple_of)]
 
+extern crate macro_rules;
+
 #[clippy::may_panic]
 pub fn dangerous(n: usize) -> bool {
     if n % 2 == 0 {
@@ -84,3 +87,181 @@ fn test_trait_object(t: &dyn MyTrait) {
     // Panic: This is safe, just trust me.
     let _ = t.trait_panic_method();
 }
+
+// Test macro handling
+macro_rules! call_dangerous {
+    () => {
+        dangerous(1)
+    };
+}
+
+macro_rules! call_dangerous_with_panic_comment {
+    () => {
+        // Panic: This is documented inside the macro
+        dangerous(1)
+    };
+}
+
+fn test_macros() {
+    // Should lint: macro callsite has no comment
+    let _ = call_dangerous!();
+    //~^ undocumented_may_panic_call
+
+    // Panic: This documents the macro callsite
+    let _ = call_dangerous!();
+
+    // Should not lint: macro body has the comment
+    let _ = call_dangerous_with_panic_comment!();
+}
+
+macro_rules! nested_macro {
+    () => {
+        call_dangerous!()
+    };
+}
+
+fn test_nested_macros() {
+    // Should lint: no comment at any level
+    let _ = nested_macro!();
+    //~^ undocumented_may_panic_call
+
+    // Panic: This documents the outermost macro callsite
+    let _ = nested_macro!();
+}
+
+#[allow(clippy::needless_bool, clippy::unnecessary_operation, clippy::no_effect)]
+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),
+            )
+        });
+    };
+}
+
+// Test external macros
+use macro_rules::external_macro_may_panic;
+
+#[clippy::may_panic]
+fn external_may_panic() {}
+
+// External macro wrapped in local macro
+macro_rules! wrap_external {
+    () => {
+        external_macro_may_panic!(external_may_panic)
+    };
+}
+
+// External macro wrapped in local macro but documented
+macro_rules! wrap_external_with_panic_doc {
+    () => {
+        // Panic: documenting may_panic here
+        external_macro_may_panic!(external_may_panic)
+    };
+}
+
+fn test_external_macro() {
+    // Should lint: external macro callsite has no comment
+    external_macro_may_panic!(external_may_panic);
+    //~^ undocumented_may_panic_call
+
+    // Panic: This documents the external macro callsite
+    external_macro_may_panic!(external_may_panic);
+
+    // External macro wrapped in local - should lint at wrap_external callsite
+    wrap_external!();
+    //~^ undocumented_may_panic_call
+
+    // Panic: wrapping an external macro
+    wrap_external!();
+
+    // No comment needed, call is documented inside the local macro
+    wrap_external_with_panic_doc!();
+}
+
+// Macro that expands to multiple calls
+macro_rules! multi_call {
+    () => {{
+        dangerous(1);
+        dangerous(3);
+        dangerous(5)
+    }};
+}
+
+// Macro with the panic comment between calls (should still lint the first)
+macro_rules! comment_between {
+    () => {{
+        dangerous(1);
+        // Panic: only covers the second call
+        dangerous(3)
+    }};
+}
+
+fn test_macro_with_mutiple_may_panic_calls() {
+    // Multiple calls in one macro - all should lint pointing to same callsite
+    multi_call!();
+    //~^ undocumented_may_panic_call
+    //~| undocumented_may_panic_call
+    //~| undocumented_may_panic_call
+
+    // Panic: Extremely safe!
+    multi_call!();
+
+    // Comment between calls in macro - but macro still has a dangerous call, so lint
+    comment_between!();
+    //~^ undocumented_may_panic_call
+
+    // Panic: We are safe now!
+    comment_between!();
+}
+
+// Macro taking an expression that contains a may_panic call
+macro_rules! wrap_expr {
+    ($e:expr) => {
+        $e
+    };
+}
+
+fn test_macro_with_may_panic_arg() {
+    // The may_panic call is in the argument, not the macro body
+    wrap_expr!(dangerous(1));
+    //~^ undocumented_may_panic_call
+
+    // Panic: Very safe!
+    wrap_expr!(dangerous(1));
+}