Skip to content

Commit ef482d1

Browse files
Centri3Centri3
committed
Do not lint if used as a fn-like argument
1 parent 867e0ec commit ef482d1

File tree

8 files changed

+186
-99
lines changed

8 files changed

+186
-99
lines changed

‎clippy_lints/src/needless_pass_by_ref_mut.rs‎

Lines changed: 86 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
use super::needless_pass_by_value::requires_exact_signature;
2-
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::diagnostics::span_lint_hir_and_then;
33
use clippy_utils::source::snippet;
4-
use clippy_utils::{is_from_proc_macro, is_self};
5-
use if_chain::if_chain;
6-
use rustc_data_structures::fx::FxHashSet;
4+
use clippy_utils::{get_parent_node, is_from_proc_macro, is_self};
5+
use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
76
use rustc_errors::Applicability;
8-
use rustc_hir::intravisit::FnKind;
9-
use rustc_hir::{Body, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind};
7+
use rustc_hir::intravisit::{walk_qpath, FnKind, Visitor};
8+
use rustc_hir::{Body, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath};
109
use rustc_hir_typeck::expr_use_visitor as euv;
1110
use rustc_infer::infer::TyCtxtInferExt;
1211
use rustc_lint::{LateContext, LateLintPass};
1312
use rustc_middle::hir::map::associated_body;
13+
use rustc_middle::hir::nested_filter::OnlyBodies;
1414
use rustc_middle::mir::FakeReadCause;
1515
use rustc_middle::ty::{self, Ty, UpvarId, UpvarPath};
1616
use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -48,20 +48,24 @@ declare_clippy_lint! {
4848
"using a `&mut` argument when it's not mutated"
4949
}
5050

51-
#[derive(Copy, Clone)]
52-
pub struct NeedlessPassByRefMut {
51+
#[derive(Clone)]
52+
pub struct NeedlessPassByRefMut<'tcx> {
5353
avoid_breaking_exported_api: bool,
54+
used_fn_def_ids: FxHashSet<LocalDefId>,
55+
fn_def_ids_to_maybe_unused_mut: FxIndexMap<LocalDefId, Vec<rustc_hir::Ty<'tcx>>>,
5456
}
5557

56-
impl NeedlessPassByRefMut {
58+
impl NeedlessPassByRefMut<'_> {
5759
pub fn new(avoid_breaking_exported_api: bool) -> Self {
5860
Self {
5961
avoid_breaking_exported_api,
62+
used_fn_def_ids: FxHashSet::default(),
63+
fn_def_ids_to_maybe_unused_mut: FxIndexMap::default(),
6064
}
6165
}
6266
}
6367

64-
impl_lint_pass!(NeedlessPassByRefMut => [NEEDLESS_PASS_BY_REF_MUT]);
68+
impl_lint_pass!(NeedlessPassByRefMut<'_> => [NEEDLESS_PASS_BY_REF_MUT]);
6569

6670
fn should_skip<'tcx>(
6771
cx: &LateContext<'tcx>,
@@ -89,12 +93,12 @@ fn should_skip<'tcx>(
8993
is_from_proc_macro(cx, &input)
9094
}
9195

92-
impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
96+
impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
9397
fn check_fn(
9498
&mut self,
9599
cx: &LateContext<'tcx>,
96100
kind: FnKind<'tcx>,
97-
decl: &'tcx FnDecl<'_>,
101+
decl: &'tcx FnDecl<'tcx>,
98102
body: &'tcx Body<'_>,
99103
span: Span,
100104
fn_def_id: LocalDefId,
@@ -140,7 +144,6 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
140144
if it.peek().is_none() {
141145
return;
142146
}
143-
144147
// Collect variables mutably used and spans which will need dereferencings from the
145148
// function body.
146149
let MutablyUsedVariablesCtxt { mutably_used_vars, .. } = {
@@ -165,30 +168,45 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
165168
}
166169
ctx
167170
};
168-
169-
let show_semver_warning = self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(fn_def_id);
170171
for ((&input, &_), arg) in it {
171172
// Only take `&mut` arguments.
172-
if_chain! {
173-
if let PatKind::Binding(_, canonical_id, ..) = arg.pat.kind;
174-
if !mutably_used_vars.contains(&canonical_id);
175-
if let rustc_hir::TyKind::Ref(_, inner_ty) = input.kind;
176-
then {
177-
// If the argument is never used mutably, we emit the warning.
178-
let sp = input.span;
179-
span_lint_and_then(
173+
if let PatKind::Binding(_, canonical_id, ..) = arg.pat.kind
174+
&& !mutably_used_vars.contains(&canonical_id)
175+
{
176+
self.fn_def_ids_to_maybe_unused_mut.entry(fn_def_id).or_default().push(input);
177+
}
178+
}
179+
}
180+
181+
fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
182+
cx.tcx.hir().visit_all_item_likes_in_crate(&mut FnNeedsMutVisitor {
183+
cx,
184+
used_fn_def_ids: &mut self.used_fn_def_ids,
185+
});
186+
187+
for (fn_def_id, unused) in self
188+
.fn_def_ids_to_maybe_unused_mut
189+
.iter()
190+
.filter(|(def_id, _)| !self.used_fn_def_ids.contains(def_id))
191+
{
192+
let show_semver_warning =
193+
self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(*fn_def_id);
194+
195+
for input in unused {
196+
// If the argument is never used mutably, we emit the warning.
197+
let sp = input.span;
198+
if let rustc_hir::TyKind::Ref(_, inner_ty) = input.kind {
199+
span_lint_hir_and_then(
180200
cx,
181201
NEEDLESS_PASS_BY_REF_MUT,
202+
cx.tcx.hir().local_def_id_to_hir_id(*fn_def_id),
182203
sp,
183204
"this argument is a mutable reference, but not used mutably",
184205
|diag| {
185206
diag.span_suggestion(
186207
sp,
187208
"consider changing to".to_string(),
188-
format!(
189-
"&{}",
190-
snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),
191-
),
209+
format!("&{}", snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),),
192210
Applicability::Unspecified,
193211
);
194212
if show_semver_warning {
@@ -316,3 +334,44 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
316334
self.prev_bind = Some(id);
317335
}
318336
}
337+
338+
/// A final pass to check for paths referencing this function that require the argument to be
339+
/// `&mut`, basically if the function is ever used as a `fn`-like argument.
340+
struct FnNeedsMutVisitor<'a, 'tcx> {
341+
cx: &'a LateContext<'tcx>,
342+
used_fn_def_ids: &'a mut FxHashSet<LocalDefId>,
343+
}
344+
345+
impl<'tcx> Visitor<'tcx> for FnNeedsMutVisitor<'_, 'tcx> {
346+
type NestedFilter = OnlyBodies;
347+
348+
fn nested_visit_map(&mut self) -> Self::Map {
349+
self.cx.tcx.hir()
350+
}
351+
352+
fn visit_qpath(&mut self, qpath: &'tcx QPath<'tcx>, hir_id: HirId, _: Span) {
353+
walk_qpath(self, qpath, hir_id);
354+
355+
let Self { cx, used_fn_def_ids } = self;
356+
357+
// #11182; do not lint if mutability is required elsewhere
358+
if let Node::Expr(expr) = cx.tcx.hir().get(hir_id)
359+
&& let Some(parent) = get_parent_node(cx.tcx, expr.hir_id)
360+
&& let ty::FnDef(def_id, _) = cx.tcx.typeck(cx.tcx.hir().enclosing_body_owner(hir_id)).expr_ty(expr).kind()
361+
&& let Some(def_id) = def_id.as_local()
362+
{
363+
if let Node::Expr(e) = parent
364+
&& let ExprKind::Call(call, _) = e.kind
365+
&& call.hir_id == expr.hir_id
366+
{
367+
return;
368+
}
369+
370+
// We don't need to check each argument individually as you cannot coerce a function
371+
// taking `&mut` -> `&`, for some reason, so if we've gotten this far we know it's
372+
// passed as a `fn`-like argument (or is unified) and should ignore every "unused"
373+
// argument entirely
374+
used_fn_def_ids.insert(def_id);
375+
}
376+
}
377+
}

‎tests/ui/infinite_loop.stderr‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,3 @@
1-
error: this argument is a mutable reference, but not used mutably
2-
--> $DIR/infinite_loop.rs:7:17
3-
|
4-
LL | fn fn_mutref(i: &mut i32) {
5-
| ^^^^^^^^ help: consider changing to: `&i32`
6-
|
7-
= note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
8-
91
error: variables in the condition are not mutated in the loop body
102
--> $DIR/infinite_loop.rs:20:11
113
|
@@ -99,5 +91,13 @@ LL | while y < 10 {
9991
= note: this loop contains `return`s or `break`s
10092
= help: rewrite it as `if cond { loop { } }`
10193

94+
error: this argument is a mutable reference, but not used mutably
95+
--> $DIR/infinite_loop.rs:7:17
96+
|
97+
LL | fn fn_mutref(i: &mut i32) {
98+
| ^^^^^^^^ help: consider changing to: `&i32`
99+
|
100+
= note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
101+
102102
error: aborting due to 12 previous errors
103103

‎tests/ui/let_underscore_future.stderr‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,3 @@
1-
error: this argument is a mutable reference, but not used mutably
2-
--> $DIR/let_underscore_future.rs:11:35
3-
|
4-
LL | fn do_something_to_future(future: &mut impl Future<Output = ()>) {}
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&impl Future<Output = ()>`
6-
|
7-
= note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
8-
91
error: non-binding `let` on a future
102
--> $DIR/let_underscore_future.rs:14:5
113
|
@@ -31,5 +23,13 @@ LL | let _ = future;
3123
|
3224
= help: consider awaiting the future or dropping explicitly with `std::mem::drop`
3325

26+
error: this argument is a mutable reference, but not used mutably
27+
--> $DIR/let_underscore_future.rs:11:35
28+
|
29+
LL | fn do_something_to_future(future: &mut impl Future<Output = ()>) {}
30+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&impl Future<Output = ()>`
31+
|
32+
= note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
33+
3434
error: aborting due to 4 previous errors
3535

‎tests/ui/mut_key.stderr‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,6 @@ error: mutable key type
1212
LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
1313
| ^^^^^^^^^^^^
1414

15-
error: this argument is a mutable reference, but not used mutably
16-
--> $DIR/mut_key.rs:31:32
17-
|
18-
LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
19-
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&HashMap<Key, usize>`
20-
|
21-
= note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
22-
2315
error: mutable key type
2416
--> $DIR/mut_key.rs:32:5
2517
|
@@ -110,5 +102,13 @@ error: mutable key type
110102
LL | let _map = HashMap::<Arc<Cell<usize>>, usize>::new();
111103
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
112104

105+
error: this argument is a mutable reference, but not used mutably
106+
--> $DIR/mut_key.rs:31:32
107+
|
108+
LL | fn should_not_take_this_arg(m: &mut HashMap<Key, usize>, _n: usize) -> HashSet<Key> {
109+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&HashMap<Key, usize>`
110+
|
111+
= note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
112+
113113
error: aborting due to 18 previous errors
114114

‎tests/ui/mut_reference.stderr‎

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,3 @@
1-
error: this argument is a mutable reference, but not used mutably
2-
--> $DIR/mut_reference.rs:4:33
3-
|
4-
LL | fn takes_a_mutable_reference(a: &mut i32) {}
5-
| ^^^^^^^^ help: consider changing to: `&i32`
6-
|
7-
= note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
8-
9-
error: this argument is a mutable reference, but not used mutably
10-
--> $DIR/mut_reference.rs:11:44
11-
|
12-
LL | fn takes_a_mutable_reference(&self, a: &mut i32) {}
13-
| ^^^^^^^^ help: consider changing to: `&i32`
14-
151
error: the function `takes_an_immutable_reference` doesn't need a mutable reference
162
--> $DIR/mut_reference.rs:17:34
173
|
@@ -32,5 +18,13 @@ error: the method `takes_an_immutable_reference` doesn't need a mutable referenc
3218
LL | my_struct.takes_an_immutable_reference(&mut 42);
3319
| ^^^^^^^
3420

35-
error: aborting due to 5 previous errors
21+
error: this argument is a mutable reference, but not used mutably
22+
--> $DIR/mut_reference.rs:11:44
23+
|
24+
LL | fn takes_a_mutable_reference(&self, a: &mut i32) {}
25+
| ^^^^^^^^ help: consider changing to: `&i32`
26+
|
27+
= note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
28+
29+
error: aborting due to 4 previous errors
3630

‎tests/ui/needless_pass_by_ref_mut.rs‎

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
#![allow(unused)]
1+
#![allow(clippy::if_same_then_else, clippy::no_effect)]
2+
#![feature(lint_reasons)]
23

34
use std::ptr::NonNull;
45

@@ -155,15 +156,48 @@ async fn a8(x: i32, a: &mut i32, y: i32, z: &mut i32) {
155156
println!("{:?}", z);
156157
}
157158

159+
// Should not warn (passed as closure which takes `&mut`).
160+
fn passed_as_closure(s: &mut u32) {}
161+
162+
// Should not warn.
163+
fn passed_as_local(s: &mut u32) {}
164+
165+
// Should not warn.
166+
fn ty_unify_1(s: &mut u32) {}
167+
168+
// Should not warn.
169+
fn ty_unify_2(s: &mut u32) {}
170+
171+
// Should not warn.
172+
fn passed_as_field(s: &mut u32) {}
173+
174+
fn closure_takes_mut(s: fn(&mut u32)) {}
175+
176+
struct A {
177+
s: fn(&mut u32),
178+
}
179+
180+
// Should warn.
181+
fn used_as_path(s: &mut u32) {}
182+
183+
// Make sure lint attributes work fine
184+
#[expect(clippy::needless_pass_by_ref_mut)]
185+
fn lint_attr(s: &mut u32) {}
186+
158187
fn main() {
159-
// let mut u = 0;
160-
// let mut v = vec![0];
161-
// foo(&mut v, &0, &mut u);
162-
// foo2(&mut v);
163-
// foo3(&mut v);
164-
// foo4(&mut v);
165-
// foo5(&mut v);
166-
// alias_check(&mut v);
167-
// alias_check2(&mut v);
168-
// println!("{u}");
188+
let mut u = 0;
189+
let mut v = vec![0];
190+
foo(&mut v, &0, &mut u);
191+
foo2(&mut v);
192+
foo3(&mut v);
193+
foo4(&mut v);
194+
foo5(&mut v);
195+
alias_check(&mut v);
196+
alias_check2(&mut v);
197+
println!("{u}");
198+
closure_takes_mut(passed_as_closure);
199+
A { s: passed_as_field };
200+
used_as_path;
201+
let _: fn(&mut u32) = passed_as_local;
202+
let _ = if v[0] == 0 { ty_unify_1 } else { ty_unify_2 };
169203
}

0 commit comments

Comments
 (0)