From 5638860ff8f3996522c6b632ca972177bc2df026 Mon Sep 17 00:00:00 2001 From: bors Date: Thu, 17 Aug 2023 15:55:23 +0000 Subject: [PATCH] Auto merge of #11314 - GuillaumeGomez:needless_ref_mut_async_block, r=Centri3 Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT Fixes https://github.com/rust-lang/rust-clippy/issues/11299. The problem was that the `async block`s are popping a closure which we didn't go into, making it miss the mutable access to the variables. cc `@Centri3` changelog: none --- clippy_lints/src/needless_pass_by_ref_mut.rs | 103 +++++++++++++++---- tests/ui/needless_pass_by_ref_mut.rs | 35 +++++++ tests/ui/needless_pass_by_ref_mut.stderr | 14 ++- 3 files changed, 129 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs index c634de960d1..7b00eabf97b 100644 --- a/clippy_lints/src/needless_pass_by_ref_mut.rs +++ b/clippy_lints/src/needless_pass_by_ref_mut.rs @@ -5,14 +5,16 @@ use rustc_data_structures::fx::{FxHashSet, FxIndexMap}; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_qpath, FnKind, Visitor}; -use rustc_hir::{Body, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath}; +use rustc_hir::{ + 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::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::associated_body; use rustc_middle::hir::nested_filter::OnlyBodies; use rustc_middle::mir::FakeReadCause; -use rustc_middle::ty::{self, Ty, UpvarId, UpvarPath}; +use rustc_middle::ty::{self, Ty, TyCtxt, UpvarId, UpvarPath}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::def_id::LocalDefId; use rustc_span::symbol::kw; @@ -147,22 +149,36 @@ fn check_fn( // Collect variables mutably used and spans which will need dereferencings from the // function body. let MutablyUsedVariablesCtxt { mutably_used_vars, .. } = { - let mut ctx = MutablyUsedVariablesCtxt::default(); + let mut ctx = MutablyUsedVariablesCtxt { + mutably_used_vars: HirIdSet::default(), + prev_bind: None, + prev_move_to_closure: HirIdSet::default(), + aliases: HirIdMap::default(), + async_closures: FxHashSet::default(), + tcx: cx.tcx, + }; let infcx = cx.tcx.infer_ctxt().build(); euv::ExprUseVisitor::new(&mut ctx, &infcx, fn_def_id, cx.param_env, cx.typeck_results()).consume_body(body); if is_async { - let closures = ctx.async_closures.clone(); - let hir = cx.tcx.hir(); - for closure in closures { - ctx.prev_bind = None; - ctx.prev_move_to_closure.clear(); - if let Some(body) = hir - .find_by_def_id(closure) - .and_then(associated_body) - .map(|(_, body_id)| hir.body(body_id)) - { - euv::ExprUseVisitor::new(&mut ctx, &infcx, closure, cx.param_env, cx.typeck_results()) - .consume_body(body); + let mut checked_closures = FxHashSet::default(); + while !ctx.async_closures.is_empty() { + let closures = ctx.async_closures.clone(); + ctx.async_closures.clear(); + let hir = cx.tcx.hir(); + for closure in closures { + if !checked_closures.insert(closure) { + continue; + } + ctx.prev_bind = None; + ctx.prev_move_to_closure.clear(); + if let Some(body) = hir + .find_by_def_id(closure) + .and_then(associated_body) + .map(|(_, body_id)| hir.body(body_id)) + { + euv::ExprUseVisitor::new(&mut ctx, &infcx, closure, cx.param_env, cx.typeck_results()) + .consume_body(body); + } } } } @@ -225,16 +241,16 @@ fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { } } -#[derive(Default)] -struct MutablyUsedVariablesCtxt { +struct MutablyUsedVariablesCtxt<'tcx> { mutably_used_vars: HirIdSet, prev_bind: Option, prev_move_to_closure: HirIdSet, aliases: HirIdMap, async_closures: FxHashSet, + tcx: TyCtxt<'tcx>, } -impl MutablyUsedVariablesCtxt { +impl<'tcx> MutablyUsedVariablesCtxt<'tcx> { fn add_mutably_used_var(&mut self, mut used_id: HirId) { while let Some(id) = self.aliases.get(&used_id) { self.mutably_used_vars.insert(used_id); @@ -242,9 +258,27 @@ fn add_mutably_used_var(&mut self, mut used_id: HirId) { } self.mutably_used_vars.insert(used_id); } + + fn would_be_alias_cycle(&self, alias: HirId, mut target: HirId) -> bool { + while let Some(id) = self.aliases.get(&target) { + if *id == alias { + return true; + } + target = *id; + } + false + } + + fn add_alias(&mut self, alias: HirId, target: HirId) { + // This is to prevent alias loop. + if alias == target || self.would_be_alias_cycle(alias, target) { + return; + } + self.aliases.insert(alias, target); + } } -impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt { +impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt<'tcx> { fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) { if let euv::Place { base: @@ -259,7 +293,7 @@ fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) { { if let Some(bind_id) = self.prev_bind.take() { if bind_id != *vid { - self.aliases.insert(bind_id, *vid); + self.add_alias(bind_id, *vid); } } else if !self.prev_move_to_closure.contains(vid) && matches!(base_ty.ref_mutability(), Some(Mutability::Mut)) @@ -289,6 +323,25 @@ fn borrow(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId, borrow: ty::Bo { 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 + // go into to ensure all "mutate" checks are found. + if let Node::Expr(Expr { + kind: + ExprKind::Call( + _, + [ + Expr { + kind: ExprKind::Closure(Closure { def_id, .. }), + .. + }, + ], + ), + .. + }) = self.tcx.hir().get(cmt.hir_id) + { + self.async_closures.insert(*def_id); + } } } @@ -296,7 +349,12 @@ fn mutate(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) { self.prev_bind = None; if let euv::Place { projections, - base: euv::PlaceBase::Local(vid), + base: + euv::PlaceBase::Local(vid) + | euv::PlaceBase::Upvar(UpvarId { + var_path: UpvarPath { hir_id: vid }, + .. + }), .. } = &cmt.place { @@ -329,8 +387,9 @@ fn fake_read( // Seems like we are inside an async function. We need to store the closure `DefId` // to go through it afterwards. self.async_closures.insert(inner); - self.aliases.insert(cmt.hir_id, *vid); + self.add_alias(cmt.hir_id, *vid); self.prev_move_to_closure.insert(*vid); + self.prev_bind = None; } } } diff --git a/tests/ui/needless_pass_by_ref_mut.rs b/tests/ui/needless_pass_by_ref_mut.rs index ae7b018d0e2..ec87d447597 100644 --- a/tests/ui/needless_pass_by_ref_mut.rs +++ b/tests/ui/needless_pass_by_ref_mut.rs @@ -196,6 +196,41 @@ fn cfg_warn(s: &mut u32) {} //~| NOTE: this is cfg-gated and may require further changes } +// Should not warn. +async fn inner_async(x: &mut i32, y: &mut u32) { + async { + *y += 1; + *x += 1; + } + .await; +} + +async fn inner_async2(x: &mut i32, y: &mut u32) { + //~^ ERROR: this argument is a mutable reference, but not used mutably + async { + *x += 1; + } + .await; +} + +async fn inner_async3(x: &mut i32, y: &mut u32) { + //~^ ERROR: this argument is a mutable reference, but not used mutably + async { + *y += 1; + } + .await; +} + +// Should not warn. +async fn async_vec(b: &mut Vec) { + b.append(&mut vec![]); +} + +// Should not warn. +async fn async_vec2(b: &mut Vec) { + b.push(true); +} + fn main() { let mut u = 0; let mut v = vec![0]; diff --git a/tests/ui/needless_pass_by_ref_mut.stderr b/tests/ui/needless_pass_by_ref_mut.stderr index 0d426ce32f9..2e06e7252d9 100644 --- a/tests/ui/needless_pass_by_ref_mut.stderr +++ b/tests/ui/needless_pass_by_ref_mut.stderr @@ -94,5 +94,17 @@ LL | fn cfg_warn(s: &mut u32) {} | = note: this is cfg-gated and may require further changes -error: aborting due to 15 previous errors +error: this argument is a mutable reference, but not used mutably + --> $DIR/needless_pass_by_ref_mut.rs:208:39 + | +LL | async fn inner_async2(x: &mut i32, y: &mut u32) { + | ^^^^^^^^ help: consider changing to: `&u32` + +error: this argument is a mutable reference, but not used mutably + --> $DIR/needless_pass_by_ref_mut.rs:216:26 + | +LL | async fn inner_async3(x: &mut i32, y: &mut u32) { + | ^^^^^^^^ help: consider changing to: `&i32` + +error: aborting due to 17 previous errors