Auto merge of #11624 - GuillaumeGomez:needless_pass_by_ref_mut-unsafe-fn-block, r=blyxyas

Don't emit `needless_pass_by_ref_mut` if the variable is used in an unsafe block or function

Fixes #11586.
Fixes #11180.

As suggested in the two issues above, this lint should not be emitted if this an unsafe function or if the argument is used in an unsafe block.

changelog: [`needless_pass_by_ref_mut`]: Don't emit if the variable is used in an unsafe block or function

Author: bors

clippy_lints/src/needless_pass_by_ref_mut.rs

@@ -7,7 +7,8 @@ use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_qpath, FnKind, Visitor};
 use rustc_hir::{
-    Body, Closure, Expr, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath,
+    BlockCheckMode, Body, Closure, Expr, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node,
+    PatKind, QPath,
 };
 use rustc_hir_typeck::expr_use_visitor as euv;
 use rustc_infer::infer::{InferCtxt, TyCtxtInferExt};
@@ -139,13 +140,23 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
         let hir_id = cx.tcx.hir().local_def_id_to_hir_id(fn_def_id);
         let is_async = match kind {
             FnKind::ItemFn(.., header) => {
+                if header.is_unsafe() {
+                    // We don't check unsafe functions.
+                    return;
+                }
                 let attrs = cx.tcx.hir().attrs(hir_id);
                 if header.abi != Abi::Rust || requires_exact_signature(attrs) {
                     return;
                 }
                 header.is_async()
             },
-            FnKind::Method(.., sig) => sig.header.is_async(),
+            FnKind::Method(.., sig) => {
+                if sig.header.is_unsafe() {
+                    // We don't check unsafe functions.
+                    return;
+                }
+                sig.header.is_async()
+            },
             FnKind::Closure => return,
         };
 
@@ -304,10 +315,27 @@ impl<'tcx> MutablyUsedVariablesCtxt<'tcx> {
         }
         self.aliases.insert(alias, target);
     }
+
+    // The goal here is to find if the current scope is unsafe or not. It stops when it finds
+    // a function or an unsafe block.
+    fn is_in_unsafe_block(&self, item: HirId) -> bool {
+        let hir = self.tcx.hir();
+        for (parent, node) in hir.parent_iter(item) {
+            if let Some(fn_sig) = hir.fn_sig_by_hir_id(parent) {
+                return fn_sig.header.is_unsafe();
+            } else if let Node::Block(block) = node {
+                if matches!(block.rules, BlockCheckMode::UnsafeBlock(_)) {
+                    return true;
+                }
+            }
+        }
+        false
+    }
 }
 
 impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt<'tcx> {
-    fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) {
+    #[allow(clippy::if_same_then_else)]
+    fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, id: HirId) {
         if let euv::Place {
             base:
                 euv::PlaceBase::Local(vid)
@@ -327,13 +355,18 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt<'tcx> {
                 && matches!(base_ty.ref_mutability(), Some(Mutability::Mut))
             {
                 self.add_mutably_used_var(*vid);
+            } else if self.is_in_unsafe_block(id) {
+                // If we are in an unsafe block, any operation on this variable must not be warned
+                // upon!
+                self.add_mutably_used_var(*vid);
             }
             self.prev_bind = None;
             self.prev_move_to_closure.remove(vid);
         }
     }
 
-    fn borrow(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId, borrow: ty::BorrowKind) {
+    #[allow(clippy::if_same_then_else)]
+    fn borrow(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, id: HirId, borrow: ty::BorrowKind) {
         self.prev_bind = None;
         if let euv::Place {
             base:
@@ -355,6 +388,10 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt<'tcx> {
                 || (borrow == ty::BorrowKind::UniqueImmBorrow && base_ty.ref_mutability() == Some(Mutability::Mut))
             {
                 self.add_mutably_used_var(*vid);
+            } else if self.is_in_unsafe_block(id) {
+                // If we are in an unsafe block, any operation on this variable must not be warned
+                // upon!
+                self.add_mutably_used_var(*vid);
             }
         } else if borrow == ty::ImmBorrow {
             // If there is an `async block`, it'll contain a call to a closure which we need to
@@ -397,7 +434,21 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt<'tcx> {
         }
     }
 
-    fn copy(&mut self, _cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) {
+    fn copy(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, id: HirId) {
+        if let euv::Place {
+            base:
+                euv::PlaceBase::Local(vid)
+                | euv::PlaceBase::Upvar(UpvarId {
+                    var_path: UpvarPath { hir_id: vid },
+                    ..
+                }),
+            ..
+        } = &cmt.place
+        {
+            if self.is_in_unsafe_block(id) {
+                self.add_mutably_used_var(*vid);
+            }
+        }
         self.prev_bind = None;
     }
 
@@ -427,8 +478,22 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt<'tcx> {
         }
     }
 
-    fn bind(&mut self, _cmt: &euv::PlaceWithHirId<'tcx>, id: HirId) {
+    fn bind(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, id: HirId) {
         self.prev_bind = Some(id);
+        if let euv::Place {
+            base:
+                euv::PlaceBase::Local(vid)
+                | euv::PlaceBase::Upvar(UpvarId {
+                    var_path: UpvarPath { hir_id: vid },
+                    ..
+                }),
+            ..
+        } = &cmt.place
+        {
+            if self.is_in_unsafe_block(id) {
+                self.add_mutably_used_var(*vid);
+            }
+        }
     }
 }
 

tests/ui/needless_pass_by_ref_mut.rs

@@ -276,6 +276,18 @@ async fn _f(v: &mut Vec<()>) {
     _ = || || x;
 }
 
+struct Data<T: ?Sized> {
+    value: T,
+}
+// Unsafe functions should not warn.
+unsafe fn get_mut_unchecked<T>(ptr: &mut NonNull<Data<T>>) -> &mut T {
+    &mut (*ptr.as_ptr()).value
+}
+// Unsafe blocks should not warn.
+fn get_mut_unchecked2<T>(ptr: &mut NonNull<Data<T>>) -> &mut T {
+    unsafe { &mut (*ptr.as_ptr()).value }
+}
+
 fn main() {
     let mut u = 0;
     let mut v = vec![0];