Skip to content

Commit

Permalink
Limit needless_borrow to only work if the pattern is not from a macro
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarcho
Jarcho committed May 19, 2021
1 parent 8f84628 commit 5ee2fc7
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 77 deletions.
100 changes: 37 additions & 63 deletions clippy_lints/src/needless_borrow.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,18 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::{snippet_opt, snippet_with_applicability, snippet_with_context};
use clippy_utils::{get_parent_expr, path_to_local};
use clippy_utils::{get_parent_expr, in_macro, path_to_local};
use if_chain::if_chain;
use rustc_ast::util::parser::PREC_POSTFIX;
use rustc_data_structures::fx::FxIndexMap;
use rustc_errors::Applicability;
use rustc_hir::{
BindingAnnotation, Block, Body, BodyId, BorrowKind, Expr, ExprKind, HirId, ImplItem, Item, Mutability, Node, Pat,
PatKind, TraitItem, UnOp,
};
use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
use rustc_hir::{BindingAnnotation, Body, BodyId, BorrowKind, Expr, ExprKind, HirId, Mutability, Pat, PatKind, UnOp};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty;
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::{Span, SyntaxContext};
use rustc_span::Span;

declare_clippy_lint! {
/// **What it does:** Checks for address of operations (`&`) that are going to
Expand Down Expand Up @@ -80,12 +77,10 @@ pub struct NeedlessBorrow {
}

struct RefPat {
/// The lint to use for this suggestion. Either `needless_borrow` or `ref_binding_to_reference`.
lint: &'static Lint,
/// Whether every usage of the binding is dereferenced.
always_deref: bool,
/// The spans of all the ref bindings for this local.
spans: Vec<Span>,
/// The context replacements are to be made in.
ctxt: SyntaxContext,
/// The applicability of this suggestion.
app: Applicability,
/// All the replacements which need to be made.
Expand Down Expand Up @@ -144,17 +139,17 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
if let Some(opt_prev_pat) = self.ref_locals.get_mut(&id) {
// This binding id has been seen before. Add this pattern to the list of changes.
if let Some(prev_pat) = opt_prev_pat {
if prev_pat.ctxt == pat.span.ctxt() {
if in_macro(pat.span) {
// Doesn't match the context of the previous pattern. Can't lint here.
*opt_prev_pat = None;
} else {
prev_pat.spans.push(pat.span);
prev_pat.replacements.push((
pat.span,
snippet_with_context(cx, name.span, prev_pat.ctxt, "..", &mut prev_pat.app)
snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut prev_pat.app)
.0
.into(),
));
} else {
// Doesn't match the context of the previous pattern. Can't lint here.
*opt_prev_pat = None;
}
}
return;
Expand All @@ -165,46 +160,20 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
if let ty::Ref(_, tam, _) = *cx.typeck_results().pat_ty(pat).kind();
// only lint immutable refs, because borrowed `&mut T` cannot be moved out
if let ty::Ref(_, _, Mutability::Not) = *tam.kind();
if !in_macro(pat.span);
then {
let map = cx.tcx.hir();
let mut iter = map.parent_iter(pat.hir_id);
let expr_ctxt = loop {
// Find the context shared by the pattern, and it's scope use site.
match iter.next() {
None => {
// This shouldn't happen, but mark this as unfixable if it does.
self.ref_locals.insert(id, None);
return;
}
Some((_, Node::ImplItem(ImplItem { span, .. })
| Node::TraitItem(TraitItem { span, .. })
| Node::Item(Item { span, .. })
| Node::Block( Block { span, ..})
| Node::Expr( Expr { span, .. }))) => break span.ctxt(),
_ => (),
}
};

if pat.span.ctxt() == expr_ctxt {
let mut app = Applicability::MachineApplicable;
let snip = snippet_with_context(cx, name.span, expr_ctxt, "..", &mut app).0;
self.current_body = self.current_body.or(cx.enclosing_body);
self.ref_locals.insert(
id,
Some(RefPat {
lint: NEEDLESS_BORROW,
spans: vec![pat.span],
ctxt: expr_ctxt,
app,
replacements: vec![(pat.span, snip.into())],
}),
);
} else {
// The context of the pattern is different than the context using the binding.
// Changing the pattern might affect other code which needs the ref binding.
// Mark the local as having been seen, but not fixable.
self.ref_locals.insert(id, None);
}
let mut app = Applicability::MachineApplicable;
let snip = snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut app).0;
self.current_body = self.current_body.or(cx.enclosing_body);
self.ref_locals.insert(
id,
Some(RefPat {
always_deref: true,
spans: vec![pat.span],
app,
replacements: vec![(pat.span, snip.into())],
}),
);
}
}
}
Expand All @@ -217,7 +186,11 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
let app = pat.app;
span_lint_and_then(
cx,
pat.lint,
if pat.always_deref {
NEEDLESS_BORROW
} else {
REF_BINDING_TO_REFERENCE
},
pat.spans,
"this pattern creates a reference to a reference",
|diag| {
Expand Down Expand Up @@ -258,24 +231,25 @@ impl NeedlessBorrow {
span,
kind: ExprKind::Unary(UnOp::Deref, _),
..
}) if span.ctxt() == pat.ctxt => {
}) if !in_macro(span) => {
// Remove explicit deref.
let snip = snippet_with_context(cx, e.span, pat.ctxt, "..", &mut pat.app).0;
let snip = snippet_with_context(cx, e.span, span.ctxt(), "..", &mut pat.app).0;
pat.replacements.push((span, snip.into()));
},
Some(parent) if parent.span.ctxt() == pat.ctxt => {
Some(parent) if !in_macro(parent.span) => {
// Double reference might be needed at this point.
if parent.precedence().order() == PREC_POSTFIX {
// Parenthesis would be needed here, don't lint.
// Parentheses would be needed here, don't lint.
*outer_pat = None;
} else {
let snip = snippet_with_context(cx, e.span, pat.ctxt, "..", &mut pat.app).0;
pat.always_deref = false;
let snip = snippet_with_context(cx, e.span, parent.span.ctxt(), "..", &mut pat.app).0;
pat.replacements.push((e.span, format!("&{}", snip)));
}
},
_ if e.span.ctxt() == pat.ctxt => {
_ if !in_macro(e.span) => {
// Double reference might be needed at this point.
pat.lint = REF_BINDING_TO_REFERENCE;
pat.always_deref = false;
let snip = snippet_with_applicability(cx, e.span, "..", &mut pat.app);
pat.replacements.push((e.span, format!("&{}", snip)));
},
Expand Down
28 changes: 27 additions & 1 deletion tests/ui/needless_borrow_pat.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,23 @@ macro_rules! m3 {
Some(ref $i)
};
}
macro_rules! if_chain {
(if $e:expr; $($rest:tt)*) => {
if $e {
if_chain!($($rest)*)
}
};

(if let $p:pat = $e:expr; $($rest:tt)*) => {
if let $p = $e {
if_chain!($($rest)*)
}
};

(then $b:block) => {
$b
};
}

#[allow(dead_code)]
fn main() {
Expand All @@ -32,7 +49,7 @@ fn main() {
None => return,
};

// Ok, the pattern is in a different context than the match arm
// Ok, the pattern is from a macro
let _: &String = match Some(&x) {
m3!(x) => x,
None => return,
Expand Down Expand Up @@ -94,6 +111,15 @@ fn main() {
let _: &u32 = match E::A(&0) {
E::A(ref x) | E::B(ref x) => *x,
};

// Err, reference to &String.
if_chain! {
if true;
if let Some(ref x) = Some(&String::new());
then {
f1(x);
}
}
}

// Err, reference to a &String
Expand Down
30 changes: 18 additions & 12 deletions tests/ui/needless_borrow_pat.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:43:14
--> $DIR/needless_borrow_pat.rs:60:14
|
LL | Some(ref x) => x,
| ^^^^^ help: try this: `x`
|
= note: `-D clippy::needless-borrow` implied by `-D warnings`

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:49:14
--> $DIR/needless_borrow_pat.rs:66:14
|
LL | Some(ref x) => *x,
| ^^^^^
Expand All @@ -18,7 +18,7 @@ LL | Some(x) => x,
| ^ ^

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:55:14
--> $DIR/needless_borrow_pat.rs:72:14
|
LL | Some(ref x) => {
| ^^^^^
Expand All @@ -31,19 +31,19 @@ LL | f1(x);
|

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:65:14
--> $DIR/needless_borrow_pat.rs:82:14
|
LL | Some(ref x) => m1!(x),
| ^^^^^ help: try this: `x`

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:70:15
--> $DIR/needless_borrow_pat.rs:87:15
|
LL | let _ = |&ref x: &&String| {
| ^^^^^ help: try this: `x`

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:75:10
--> $DIR/needless_borrow_pat.rs:92:10
|
LL | let (ref y,) = (&x,);
| ^^^^^
Expand All @@ -55,13 +55,13 @@ LL | let _: &String = y;
|

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:85:14
--> $DIR/needless_borrow_pat.rs:102:14
|
LL | Some(ref x) => x.0,
| ^^^^^ help: try this: `x`

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:95:14
--> $DIR/needless_borrow_pat.rs:112:14
|
LL | E::A(ref x) | E::B(ref x) => *x,
| ^^^^^ ^^^^^
Expand All @@ -72,7 +72,13 @@ LL | E::A(x) | E::B(x) => x,
| ^ ^ ^

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:100:12
--> $DIR/needless_borrow_pat.rs:118:21
|
LL | if let Some(ref x) = Some(&String::new());
| ^^^^^ help: try this: `x`

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:126:12
|
LL | fn f2<'a>(&ref x: &&'a String) -> &'a String {
| ^^^^^
Expand All @@ -85,13 +91,13 @@ LL | x
|

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:107:11
--> $DIR/needless_borrow_pat.rs:133:11
|
LL | fn f(&ref x: &&String) {
| ^^^^^ help: try this: `x`

error: this pattern creates a reference to a reference
--> $DIR/needless_borrow_pat.rs:115:11
--> $DIR/needless_borrow_pat.rs:141:11
|
LL | fn f(&ref x: &&String) {
| ^^^^^
Expand All @@ -102,5 +108,5 @@ LL | fn f(&x: &&String) {
LL | let _: &String = x;
|

error: aborting due to 11 previous errors
error: aborting due to 12 previous errors

2 changes: 1 addition & 1 deletion tests/ui/ref_binding_to_reference.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ macro_rules! m3 {
fn main() {
let x = String::new();

// Ok, the pattern is in a different context than the match arm
// Ok, the pattern is from a macro
let _: &&String = match Some(&x) {
m3!(x) => x,
None => return,
Expand Down

0 comments on commit 5ee2fc7

Please sign in to comment.