don't lint on doc(hidden) macros and metavars that are expanded twice

Author: y21

clippy_lints/src/expr_metavars_in_unsafe.rs

@@ -15,6 +15,7 @@
 //! The first delimiter, up to the `=>`, is the rule itself, and we look for
 //! any `$ident:expr` metavariable declarations (implemented in
 //! [`UnsafeMetavariableFinder::collect_expr_metavariables_in_rule`]).
+//! All metavariables start out as [`MetavarState::Unreferenced`] at this point.
 //!
 //! Once we have those, we know the next token after the delimited rule is the `=>` and after that
 //! we have the delimited macro rule body.
@@ -24,9 +25,20 @@
 //! The metavariable finder stores a stack of [`EnclosingNode`]. The top always tells us if we're
 //! within an unsafe block, and we push `EnclosingNode::UnsafeBlock` when we encounter `unsafe {`
 //! (and pop from it at the right time when leaving the delimited block).
-//! So, if we find `$x` anywhere in the body and the last item is an unsafe block, emit a lint.
 //!
-//! Note that it's possible to be inside of an unsafe block syntactically, but not semantically:
+//! If we find a `$x` anywhere in the body and the last item is an unsafe block, this tells us that
+//! this is likely something we want to lint, however we don't emit a warning right away,
+//! but update the metavar to [`MetavarState::Referenced`].
+//! Only at the end of the lint will we emit warnings for each `MetavarState::Referenced`.
+//!
+//! The reason for that is that it's possible that we might come across the same metavariable twice:
+//! Once in an unsafe block, and then outside of one. There's no reason to lint that because the
+//! user *does* need an unsafe block at call site to write unsafe code.
+//! So if we do come across a metavar reference outside of an unsafe block, we have to update it to
+//! `MetavarState::Suppressed` (indicating that this should never be linted or updated).
+//!
+//! Note also that it's possible to be inside of an unsafe block syntactically, but not
+//! semantically:
 //! ```ignore
 //! macro_rules! x {
 //!     ($v:expr) => {
@@ -39,7 +51,7 @@
 //! ```
 //! The `$v` is in an unsafe block, but that is *not* considered by the unsafety checker because
 //! it's part of another body.
-//! This is handled by having `EnclosingNode::Body`. In the above example, `::Body` is at the top,
+//! This is handled by having [`EnclosingNode::Body`]. In the above example, `Body` is at the top,
 //! so it's not linted.
 //!
 //! There are other edge cases for bodies (such as const generics, e.g. `Ty<{ $v > 3 }>`),
@@ -139,10 +151,38 @@ fn extract_metavariable<'a>(
     }
 }
 
-struct UnsafeMetavariableFinder<'a, 'tcx> {
+enum MetavarState {
+    /// The metavariable was referenced inside of an unsafe block
+    Referenced {
+        /// unsafe { $v }
+        ///          ^^
+        reference_span: Span,
+        /// macro_rules! x { (... $v:expr ...) => {} }
+        ///                       ^^^^^^^
+        decl_span: Span,
+        /// unsafe { $v }
+        /// ^^^^^^^^
+        unsafe_bl_span: Span,
+    },
+
+    /// The metavariable was not found yet, but we're still looking for it
+    Unreferenced { decl_span: Span },
+
+    /// The metavariable was found, but was then suppressed.
+    ///
+    /// For example:
+    /// ```ignore
+    /// unsafe { $v; }
+    ///          ^^ Found reference to $v in unsafe block, set to Referenced
+    /// $v;
+    /// ^^ ... then later referenced outside of an `unsafe {}` block, set to Suppressed and avoid linting
+    /// ```
+    Suppressed,
+}
+
+struct UnsafeMetavariableFinder {
     enclosing: Vec<EnclosingNode>,
-    cx: &'a LateContext<'tcx>,
-    expr_variables: FxHashMap<Symbol, Span>,
+    expr_variables: FxHashMap<Symbol, MetavarState>,
 }
 
 enum EnclosingNode {
@@ -163,7 +203,7 @@ enum EnclosingNode {
     UnsafeBlock(Span),
 }
 
-impl<'a, 'tcx> UnsafeMetavariableFinder<'a, 'tcx> {
+impl UnsafeMetavariableFinder {
     fn clear(&mut self) {
         self.expr_variables.clear();
         self.enclosing.clear();
@@ -174,7 +214,7 @@ impl<'a, 'tcx> UnsafeMetavariableFinder<'a, 'tcx> {
     /// macro_rules! x { (... $var:expr ...) => { ... } }
     ///                  ^^^^^^^^^^^^^^^^^^^
     /// ```
-    /// Returns the variable `var`
+    /// Puts `var` in the variable map.
     fn collect_expr_metavariables_in_rule(&mut self, tts: &TokenStream) {
         let mut tts = tts.trees().peekable();
         while let Some(tt) = tts.next() {
@@ -195,7 +235,12 @@ impl<'a, 'tcx> UnsafeMetavariableFinder<'a, 'tcx> {
                             _,
                         )) = tts.next()
                     {
-                        self.expr_variables.insert(sym, start_span.to(end_span));
+                        self.expr_variables.insert(
+                            sym,
+                            MetavarState::Unreferenced {
+                                decl_span: start_span.to(end_span),
+                            },
+                        );
                     }
                 },
                 TokenTree::Delimited(.., body) => self.collect_expr_metavariables_in_rule(body),
@@ -325,23 +370,29 @@ impl<'a, 'tcx> UnsafeMetavariableFinder<'a, 'tcx> {
                             self.find_metavariables_in_fn(&mut tts);
                         }
                     } else if let Some((sym, span)) = extract_metavariable(tt, &mut tts)
-                        && let Some(decl_span) = self.expr_variables.get(&sym).copied()
-                        && let Some(&EnclosingNode::UnsafeBlock(unsafe_bl_span)) = self.enclosing.last()
+                        && let Some(state) = self.expr_variables.get_mut(&sym)
                     {
-                        span_lint_and_then(
-                            self.cx,
-                            EXPR_METAVARS_IN_UNSAFE,
-                            span,
-                            "expanding an expression metavariable in an unsafe block",
-                            |diag| {
-                                diag.span_note(decl_span, "metavariable declared here");
-                                diag.span_note(unsafe_bl_span, "unsafe block entered here");
-                                diag.help(
-                                    "consider expanding it outside of this block, e.g. by storing it in a variable",
-                                );
-                                diag.note("this allows the user of the macro to write unsafe code without an unsafe block (at the callsite)");
+                        let unsafe_block_span = self.enclosing.last().and_then(|v| match v {
+                            EnclosingNode::Body => None,
+                            EnclosingNode::UnsafeBlock(span) => Some(*span),
+                        });
+
+                        match (&*state, unsafe_block_span) {
+                            (MetavarState::Referenced { .. }, Some(_)) => {},
+                            (_, None) => {
+                                // Metavar was referenced outside of an unsafe block. Set to
+                                // suppressed because we don't want to lint.
+                                *state = MetavarState::Suppressed;
                             },
-                        );
+                            (MetavarState::Unreferenced { decl_span }, Some(unsafe_bl_span)) => {
+                                *state = MetavarState::Referenced {
+                                    reference_span: span,
+                                    decl_span: *decl_span,
+                                    unsafe_bl_span,
+                                };
+                            },
+                            (MetavarState::Suppressed, _) => {},
+                        }
                     }
                 },
                 TokenTree::Delimited(.., body) => self.find_metavariables_in_body(body),
@@ -361,12 +412,12 @@ impl<'tcx> LateLintPass<'tcx> for ExprMetavarsInUnsafe {
         if let ItemKind::Macro(def, MacroKind::Bang) = item.kind
             && let def_id = item.owner_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)
         {
             let mut tts = def.body.tokens.trees().peekable();
             let mut finder = UnsafeMetavariableFinder {
                 enclosing: Vec::new(),
                 expr_variables: FxHashMap::default(),
-                cx,
             };
 
             while let Some(tt) = tts.next() {
@@ -401,6 +452,30 @@ impl<'tcx> LateLintPass<'tcx> for ExprMetavarsInUnsafe {
                     .is_some()
                 {}
             }
+
+            for metavar in finder.expr_variables.values() {
+                if let &MetavarState::Referenced {
+                    decl_span,
+                    reference_span,
+                    unsafe_bl_span,
+                } = metavar
+                {
+                    span_lint_and_then(
+                        cx,
+                        EXPR_METAVARS_IN_UNSAFE,
+                        reference_span,
+                        "expanding an expression metavariable in an unsafe block",
+                        |diag| {
+                            diag.span_note(decl_span, "metavariable declared here");
+                            diag.span_note(unsafe_bl_span, "unsafe block entered here");
+                            diag.help("consider expanding it outside of this block, e.g. by storing it in a variable");
+                            diag.note(
+                                "this allows the user of the macro to write unsafe code without an unsafe block (at the callsite)",
+                            );
+                        },
+                    );
+                }
+            }
         }
     }
 }