Skip to content

Commit 5ffc58c

Browse files
Centri3Centri3
committed
Do not lint if used as a fn-like argument
1 parent a4e64ff commit 5ffc58c

File tree

8 files changed

+193
-99
lines changed

8 files changed

+193
-99
lines changed

‎clippy_lints/src/needless_pass_by_ref_mut.rs

Lines changed: 93 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,18 @@
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::{
9+
Body, ExprField, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath,
10+
};
1011
use rustc_hir_typeck::expr_use_visitor as euv;
1112
use rustc_infer::infer::TyCtxtInferExt;
1213
use rustc_lint::{LateContext, LateLintPass};
1314
use rustc_middle::hir::map::associated_body;
15+
use rustc_middle::hir::nested_filter::OnlyBodies;
1416
use rustc_middle::mir::FakeReadCause;
1517
use rustc_middle::ty::{self, Ty, UpvarId, UpvarPath};
1618
use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -48,20 +50,24 @@ declare_clippy_lint! {
4850
"using a `&mut` argument when it's not mutated"
4951
}
5052

51-
#[derive(Copy, Clone)]
52-
pub struct NeedlessPassByRefMut {
53+
#[derive(Clone)]
54+
pub struct NeedlessPassByRefMut<'tcx> {
5355
avoid_breaking_exported_api: bool,
56+
used_fn_def_ids: FxHashSet<LocalDefId>,
57+
fn_def_ids_to_maybe_unused_mut: FxIndexMap<LocalDefId, Vec<rustc_hir::Ty<'tcx>>>,
5458
}
5559

56-
impl NeedlessPassByRefMut {
60+
impl NeedlessPassByRefMut<'_> {
5761
pub fn new(avoid_breaking_exported_api: bool) -> Self {
5862
Self {
5963
avoid_breaking_exported_api,
64+
used_fn_def_ids: FxHashSet::default(),
65+
fn_def_ids_to_maybe_unused_mut: FxIndexMap::default(),
6066
}
6167
}
6268
}
6369

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

6672
fn should_skip<'tcx>(
6773
cx: &LateContext<'tcx>,
@@ -89,12 +95,12 @@ fn should_skip<'tcx>(
8995
is_from_proc_macro(cx, &input)
9096
}
9197

92-
impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
98+
impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
9399
fn check_fn(
94100
&mut self,
95101
cx: &LateContext<'tcx>,
96102
kind: FnKind<'tcx>,
97-
decl: &'tcx FnDecl<'_>,
103+
decl: &'tcx FnDecl<'tcx>,
98104
body: &'tcx Body<'_>,
99105
span: Span,
100106
fn_def_id: LocalDefId,
@@ -140,7 +146,6 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
140146
if it.peek().is_none() {
141147
return;
142148
}
143-
144149
// Collect variables mutably used and spans which will need dereferencings from the
145150
// function body.
146151
let MutablyUsedVariablesCtxt { mutably_used_vars, .. } = {
@@ -165,30 +170,45 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
165170
}
166171
ctx
167172
};
168-
169-
let show_semver_warning = self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(fn_def_id);
170173
for ((&input, &_), arg) in it {
171174
// 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(
175+
if let PatKind::Binding(_, canonical_id, ..) = arg.pat.kind
176+
&& !mutably_used_vars.contains(&canonical_id)
177+
{
178+
self.fn_def_ids_to_maybe_unused_mut.entry(fn_def_id).or_insert(vec![]).push(input);
179+
}
180+
}
181+
}
182+
183+
fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
184+
cx.tcx.hir().visit_all_item_likes_in_crate(&mut FnNeedsMutVisitor {
185+
cx,
186+
used_fn_def_ids: &mut self.used_fn_def_ids,
187+
});
188+
189+
for (fn_def_id, unused) in self
190+
.fn_def_ids_to_maybe_unused_mut
191+
.iter()
192+
.filter(|(def_id, _)| !self.used_fn_def_ids.contains(def_id))
193+
{
194+
let show_semver_warning =
195+
self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(*fn_def_id);
196+
197+
for input in unused {
198+
// If the argument is never used mutably, we emit the warning.
199+
let sp = input.span;
200+
if let rustc_hir::TyKind::Ref(_, inner_ty) = input.kind {
201+
span_lint_hir_and_then(
180202
cx,
181203
NEEDLESS_PASS_BY_REF_MUT,
204+
cx.tcx.hir().local_def_id_to_hir_id(*fn_def_id),
182205
sp,
183206
"this argument is a mutable reference, but not used mutably",
184207
|diag| {
185208
diag.span_suggestion(
186209
sp,
187210
"consider changing to".to_string(),
188-
format!(
189-
"&{}",
190-
snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),
191-
),
211+
format!("&{}", snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),),
192212
Applicability::Unspecified,
193213
);
194214
if show_semver_warning {
@@ -316,3 +336,49 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
316336
self.prev_bind = Some(id);
317337
}
318338
}
339+
340+
/// A final pass to check for paths referencing this function that require the argument to be
341+
/// `&mut`, basically if the function is ever used as a `fn`-like argument.
342+
struct FnNeedsMutVisitor<'a, 'tcx> {
343+
cx: &'a LateContext<'tcx>,
344+
used_fn_def_ids: &'a mut FxHashSet<LocalDefId>,
345+
}
346+
347+
impl<'tcx> Visitor<'tcx> for FnNeedsMutVisitor<'_, 'tcx> {
348+
type NestedFilter = OnlyBodies;
349+
350+
fn nested_visit_map(&mut self) -> Self::Map {
351+
self.cx.tcx.hir()
352+
}
353+
354+
fn visit_qpath(&mut self, qpath: &'tcx QPath<'tcx>, hir_id: HirId, _: Span) {
355+
walk_qpath(self, qpath, hir_id);
356+
357+
let Self { cx, used_fn_def_ids } = self;
358+
359+
if let Node::Expr(expr) = cx.tcx.hir().get(hir_id)
360+
&& let Some(parent) = get_parent_node(cx.tcx, expr.hir_id)
361+
&& let ty::FnDef(def_id, _) = cx.tcx.typeck(cx.tcx.hir().enclosing_body_owner(hir_id)).expr_ty(expr).kind()
362+
&& let Some(def_id) = def_id.as_local()
363+
{
364+
if let Some(e) = match parent {
365+
// #11182
366+
Node::Expr(e) => Some(e),
367+
// #11199
368+
Node::ExprField(ExprField { expr, .. }) => Some(*expr),
369+
_ => None,
370+
}
371+
&& let ExprKind::Call(call, _) = e.kind
372+
&& call.hir_id == expr.hir_id
373+
{
374+
return;
375+
}
376+
377+
// We don't need to check each argument individually as you cannot coerce a function
378+
// taking `&mut` -> `&`, for some reason, so if we've gotten this far we know it's
379+
// passed as a `fn`-like argument (or is unified) and should ignore every "unused"
380+
// argument entirely
381+
used_fn_def_ids.insert(def_id);
382+
}
383+
}
384+
}

‎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 true { ty_unify_1 } else { ty_unify_2 };
169203
}

0 commit comments

Comments
 (0)