Skip to content

Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT #11314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 17, 2023
Merged

Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT #11314

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
42186af
Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT
GuillaumeGomez Aug 10, 2023
1d01f1b
Update UI test for async blocks for NEEDLESS_PASS_BY_REF_MUT
GuillaumeGomez Aug 10, 2023
7e46217
Add new regression test for `needless_pass_by_ref_mut`
GuillaumeGomez Aug 17, 2023
5875bd2
Use `HirId` from `PlaceWithHirId` rather than using the one provided …
GuillaumeGomez Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 82 additions & 23 deletions clippy_lints/src/needless_pass_by_ref_mut.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@ use clippy_utils::{get_parent_node, inherits_cfg, is_from_proc_macro, is_self};
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;
Expand Down Expand Up @@ -147,22 +149,36 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
// 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);
}
}
}
}
Expand Down Expand Up @@ -225,26 +241,44 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
}
}

#[derive(Default)]
struct MutablyUsedVariablesCtxt {
struct MutablyUsedVariablesCtxt<'tcx> {
mutably_used_vars: HirIdSet,
prev_bind: Option<HirId>,
prev_move_to_closure: HirIdSet,
aliases: HirIdMap<HirId>,
async_closures: FxHashSet<LocalDefId>,
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);
used_id = *id;
}
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:
Expand All @@ -259,7 +293,7 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
{
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))
Expand All @@ -271,7 +305,7 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
}
}

fn borrow(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId, borrow: ty::BorrowKind) {
fn borrow(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, id: HirId, borrow: ty::BorrowKind) {
self.prev_bind = None;
if let euv::Place {
base: euv::PlaceBase::Local(vid),
Expand All @@ -289,14 +323,38 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
{
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(id)
Copy link
Member

@Centri3 Centri3 Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious how stable diag_expr_id is. Is it free to change at any time if, perhaps, only highlighting the arguments of the closure is deemed better? Because using this to determine if it's from a closure feels a bit error-prone.

Maybe we need another parameter that's the borrower's HirId, or consumer, etc.

If it is ok, then the wording in consume should probably be updated in rustc since it's likely interpreted as the HirId for getting spans/the id for linting, not analysis.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm a bit lost: what is diag_expr_id here? What's the connection with consume too?

In this case, async blocks generate a closure and an Expr containing a call to a closure which has a DefId and that's what I retrieve with this code. If this is changed in the compiler, the UI test will catch it.

Copy link
Member

@Centri3 Centri3 Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diag_expr_id i.e., id. That's the default name for it.

Its intended use is clarified in Delegate::consume's docs, from what I can tell it's not supposed to be used for analysis.

If this is changed in the compiler, the UI test will catch it.

That's true, but it'll be annoying for whoever wants to. If this is only supposed to be used pointing at something in diagnostics, then we probably shouldn't use it for this (at least, once we have an alternative). It would be reasonable for it to be changed, since if my assumption is true then code isn't supposed to depend on this (turning what would be a couple line change into adding a stable HirId you can use.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok I see. In this case, it's all generated by the compiler (it doesn't exist in user code). The case of async block seems to be special if you look at ExprKind::Closure. So what do you want to do in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably fine as is, but once we can we should change it. For now however, we could change this to cmt.hir_id which would convey this better (diag_expr_id is currently always cmt.hir_id for borrow) as cmt (place_with_id) is usually where the binding was introduced I believe (in this case, the closure, though it would realistically be the parameters' HirId)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes perfect sense, thanks for the detailed explanation!

{
self.async_closures.insert(*def_id);
}
}
}

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
{
Expand Down Expand Up @@ -329,8 +387,9 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
// 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;
}
}
}
Expand Down
25 changes: 25 additions & 0 deletions tests/ui/needless_pass_by_ref_mut.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,31 @@ mod foo {
//~| 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;
}

fn main() {
let mut u = 0;
let mut v = vec![0];
Expand Down
14 changes: 13 additions & 1 deletion tests/ui/needless_pass_by_ref_mut.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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