Address PR comments

Author: y21

CHANGELOG.md

@@ -5297,7 +5297,6 @@ Released 2018-09-13
 [`explicit_into_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_into_iter_loop
 [`explicit_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_iter_loop
 [`explicit_write`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_write
-[`expr_metavars_in_unsafe`]: https://rust-lang.github.io/rust-clippy/master/index.html#expr_metavars_in_unsafe
 [`extend_from_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#extend_from_slice
 [`extend_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#extend_with_drain
 [`extra_unused_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes
@@ -5448,6 +5447,7 @@ Released 2018-09-13
 [`little_endian_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#little_endian_bytes
 [`logic_bug`]: https://rust-lang.github.io/rust-clippy/master/index.html#logic_bug
 [`lossy_float_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#lossy_float_literal
+[`macro_metavars_in_unsafe`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_metavars_in_unsafe
 [`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports
 [`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion
 [`manual_assert`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert
@@ -6003,4 +6003,5 @@ Released 2018-09-13
 [`vec-box-size-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#vec-box-size-threshold
 [`verbose-bit-mask-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#verbose-bit-mask-threshold
 [`warn-on-all-wildcard-imports`]: https://doc.rust-lang.org/clippy/lint_configuration.html#warn-on-all-wildcard-imports
+[`warn-unsafe-macro-metavars-in-private-macros`]: https://doc.rust-lang.org/clippy/lint_configuration.html#warn-unsafe-macro-metavars-in-private-macros
 <!-- end autogenerated links to configuration documentation -->

book/src/lint_configuration.md

@@ -900,3 +900,13 @@ Whether to allow certain wildcard imports (prelude, super in tests).
 * [`wildcard_imports`](https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_imports)
 
 
+## `warn-unsafe-macro-metavars-in-private-macros`
+Whether to also emit warnings for unsafe blocks with metavariable expansions in **private** macros.
+
+**Default Value:** `false`
+
+---
+**Affected lints:**
+* [`macro_metavars_in_unsafe`](https://rust-lang.github.io/rust-clippy/master/index.html#macro_metavars_in_unsafe)
+
+

clippy_config/src/conf.rs

@@ -613,6 +613,10 @@ define_Conf! {
     /// - Use `".."` as part of the list to indicate that the configured values should be appended to the
     /// default configuration of Clippy. By default, any configuration will replace the default value
     (allowed_prefixes: Vec<String> = DEFAULT_ALLOWED_PREFIXES.iter().map(ToString::to_string).collect()),
+    /// Lint: MACRO_METAVARS_IN_UNSAFE.
+    ///
+    /// Whether to also emit warnings for unsafe blocks with metavariable expansions in **private** macros.
+    (warn_unsafe_macro_metavars_in_private_macros: bool = false),
 }
 
 /// Search for the configuration file.

clippy_lints/src/declared_lints.rs

@@ -177,7 +177,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::exhaustive_items::EXHAUSTIVE_STRUCTS_INFO,
     crate::exit::EXIT_INFO,
     crate::explicit_write::EXPLICIT_WRITE_INFO,
-    crate::expr_metavars_in_unsafe::EXPR_METAVARS_IN_UNSAFE_INFO,
     crate::extra_unused_type_parameters::EXTRA_UNUSED_TYPE_PARAMETERS_INFO,
     crate::fallible_impl_from::FALLIBLE_IMPL_FROM_INFO,
     crate::float_literal::EXCESSIVE_PRECISION_INFO,
@@ -295,6 +294,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::loops::WHILE_IMMUTABLE_CONDITION_INFO,
     crate::loops::WHILE_LET_LOOP_INFO,
     crate::loops::WHILE_LET_ON_ITERATOR_INFO,
+    crate::macro_metavars_in_unsafe::MACRO_METAVARS_IN_UNSAFE_INFO,
     crate::macro_use::MACRO_USE_IMPORTS_INFO,
     crate::main_recursion::MAIN_RECURSION_INFO,
     crate::manual_assert::MANUAL_ASSERT_INFO,

clippy_lints/src/lib.rs

@@ -133,7 +133,6 @@ mod excessive_nesting;
 mod exhaustive_items;
 mod exit;
 mod explicit_write;
-mod expr_metavars_in_unsafe;
 mod extra_unused_type_parameters;
 mod fallible_impl_from;
 mod float_literal;
@@ -194,6 +193,7 @@ mod lifetimes;
 mod lines_filter_map_ok;
 mod literal_representation;
 mod loops;
+mod macro_metavars_in_unsafe;
 mod macro_use;
 mod main_recursion;
 mod manual_assert;
@@ -599,6 +599,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
 
         blacklisted_names: _,
         cyclomatic_complexity_threshold: _,
+        warn_unsafe_macro_metavars_in_private_macros,
     } = *conf;
     let msrv = || msrv.clone();
 
@@ -1154,7 +1155,12 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(zero_repeat_side_effects::ZeroRepeatSideEffects));
     store.register_late_pass(|_| Box::new(manual_unwrap_or_default::ManualUnwrapOrDefault));
     store.register_late_pass(|_| Box::new(integer_division_remainder_used::IntegerDivisionRemainderUsed));
-    store.register_late_pass(|_| Box::<expr_metavars_in_unsafe::ExprMetavarsInUnsafe>::default());
+    store.register_late_pass(move |_| {
+        Box::new(macro_metavars_in_unsafe::ExprMetavarsInUnsafe {
+            warn_unsafe_macro_metavars_in_private_macros,
+            ..Default::default()
+        })
+    });
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/expr_metavars_in_unsafe.rs renamed to clippy_lints/src/macro_metavars_in_unsafe.rs

@@ -4,27 +4,40 @@ use std::collections::BTreeMap;
 use clippy_utils::diagnostics::span_lint_hir_and_then;
 use clippy_utils::is_lint_allowed;
 use itertools::Itertools;
+use rustc_hir::def_id::LocalDefId;
 use rustc_hir::intravisit::{walk_block, walk_expr, walk_stmt, Visitor};
 use rustc_hir::{BlockCheckMode, Expr, ExprKind, HirId, Stmt, UnsafeSource};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
-use rustc_span::def_id::DefId;
 use rustc_span::{sym, Span, SyntaxContext};
 
 declare_clippy_lint! {
     /// ### What it does
     /// Looks for macros that expand metavariables in an unsafe block.
     ///
     /// ### Why is this bad?
-    /// This is unsound: it allows the user of the macro to write unsafe code outside of an
-    /// unsafe block at callsite, potentially invoking undefined behavior in safe code.
+    /// This hides an unsafe block and allows the user of the macro to write unsafe code without an explicit
+    /// unsafe block at callsite, making it possible to perform unsafe operations in seemingly safe code.
+    ///
+    /// The macro should be restructured so that these metavariables are referenced outside of unsafe blocks
+    /// and that the usual unsafety checks apply to the macro argument.
+    ///
+    /// This is usually done by binding it to a variable outside of the unsafe block
+    /// and then using that variable inside of the block as shown in the example, or by referencing it a second time
+    /// in a safe context, e.g. `if false { $expr }`.
     ///
     /// ### Known limitations
     /// Due to how macros are represented in the compiler at the time Clippy runs its lints,
     /// it's not possible to look for metavariables in macro definitions directly.
     ///
-    /// Instead, this lint looks at expansions of macros defined in the same crate.
-    /// This leads to false negatives when a macro is never actually invoked.
+    /// Instead, this lint looks at expansions of macros.
+    /// This leads to false negatives for macros that are never actually invoked.
+    ///
+    /// By default, this lint is rather conservative and will only emit warnings on publicly-exported
+    /// macros from the same crate, because oftentimes private internal macros are one-off macros where
+    /// this lint would just be noise (e.g. macros that generate `impl` blocks).
+    /// The default behavior should help with preventing a high number of such false positives,
+    /// however it can be configured to also emit warnings in private macros if desired.
     ///
     /// ### Example
     /// ```no_run
@@ -65,19 +78,21 @@ declare_clippy_lint! {
     /// assert_eq!(*first!(std::hint::unreachable_unchecked() as &[i32]), 1);
     /// ```
     #[clippy::version = "1.77.0"]
-    pub EXPR_METAVARS_IN_UNSAFE,
-    nursery,
-    "expanding expr metavariables in an unsafe block"
+    pub MACRO_METAVARS_IN_UNSAFE,
+    suspicious,
+    "expanding macro metavariables in an unsafe block"
 }
+impl_lint_pass!(ExprMetavarsInUnsafe => [MACRO_METAVARS_IN_UNSAFE]);
 
 #[derive(Clone, Debug)]
-enum MetavarState {
+pub enum MetavarState {
     ReferencedInUnsafe { unsafe_blocks: Vec<HirId> },
     ReferencedInSafe,
 }
 
 #[derive(Default)]
 pub struct ExprMetavarsInUnsafe {
+    pub warn_unsafe_macro_metavars_in_private_macros: bool,
     /// A metavariable can be expanded more than once, potentially across multiple bodies, so it
     /// requires some state kept across HIR nodes to make it possible to delay a warning
     /// and later undo:
@@ -91,21 +106,27 @@ pub struct ExprMetavarsInUnsafe {
     ///     }
     /// }
     /// ```
-    metavar_expns: BTreeMap<Span, MetavarState>,
+    pub metavar_expns: BTreeMap<Span, MetavarState>,
 }
-impl_lint_pass!(ExprMetavarsInUnsafe => [EXPR_METAVARS_IN_UNSAFE]);
 
-struct BodyVisitor<'a> {
-    /// The top item always represents the last seen unsafe block
+struct BodyVisitor<'a, 'tcx> {
+    /// Stack of unsafe blocks -- the top item always represents the last seen unsafe block from
+    /// within a relevant macro.
     macro_unsafe_blocks: Vec<HirId>,
-    /// When this is >0, it means that the node in the visitor currently being visited is "within" a
-    /// macro definition. This helps reduce the number of spans we need to insert into the map,
-    /// since only spans from macros are relevant.
+    /// When this is >0, it means that the node currently being visited is "within" a
+    /// macro definition. This is not necessary for correctness, it merely helps reduce the number
+    /// of spans we need to insert into the map, since only spans from macros are relevant.
     expn_depth: u32,
-    metavar_map: &'a mut BTreeMap<Span, MetavarState>,
+    cx: &'a LateContext<'tcx>,
+    lint: &'a mut ExprMetavarsInUnsafe,
+}
+
+fn is_public_macro(cx: &LateContext<'_>, def_id: LocalDefId) -> bool {
+    (cx.effective_visibilities.is_exported(def_id) || cx.tcx.has_attr(def_id, sym::macro_export))
+        && !cx.tcx.is_doc_hidden(def_id)
 }
 
-impl<'a, 'tcx> Visitor<'tcx> for BodyVisitor<'a> {
+impl<'a, 'tcx> Visitor<'tcx> for BodyVisitor<'a, 'tcx> {
     fn visit_stmt(&mut self, s: &'tcx Stmt<'tcx>) {
         let from_expn = s.span.from_expansion();
         if from_expn {
@@ -123,15 +144,17 @@ impl<'a, 'tcx> Visitor<'tcx> for BodyVisitor<'a> {
         if let ExprKind::Block(block, _) = e.kind
             && let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules
             && !ctxt.is_root()
-            && ctxt.outer_expn_data().macro_def_id.is_some_and(DefId::is_local)
+            && let Some(macro_def_id) = ctxt.outer_expn_data().macro_def_id
+            && let Some(macro_def_id) = macro_def_id.as_local()
+            && (self.lint.warn_unsafe_macro_metavars_in_private_macros || is_public_macro(self.cx, macro_def_id))
         {
             self.macro_unsafe_blocks.push(block.hir_id);
             walk_block(self, block);
             self.macro_unsafe_blocks.pop();
         } else if ctxt.is_root() && self.expn_depth > 0 {
             let unsafe_block = self.macro_unsafe_blocks.last().copied();
 
-            match (self.metavar_map.entry(e.span), unsafe_block) {
+            match (self.lint.metavar_expns.entry(e.span), unsafe_block) {
                 (Entry::Vacant(e), None) => {
                     e.insert(MetavarState::ReferencedInSafe);
                 },
@@ -164,7 +187,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BodyVisitor<'a> {
 
 impl<'tcx> LateLintPass<'tcx> for ExprMetavarsInUnsafe {
     fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx rustc_hir::Body<'tcx>) {
-        if is_lint_allowed(cx, EXPR_METAVARS_IN_UNSAFE, body.value.hir_id) {
+        if is_lint_allowed(cx, MACRO_METAVARS_IN_UNSAFE, body.value.hir_id) {
             return;
         }
 
@@ -175,7 +198,8 @@ impl<'tcx> LateLintPass<'tcx> for ExprMetavarsInUnsafe {
             #[expect(clippy::bool_to_int_with_if)] // obfuscates the meaning
             expn_depth: if body.value.span.from_expansion() { 1 } else { 0 },
             macro_unsafe_blocks: Vec::new(),
-            metavar_map: &mut self.metavar_expns,
+            lint: self,
+            cx
         };
         vis.visit_body(body);
     }
@@ -205,37 +229,28 @@ impl<'tcx> LateLintPass<'tcx> for ExprMetavarsInUnsafe {
                 // The invocation doesn't matter. Also we want to dedupe by the unsafe block and not by anything
                 // related to the callsite.
                 let span = cx.tcx.hir().span(id);
-                let macro_def_id = span.ctxt().outer_expn_data().macro_def_id.and_then(DefId::as_local);
-                (
-                    id,
-                    Span::new(span.lo(), span.hi(), SyntaxContext::root(), None),
-                    macro_def_id,
-                )
+
+                (id, Span::new(span.lo(), span.hi(), SyntaxContext::root(), None))
             })
-            .dedup_by(|(_, a, _), (_, b, _)| a == b);
-
-        for (id, span, def_id) in bad_unsafe_blocks {
-            if let Some(def_id) = def_id
-                && (cx.effective_visibilities.is_exported(def_id) || cx.tcx.has_attr(def_id, sym::macro_export))
-                && !cx.tcx.is_doc_hidden(def_id)
-            {
-                span_lint_hir_and_then(
-                    cx,
-                    EXPR_METAVARS_IN_UNSAFE,
-                    id,
-                    span,
-                    "this unsafe block in a macro expands `expr` metavariables",
-                    |diag| {
-                        diag.note("this allows the user of the macro to write unsafe code outside of an unsafe block");
-                        diag.help(
+            .dedup_by(|&(_, a), &(_, b)| a == b);
+
+        for (id, span) in bad_unsafe_blocks {
+            span_lint_hir_and_then(
+                cx,
+                MACRO_METAVARS_IN_UNSAFE,
+                id,
+                span,
+                "this unsafe block in a macro expands metavariables",
+                |diag| {
+                    diag.note("this allows the user of the macro to write unsafe code outside of an unsafe block");
+                    diag.help(
                             "consider expanding any metavariables outside of this block, e.g. by storing them in a variable",
                         );
-                        diag.help(
+                    diag.help(
                             "... or also expand referenced metavariables in a safe context to require an unsafe block at callsite",
                         );
-                    },
-                );
-            }
+                },
+            );
         }
     }
 }