Skip to content

Commit 4a64948

Browse files
JarchoJarcho
committed
Suggest parens when needed in needless_borrow and cleanup
1 parent 4192bbb commit 4a64948

File tree

8 files changed

+121
-92
lines changed

8 files changed

+121
-92
lines changed

‎clippy_lints/src/dereference.rs

Lines changed: 65 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
22
use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
3+
use clippy_utils::sugg::has_enclosing_paren;
34
use clippy_utils::ty::peel_mid_ty_refs;
45
use clippy_utils::{get_parent_expr, get_parent_node, is_lint_allowed, path_to_local};
56
use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX};
@@ -143,6 +144,7 @@ enum State {
143144
},
144145
DerefedBorrow {
145146
count: usize,
147+
required_precedence: i8,
146148
},
147149
}
148150

@@ -238,32 +240,49 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
238240
};
239241
};
240242

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
243+
// Determine the required number of references before any can be removed. In all cases the
244+
// reference made by the current expression will be removed. After that there are four cases to
245+
// handle.
246+
//
247+
// 1. Auto-borrow will trigger in the current position, so no further references are required.
248+
// 2. Auto-deref ends at a reference, or the underlying type, so one extra needs to be left to
249+
// handle the automatically inserted re-borrow.
250+
// 3. Auto-deref hits a user-defined `Deref` impl, so at least one reference needs to exist to
251+
// start auto-deref.
252+
// 4. If the chain of non-user-defined derefs ends with a mutable re-borrow, and re-borrow
253+
// adjustments will not be inserted automatically, then leave one further reference to avoid
254+
// moving a mutable borrow.
255+
// e.g.
256+
// fn foo<T>(x: &mut Option<&mut T>, y: &mut T) {
257+
// let x = match x {
258+
// // Removing the borrow will cause `x` to be moved
259+
// Some(x) => &mut *x,
260+
// None => y
261+
// };
262+
// }
263+
let (required_refs, required_precedence) = if is_auto_borrow_position(parent, expr.hir_id) {
264+
(1, PREC_POSTFIX)
244265
} else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) =
245266
next_adjust.map(|a| &a.kind)
246267
{
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.
250268
if matches!(mutability, AutoBorrowMutability::Mut { .. })
251-
&& !is_auto_reborrow_position(parent, expr.hir_id)
269+
&& !is_auto_reborrow_position(parent)
252270
{
253-
deref_count = deref_count.saturating_sub(1);
271+
(3, 0)
272+
} else {
273+
(2, 0)
254274
}
255-
2
256275
} else {
257-
2
276+
(2, 0)
258277
};
259278

260-
if deref_count >= required_derefs {
279+
if deref_count >= required_refs {
261280
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).
265281
State::DerefedBorrow {
266-
count: deref_count - required_derefs,
282+
// One of the required refs is for the current borrow expression, the remaining ones
283+
// can't be removed without breaking the code. See earlier comment.
284+
count: deref_count - required_refs,
285+
required_precedence,
267286
},
268287
StateData { span: expr.span },
269288
));
@@ -296,8 +315,23 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
296315
data,
297316
));
298317
},
299-
(Some((State::DerefedBorrow { count }, data)), RefOp::AddrOf) if count != 0 => {
300-
self.state = Some((State::DerefedBorrow { count: count - 1 }, data));
318+
(
319+
Some((
320+
State::DerefedBorrow {
321+
count,
322+
required_precedence,
323+
},
324+
data,
325+
)),
326+
RefOp::AddrOf,
327+
) if count != 0 => {
328+
self.state = Some((
329+
State::DerefedBorrow {
330+
count: count - 1,
331+
required_precedence,
332+
},
333+
data,
334+
));
301335
},
302336

303337
(Some((state, data)), _) => report(cx, expr, state, data),
@@ -475,24 +509,24 @@ fn is_linted_explicit_deref_position(parent: Option<Node<'_>>, child_id: HirId,
475509

476510
/// Checks if the given expression is in a position which can be auto-reborrowed.
477511
/// Note: This is only correct assuming auto-deref is already occurring.
478-
fn is_auto_reborrow_position(parent: Option<Node<'_>>, child_id: HirId) -> bool {
512+
fn is_auto_reborrow_position(parent: Option<Node<'_>>) -> bool {
479513
match parent {
480514
Some(Node::Expr(parent)) => match parent.kind {
481-
ExprKind::MethodCall(..) => true,
482-
ExprKind::Call(callee, _) => callee.hir_id != child_id,
515+
ExprKind::MethodCall(..) | ExprKind::Call(..) => true,
483516
_ => false,
484517
},
485518
Some(Node::Local(_)) => true,
486519
_ => false,
487520
}
488521
}
489522

490-
/// Checks if the given expression is a method of field access.
491-
fn is_member_access(parent: Option<Node<'_>>, child_id: HirId) -> bool {
523+
/// Checks if the given expression is a position which can auto-borrow.
524+
fn is_auto_borrow_position(parent: Option<Node<'_>>, child_id: HirId) -> bool {
492525
if let Some(Node::Expr(parent)) = parent {
493526
match parent.kind {
494527
ExprKind::MethodCall(_, _, [self_arg, ..], _) => self_arg.hir_id == child_id,
495528
ExprKind::Field(..) => true,
529+
ExprKind::Call(f, _) => f.hir_id == child_id,
496530
_ => false,
497531
}
498532
} else {
@@ -593,19 +627,22 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, data: StateData)
593627
app,
594628
);
595629
},
596-
State::DerefedBorrow { .. } => {
630+
State::DerefedBorrow {
631+
required_precedence, ..
632+
} => {
597633
let mut app = Applicability::MachineApplicable;
598634
let snip = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app).0;
599635
span_lint_and_sugg(
600636
cx,
601637
NEEDLESS_BORROW,
602638
data.span,
603-
&format!(
604-
"this expression borrows a reference (`{}`) that is immediately dereferenced by the compiler",
605-
cx.typeck_results().expr_ty(expr),
606-
),
639+
"this expression creates a reference which is immediately dereferenced by the compiler",
607640
"change this to",
608-
snip.into(),
641+
if required_precedence > expr.precedence().order() && !has_enclosing_paren(&snip) {
642+
format!("({})", snip)
643+
} else {
644+
snip.into()
645+
},
609646
app,
610647
);
611648
},

‎clippy_utils/src/sugg.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,7 @@ fn binop_to_string(op: AssocOp, lhs: &str, rhs: &str) -> String {
388388
}
389389

390390
/// Return `true` if `sugg` is enclosed in parenthesis.
391-
fn has_enclosing_paren(sugg: impl AsRef<str>) -> bool {
391+
pub fn has_enclosing_paren(sugg: impl AsRef<str>) -> bool {
392392
let mut chars = sugg.as_ref().chars();
393393
if chars.next() == Some('(') {
394394
let mut depth = 1;

‎tests/ui/eta.fixed

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,10 @@
55
clippy::no_effect,
66
clippy::redundant_closure_call,
77
clippy::needless_pass_by_value,
8-
clippy::option_map_unit_fn
9-
)]
10-
#![warn(
11-
clippy::redundant_closure,
12-
clippy::redundant_closure_for_method_calls,
8+
clippy::option_map_unit_fn,
139
clippy::needless_borrow
1410
)]
11+
#![warn(clippy::redundant_closure, clippy::redundant_closure_for_method_calls)]
1512

1613
use std::path::{Path, PathBuf};
1714

@@ -34,7 +31,7 @@ fn main() {
3431
Some(1).map(closure_mac!()); // don't lint closure in macro expansion
3532
let _: Option<Vec<u8>> = true.then(std::vec::Vec::new); // special case vec!
3633
let d = Some(1u8).map(|a| foo(foo2(a))); //is adjusted?
37-
all(&[1, 2, 3], &2, below); //is adjusted
34+
all(&[1, 2, 3], &&2, below); //is adjusted
3835
unsafe {
3936
Some(1u8).map(|a| unsafe_fn(a)); // unsafe fn
4037
}

‎tests/ui/eta.rs

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,10 @@
55
clippy::no_effect,
66
clippy::redundant_closure_call,
77
clippy::needless_pass_by_value,
8-
clippy::option_map_unit_fn
9-
)]
10-
#![warn(
11-
clippy::redundant_closure,
12-
clippy::redundant_closure_for_method_calls,
8+
clippy::option_map_unit_fn,
139
clippy::needless_borrow
1410
)]
11+
#![warn(clippy::redundant_closure, clippy::redundant_closure_for_method_calls)]
1512

1613
use std::path::{Path, PathBuf};
1714

‎tests/ui/eta.stderr

Lines changed: 21 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,134 +1,126 @@
11
error: redundant closure
2-
--> $DIR/eta.rs:31:27
2+
--> $DIR/eta.rs:28:27
33
|
44
LL | let a = Some(1u8).map(|a| foo(a));
55
| ^^^^^^^^^^ help: replace the closure with the function itself: `foo`
66
|
77
= note: `-D clippy::redundant-closure` implied by `-D warnings`
88

99
error: redundant closure
10-
--> $DIR/eta.rs:35:40
10+
--> $DIR/eta.rs:32:40
1111
|
1212
LL | let _: Option<Vec<u8>> = true.then(|| vec![]); // special case vec!
1313
| ^^^^^^^^^ help: replace the closure with `Vec::new`: `std::vec::Vec::new`
1414

1515
error: redundant closure
16-
--> $DIR/eta.rs:36:35
16+
--> $DIR/eta.rs:33:35
1717
|
1818
LL | let d = Some(1u8).map(|a| foo((|b| foo2(b))(a))); //is adjusted?
1919
| ^^^^^^^^^^^^^ help: replace the closure with the function itself: `foo2`
2020

21-
error: this expression borrows a reference (`&u8`) that is immediately dereferenced by the compiler
22-
--> $DIR/eta.rs:37:21
23-
|
24-
LL | all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted
25-
| ^^^ help: change this to: `&2`
26-
|
27-
= note: `-D clippy::needless-borrow` implied by `-D warnings`
28-
2921
error: redundant closure
30-
--> $DIR/eta.rs:37:26
22+
--> $DIR/eta.rs:34:26
3123
|
3224
LL | all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted
3325
| ^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `below`
3426

3527
error: redundant closure
36-
--> $DIR/eta.rs:43:27
28+
--> $DIR/eta.rs:40:27
3729
|
3830
LL | let e = Some(1u8).map(|a| divergent(a));
3931
| ^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `divergent`
4032

4133
error: redundant closure
42-
--> $DIR/eta.rs:44:27
34+
--> $DIR/eta.rs:41:27
4335
|
4436
LL | let e = Some(1u8).map(|a| generic(a));
4537
| ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `generic`
4638

4739
error: redundant closure
48-
--> $DIR/eta.rs:90:51
40+
--> $DIR/eta.rs:87:51
4941
|
5042
LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo());
5143
| ^^^^^^^^^^^ help: replace the closure with the method itself: `TestStruct::foo`
5244
|
5345
= note: `-D clippy::redundant-closure-for-method-calls` implied by `-D warnings`
5446

5547
error: redundant closure
56-
--> $DIR/eta.rs:91:51
48+
--> $DIR/eta.rs:88:51
5749
|
5850
LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo());
5951
| ^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `TestTrait::trait_foo`
6052

6153
error: redundant closure
62-
--> $DIR/eta.rs:93:42
54+
--> $DIR/eta.rs:90:42
6355
|
6456
LL | let e = Some(&mut vec![1, 2, 3]).map(|v| v.clear());
6557
| ^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::vec::Vec::clear`
6658

6759
error: redundant closure
68-
--> $DIR/eta.rs:97:29
60+
--> $DIR/eta.rs:94:29
6961
|
7062
LL | let e = Some("str").map(|s| s.to_string());
7163
| ^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::string::ToString::to_string`
7264

7365
error: redundant closure
74-
--> $DIR/eta.rs:98:27
66+
--> $DIR/eta.rs:95:27
7567
|
7668
LL | let e = Some('a').map(|s| s.to_uppercase());
7769
| ^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `char::to_uppercase`
7870

7971
error: redundant closure
80-
--> $DIR/eta.rs:100:65
72+
--> $DIR/eta.rs:97:65
8173
|
8274
LL | let e: std::vec::Vec<char> = vec!['a', 'b', 'c'].iter().map(|c| c.to_ascii_uppercase()).collect();
8375
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `char::to_ascii_uppercase`
8476

8577
error: redundant closure
86-
--> $DIR/eta.rs:163:22
78+
--> $DIR/eta.rs:160:22
8779
|
8880
LL | requires_fn_once(|| x());
8981
| ^^^^^^ help: replace the closure with the function itself: `x`
9082

9183
error: redundant closure
92-
--> $DIR/eta.rs:170:27
84+
--> $DIR/eta.rs:167:27
9385
|
9486
LL | let a = Some(1u8).map(|a| foo_ptr(a));
9587
| ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `foo_ptr`
9688

9789
error: redundant closure
98-
--> $DIR/eta.rs:175:27
90+
--> $DIR/eta.rs:172:27
9991
|
10092
LL | let a = Some(1u8).map(|a| closure(a));
10193
| ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `closure`
10294

10395
error: redundant closure
104-
--> $DIR/eta.rs:207:28
96+
--> $DIR/eta.rs:204:28
10597
|
10698
LL | x.into_iter().for_each(|x| add_to_res(x));
10799
| ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut add_to_res`
108100

109101
error: redundant closure
110-
--> $DIR/eta.rs:208:28
102+
--> $DIR/eta.rs:205:28
111103
|
112104
LL | y.into_iter().for_each(|x| add_to_res(x));
113105
| ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut add_to_res`
114106

115107
error: redundant closure
116-
--> $DIR/eta.rs:209:28
108+
--> $DIR/eta.rs:206:28
117109
|
118110
LL | z.into_iter().for_each(|x| add_to_res(x));
119111
| ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `add_to_res`
120112

121113
error: redundant closure
122-
--> $DIR/eta.rs:216:21
114+
--> $DIR/eta.rs:213:21
123115
|
124116
LL | Some(1).map(|n| closure(n));
125117
| ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut closure`
126118

127119
error: redundant closure
128-
--> $DIR/eta.rs:235:21
120+
--> $DIR/eta.rs:232:21
129121
|
130122
LL | map_str_to_path(|s| s.as_ref());
131123
| ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::convert::AsRef::as_ref`
132124

133-
error: aborting due to 21 previous errors
125+
error: aborting due to 20 previous errors
134126

‎tests/ui/needless_borrow.fixed

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ fn main() {
7070

7171
let x = (1, 2);
7272
let _ = x.0;
73-
// let x = &x as *const (i32, i32);
74-
// let _ = unsafe { (&*x).0 };
73+
let x = &x as *const (i32, i32);
74+
let _ = unsafe { (*x).0 };
7575
}
7676

7777
#[allow(clippy::needless_borrowed_reference)]

‎tests/ui/needless_borrow.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ fn main() {
7070

7171
let x = (1, 2);
7272
let _ = (&x).0;
73-
// let x = &x as *const (i32, i32);
74-
// let _ = unsafe { (&*x).0 };
73+
let x = &x as *const (i32, i32);
74+
let _ = unsafe { (&*x).0 };
7575
}
7676

7777
#[allow(clippy::needless_borrowed_reference)]

0 commit comments

Comments
 (0)