Skip to content

Extend needless_borrowed_reference to structs and tuples, ignore _ #9855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Extend needless_borrowed_reference to structs and tuples, ignore _ #9855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clippy_lints/src/enum_variants.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ impl LateLintPass<'_> for EnumVariantNames {
let item_name = item.ident.name.as_str();
let item_camel = to_camel_case(item_name);
if !item.span.from_expansion() && is_present_in_source(cx, item.span) {
if let Some(&(ref mod_name, ref mod_camel)) = self.modules.last() {
if let Some((mod_name, mod_camel)) = self.modules.last() {
// constants don't have surrounding modules
if !mod_camel.is_empty() {
if mod_name == &item.ident.name {
Expand Down
28 changes: 12 additions & 16 deletions clippy_lints/src/int_plus_one.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,58 +62,54 @@ impl IntPlusOne {
fn check_binop(cx: &EarlyContext<'_>, binop: BinOpKind, lhs: &Expr, rhs: &Expr) -> Option<String> {
match (binop, &lhs.kind, &rhs.kind) {
// case where `x - 1 >= ...` or `-1 + x >= ...`
(BinOpKind::Ge, &ExprKind::Binary(ref lhskind, ref lhslhs, ref lhsrhs), _) => {
(BinOpKind::Ge, ExprKind::Binary(lhskind, lhslhs, lhsrhs), _) => {
match (lhskind.node, &lhslhs.kind, &lhsrhs.kind) {
// `-1 + x`
(BinOpKind::Add, &ExprKind::Lit(ref lit), _) if Self::check_lit(lit, -1) => {
(BinOpKind::Add, ExprKind::Lit(lit), _) if Self::check_lit(lit, -1) => {
Self::generate_recommendation(cx, binop, lhsrhs, rhs, Side::Lhs)
},
// `x - 1`
(BinOpKind::Sub, _, &ExprKind::Lit(ref lit)) if Self::check_lit(lit, 1) => {
(BinOpKind::Sub, _, ExprKind::Lit(lit)) if Self::check_lit(lit, 1) => {
Self::generate_recommendation(cx, binop, lhslhs, rhs, Side::Lhs)
},
_ => None,
}
},
// case where `... >= y + 1` or `... >= 1 + y`
(BinOpKind::Ge, _, &ExprKind::Binary(ref rhskind, ref rhslhs, ref rhsrhs))
if rhskind.node == BinOpKind::Add =>
{
(BinOpKind::Ge, _, ExprKind::Binary(rhskind, rhslhs, rhsrhs)) if rhskind.node == BinOpKind::Add => {
match (&rhslhs.kind, &rhsrhs.kind) {
// `y + 1` and `1 + y`
(&ExprKind::Lit(ref lit), _) if Self::check_lit(lit, 1) => {
(ExprKind::Lit(lit), _) if Self::check_lit(lit, 1) => {
Self::generate_recommendation(cx, binop, rhsrhs, lhs, Side::Rhs)
},
(_, &ExprKind::Lit(ref lit)) if Self::check_lit(lit, 1) => {
(_, ExprKind::Lit(lit)) if Self::check_lit(lit, 1) => {
Self::generate_recommendation(cx, binop, rhslhs, lhs, Side::Rhs)
},
_ => None,
}
},
// case where `x + 1 <= ...` or `1 + x <= ...`
(BinOpKind::Le, &ExprKind::Binary(ref lhskind, ref lhslhs, ref lhsrhs), _)
if lhskind.node == BinOpKind::Add =>
{
(BinOpKind::Le, ExprKind::Binary(lhskind, lhslhs, lhsrhs), _) if lhskind.node == BinOpKind::Add => {
match (&lhslhs.kind, &lhsrhs.kind) {
// `1 + x` and `x + 1`
(&ExprKind::Lit(ref lit), _) if Self::check_lit(lit, 1) => {
(ExprKind::Lit(lit), _) if Self::check_lit(lit, 1) => {
Self::generate_recommendation(cx, binop, lhsrhs, rhs, Side::Lhs)
},
(_, &ExprKind::Lit(ref lit)) if Self::check_lit(lit, 1) => {
(_, ExprKind::Lit(lit)) if Self::check_lit(lit, 1) => {
Self::generate_recommendation(cx, binop, lhslhs, rhs, Side::Lhs)
},
_ => None,
}
},
// case where `... >= y - 1` or `... >= -1 + y`
(BinOpKind::Le, _, &ExprKind::Binary(ref rhskind, ref rhslhs, ref rhsrhs)) => {
(BinOpKind::Le, _, ExprKind::Binary(rhskind, rhslhs, rhsrhs)) => {
match (rhskind.node, &rhslhs.kind, &rhsrhs.kind) {
// `-1 + y`
(BinOpKind::Add, &ExprKind::Lit(ref lit), _) if Self::check_lit(lit, -1) => {
(BinOpKind::Add, ExprKind::Lit(lit), _) if Self::check_lit(lit, -1) => {
Self::generate_recommendation(cx, binop, rhsrhs, lhs, Side::Rhs)
},
// `y - 1`
(BinOpKind::Sub, _, &ExprKind::Lit(ref lit)) if Self::check_lit(lit, 1) => {
(BinOpKind::Sub, _, ExprKind::Lit(lit)) if Self::check_lit(lit, 1) => {
Self::generate_recommendation(cx, binop, rhslhs, lhs, Side::Rhs)
},
_ => None,
Expand Down
3 changes: 1 addition & 2 deletions clippy_lints/src/len_zero.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,7 @@ fn check_for_is_empty<'tcx>(
}

fn check_cmp(cx: &LateContext<'_>, span: Span, method: &Expr<'_>, lit: &Expr<'_>, op: &str, compare_to: u32) {
if let (&ExprKind::MethodCall(method_path, receiver, args, _), &ExprKind::Lit(ref lit)) = (&method.kind, &lit.kind)
{
if let (&ExprKind::MethodCall(method_path, receiver, args, _), ExprKind::Lit(lit)) = (&method.kind, &lit.kind) {
// check if we are in an is_empty() method
if let Some(name) = get_item_name(cx, method) {
if name.as_str() == "is_empty" {
Expand Down
108 changes: 69 additions & 39 deletions clippy_lints/src/needless_borrowed_ref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,14 @@ declare_clippy_lint! {
declare_lint_pass!(NeedlessBorrowedRef => [NEEDLESS_BORROWED_REFERENCE]);

impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowedRef {
fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) {
if pat.span.from_expansion() {
fn check_pat(&mut self, cx: &LateContext<'tcx>, ref_pat: &'tcx Pat<'_>) {
if ref_pat.span.from_expansion() {
// OK, simple enough, lints doesn't check in macro.
return;
}

// Do not lint patterns that are part of an OR `|` pattern, the binding mode must match in all arms
for (_, node) in cx.tcx.hir().parent_iter(pat.hir_id) {
for (_, node) in cx.tcx.hir().parent_iter(ref_pat.hir_id) {
let Node::Pat(pat) = node else { break };

if matches!(pat.kind, PatKind::Or(_)) {
Expand All @@ -52,20 +52,20 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowedRef {
}

// Only lint immutable refs, because `&mut ref T` may be useful.
let PatKind::Ref(sub_pat, Mutability::Not) = pat.kind else { return };
let PatKind::Ref(pat, Mutability::Not) = ref_pat.kind else { return };

match sub_pat.kind {
match pat.kind {
// Check sub_pat got a `ref` keyword (excluding `ref mut`).
PatKind::Binding(BindingAnnotation::REF, _, ident, None) => {
span_lint_and_then(
cx,
NEEDLESS_BORROWED_REFERENCE,
pat.span,
ref_pat.span,
"this pattern takes a reference on something that is being dereferenced",
|diag| {
// `&ref ident`
// ^^^^^
let span = pat.span.until(ident.span);
let span = ref_pat.span.until(ident.span);
diag.span_suggestion_verbose(
span,
"try removing the `&ref` part",
Expand All @@ -84,41 +84,71 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowedRef {
}),
after,
) => {
let mut suggestions = Vec::new();

for element_pat in itertools::chain(before, after) {
if let PatKind::Binding(BindingAnnotation::REF, _, ident, None) = element_pat.kind {
// `&[..., ref ident, ...]`
// ^^^^
let span = element_pat.span.until(ident.span);
suggestions.push((span, String::new()));
} else {
return;
}
}
check_subpatterns(
cx,
"dereferencing a slice pattern where every element takes a reference",
ref_pat,
pat,
itertools::chain(before, after),
);
},
PatKind::Tuple(subpatterns, _) | PatKind::TupleStruct(_, subpatterns, _) => {
check_subpatterns(
cx,
"dereferencing a tuple pattern where every element takes a reference",
ref_pat,
pat,
subpatterns,
);
},
PatKind::Struct(_, fields, _) => {
check_subpatterns(
cx,
"dereferencing a struct pattern where every field's pattern takes a reference",
ref_pat,
pat,
fields.iter().map(|field| field.pat),
);
},
_ => {},
}
}
}

if !suggestions.is_empty() {
span_lint_and_then(
cx,
NEEDLESS_BORROWED_REFERENCE,
pat.span,
"dereferencing a slice pattern where every element takes a reference",
|diag| {
// `&[...]`
// ^
let span = pat.span.until(sub_pat.span);
suggestions.push((span, String::new()));
fn check_subpatterns<'tcx>(
cx: &LateContext<'tcx>,
message: &str,
ref_pat: &Pat<'_>,
pat: &Pat<'_>,
subpatterns: impl IntoIterator<Item = &'tcx Pat<'tcx>>,
) {
let mut suggestions = Vec::new();

diag.multipart_suggestion(
"try removing the `&` and `ref` parts",
suggestions,
Applicability::MachineApplicable,
);
},
);
}
for subpattern in subpatterns {
match subpattern.kind {
PatKind::Binding(BindingAnnotation::REF, _, ident, None) => {
// `ref ident`
// ^^^^
let span = subpattern.span.until(ident.span);
suggestions.push((span, String::new()));
},
_ => {},
PatKind::Wild => {},
_ => return,
}
}

if !suggestions.is_empty() {
span_lint_and_then(cx, NEEDLESS_BORROWED_REFERENCE, ref_pat.span, message, |diag| {
// `&pat`
// ^
let span = ref_pat.span.until(pat.span);
suggestions.push((span, String::new()));

diag.multipart_suggestion(
"try removing the `&` and `ref` parts",
suggestions,
Applicability::MachineApplicable,
);
});
}
}
2 changes: 1 addition & 1 deletion clippy_lints/src/unsafe_removed_from_name.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ fn check_use_tree(use_tree: &UseTree, cx: &EarlyContext<'_>, span: Span) {
},
UseTreeKind::Simple(None, ..) | UseTreeKind::Glob => {},
UseTreeKind::Nested(ref nested_use_tree) => {
for &(ref use_tree, _) in nested_use_tree {
for (use_tree, _) in nested_use_tree {
check_use_tree(use_tree, cx, span);
}
},
Expand Down
22 changes: 11 additions & 11 deletions clippy_utils/src/consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ pub enum Constant {
impl PartialEq for Constant {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
(&Self::Str(ref ls), &Self::Str(ref rs)) => ls == rs,
(&Self::Binary(ref l), &Self::Binary(ref r)) => l == r,
(Self::Str(ls), Self::Str(rs)) => ls == rs,
(Self::Binary(l), Self::Binary(r)) => l == r,
(&Self::Char(l), &Self::Char(r)) => l == r,
(&Self::Int(l), &Self::Int(r)) => l == r,
(&Self::F64(l), &Self::F64(r)) => {
Expand All @@ -69,8 +69,8 @@ impl PartialEq for Constant {
},
(&Self::Bool(l), &Self::Bool(r)) => l == r,
(&Self::Vec(ref l), &Self::Vec(ref r)) | (&Self::Tuple(ref l), &Self::Tuple(ref r)) => l == r,
(&Self::Repeat(ref lv, ref ls), &Self::Repeat(ref rv, ref rs)) => ls == rs && lv == rv,
(&Self::Ref(ref lb), &Self::Ref(ref rb)) => *lb == *rb,
(Self::Repeat(lv, ls), Self::Repeat(rv, rs)) => ls == rs && lv == rv,
(Self::Ref(lb), Self::Ref(rb)) => *lb == *rb,
// TODO: are there inter-type equalities?
_ => false,
}
Expand Down Expand Up @@ -126,17 +126,17 @@ impl Hash for Constant {
impl Constant {
pub fn partial_cmp(tcx: TyCtxt<'_>, cmp_type: Ty<'_>, left: &Self, right: &Self) -> Option<Ordering> {
match (left, right) {
(&Self::Str(ref ls), &Self::Str(ref rs)) => Some(ls.cmp(rs)),
(&Self::Char(ref l), &Self::Char(ref r)) => Some(l.cmp(r)),
(Self::Str(ls), Self::Str(rs)) => Some(ls.cmp(rs)),
(Self::Char(l), Self::Char(r)) => Some(l.cmp(r)),
(&Self::Int(l), &Self::Int(r)) => match *cmp_type.kind() {
ty::Int(int_ty) => Some(sext(tcx, l, int_ty).cmp(&sext(tcx, r, int_ty))),
ty::Uint(_) => Some(l.cmp(&r)),
_ => bug!("Not an int type"),
},
(&Self::F64(l), &Self::F64(r)) => l.partial_cmp(&r),
(&Self::F32(l), &Self::F32(r)) => l.partial_cmp(&r),
(&Self::Bool(ref l), &Self::Bool(ref r)) => Some(l.cmp(r)),
(&Self::Tuple(ref l), &Self::Tuple(ref r)) if l.len() == r.len() => match *cmp_type.kind() {
(Self::Bool(l), Self::Bool(r)) => Some(l.cmp(r)),
(Self::Tuple(l), Self::Tuple(r)) if l.len() == r.len() => match *cmp_type.kind() {
ty::Tuple(tys) if tys.len() == l.len() => l
.iter()
.zip(r)
Expand All @@ -146,7 +146,7 @@ impl Constant {
.unwrap_or_else(|| Some(l.len().cmp(&r.len()))),
_ => None,
},
(&Self::Vec(ref l), &Self::Vec(ref r)) => {
(Self::Vec(l), Self::Vec(r)) => {
let (ty::Array(cmp_type, _) | ty::Slice(cmp_type)) = *cmp_type.kind() else {
return None
};
Expand All @@ -155,7 +155,7 @@ impl Constant {
.find(|r| r.map_or(true, |o| o != Ordering::Equal))
.unwrap_or_else(|| Some(l.len().cmp(&r.len())))
},
(&Self::Repeat(ref lv, ref ls), &Self::Repeat(ref rv, ref rs)) => {
(Self::Repeat(lv, ls), Self::Repeat(rv, rs)) => {
match Self::partial_cmp(
tcx,
match *cmp_type.kind() {
Expand All @@ -169,7 +169,7 @@ impl Constant {
x => x,
}
},
(&Self::Ref(ref lb), &Self::Ref(ref rb)) => Self::partial_cmp(
(Self::Ref(lb), Self::Ref(rb)) => Self::partial_cmp(
tcx,
match *cmp_type.kind() {
ty::Ref(_, ty, _) => ty,
Expand Down
16 changes: 7 additions & 9 deletions clippy_utils/src/hir_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ impl HirEqInterExpr<'_, '_, '_> {
(&ExprKind::Let(l), &ExprKind::Let(r)) => {
self.eq_pat(l.pat, r.pat) && both(&l.ty, &r.ty, |l, r| self.eq_ty(l, r)) && self.eq_expr(l.init, r.init)
},
(&ExprKind::Lit(ref l), &ExprKind::Lit(ref r)) => l.node == r.node,
(ExprKind::Lit(l), ExprKind::Lit(r)) => l.node == r.node,
(&ExprKind::Loop(lb, ref ll, ref lls, _), &ExprKind::Loop(rb, ref rl, ref rls, _)) => {
lls == rls && self.eq_block(lb, rb) && both(ll, rl, |l, r| l.ident.name == r.ident.name)
},
Expand All @@ -291,8 +291,8 @@ impl HirEqInterExpr<'_, '_, '_> {
(&ExprKind::Repeat(le, ll), &ExprKind::Repeat(re, rl)) => {
self.eq_expr(le, re) && self.eq_array_length(ll, rl)
},
(&ExprKind::Ret(ref l), &ExprKind::Ret(ref r)) => both(l, r, |l, r| self.eq_expr(l, r)),
(&ExprKind::Path(ref l), &ExprKind::Path(ref r)) => self.eq_qpath(l, r),
(ExprKind::Ret(l), ExprKind::Ret(r)) => both(l, r, |l, r| self.eq_expr(l, r)),
(ExprKind::Path(l), ExprKind::Path(r)) => self.eq_qpath(l, r),
(&ExprKind::Struct(l_path, lf, ref lo), &ExprKind::Struct(r_path, rf, ref ro)) => {
self.eq_qpath(l_path, r_path)
&& both(lo, ro, |l, r| self.eq_expr(l, r))
Expand Down Expand Up @@ -362,7 +362,7 @@ impl HirEqInterExpr<'_, '_, '_> {
}
eq
},
(&PatKind::Path(ref l), &PatKind::Path(ref r)) => self.eq_qpath(l, r),
(PatKind::Path(l), PatKind::Path(r)) => self.eq_qpath(l, r),
(&PatKind::Lit(l), &PatKind::Lit(r)) => self.eq_expr(l, r),
(&PatKind::Tuple(l, ls), &PatKind::Tuple(r, rs)) => ls == rs && over(l, r, |l, r| self.eq_pat(l, r)),
(&PatKind::Range(ref ls, ref le, li), &PatKind::Range(ref rs, ref re, ri)) => {
Expand Down Expand Up @@ -429,13 +429,11 @@ impl HirEqInterExpr<'_, '_, '_> {
match (&left.kind, &right.kind) {
(&TyKind::Slice(l_vec), &TyKind::Slice(r_vec)) => self.eq_ty(l_vec, r_vec),
(&TyKind::Array(lt, ll), &TyKind::Array(rt, rl)) => self.eq_ty(lt, rt) && self.eq_array_length(ll, rl),
(&TyKind::Ptr(ref l_mut), &TyKind::Ptr(ref r_mut)) => {
l_mut.mutbl == r_mut.mutbl && self.eq_ty(l_mut.ty, r_mut.ty)
},
(&TyKind::Rptr(_, ref l_rmut), &TyKind::Rptr(_, ref r_rmut)) => {
(TyKind::Ptr(l_mut), TyKind::Ptr(r_mut)) => l_mut.mutbl == r_mut.mutbl && self.eq_ty(l_mut.ty, r_mut.ty),
(TyKind::Rptr(_, l_rmut), TyKind::Rptr(_, r_rmut)) => {
l_rmut.mutbl == r_rmut.mutbl && self.eq_ty(l_rmut.ty, r_rmut.ty)
},
(&TyKind::Path(ref l), &TyKind::Path(ref r)) => self.eq_qpath(l, r),
(TyKind::Path(l), TyKind::Path(r)) => self.eq_qpath(l, r),
(&TyKind::Tup(l), &TyKind::Tup(r)) => over(l, r, |l, r| self.eq_ty(l, r)),
(&TyKind::Infer, &TyKind::Infer) => true,
_ => false,
Expand Down
7 changes: 6 additions & 1 deletion tests/ui/match_expr_like_matches_macro.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@

#![feature(custom_inner_attributes)]
#![warn(clippy::match_like_matches_macro)]
#![allow(unreachable_patterns, dead_code, clippy::equatable_if_let)]
#![allow(
unreachable_patterns,
dead_code,
clippy::equatable_if_let,
clippy::needless_borrowed_reference
)]

fn main() {
let x = Some(5);
Expand Down
7 changes: 6 additions & 1 deletion tests/ui/match_expr_like_matches_macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@

#![feature(custom_inner_attributes)]
#![warn(clippy::match_like_matches_macro)]
#![allow(unreachable_patterns, dead_code, clippy::equatable_if_let)]
#![allow(
unreachable_patterns,
dead_code,
clippy::equatable_if_let,
clippy::needless_borrowed_reference
)]

fn main() {
let x = Some(5);
Expand Down
Loading