Skip to content

Commit f48c0a8

Browse files
JarchoJarcho
committed
Remove final reference on fields and method calls in needless_borrow 
1 parent 1d09d3d commit f48c0a8

25 files changed

+346
-290
lines changed

‎clippy_lints/src/dereference.rs

Lines changed: 76 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,6 @@ pub struct Dereferencing {
130130
struct StateData {
131131
/// Span of the top level expression
132132
span: Span,
133-
/// The required mutability
134-
target_mut: Mutability,
135133
}
136134

137135
enum State {
@@ -140,9 +138,11 @@ enum State {
140138
// The number of calls in a sequence which changed the referenced type
141139
ty_changed_count: usize,
142140
is_final_ufcs: bool,
141+
/// The required mutability
142+
target_mut: Mutability,
143143
},
144144
DerefedBorrow {
145-
count: u32,
145+
count: usize,
146146
},
147147
}
148148

@@ -213,77 +213,76 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
213213
1
214214
},
215215
is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
216-
},
217-
StateData {
218-
span: expr.span,
219216
target_mut,
220217
},
218+
StateData { span: expr.span },
221219
));
222220
},
223221
RefOp::AddrOf => {
224222
// Find the number of times the borrow is auto-derefed.
225223
let mut iter = find_adjustments(cx.tcx, typeck, expr).iter();
226-
if let Some((i, adjust)) = iter.by_ref().enumerate().find_map(|(i, adjust)| {
227-
if !matches!(adjust.kind, Adjust::Deref(_)) {
228-
Some((i, Some(adjust)))
229-
} else if !adjust.target.is_ref() {
230-
// Include the current deref.
231-
Some((i + 1, None))
232-
} else {
233-
None
234-
}
235-
}) {
236-
if i > 1 {
237-
// If the next adjustment is a mutable borrow, then check to see if the compiler will
238-
// insert a re-borrow here. If not, leave an extra borrow here to avoid attempting to
239-
// move the a mutable reference.
240-
let (i, target_mut) = if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) =
241-
adjust.or_else(|| iter.next()).map(|a| &a.kind)
242-
{
243-
if matches!(mutability, AutoBorrowMutability::Mut { .. })
244-
&& !is_auto_reborrow_position(parent, expr.hir_id)
245-
{
246-
(i - 1, Mutability::Mut)
224+
let mut deref_count = 0usize;
225+
let next_adjust = loop {
226+
match iter.next() {
227+
Some(adjust) => {
228+
if !matches!(adjust.kind, Adjust::Deref(_)) {
229+
break Some(adjust);
230+
} else if !adjust.target.is_ref() {
231+
deref_count += 1;
232+
break iter.next();
247233
} else {
248-
(i, mutability.into())
234+
deref_count += 1;
249235
}
250-
} else {
251-
(
252-
i,
253-
iter.find_map(|adjust| match adjust.kind {
254-
Adjust::Borrow(AutoBorrow::Ref(_, m)) => Some(m.into()),
255-
_ => None,
256-
})
257-
// This default should never happen. Auto-deref always reborrows.
258-
.unwrap_or(Mutability::Not),
259-
)
260-
};
261-
262-
if i > 1 {
263-
self.state = Some((
264-
// Subtract one for the current borrow expression, and one to cover the last
265-
// reference which can't be removed (it's either reborrowed, or needed for
266-
// auto-deref to happen).
267-
State::DerefedBorrow {
268-
count:
269-
// Truncation here would require more than a `u32::MAX` level reference. The compiler
270-
// does not support this.
271-
#[allow(clippy::cast_possible_truncation)]
272-
{ i as u32 - 2 }
273-
},
274-
StateData {
275-
span: expr.span,
276-
target_mut,
277-
},
278-
));
279-
}
236+
},
237+
None => break None,
238+
};
239+
};
240+
241+
let required_derefs: usize = if is_member_access(parent, expr.hir_id) {
242+
// Neither field accesses nor method calls require a borrow to trigger auto-deref.
243+
1
244+
} else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) =
245+
next_adjust.map(|a| &a.kind)
246+
{
247+
// If the next adjustment is a mutable borrow, then check to see if the compiler will
248+
// insert a re-borrow here. If not, leave an extra borrow here to avoid attempting to
249+
// move the mutable reference.
250+
if matches!(mutability, AutoBorrowMutability::Mut { .. })
251+
&& !is_auto_reborrow_position(parent, expr.hir_id)
252+
{
253+
deref_count = deref_count.saturating_sub(1);
280254
}
255+
2
256+
} else {
257+
2
258+
};
259+
260+
if deref_count >= required_derefs {
261+
self.state = Some((
262+
// Subtract one for the current borrow expression, and, optionally, one to cover the
263+
// last reference which can't be removed (it's either reborrowed, or needed for
264+
// auto-deref to happen).
265+
State::DerefedBorrow {
266+
count: deref_count - required_derefs,
267+
},
268+
StateData { span: expr.span },
269+
));
281270
}
282271
},
283272
_ => (),
284273
}
285274
},
286-
(Some((State::DerefMethod { ty_changed_count, .. }, data)), RefOp::Method(_)) => {
275+
(
276+
Some((
277+
State::DerefMethod {
278+
target_mut,
279+
ty_changed_count,
280+
..
281+
},
282+
data,
283+
)),
284+
RefOp::Method(_),
285+
) => {
287286
self.state = Some((
288287
State::DerefMethod {
289288
ty_changed_count: if deref_method_same_type(typeck.expr_ty(expr), typeck.expr_ty(sub_expr)) {
@@ -292,6 +291,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
292291
ty_changed_count + 1
293292
},
294293
is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
294+
target_mut,
295295
},
296296
data,
297297
));
@@ -487,6 +487,19 @@ fn is_auto_reborrow_position(parent: Option<Node<'_>>, child_id: HirId) -> bool
487487
}
488488
}
489489

490+
/// Checks if the given expression is a method of field access.
491+
fn is_member_access(parent: Option<Node<'_>>, child_id: HirId) -> bool {
492+
if let Some(Node::Expr(parent)) = parent {
493+
match parent.kind {
494+
ExprKind::MethodCall(_, _, [self_arg, ..], _) => self_arg.hir_id == child_id,
495+
ExprKind::Field(..) => true,
496+
_ => false,
497+
}
498+
} else {
499+
false
500+
}
501+
}
502+
490503
/// Adjustments are sometimes made in the parent block rather than the expression itself.
491504
fn find_adjustments(
492505
tcx: TyCtxt<'tcx>,
@@ -535,6 +548,7 @@ fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: Stat
535548
State::DerefMethod {
536549
ty_changed_count,
537550
is_final_ufcs,
551+
target_mut,
538552
} => {
539553
let mut app = Applicability::MachineApplicable;
540554
let (expr_str, expr_is_macro_call) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app);
@@ -549,12 +563,12 @@ fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: Stat
549563
};
550564
let addr_of_str = if ty_changed_count < ref_count {
551565
// Check if a reborrow from &mut T -> &T is required.
552-
if data.target_mut == Mutability::Not && matches!(ty.kind(), ty::Ref(_, _, Mutability::Mut)) {
566+
if target_mut == Mutability::Not && matches!(ty.kind(), ty::Ref(_, _, Mutability::Mut)) {
553567
"&*"
554568
} else {
555569
""
556570
}
557-
} else if data.target_mut == Mutability::Mut {
571+
} else if target_mut == Mutability::Mut {
558572
"&mut "
559573
} else {
560574
"&"
@@ -570,7 +584,7 @@ fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: Stat
570584
cx,
571585
EXPLICIT_DEREF_METHODS,
572586
data.span,
573-
match data.target_mut {
587+
match target_mut {
574588
Mutability::Not => "explicit `deref` method call",
575589
Mutability::Mut => "explicit `deref_mut` method call",
576590
},

‎tests/ui/bytecount.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
#![allow(clippy::needless_borrow)]
2+
13
#[deny(clippy::naive_bytecount)]
24
fn main() {
35
let x = vec![0_u8; 16];

‎tests/ui/bytecount.stderr

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,23 @@
11
error: you appear to be counting bytes the naive way
2-
--> $DIR/bytecount.rs:5:13
2+
--> $DIR/bytecount.rs:7:13
33
|
44
LL | let _ = x.iter().filter(|&&a| a == 0).count(); // naive byte count
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the bytecount crate: `bytecount::count(x, 0)`
66
|
77
note: the lint level is defined here
8-
--> $DIR/bytecount.rs:1:8
8+
--> $DIR/bytecount.rs:3:8
99
|
1010
LL | #[deny(clippy::naive_bytecount)]
1111
| ^^^^^^^^^^^^^^^^^^^^^^^
1212

1313
error: you appear to be counting bytes the naive way
14-
--> $DIR/bytecount.rs:7:13
14+
--> $DIR/bytecount.rs:9:13
1515
|
1616
LL | let _ = (&x[..]).iter().filter(|&a| *a == 0).count(); // naive byte count
1717
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the bytecount crate: `bytecount::count((&x[..]), 0)`
1818

1919
error: you appear to be counting bytes the naive way
20-
--> $DIR/bytecount.rs:19:13
20+
--> $DIR/bytecount.rs:21:13
2121
|
2222
LL | let _ = x.iter().filter(|a| b + 1 == **a).count(); // naive byte count
2323
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the bytecount crate: `bytecount::count(x, b + 1)`

‎tests/ui/clone_on_copy.fixed

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77
clippy::no_effect,
88
clippy::unnecessary_operation,
99
clippy::vec_init_then_push,
10-
clippy::toplevel_ref_arg
10+
clippy::toplevel_ref_arg,
11+
clippy::needless_borrow
1112
)]
1213

1314
use std::cell::RefCell;

‎tests/ui/clone_on_copy.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77
clippy::no_effect,
88
clippy::unnecessary_operation,
99
clippy::vec_init_then_push,
10-
clippy::toplevel_ref_arg
10+
clippy::toplevel_ref_arg,
11+
clippy::needless_borrow
1112
)]
1213

1314
use std::cell::RefCell;

‎tests/ui/clone_on_copy.stderr

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,49 +1,49 @@
11
error: using `clone` on type `i32` which implements the `Copy` trait
2-
--> $DIR/clone_on_copy.rs:24:5
2+
--> $DIR/clone_on_copy.rs:25:5
33
|
44
LL | 42.clone();
55
| ^^^^^^^^^^ help: try removing the `clone` call: `42`
66
|
77
= note: `-D clippy::clone-on-copy` implied by `-D warnings`
88

99
error: using `clone` on type `i32` which implements the `Copy` trait
10-
--> $DIR/clone_on_copy.rs:28:5
10+
--> $DIR/clone_on_copy.rs:29:5
1111
|
1212
LL | (&42).clone();
1313
| ^^^^^^^^^^^^^ help: try dereferencing it: `*(&42)`
1414

1515
error: using `clone` on type `i32` which implements the `Copy` trait
16-
--> $DIR/clone_on_copy.rs:31:5
16+
--> $DIR/clone_on_copy.rs:32:5
1717
|
1818
LL | rc.borrow().clone();
1919
| ^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*rc.borrow()`
2020

2121
error: using `clone` on type `u32` which implements the `Copy` trait
22-
--> $DIR/clone_on_copy.rs:34:5
22+
--> $DIR/clone_on_copy.rs:35:5
2323
|
2424
LL | x.clone().rotate_left(1);
2525
| ^^^^^^^^^ help: try removing the `clone` call: `x`
2626

2727
error: using `clone` on type `i32` which implements the `Copy` trait
28-
--> $DIR/clone_on_copy.rs:48:5
28+
--> $DIR/clone_on_copy.rs:49:5
2929
|
3030
LL | m!(42).clone();
3131
| ^^^^^^^^^^^^^^ help: try removing the `clone` call: `m!(42)`
3232

3333
error: using `clone` on type `[u32; 2]` which implements the `Copy` trait
34-
--> $DIR/clone_on_copy.rs:58:5
34+
--> $DIR/clone_on_copy.rs:59:5
3535
|
3636
LL | x.clone()[0];
3737
| ^^^^^^^^^ help: try dereferencing it: `(*x)`
3838

3939
error: using `clone` on type `char` which implements the `Copy` trait
40-
--> $DIR/clone_on_copy.rs:68:14
40+
--> $DIR/clone_on_copy.rs:69:14
4141
|
4242
LL | is_ascii('z'.clone());
4343
| ^^^^^^^^^^^ help: try removing the `clone` call: `'z'`
4444

4545
error: using `clone` on type `i32` which implements the `Copy` trait
46-
--> $DIR/clone_on_copy.rs:72:14
46+
--> $DIR/clone_on_copy.rs:73:14
4747
|
4848
LL | vec.push(42.clone());
4949
| ^^^^^^^^^^ help: try removing the `clone` call: `42`

‎tests/ui/duration_subsec.fixed

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// run-rustfix
2-
#![allow(dead_code)]
2+
#![allow(dead_code, clippy::needless_borrow)]
33
#![warn(clippy::duration_subsec)]
44

55
use std::time::Duration;

‎tests/ui/duration_subsec.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// run-rustfix
2-
#![allow(dead_code)]
2+
#![allow(dead_code, clippy::needless_borrow)]
33
#![warn(clippy::duration_subsec)]
44

55
use std::time::Duration;

‎tests/ui/for_loop_fixable.fixed

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,12 @@ impl Unrelated {
2323
clippy::iter_next_loop,
2424
clippy::for_kv_map
2525
)]
26-
#[allow(clippy::linkedlist, clippy::unnecessary_mut_passed, clippy::similar_names)]
26+
#[allow(
27+
clippy::linkedlist,
28+
clippy::unnecessary_mut_passed,
29+
clippy::similar_names,
30+
clippy::needless_borrow
31+
)]
2732
#[allow(unused_variables)]
2833
fn main() {
2934
let mut vec = vec![1, 2, 3, 4];

‎tests/ui/for_loop_fixable.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,12 @@ impl Unrelated {
2323
clippy::iter_next_loop,
2424
clippy::for_kv_map
2525
)]
26-
#[allow(clippy::linkedlist, clippy::unnecessary_mut_passed, clippy::similar_names)]
26+
#[allow(
27+
clippy::linkedlist,
28+
clippy::unnecessary_mut_passed,
29+
clippy::similar_names,
30+
clippy::needless_borrow
31+
)]
2732
#[allow(unused_variables)]
2833
fn main() {
2934
let mut vec = vec![1, 2, 3, 4];

0 commit comments

Comments
 (0)