Skip to content

Commit 788a8bc

Browse files
borsbors
committed
Auto merge of rust-lang#8217 - Jarcho:needless_borrow_8191, r=camsteffen
Fix `needless_borrow` causing mutable borrows to be moved fixes rust-lang#8191 changelog: Fix `needless_borrow` causing mutable borrows to be moved changelog: Rename `ref_in_deref` to `needless_borrow` changelog: Suggest removing the borrow on method call receivers in `needless_borrow`
2 parents 1e546c5 + c615140 commit 788a8bc

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

52 files changed

+601
-519
lines changed

‎CHANGELOG.md

+1-2
Original file line numberDiff line numberDiff line change
@@ -981,7 +981,7 @@ Released 2021-03-25
981981
[#6532](https://github.com/rust-lang/rust-clippy/pull/6532)
982982
* [`single_match`] Suggest `if` over `if let` when possible
983983
[#6574](https://github.com/rust-lang/rust-clippy/pull/6574)
984-
* [`ref_in_deref`] Use parentheses correctly in suggestion
984+
* `ref_in_deref` Use parentheses correctly in suggestion
985985
[#6609](https://github.com/rust-lang/rust-clippy/pull/6609)
986986
* [`stable_sort_primitive`] Clarify error message
987987
[#6611](https://github.com/rust-lang/rust-clippy/pull/6611)
@@ -3227,7 +3227,6 @@ Released 2018-09-13
32273227
[`redundant_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_slicing
32283228
[`redundant_static_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes
32293229
[`ref_binding_to_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_binding_to_reference
3230-
[`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref
32313230
[`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref
32323231
[`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro
32333232
[`repeat_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_once

‎clippy_lints/src/dereference.rs

+144-55
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};
@@ -10,11 +11,10 @@ use rustc_hir::{
1011
Pat, PatKind, UnOp,
1112
};
1213
use rustc_lint::{LateContext, LateLintPass};
13-
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
14+
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
1415
use rustc_middle::ty::{self, Ty, TyCtxt, TyS, TypeckResults};
1516
use rustc_session::{declare_tool_lint, impl_lint_pass};
1617
use rustc_span::{symbol::sym, Span};
17-
use std::iter;
1818

1919
declare_clippy_lint! {
2020
/// ### What it does
@@ -131,8 +131,6 @@ pub struct Dereferencing {
131131
struct StateData {
132132
/// Span of the top level expression
133133
span: Span,
134-
/// The required mutability
135-
target_mut: Mutability,
136134
}
137135

138136
enum State {
@@ -141,9 +139,13 @@ enum State {
141139
// The number of calls in a sequence which changed the referenced type
142140
ty_changed_count: usize,
143141
is_final_ufcs: bool,
142+
/// The required mutability
143+
target_mut: Mutability,
144144
},
145145
DerefedBorrow {
146-
count: u32,
146+
count: usize,
147+
required_precedence: i8,
148+
msg: &'static str,
147149
},
148150
}
149151

@@ -214,59 +216,98 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
214216
1
215217
},
216218
is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
217-
},
218-
StateData {
219-
span: expr.span,
220219
target_mut,
221220
},
221+
StateData { span: expr.span },
222222
));
223223
},
224224
RefOp::AddrOf => {
225225
// Find the number of times the borrow is auto-derefed.
226226
let mut iter = find_adjustments(cx.tcx, typeck, expr).iter();
227-
if let Some((i, adjust)) = iter.by_ref().enumerate().find_map(|(i, adjust)| {
228-
if !matches!(adjust.kind, Adjust::Deref(_)) {
229-
Some((i, adjust))
230-
} else if !adjust.target.is_ref() {
231-
// Add one to the number of references found.
232-
Some((i + 1, adjust))
227+
let mut deref_count = 0usize;
228+
let next_adjust = loop {
229+
match iter.next() {
230+
Some(adjust) => {
231+
if !matches!(adjust.kind, Adjust::Deref(_)) {
232+
break Some(adjust);
233+
} else if !adjust.target.is_ref() {
234+
deref_count += 1;
235+
break iter.next();
236+
}
237+
deref_count += 1;
238+
},
239+
None => break None,
240+
};
241+
};
242+
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 deref_msg =
264+
"this expression creates a reference which is immediately dereferenced by the compiler";
265+
let borrow_msg = "this expression borrows a value the compiler would automatically borrow";
266+
267+
let (required_refs, required_precedence, msg) = if is_auto_borrow_position(parent, expr.hir_id)
268+
{
269+
(1, PREC_POSTFIX, if deref_count == 1 { borrow_msg } else { deref_msg })
270+
} else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) =
271+
next_adjust.map(|a| &a.kind)
272+
{
273+
if matches!(mutability, AutoBorrowMutability::Mut { .. })
274+
&& !is_auto_reborrow_position(parent)
275+
{
276+
(3, 0, deref_msg)
233277
} else {
234-
None
235-
}
236-
}) {
237-
// Found two consecutive derefs. At least one can be removed.
238-
if i > 1 {
239-
let target_mut = iter::once(adjust)
240-
.chain(iter)
241-
.find_map(|adjust| match adjust.kind {
242-
Adjust::Borrow(AutoBorrow::Ref(_, m)) => Some(m.into()),
243-
_ => None,
244-
})
245-
// This default should never happen. Auto-deref always reborrows.
246-
.unwrap_or(Mutability::Not);
247-
self.state = Some((
248-
// Subtract one for the current borrow expression, and one to cover the last
249-
// reference which can't be removed (it's either reborrowed, or needed for
250-
// auto-deref to happen).
251-
State::DerefedBorrow {
252-
count:
253-
// Truncation here would require more than a `u32::MAX` level reference. The compiler
254-
// does not support this.
255-
#[allow(clippy::cast_possible_truncation)]
256-
{ i as u32 - 2 }
257-
},
258-
StateData {
259-
span: expr.span,
260-
target_mut,
261-
},
262-
));
278+
(2, 0, deref_msg)
263279
}
280+
} else {
281+
(2, 0, deref_msg)
282+
};
283+
284+
if deref_count >= required_refs {
285+
self.state = Some((
286+
State::DerefedBorrow {
287+
// One of the required refs is for the current borrow expression, the remaining ones
288+
// can't be removed without breaking the code. See earlier comment.
289+
count: deref_count - required_refs,
290+
required_precedence,
291+
msg,
292+
},
293+
StateData { span: expr.span },
294+
));
264295
}
265296
},
266297
_ => (),
267298
}
268299
},
269-
(Some((State::DerefMethod { ty_changed_count, .. }, data)), RefOp::Method(_)) => {
300+
(
301+
Some((
302+
State::DerefMethod {
303+
target_mut,
304+
ty_changed_count,
305+
..
306+
},
307+
data,
308+
)),
309+
RefOp::Method(_),
310+
) => {
270311
self.state = Some((
271312
State::DerefMethod {
272313
ty_changed_count: if deref_method_same_type(typeck.expr_ty(expr), typeck.expr_ty(sub_expr)) {
@@ -275,12 +316,30 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
275316
ty_changed_count + 1
276317
},
277318
is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
319+
target_mut,
278320
},
279321
data,
280322
));
281323
},
282-
(Some((State::DerefedBorrow { count }, data)), RefOp::AddrOf) if count != 0 => {
283-
self.state = Some((State::DerefedBorrow { count: count - 1 }, data));
324+
(
325+
Some((
326+
State::DerefedBorrow {
327+
count,
328+
required_precedence,
329+
msg,
330+
},
331+
data,
332+
)),
333+
RefOp::AddrOf,
334+
) if count != 0 => {
335+
self.state = Some((
336+
State::DerefedBorrow {
337+
count: count - 1,
338+
required_precedence,
339+
msg,
340+
},
341+
data,
342+
));
284343
},
285344

286345
(Some((state, data)), _) => report(cx, expr, state, data),
@@ -456,6 +515,30 @@ fn is_linted_explicit_deref_position(parent: Option<Node<'_>>, child_id: HirId,
456515
}
457516
}
458517

518+
/// Checks if the given expression is in a position which can be auto-reborrowed.
519+
/// Note: This is only correct assuming auto-deref is already occurring.
520+
fn is_auto_reborrow_position(parent: Option<Node<'_>>) -> bool {
521+
match parent {
522+
Some(Node::Expr(parent)) => matches!(parent.kind, ExprKind::MethodCall(..) | ExprKind::Call(..)),
523+
Some(Node::Local(_)) => true,
524+
_ => false,
525+
}
526+
}
527+
528+
/// Checks if the given expression is a position which can auto-borrow.
529+
fn is_auto_borrow_position(parent: Option<Node<'_>>, child_id: HirId) -> bool {
530+
if let Some(Node::Expr(parent)) = parent {
531+
match parent.kind {
532+
ExprKind::MethodCall(_, _, [self_arg, ..], _) => self_arg.hir_id == child_id,
533+
ExprKind::Field(..) => true,
534+
ExprKind::Call(f, _) => f.hir_id == child_id,
535+
_ => false,
536+
}
537+
} else {
538+
false
539+
}
540+
}
541+
459542
/// Adjustments are sometimes made in the parent block rather than the expression itself.
460543
fn find_adjustments<'tcx>(
461544
tcx: TyCtxt<'tcx>,
@@ -504,6 +587,7 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, data: StateData)
504587
State::DerefMethod {
505588
ty_changed_count,
506589
is_final_ufcs,
590+
target_mut,
507591
} => {
508592
let mut app = Applicability::MachineApplicable;
509593
let (expr_str, expr_is_macro_call) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app);
@@ -518,12 +602,12 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, data: StateData)
518602
};
519603
let addr_of_str = if ty_changed_count < ref_count {
520604
// Check if a reborrow from &mut T -> &T is required.
521-
if data.target_mut == Mutability::Not && matches!(ty.kind(), ty::Ref(_, _, Mutability::Mut)) {
605+
if target_mut == Mutability::Not && matches!(ty.kind(), ty::Ref(_, _, Mutability::Mut)) {
522606
"&*"
523607
} else {
524608
""
525609
}
526-
} else if data.target_mut == Mutability::Mut {
610+
} else if target_mut == Mutability::Mut {
527611
"&mut "
528612
} else {
529613
"&"
@@ -539,7 +623,7 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, data: StateData)
539623
cx,
540624
EXPLICIT_DEREF_METHODS,
541625
data.span,
542-
match data.target_mut {
626+
match target_mut {
543627
Mutability::Not => "explicit `deref` method call",
544628
Mutability::Mut => "explicit `deref_mut` method call",
545629
},
@@ -548,19 +632,24 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, data: StateData)
548632
app,
549633
);
550634
},
551-
State::DerefedBorrow { .. } => {
635+
State::DerefedBorrow {
636+
required_precedence,
637+
msg,
638+
..
639+
} => {
552640
let mut app = Applicability::MachineApplicable;
553641
let snip = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app).0;
554642
span_lint_and_sugg(
555643
cx,
556644
NEEDLESS_BORROW,
557645
data.span,
558-
&format!(
559-
"this expression borrows a reference (`{}`) that is immediately dereferenced by the compiler",
560-
cx.typeck_results().expr_ty(expr),
561-
),
646+
msg,
562647
"change this to",
563-
snip.into(),
648+
if required_precedence > expr.precedence().order() && !has_enclosing_paren(&snip) {
649+
format!("({})", snip)
650+
} else {
651+
snip.into()
652+
},
564653
app,
565654
);
566655
},

‎clippy_lints/src/implicit_hasher.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitHasher {
179179
)
180180
.and_then(|snip| {
181181
let i = snip.find("fn")?;
182-
Some(item.span.lo() + BytePos((i + (&snip[i..]).find('(')?) as u32))
182+
Some(item.span.lo() + BytePos((i + snip[i..].find('(')?) as u32))
183183
})
184184
.expect("failed to create span for type parameters");
185185
Span::new(pos, pos, item.span.ctxt(), item.span.parent())

‎clippy_lints/src/lib.register_all.rs

-1
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
247247
LintId::of(redundant_slicing::REDUNDANT_SLICING),
248248
LintId::of(redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES),
249249
LintId::of(reference::DEREF_ADDROF),
250-
LintId::of(reference::REF_IN_DEREF),
251250
LintId::of(regex::INVALID_REGEX),
252251
LintId::of(repeat_once::REPEAT_ONCE),
253252
LintId::of(returns::LET_AND_RETURN),

‎clippy_lints/src/lib.register_complexity.rs

-1
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
7171
LintId::of(redundant_closure_call::REDUNDANT_CLOSURE_CALL),
7272
LintId::of(redundant_slicing::REDUNDANT_SLICING),
7373
LintId::of(reference::DEREF_ADDROF),
74-
LintId::of(reference::REF_IN_DEREF),
7574
LintId::of(repeat_once::REPEAT_ONCE),
7675
LintId::of(strings::STRING_FROM_UTF8_AS_BYTES),
7776
LintId::of(strlen_on_c_strings::STRLEN_ON_C_STRINGS),

‎clippy_lints/src/lib.register_lints.rs

-1
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,6 @@ store.register_lints(&[
423423
redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES,
424424
ref_option_ref::REF_OPTION_REF,
425425
reference::DEREF_ADDROF,
426-
reference::REF_IN_DEREF,
427426
regex::INVALID_REGEX,
428427
regex::TRIVIAL_REGEX,
429428
repeat_once::REPEAT_ONCE,

‎clippy_lints/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -703,7 +703,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
703703
store.register_late_pass(|| Box::new(mut_key::MutableKeyType));
704704
store.register_late_pass(|| Box::new(modulo_arithmetic::ModuloArithmetic));
705705
store.register_early_pass(|| Box::new(reference::DerefAddrOf));
706-
store.register_early_pass(|| Box::new(reference::RefInDeref));
707706
store.register_early_pass(|| Box::new(double_parens::DoubleParens));
708707
store.register_late_pass(|| Box::new(to_string_in_display::ToStringInDisplay::new()));
709708
store.register_early_pass(|| Box::new(unsafe_removed_from_name::UnsafeNameRemoval));
@@ -935,6 +934,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
935934
ls.register_renamed("clippy::if_let_some_result", "clippy::match_result_ok");
936935
ls.register_renamed("clippy::disallowed_type", "clippy::disallowed_types");
937936
ls.register_renamed("clippy::disallowed_method", "clippy::disallowed_methods");
937+
ls.register_renamed("clippy::ref_in_deref", "clippy::needless_borrow");
938938

939939
// uplifted lints
940940
ls.register_renamed("clippy::invalid_ref", "invalid_value");

‎clippy_lints/src/octal_escapes.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ fn check_lit(cx: &EarlyContext<'_>, lit: &Lit, span: Span, is_string: bool) {
101101
// construct a replacement escape
102102
// the maximum value is \077, or \x3f, so u8 is sufficient here
103103
if let Ok(n) = u8::from_str_radix(&contents[from + 1..to], 8) {
104-
write!(&mut suggest_1, "\\x{:02x}", n).unwrap();
104+
write!(suggest_1, "\\x{:02x}", n).unwrap();
105105
}
106106

107107
// append the null byte as \x00 and the following digits literally

0 commit comments

Comments
 (0)