Skip to content

Commit

Permalink
Auto merge of #11624 - GuillaumeGomez:needless_pass_by_ref_mut-unsafe…
Browse files Browse the repository at this point in the history
…-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
bors committed Oct 18, 2023
2 parents 2640d5c + 80a092c commit 5fb312e
Showing 2 changed files with 83 additions and 6 deletions.
77 changes: 71 additions & 6 deletions clippy_lints/src/needless_pass_by_ref_mut.rs
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
}

12 changes: 12 additions & 0 deletions tests/ui/needless_pass_by_ref_mut.rs
Original file line number Diff line number Diff line change
@@ -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];

0 comments on commit 5fb312e

Please sign in to comment.