Add one_unsafe_op_per_block lint

Niki4tap · Niki4tap · commit bb5cfeee5d5c · 2023-01-17T01:35:34.000+03:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4434,6 +4434,7 @@ Released 2018-09-13
 [`obfuscated_if_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#obfuscated_if_else
 [`octal_escapes`]: https://rust-lang.github.io/rust-clippy/master/index.html#octal_escapes
 [`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect
+[`one_unsafe_op_per_block`]: https://rust-lang.github.io/rust-clippy/master/index.html#one_unsafe_op_per_block
 [`only_used_in_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion
 [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
 [`option_and_then_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_and_then_some
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -454,6 +454,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY_INFO,
     crate::nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES_INFO,
     crate::octal_escapes::OCTAL_ESCAPES_INFO,
+    crate::one_unsafe_op_per_block::ONE_UNSAFE_OP_PER_BLOCK_INFO,
     crate::only_used_in_recursion::ONLY_USED_IN_RECURSION_INFO,
     crate::operators::ABSURD_EXTREME_COMPARISONS_INFO,
     crate::operators::ARITHMETIC_SIDE_EFFECTS_INFO,
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -223,6 +223,7 @@ mod non_octal_unix_permissions;
 mod non_send_fields_in_send_ty;
 mod nonstandard_macro_braces;
 mod octal_escapes;
+mod one_unsafe_op_per_block;
 mod only_used_in_recursion;
 mod operators;
 mod option_env_unwrap;
@@ -908,6 +909,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(fn_null_check::FnNullCheck));
     store.register_late_pass(|_| Box::new(permissions_set_readonly_false::PermissionsSetReadonlyFalse));
     store.register_late_pass(|_| Box::new(size_of_ref::SizeOfRef));
+    store.register_late_pass(|_| Box::new(one_unsafe_op_per_block::OneUnsafeOpPerBlock));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 
diff --git a/clippy_lints/src/one_unsafe_op_per_block.rs b/clippy_lints/src/one_unsafe_op_per_block.rs
@@ -0,0 +1,252 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use hir::{
+    def::{DefKind, Res},
+    BlockCheckMode, ExprKind, Local, QPath, StmtKind, UnOp, Unsafety,
+};
+use rustc_ast::Mutability;
+use rustc_hir as hir;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::Span;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `unsafe` blocks that contain more than one unsafe operation.
+    ///
+    /// ### Why is this bad?
+    /// Combined with `undocumented_unsafe_blocks`,
+    /// this lint ensures that each unsafe operation must be independently justified.
+    /// Combined with `unused_unsafe`, this lint also ensures
+    /// elimination of unnecessary unsafe blocks through refactoring.
+    ///
+    /// ### Example
+    /// ```rust
+    /// /// Reads a `char` from the given pointer.
+    /// ///
+    /// /// # Safety
+    /// ///
+    /// /// `ptr` must point to four consecutive, initialized bytes which
+    /// /// form a valid `char` when interpreted in the native byte order.
+    /// fn read_char(ptr: *const u8) -> char {
+    ///     // SAFETY: The caller has guaranteed that the value pointed
+    ///     // to by `bytes` is a valid `char`.
+    ///     unsafe { char::from_u32_unchecked(*ptr.cast::<u32>()) }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// /// Reads a `char` from the given pointer.
+    /// ///
+    /// /// # Safety
+    /// ///
+    /// /// - `ptr` must be 4-byte aligned, point to four consecutive
+    /// ///   initialized bytes, and be valid for reads of 4 bytes.
+    /// /// - The bytes pointed to by `ptr` must represent a valid
+    /// ///   `char` when interpreted in the native byte order.
+    /// fn read_char(ptr: *const u8) -> char {
+    ///     // SAFETY: `ptr` is 4-byte aligned, points to four consecutive
+    ///     // initialized bytes, and is valid for reads of 4 bytes.
+    ///     let int_value = unsafe { *ptr.cast::<u32>() };
+    ///
+    ///     // SAFETY: The caller has guaranteed that the four bytes
+    ///     // pointed to by `bytes` represent a valid `char`.
+    ///     unsafe { char::from_u32_unchecked(int_value) }
+    /// }
+    /// ```
+    #[clippy::version = "1.68.0"]
+    pub ONE_UNSAFE_OP_PER_BLOCK,
+    restriction,
+    "more than one unsafe operation per `unsafe` block"
+}
+declare_lint_pass!(OneUnsafeOpPerBlock => [ONE_UNSAFE_OP_PER_BLOCK]);
+
+impl<'tcx> LateLintPass<'tcx> for OneUnsafeOpPerBlock {
+    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &hir::Block<'_>) {
+        let mut unsafe_ops = vec![];
+        collect_block(cx, block, &mut unsafe_ops);
+        if unsafe_ops.len() > 1 {
+            span_lint_and_then(
+                cx,
+                ONE_UNSAFE_OP_PER_BLOCK,
+                block.span,
+                &format!(
+                    "this `unsafe` block contains {} unsafe operations, expected only one",
+                    unsafe_ops.len()
+                ),
+                |diag| {
+                    for (msg, span) in unsafe_ops {
+                        diag.span_note(span, msg);
+                    }
+                },
+            );
+        }
+    }
+}
+
+fn collect_block(cx: &LateContext<'_>, block: &hir::Block<'_>, unsafe_ops: &mut Vec<(&'static str, Span)>) {
+    if let BlockCheckMode::UnsafeBlock(_) = block.rules {
+        if let Some(expr) = block.expr {
+            collect_unsafe_exprs(cx, expr, unsafe_ops);
+        }
+        for stmt in block.stmts {
+            match stmt.kind {
+                StmtKind::Local(Local { init, els: r#else, .. }) => {
+                    if let Some(init) = init {
+                        collect_unsafe_exprs(cx, init, unsafe_ops);
+                    }
+                    if let Some(r#else) = r#else {
+                        collect_block(cx, r#else, unsafe_ops);
+                    }
+                },
+                StmtKind::Expr(e) | StmtKind::Semi(e) => collect_unsafe_exprs(cx, e, unsafe_ops),
+                StmtKind::Item(_) => {},
+            }
+        }
+    }
+}
+
+// This whole function is practically one big `match` block
+#[expect(
+    clippy::too_many_lines,
+    reason = "long match means long function, not much you can do here"
+)]
+fn collect_unsafe_exprs(cx: &LateContext<'_>, expr: &hir::Expr<'_>, unsafe_ops: &mut Vec<(&'static str, Span)>) {
+    let expr = expr.peel_drop_temps();
+
+    match expr.kind {
+        // Relevant matches (contain `unsafe_ops.push`)
+        ExprKind::InlineAsm(_) => unsafe_ops.push(("inline assembly used here", expr.span)),
+
+        ExprKind::Field(e, _) => {
+            collect_unsafe_exprs(cx, e, unsafe_ops);
+            if cx.typeck_results().expr_ty(e).is_union() {
+                unsafe_ops.push(("union field access occurs here", expr.span));
+            }
+        },
+
+        ExprKind::Path(QPath::Resolved(_, hir::Path { res: Res::Def(DefKind::Static(Mutability::Mut), _), ..})) => {
+            unsafe_ops.push(("access of a mutable static occurs here", expr.span));
+        },
+
+        ExprKind::Unary(UnOp::Deref, e) if cx.typeck_results().expr_ty_adjusted(e).is_unsafe_ptr() => {
+            unsafe_ops.push(("raw pointer dereference occurs here", expr.span));
+            collect_unsafe_exprs(cx, e, unsafe_ops);
+        },
+
+        ExprKind::Call(path_expr, args) => {
+            args.iter().for_each(|arg| collect_unsafe_exprs(cx, arg, unsafe_ops));
+
+            match path_expr.kind {
+                ExprKind::Path(QPath::Resolved(_, hir::Path { res: Res::Def(kind, def_id), ..}))
+                if kind.is_fn_like() => {
+                    let sig = cx.tcx.bound_fn_sig(*def_id);
+                    if sig.0.unsafety() == Unsafety::Unsafe {
+                        unsafe_ops.push(("unsafe function call occurs here", expr.span));
+                    }
+                },
+
+                ExprKind::Path(QPath::TypeRelative(..)) => {
+                    if let Some(sig) = cx
+                        .typeck_results()
+                        .type_dependent_def_id(path_expr.hir_id)
+                        .map(|def_id| cx.tcx.bound_fn_sig(def_id))
+                    {
+                        if sig.0.unsafety() == Unsafety::Unsafe {
+                            unsafe_ops.push(("unsafe function call occurs here", expr.span));
+                        }
+                    }
+                },
+
+                _ => {}
+            }
+        },
+
+        ExprKind::MethodCall(_, recv, args, _) => {
+            collect_unsafe_exprs(cx, recv, unsafe_ops);
+            args.iter().for_each(|arg| collect_unsafe_exprs(cx, arg, unsafe_ops));
+            if let Some(sig) = cx
+                .typeck_results()
+                .type_dependent_def_id(expr.hir_id)
+                .map(|def_id| cx.tcx.bound_fn_sig(def_id))
+            {
+                if sig.0.unsafety() == Unsafety::Unsafe {
+                    unsafe_ops.push(("unsafe method call occurs here", expr.span));
+                }
+            }
+        },
+
+        ExprKind::AssignOp(_, lhs, rhs) | ExprKind::Assign(lhs, rhs, _) => {
+            collect_unsafe_exprs(cx, rhs, unsafe_ops);
+            if matches!(
+                lhs.kind,
+                ExprKind::Path(QPath::Resolved(_, hir::Path { res: Res::Def(DefKind::Static(Mutability::Mut), _), .. }))
+            ) {
+                unsafe_ops.push(("modification of a mutable static occurs here", expr.span));
+            }
+        },
+
+        // Not-so-relevant matches (mostly delegating)
+        ExprKind::Box(e)
+        | ExprKind::Ret(Some(e))
+        | ExprKind::Repeat(e, _)
+        | ExprKind::Cast(e, _)
+        | ExprKind::Type(e, _)
+        | ExprKind::Yield(e, _)
+        | ExprKind::Break(_, Some(e))
+        | ExprKind::Unary(_, e)
+        | ExprKind::AddrOf(_, _, e) => collect_unsafe_exprs(cx, e, unsafe_ops),
+
+        ExprKind::Array(exprs) => exprs.iter().for_each(|e| collect_unsafe_exprs(cx, e, unsafe_ops)),
+
+        ExprKind::Binary(_, first, second) => {
+            collect_unsafe_exprs(cx, first, unsafe_ops);
+            collect_unsafe_exprs(cx, second, unsafe_ops);
+        },
+
+        ExprKind::Struct(_, exprs, base) => {
+            if let Some(base) = base {
+                collect_unsafe_exprs(cx, base, unsafe_ops);
+            }
+            exprs.iter().for_each(|e| collect_unsafe_exprs(cx, e.expr, unsafe_ops));
+        },
+
+        ExprKind::Tup(exprs) => exprs.iter().for_each(|e| collect_unsafe_exprs(cx, e, unsafe_ops)),
+
+        ExprKind::Match(base, arms, _) => {
+            collect_unsafe_exprs(cx, base, unsafe_ops);
+            for arm in arms.iter() {
+                collect_unsafe_exprs(cx, arm.body, unsafe_ops);
+                if let Some(ref guard) = arm.guard {
+                    collect_unsafe_exprs(cx, guard.body(), unsafe_ops);
+                }
+            }
+        },
+
+        ExprKind::Let(r#let) => collect_unsafe_exprs(cx, r#let.init, unsafe_ops),
+
+        ExprKind::If(base, r#if, r#else) => {
+            collect_unsafe_exprs(cx, base, unsafe_ops);
+            collect_unsafe_exprs(cx, r#if, unsafe_ops);
+            if let Some(r#else) = r#else {
+                collect_unsafe_exprs(cx, r#else, unsafe_ops);
+            }
+        },
+
+        ExprKind::Index(recv, idx) => {
+            collect_unsafe_exprs(cx, recv, unsafe_ops);
+            collect_unsafe_exprs(cx, idx, unsafe_ops);
+        },
+
+        ExprKind::ConstBlock(_)
+        | ExprKind::Path(_)
+        | ExprKind::Break(_, _)
+        | ExprKind::Ret(None)
+        | ExprKind::Continue(_)
+        | ExprKind::DropTemps(_)
+        | ExprKind::Closure(_)
+        | ExprKind::Lit(_)
+        | ExprKind::Loop(..)
+        | ExprKind::Block(..) // that (maybe) unsafe block will be checked later by another `check_block` pass
+        | ExprKind::Err => {},
+    }
+}
diff --git a/tests/ui/one_unsafe_op_per_block.rs b/tests/ui/one_unsafe_op_per_block.rs
@@ -0,0 +1,110 @@
+#![allow(unused)]
+#![allow(deref_nullptr)]
+#![allow(clippy::unnecessary_operation)]
+#![allow(clippy::drop_copy)]
+#![warn(clippy::one_unsafe_op_per_block)]
+
+use core::arch::asm;
+
+fn raw_ptr() -> *const () {
+    core::ptr::null()
+}
+
+unsafe fn not_very_safe() {}
+
+struct Sample;
+
+impl Sample {
+    unsafe fn not_very_safe(&self) {}
+}
+
+#[allow(non_upper_case_globals)]
+const sample: Sample = Sample;
+
+union U {
+    i: i32,
+    u: u32,
+}
+
+static mut STATIC: i32 = 0;
+
+fn test1() {
+    unsafe {
+        STATIC += 1;
+        not_very_safe();
+    }
+}
+
+fn test2() {
+    let u = U { i: 0 };
+
+    unsafe {
+        drop(u.u);
+        *raw_ptr();
+    }
+}
+
+fn test3() {
+    unsafe {
+        asm!("nop");
+        sample.not_very_safe();
+        STATIC = 0;
+    }
+}
+
+fn test_all() {
+    let u = U { i: 0 };
+    unsafe {
+        drop(u.u);
+        drop(STATIC);
+        sample.not_very_safe();
+        not_very_safe();
+        *raw_ptr();
+        asm!("nop");
+    }
+}
+
+// no lint
+fn correct1() {
+    unsafe {
+        STATIC += 1;
+    }
+}
+
+// no lint
+fn correct2() {
+    unsafe {
+        STATIC += 1;
+    }
+
+    unsafe {
+        *raw_ptr();
+    }
+}
+
+// no lint
+fn correct3() {
+    let u = U { u: 0 };
+
+    unsafe {
+        not_very_safe();
+    }
+
+    unsafe {
+        drop(u.i);
+    }
+}
+
+// tests from the issue (https://github.com/rust-lang/rust-clippy/issues/10064)
+
+unsafe fn read_char_bad(ptr: *const u8) -> char {
+    unsafe { char::from_u32_unchecked(*ptr.cast::<u32>()) }
+}
+
+// no lint
+unsafe fn read_char_good(ptr: *const u8) -> char {
+    let int_value = unsafe { *ptr.cast::<u32>() };
+    unsafe { core::char::from_u32_unchecked(int_value) }
+}
+
+fn main() {}
diff --git a/tests/ui/one_unsafe_op_per_block.stderr b/tests/ui/one_unsafe_op_per_block.stderr
