Skip to content

Commit 929e266

Browse files
Centri3Centri3
committed
Do not lint if used as a fn-like argument
1 parent ee8a429 commit 929e266

File tree

8 files changed

+183
-83
lines changed

8 files changed

+183
-83
lines changed

‎clippy_lints/src/needless_pass_by_ref_mut.rs

Lines changed: 95 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,17 @@
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;
4+
use clippy_utils::{get_parent_node, is_from_proc_macro, is_self};
5+
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
66
use rustc_errors::Applicability;
7-
use rustc_hir::intravisit::FnKind;
8-
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::{
9+
Body, ExprField, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath,
10+
};
911
use rustc_hir_typeck::expr_use_visitor as euv;
1012
use rustc_infer::infer::TyCtxtInferExt;
1113
use rustc_lint::{LateContext, LateLintPass};
14+
use rustc_middle::hir::nested_filter::OnlyBodies;
1215
use rustc_middle::mir::FakeReadCause;
1316
use rustc_middle::ty::{self, Ty};
1417
use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -46,20 +49,26 @@ declare_clippy_lint! {
4649
"using a `&mut` argument when it's not mutated"
4750
}
4851

49-
#[derive(Copy, Clone)]
50-
pub struct NeedlessPassByRefMut {
52+
#[derive(Clone)]
53+
pub struct NeedlessPassByRefMut<'tcx> {
5154
avoid_breaking_exported_api: bool,
55+
used_fn_def_ids: FxHashSet<LocalDefId>,
56+
// TODO(Centri3): Change this to a `Vec`. This will cause a performance deficit but it'll make the output a lot
57+
// more consistent, as adding or removing a function won't change the order of them
58+
fn_def_ids_to_maybe_unused_mut: FxHashMap<LocalDefId, Vec<rustc_hir::Ty<'tcx>>>,
5259
}
5360

54-
impl NeedlessPassByRefMut {
61+
impl NeedlessPassByRefMut<'_> {
5562
pub fn new(avoid_breaking_exported_api: bool) -> Self {
5663
Self {
5764
avoid_breaking_exported_api,
65+
used_fn_def_ids: FxHashSet::default(),
66+
fn_def_ids_to_maybe_unused_mut: FxHashMap::default(),
5867
}
5968
}
6069
}
6170

62-
impl_lint_pass!(NeedlessPassByRefMut => [NEEDLESS_PASS_BY_REF_MUT]);
71+
impl_lint_pass!(NeedlessPassByRefMut<'_> => [NEEDLESS_PASS_BY_REF_MUT]);
6372

6473
fn should_skip<'tcx>(
6574
cx: &LateContext<'tcx>,
@@ -87,12 +96,12 @@ fn should_skip<'tcx>(
8796
is_from_proc_macro(cx, &input)
8897
}
8998

90-
impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
99+
impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
91100
fn check_fn(
92101
&mut self,
93102
cx: &LateContext<'tcx>,
94103
kind: FnKind<'tcx>,
95-
decl: &'tcx FnDecl<'_>,
104+
decl: &'tcx FnDecl<'tcx>,
96105
body: &'tcx Body<'_>,
97106
span: Span,
98107
fn_def_id: LocalDefId,
@@ -157,29 +166,45 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
157166
if it.peek().is_none() {
158167
return;
159168
}
160-
let show_semver_warning = self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(fn_def_id);
161169
for ((&input, &_), arg) in it {
162170
// Only take `&mut` arguments.
163-
if_chain! {
164-
if let PatKind::Binding(_, canonical_id, ..) = arg.pat.kind;
165-
if !mutably_used_vars.contains(&canonical_id);
166-
if let rustc_hir::TyKind::Ref(_, inner_ty) = input.kind;
167-
then {
168-
// If the argument is never used mutably, we emit the warning.
169-
let sp = input.span;
170-
span_lint_and_then(
171+
if let PatKind::Binding(_, canonical_id, ..) = arg.pat.kind
172+
&& !mutably_used_vars.contains(&canonical_id)
173+
{
174+
self.fn_def_ids_to_maybe_unused_mut.entry(fn_def_id).or_insert(vec![]).push(input);
175+
}
176+
}
177+
}
178+
179+
fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
180+
cx.tcx.hir().visit_all_item_likes_in_crate(&mut FnNeedsMutVisitor {
181+
cx,
182+
used_fn_def_ids: &mut self.used_fn_def_ids,
183+
});
184+
185+
for (fn_def_id, unused) in self
186+
.fn_def_ids_to_maybe_unused_mut
187+
.iter()
188+
.filter(|(def_id, _)| !self.used_fn_def_ids.contains(def_id))
189+
{
190+
let show_semver_warning =
191+
self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(*fn_def_id);
192+
193+
for input in unused {
194+
// If the argument is never used mutably, we emit the warning.
195+
let sp = input.span;
196+
if let rustc_hir::TyKind::Ref(_, inner_ty) = input.kind {
197+
span_lint_hir_and_then(
171198
cx,
172199
NEEDLESS_PASS_BY_REF_MUT,
200+
cx.tcx.hir().local_def_id_to_hir_id(*fn_def_id),
173201
sp,
174202
"this argument is a mutable reference, but not used mutably",
175203
|diag| {
176204
diag.span_suggestion(
177205
sp,
178206
"consider changing to".to_string(),
179-
format!(
180-
"&{}",
181-
snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),
182-
),
207+
format!("&{}", snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),),
183208
Applicability::Unspecified,
184209
);
185210
if show_semver_warning {
@@ -271,3 +296,49 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
271296
self.prev_bind = Some(id);
272297
}
273298
}
299+
300+
/// A final pass to check for paths referencing this function that require the argument to be
301+
/// `&mut`, basically if the function is ever used as a `fn`-like argument.
302+
struct FnNeedsMutVisitor<'a, 'tcx> {
303+
cx: &'a LateContext<'tcx>,
304+
used_fn_def_ids: &'a mut FxHashSet<LocalDefId>,
305+
}
306+
307+
impl<'tcx> Visitor<'tcx> for FnNeedsMutVisitor<'_, 'tcx> {
308+
type NestedFilter = OnlyBodies;
309+
310+
fn nested_visit_map(&mut self) -> Self::Map {
311+
self.cx.tcx.hir()
312+
}
313+
314+
fn visit_qpath(&mut self, qpath: &'tcx QPath<'tcx>, hir_id: HirId, _: Span) {
315+
walk_qpath(self, qpath, hir_id);
316+
317+
let Self { cx, used_fn_def_ids } = self;
318+
319+
if let Node::Expr(expr) = cx.tcx.hir().get(hir_id)
320+
&& let Some(parent) = get_parent_node(cx.tcx, expr.hir_id)
321+
&& let ty::FnDef(def_id, _) = cx.tcx.typeck(cx.tcx.hir().enclosing_body_owner(hir_id)).expr_ty(expr).kind()
322+
&& let Some(def_id) = def_id.as_local()
323+
{
324+
let Some(e) = (match parent {
325+
// #11182
326+
Node::Expr(e) => Some(e),
327+
// #11199
328+
Node::ExprField(ExprField { expr, .. }) => Some(*expr),
329+
_ => None,
330+
}) else {
331+
return;
332+
};
333+
334+
if matches!(e.kind, ExprKind::Call(call, _) if call.hir_id == expr.hir_id) {
335+
return;
336+
}
337+
338+
// We don't need to check each argument individually as you cannot coerce a function
339+
// taking `&mut` -> `&`, for some reason, so if we've gotten this far we know it's
340+
// passed as a `fn`-like argument and should ignore every "unused" argument entirely
341+
used_fn_def_ids.insert(def_id);
342+
}
343+
}
344+
}

‎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: 14 additions & 14 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,19 @@ 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

21+
error: this argument is a mutable reference, but not used mutably
22+
--> $DIR/mut_reference.rs:4:33
23+
|
24+
LL | fn takes_a_mutable_reference(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: this argument is a mutable reference, but not used mutably
30+
--> $DIR/mut_reference.rs:11:44
31+
|
32+
LL | fn takes_a_mutable_reference(&self, a: &mut i32) {}
33+
| ^^^^^^^^ help: consider changing to: `&i32`
34+
3535
error: aborting due to 5 previous errors
3636

‎tests/ui/needless_pass_by_ref_mut.rs

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

34
use std::ptr::NonNull;
45

@@ -91,6 +92,25 @@ impl<T> Mut<T> {
9192
// Should not warn.
9293
fn unused(_: &mut u32, _b: &mut u8) {}
9394

95+
// Should not warn (passed as closure which takes `&mut`).
96+
fn passed_as_closure(s: &mut u32) {}
97+
98+
// Should not warn.
99+
fn passed_as_field(s: &mut u32) {}
100+
101+
fn closure_takes_mut(s: fn(&mut u32)) {}
102+
103+
struct A {
104+
s: fn(&mut u32),
105+
}
106+
107+
// Should warn.
108+
fn used_as_path(s: &mut u32) {}
109+
110+
// Make sure lint attributes work fine
111+
#[expect(clippy::needless_pass_by_ref_mut)]
112+
fn lint_attr(s: &mut u32) {}
113+
94114
fn main() {
95115
let mut u = 0;
96116
let mut v = vec![0];
@@ -102,4 +122,7 @@ fn main() {
102122
alias_check(&mut v);
103123
alias_check2(&mut v);
104124
println!("{u}");
125+
closure_takes_mut(passed_as_closure);
126+
A { s: passed_as_field };
127+
used_as_path;
105128
}

‎tests/ui/needless_pass_by_ref_mut.stderr

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,34 @@
11
error: this argument is a mutable reference, but not used mutably
2-
--> $DIR/needless_pass_by_ref_mut.rs:6:11
2+
--> $DIR/needless_pass_by_ref_mut.rs:50:31
33
|
4-
LL | fn foo(s: &mut Vec<u32>, b: &u32, x: &mut u32) {
5-
| ^^^^^^^^^^^^^ help: consider changing to: `&Vec<u32>`
4+
LL | fn badger(&mut self, vec: &mut Vec<i32>) -> usize {
5+
| ^^^^^^^^^^^^^ help: consider changing to: `&Vec<i32>`
66
|
77
= note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`
88

99
error: this argument is a mutable reference, but not used mutably
10-
--> $DIR/needless_pass_by_ref_mut.rs:31:12
10+
--> $DIR/needless_pass_by_ref_mut.rs:7:11
1111
|
12-
LL | fn foo6(s: &mut Vec<u32>) {
13-
| ^^^^^^^^^^^^^ help: consider changing to: `&Vec<u32>`
12+
LL | fn foo(s: &mut Vec<u32>, b: &u32, x: &mut u32) {
13+
| ^^^^^^^^^^^^^ help: consider changing to: `&Vec<u32>`
1414

1515
error: this argument is a mutable reference, but not used mutably
16-
--> $DIR/needless_pass_by_ref_mut.rs:44:29
16+
--> $DIR/needless_pass_by_ref_mut.rs:45:29
1717
|
1818
LL | fn mushroom(&self, vec: &mut Vec<i32>) -> usize {
1919
| ^^^^^^^^^^^^^ help: consider changing to: `&Vec<i32>`
2020

2121
error: this argument is a mutable reference, but not used mutably
22-
--> $DIR/needless_pass_by_ref_mut.rs:49:31
22+
--> $DIR/needless_pass_by_ref_mut.rs:32:12
2323
|
24-
LL | fn badger(&mut self, vec: &mut Vec<i32>) -> usize {
25-
| ^^^^^^^^^^^^^ help: consider changing to: `&Vec<i32>`
24+
LL | fn foo6(s: &mut Vec<u32>) {
25+
| ^^^^^^^^^^^^^ help: consider changing to: `&Vec<u32>`
26+
27+
error: this argument is a mutable reference, but not used mutably
28+
--> $DIR/needless_pass_by_ref_mut.rs:108:20
29+
|
30+
LL | fn used_as_path(s: &mut u32) {}
31+
| ^^^^^^^^ help: consider changing to: `&u32`
2632

27-
error: aborting due to 4 previous errors
33+
error: aborting due to 5 previous errors
2834

0 commit comments

Comments
 (0)