From e3267b1fe7bf90348f76b42fba6ebd09ef8ef714 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 13 Sep 2023 14:39:41 +0200 Subject: [PATCH 1/2] Fix mutaby used async function argument in closure for `needless_pass_by_ref_mut` --- clippy_lints/src/needless_pass_by_ref_mut.rs | 85 +++++++++++++++----- tests/ui/needless_pass_by_ref_mut.rs | 28 ++++++- tests/ui/needless_pass_by_ref_mut.stderr | 26 +++++- 3 files changed, 118 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs index 7b00eabf97b..bea44ed92ef 100644 --- a/clippy_lints/src/needless_pass_by_ref_mut.rs +++ b/clippy_lints/src/needless_pass_by_ref_mut.rs @@ -4,12 +4,13 @@ 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::intravisit::{walk_fn, walk_qpath, FnKind, Visitor}; use rustc_hir::{ - Body, Closure, Expr, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath, + Body, BodyId, 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_infer::infer::{InferCtxt, TyCtxtInferExt}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::associated_body; use rustc_middle::hir::nested_filter::OnlyBodies; @@ -95,6 +96,30 @@ fn should_skip<'tcx>( is_from_proc_macro(cx, &input) } +fn check_closures<'tcx>( + ctx: &mut MutablyUsedVariablesCtxt<'tcx>, + cx: &LateContext<'tcx>, + infcx: &InferCtxt<'tcx>, + checked_closures: &mut FxHashSet, + closures: FxHashSet, +) { + 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(ctx, infcx, closure, cx.param_env, cx.typeck_results()).consume_body(body); + } + } +} + impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> { fn check_fn( &mut self, @@ -161,25 +186,20 @@ fn check_fn( euv::ExprUseVisitor::new(&mut ctx, &infcx, fn_def_id, cx.param_env, cx.typeck_results()).consume_body(body); if is_async { let mut checked_closures = FxHashSet::default(); + + // We retrieve all the closures declared in the async function because they will + // not be found by `euv::Delegate`. + let mut closures_retriever = ClosuresRetriever { + cx, + closures: FxHashSet::default(), + }; + walk_fn(&mut closures_retriever, kind, decl, body.id(), fn_def_id); + check_closures(&mut ctx, cx, &infcx, &mut checked_closures, closures_retriever.closures); + 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); - } - } + check_closures(&mut ctx, cx, &infcx, &mut checked_closures, closures); } } ctx @@ -439,3 +459,30 @@ fn visit_qpath(&mut self, qpath: &'tcx QPath<'tcx>, hir_id: HirId, _: Span) { } } } + +struct ClosuresRetriever<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + closures: FxHashSet, +} + +impl<'a, 'tcx> Visitor<'tcx> for ClosuresRetriever<'a, 'tcx> { + type NestedFilter = OnlyBodies; + + fn nested_visit_map(&mut self) -> Self::Map { + self.cx.tcx.hir() + } + + fn visit_fn( + &mut self, + kind: FnKind<'tcx>, + decl: &'tcx FnDecl<'tcx>, + body_id: BodyId, + _span: Span, + fn_def_id: LocalDefId, + ) { + if matches!(kind, FnKind::Closure) { + self.closures.insert(fn_def_id); + } + walk_fn(self, kind, decl, body_id, fn_def_id); + } +} diff --git a/tests/ui/needless_pass_by_ref_mut.rs b/tests/ui/needless_pass_by_ref_mut.rs index e1e5e8fd220..da2a72caacb 100644 --- a/tests/ui/needless_pass_by_ref_mut.rs +++ b/tests/ui/needless_pass_by_ref_mut.rs @@ -1,4 +1,4 @@ -#![allow(clippy::if_same_then_else, clippy::no_effect)] +#![allow(clippy::if_same_then_else, clippy::no_effect, clippy::redundant_closure_call)] #![feature(lint_reasons)] //@no-rustfix use std::ptr::NonNull; @@ -231,6 +231,32 @@ async fn async_vec2(b: &mut Vec) { b.push(true); } +// Should not warn. +pub async fn closure(n: &mut usize) -> impl '_ + FnMut() { + || { + *n += 1; + } +} + +// Should warn. +pub fn closure2(n: &mut usize) -> impl '_ + FnMut() -> usize { + //~^ ERROR: this argument is a mutable reference, but not used mutably + || *n + 1 +} + +// Should not warn. +pub async fn closure3(n: &mut usize) { + (|| *n += 1)(); +} + +// Should warn. +pub async fn closure4(n: &mut usize) { + //~^ ERROR: this argument is a mutable reference, but not used mutably + (|| { + let _x = *n + 1; + })(); +} + 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 df3df045776..0fb9458d794 100644 --- a/tests/ui/needless_pass_by_ref_mut.stderr +++ b/tests/ui/needless_pass_by_ref_mut.stderr @@ -107,5 +107,29 @@ error: this argument is a mutable reference, but not used mutably LL | async fn inner_async3(x: &mut i32, y: &mut u32) { | ^^^^^^^^ help: consider changing to: `&i32` -error: aborting due to 17 previous errors +error: this argument is a mutable reference, but not used mutably + --> $DIR/needless_pass_by_ref_mut.rs:235:25 + | +LL | pub async fn closure(n: &mut usize) -> impl '_ + FnMut() { + | ^^^^^^^^^^ help: consider changing to: `&usize` + | + = warning: changing this function will impact semver compatibility + +error: this argument is a mutable reference, but not used mutably + --> $DIR/needless_pass_by_ref_mut.rs:242:20 + | +LL | pub fn closure2(n: &mut usize) -> impl '_ + FnMut() -> usize { + | ^^^^^^^^^^ help: consider changing to: `&usize` + | + = warning: changing this function will impact semver compatibility + +error: this argument is a mutable reference, but not used mutably + --> $DIR/needless_pass_by_ref_mut.rs:253:26 + | +LL | pub async fn closure4(n: &mut usize) { + | ^^^^^^^^^^ help: consider changing to: `&usize` + | + = warning: changing this function will impact semver compatibility + +error: aborting due to 20 previous errors From b8b420cc79dc18423a9bf59ee3397a398e9aac2a Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 18 Sep 2023 15:47:24 +0200 Subject: [PATCH 2/2] Improve code readability by moving the retrieval of closures inside async functions right besides other closures handling. Add doc comment explaining what `MutablyUsedVariablesCtxt::prev_move_to_closure` is about. --- clippy_lints/src/needless_pass_by_ref_mut.rs | 57 +++++++------------- 1 file changed, 19 insertions(+), 38 deletions(-) diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs index bea44ed92ef..36ca61c0b44 100644 --- a/clippy_lints/src/needless_pass_by_ref_mut.rs +++ b/clippy_lints/src/needless_pass_by_ref_mut.rs @@ -1,13 +1,13 @@ use super::needless_pass_by_value::requires_exact_signature; use clippy_utils::diagnostics::span_lint_hir_and_then; use clippy_utils::source::snippet; +use clippy_utils::visitors::for_each_expr_with_closures; 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_fn, walk_qpath, FnKind, Visitor}; +use rustc_hir::intravisit::{walk_qpath, FnKind, Visitor}; use rustc_hir::{ - Body, BodyId, Closure, Expr, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, - PatKind, QPath, + 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}; @@ -22,6 +22,8 @@ use rustc_span::Span; use rustc_target::spec::abi::Abi; +use core::ops::ControlFlow; + declare_clippy_lint! { /// ### What it does /// Check if a `&mut` function argument is actually used mutably. @@ -189,17 +191,19 @@ fn check_fn( // We retrieve all the closures declared in the async function because they will // not be found by `euv::Delegate`. - let mut closures_retriever = ClosuresRetriever { - cx, - closures: FxHashSet::default(), - }; - walk_fn(&mut closures_retriever, kind, decl, body.id(), fn_def_id); - check_closures(&mut ctx, cx, &infcx, &mut checked_closures, closures_retriever.closures); + let mut closures: FxHashSet = FxHashSet::default(); + for_each_expr_with_closures(cx, body, |expr| { + if let ExprKind::Closure(closure) = expr.kind { + closures.insert(closure.def_id); + } + ControlFlow::<()>::Continue(()) + }); + check_closures(&mut ctx, cx, &infcx, &mut checked_closures, closures); while !ctx.async_closures.is_empty() { - let closures = ctx.async_closures.clone(); + let async_closures = ctx.async_closures.clone(); ctx.async_closures.clear(); - check_closures(&mut ctx, cx, &infcx, &mut checked_closures, closures); + check_closures(&mut ctx, cx, &infcx, &mut checked_closures, async_closures); } } ctx @@ -264,6 +268,10 @@ fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { struct MutablyUsedVariablesCtxt<'tcx> { mutably_used_vars: HirIdSet, prev_bind: Option, + /// In async functions, the inner AST is composed of multiple layers until we reach the code + /// defined by the user. Because of that, some variables are marked as mutably borrowed even + /// though they're not. This field lists the `HirId` that should not be considered as mutable + /// use of a variable. prev_move_to_closure: HirIdSet, aliases: HirIdMap, async_closures: FxHashSet, @@ -459,30 +467,3 @@ fn visit_qpath(&mut self, qpath: &'tcx QPath<'tcx>, hir_id: HirId, _: Span) { } } } - -struct ClosuresRetriever<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - closures: FxHashSet, -} - -impl<'a, 'tcx> Visitor<'tcx> for ClosuresRetriever<'a, 'tcx> { - type NestedFilter = OnlyBodies; - - fn nested_visit_map(&mut self) -> Self::Map { - self.cx.tcx.hir() - } - - fn visit_fn( - &mut self, - kind: FnKind<'tcx>, - decl: &'tcx FnDecl<'tcx>, - body_id: BodyId, - _span: Span, - fn_def_id: LocalDefId, - ) { - if matches!(kind, FnKind::Closure) { - self.closures.insert(fn_def_id); - } - walk_fn(self, kind, decl, body_id, fn_def_id); - } -}