new lint: macro_metavars_in_unsafe

CHANGELOG.md

@@ -5448,6 +5448,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
@@ -6005,4 +6006,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

@@ -922,3 +922,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

@@ -632,6 +632,10 @@ define_Conf! {
     /// default configuration of Clippy. By default, any configuration will replace the default value.
     (allow_renamed_params_for: Vec<String> =
         DEFAULT_ALLOWED_TRAITS_WITH_RENAMED_PARAMS.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

@@ -296,6 +296,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

@@ -193,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 +600,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();
 
@@ -1155,6 +1157,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(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/macro_metavars_in_unsafe.rs

@@ -0,0 +1,256 @@
+use std::collections::btree_map::Entry;
+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::{sym, Span, SyntaxContext};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Looks for macros that expand metavariables in an unsafe block.
+    ///
+    /// ### Why is this bad?
+    /// 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.
+    /// 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
+    /// /// Gets the first element of a slice
+    /// macro_rules! first {
+    ///     ($slice:expr) => {
+    ///         unsafe {
+    ///             let slice = $slice; // ⚠️ expansion inside of `unsafe {}`
+    ///
+    ///             assert!(!slice.is_empty());
+    ///             // SAFETY: slice is checked to have at least one element
+    ///             slice.first().unwrap_unchecked()
+    ///         }
+    ///     }
+    /// }
+    ///
+    /// assert_eq!(*first!(&[1i32]), 1);
+    ///
+    /// // This will compile as a consequence (note the lack of `unsafe {}`)
+    /// assert_eq!(*first!(std::hint::unreachable_unchecked() as &[i32]), 1);
+    /// ```
+    /// Use instead:
+    /// ```compile_fail
+    /// macro_rules! first {
+    ///     ($slice:expr) => {{
+    ///         let slice = $slice; // ✅ outside of `unsafe {}`
+    ///         unsafe {
+    ///             assert!(!slice.is_empty());
+    ///             // SAFETY: slice is checked to have at least one element
+    ///             slice.first().unwrap_unchecked()
+    ///         }
+    ///     }}
+    /// }
+    ///
+    /// assert_eq!(*first!(&[1]), 1);
+    ///
+    /// // This won't compile:
+    /// assert_eq!(*first!(std::hint::unreachable_unchecked() as &[i32]), 1);
+    /// ```
+    #[clippy::version = "1.80.0"]
+    pub MACRO_METAVARS_IN_UNSAFE,
+    suspicious,
+    "expanding macro metavariables in an unsafe block"
+}
+impl_lint_pass!(ExprMetavarsInUnsafe => [MACRO_METAVARS_IN_UNSAFE]);
+
+#[derive(Clone, Debug)]
+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:
+    ///
+    /// ```ignore
+    /// macro_rules! x {
+    ///     ($v:expr) => {
+    ///         unsafe { $v; } // unsafe context, it might be possible to emit a warning here, so add it to the map
+    ///
+    ///         $v;            // `$v` expanded another time but in a safe context, set to ReferencedInSafe to suppress
+    ///     }
+    /// }
+    /// ```
+    pub metavar_expns: BTreeMap<Span, MetavarState>,
+}
+
+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 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,
+    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, 'tcx> {
+    fn visit_stmt(&mut self, s: &'tcx Stmt<'tcx>) {
+        let from_expn = s.span.from_expansion();
+        if from_expn {
+            self.expn_depth += 1;
+        }
+        walk_stmt(self, s);
+        if from_expn {
+            self.expn_depth -= 1;
+        }
+    }
+
+    fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) {
+        let ctxt = e.span.ctxt();
+
+        if let ExprKind::Block(block, _) = e.kind
+            && let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules
+            && !ctxt.is_root()
+            && 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.lint.metavar_expns.entry(e.span), unsafe_block) {
+                (Entry::Vacant(e), None) => {
+                    e.insert(MetavarState::ReferencedInSafe);
+                },
+                (Entry::Vacant(e), Some(unsafe_block)) => {
+                    e.insert(MetavarState::ReferencedInUnsafe {
+                        unsafe_blocks: vec![unsafe_block],
+                    });
+                },
+                (Entry::Occupied(mut e), None) => {
+                    if let MetavarState::ReferencedInUnsafe { .. } = *e.get() {
+                        e.insert(MetavarState::ReferencedInSafe);
+                    }
+                },
+                (Entry::Occupied(mut e), Some(unsafe_block)) => {
+                    if let MetavarState::ReferencedInUnsafe { unsafe_blocks } = e.get_mut()
+                        && !unsafe_blocks.contains(&unsafe_block)
+                    {
+                        unsafe_blocks.push(unsafe_block);
+                    }
+                },
+            }
+
+            // NB: No need to visit descendant nodes. They're guaranteed to represent the same
+            // metavariable
+        } else {
+            walk_expr(self, e);
+        }
+    }
+}
+
+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, MACRO_METAVARS_IN_UNSAFE, body.value.hir_id) {
+            return;
+        }
+
+        // This BodyVisitor is separate and not part of the lint pass because there is no
+        // `check_stmt_post` on `(Late)LintPass`, which we'd need to detect when we're leaving a macro span
+
+        let mut vis = BodyVisitor {
+            #[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(),
+            lint: self,
+            cx
+        };
+        vis.visit_body(body);
+    }
+
+    fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
+        // Aggregate all unsafe blocks from all spans:
+        // ```
+        // macro_rules! x {
+        //   ($w:expr, $x:expr, $y:expr) => { $w; unsafe { $w; $x; }; unsafe { $x; $y; }; }
+        // }
+        // $w: []  (unsafe#0 is never added because it was referenced in a safe context)
+        // $x: [unsafe#0, unsafe#1]
+        // $y: [unsafe#1]
+        // ```
+        // We want to lint unsafe blocks #0 and #1
+        let bad_unsafe_blocks = self
+            .metavar_expns
+            .iter()
+            .filter_map(|(_, state)| match state {
+                MetavarState::ReferencedInUnsafe { unsafe_blocks } => Some(unsafe_blocks.as_slice()),
+                MetavarState::ReferencedInSafe => None,
+            })
+            .flatten()
+            .copied()
+            .map(|id| {
+                // Remove the syntax context to hide "in this macro invocation" in the diagnostic.
+                // 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);
+
+                (id, Span::new(span.lo(), span.hi(), SyntaxContext::root(), None))
+            })
+            .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 macro expands metavariables in an unsafe block",
+                |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(
+                            "... or also expand referenced metavariables in a safe context to require an unsafe block at callsite",
+                        );
+                },
+            );
+        }
+    }
+}