Skip to content

Commit 46363df

Browse files
borsbors
committed
Auto merge of #7474 - camsteffen:binop, r=Manishearth
Use lang items for BinOp lints changelog: none
2 parents e93df0b + 98c500c commit 46363df

File tree

7 files changed

+98
-216
lines changed

7 files changed

+98
-216
lines changed

‎clippy_lints/src/assign_ops.rs

Lines changed: 28 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::source::snippet_opt;
33
use clippy_utils::ty::implements_trait;
4-
use clippy_utils::{eq_expr_value, get_trait_def_id, trait_ref_of_method};
5-
use clippy_utils::{higher, paths, sugg};
4+
use clippy_utils::{binop_traits, sugg};
5+
use clippy_utils::{eq_expr_value, trait_ref_of_method};
66
use if_chain::if_chain;
77
use rustc_errors::Applicability;
88
use rustc_hir as hir;
@@ -85,71 +85,34 @@ impl<'tcx> LateLintPass<'tcx> for AssignOps {
8585
let lint = |assignee: &hir::Expr<'_>, rhs: &hir::Expr<'_>| {
8686
let ty = cx.typeck_results().expr_ty(assignee);
8787
let rty = cx.typeck_results().expr_ty(rhs);
88-
macro_rules! ops {
89-
($op:expr,
90-
$cx:expr,
91-
$ty:expr,
92-
$rty:expr,
93-
$($trait_name:ident),+) => {
94-
match $op {
95-
$(hir::BinOpKind::$trait_name => {
96-
let [krate, module] = paths::OPS_MODULE;
97-
let path: [&str; 3] = [krate, module, concat!(stringify!($trait_name), "Assign")];
98-
let trait_id = if let Some(trait_id) = get_trait_def_id($cx, &path) {
99-
trait_id
100-
} else {
101-
return; // useless if the trait doesn't exist
102-
};
103-
// check that we are not inside an `impl AssignOp` of this exact operation
104-
let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id);
105-
if_chain! {
106-
if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
107-
if trait_ref.path.res.def_id() == trait_id;
108-
then { return; }
88+
if_chain! {
89+
if let Some((_, lang_item)) = binop_traits(op.node);
90+
if let Ok(trait_id) = cx.tcx.lang_items().require(lang_item);
91+
let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id);
92+
if trait_ref_of_method(cx, parent_fn)
93+
.map_or(true, |t| t.path.res.def_id() != trait_id);
94+
if implements_trait(cx, ty, trait_id, &[rty.into()]);
95+
then {
96+
span_lint_and_then(
97+
cx,
98+
ASSIGN_OP_PATTERN,
99+
expr.span,
100+
"manual implementation of an assign operation",
101+
|diag| {
102+
if let (Some(snip_a), Some(snip_r)) =
103+
(snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span))
104+
{
105+
diag.span_suggestion(
106+
expr.span,
107+
"replace it with",
108+
format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
109+
Applicability::MachineApplicable,
110+
);
109111
}
110-
implements_trait($cx, $ty, trait_id, &[$rty])
111-
},)*
112-
_ => false,
113-
}
112+
},
113+
);
114114
}
115115
}
116-
if ops!(
117-
op.node,
118-
cx,
119-
ty,
120-
rty.into(),
121-
Add,
122-
Sub,
123-
Mul,
124-
Div,
125-
Rem,
126-
And,
127-
Or,
128-
BitAnd,
129-
BitOr,
130-
BitXor,
131-
Shr,
132-
Shl
133-
) {
134-
span_lint_and_then(
135-
cx,
136-
ASSIGN_OP_PATTERN,
137-
expr.span,
138-
"manual implementation of an assign operation",
139-
|diag| {
140-
if let (Some(snip_a), Some(snip_r)) =
141-
(snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span))
142-
{
143-
diag.span_suggestion(
144-
expr.span,
145-
"replace it with",
146-
format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
147-
Applicability::MachineApplicable,
148-
);
149-
}
150-
},
151-
);
152-
}
153116
};
154117

155118
let mut visitor = ExprVisitor {
@@ -206,7 +169,7 @@ fn lint_misrefactored_assign_op(
206169
if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span)) {
207170
let a = &sugg::Sugg::hir(cx, assignee, "..");
208171
let r = &sugg::Sugg::hir(cx, rhs, "..");
209-
let long = format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r));
172+
let long = format!("{} = {}", snip_a, sugg::make_binop(op.node.into(), a, r));
210173
diag.span_suggestion(
211174
expr.span,
212175
&format!(

‎clippy_lints/src/eq_op.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ impl<'tcx> LateLintPass<'tcx> for EqOp {
102102
if macro_with_not_op(&left.kind) || macro_with_not_op(&right.kind) {
103103
return;
104104
}
105-
if is_useless_with_eq_exprs(higher::binop(op.node)) && eq_expr_value(cx, left, right) {
105+
if is_useless_with_eq_exprs(op.node.into()) && eq_expr_value(cx, left, right) {
106106
span_lint(
107107
cx,
108108
EQ_OP,

‎clippy_lints/src/suspicious_trait_impl.rs

Lines changed: 28 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use clippy_utils::diagnostics::span_lint;
2-
use clippy_utils::{get_trait_def_id, paths, trait_ref_of_method};
2+
use clippy_utils::{binop_traits, trait_ref_of_method, BINOP_TRAITS, OP_ASSIGN_TRAITS};
33
use if_chain::if_chain;
44
use rustc_hir as hir;
55
use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
@@ -55,135 +55,48 @@ declare_lint_pass!(SuspiciousImpl => [SUSPICIOUS_ARITHMETIC_IMPL, SUSPICIOUS_OP_
5555

5656
impl<'tcx> LateLintPass<'tcx> for SuspiciousImpl {
5757
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
58-
if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind {
59-
match binop.node {
60-
hir::BinOpKind::Eq
61-
| hir::BinOpKind::Lt
62-
| hir::BinOpKind::Le
63-
| hir::BinOpKind::Ne
64-
| hir::BinOpKind::Ge
65-
| hir::BinOpKind::Gt => return,
66-
_ => {},
67-
}
58+
if_chain! {
59+
if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind;
60+
if let Some((binop_trait_lang, op_assign_trait_lang)) = binop_traits(binop.node);
61+
if let Ok(binop_trait_id) = cx.tcx.lang_items().require(binop_trait_lang);
62+
if let Ok(op_assign_trait_id) = cx.tcx.lang_items().require(op_assign_trait_lang);
6863

6964
// Check for more than one binary operation in the implemented function
7065
// Linting when multiple operations are involved can result in false positives
7166
let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);
72-
if_chain! {
73-
if let hir::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn);
74-
if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind;
75-
then {
76-
let body = cx.tcx.hir().body(body_id);
77-
let mut visitor = BinaryExprVisitor { nb_binops: 0 };
78-
walk_expr(&mut visitor, &body.value);
79-
if visitor.nb_binops > 1 {
80-
return;
81-
}
82-
}
83-
}
84-
85-
if let Some(impl_trait) = check_binop(
86-
cx,
87-
expr,
88-
binop.node,
89-
&[
90-
"Add", "Sub", "Mul", "Div", "Rem", "BitAnd", "BitOr", "BitXor", "Shl", "Shr",
91-
],
92-
&[
93-
hir::BinOpKind::Add,
94-
hir::BinOpKind::Sub,
95-
hir::BinOpKind::Mul,
96-
hir::BinOpKind::Div,
97-
hir::BinOpKind::Rem,
98-
hir::BinOpKind::BitAnd,
99-
hir::BinOpKind::BitOr,
100-
hir::BinOpKind::BitXor,
101-
hir::BinOpKind::Shl,
102-
hir::BinOpKind::Shr,
103-
],
104-
) {
105-
span_lint(
106-
cx,
107-
SUSPICIOUS_ARITHMETIC_IMPL,
108-
binop.span,
109-
&format!("suspicious use of binary operator in `{}` impl", impl_trait),
110-
);
111-
}
112-
113-
if let Some(impl_trait) = check_binop(
114-
cx,
115-
expr,
116-
binop.node,
117-
&[
118-
"AddAssign",
119-
"SubAssign",
120-
"MulAssign",
121-
"DivAssign",
122-
"BitAndAssign",
123-
"BitOrAssign",
124-
"BitXorAssign",
125-
"RemAssign",
126-
"ShlAssign",
127-
"ShrAssign",
128-
],
129-
&[
130-
hir::BinOpKind::Add,
131-
hir::BinOpKind::Sub,
132-
hir::BinOpKind::Mul,
133-
hir::BinOpKind::Div,
134-
hir::BinOpKind::BitAnd,
135-
hir::BinOpKind::BitOr,
136-
hir::BinOpKind::BitXor,
137-
hir::BinOpKind::Rem,
138-
hir::BinOpKind::Shl,
139-
hir::BinOpKind::Shr,
140-
],
141-
) {
67+
if let hir::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn);
68+
if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind;
69+
let body = cx.tcx.hir().body(body_id);
70+
let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);
71+
if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
72+
let trait_id = trait_ref.path.res.def_id();
73+
if ![binop_trait_id, op_assign_trait_id].contains(&trait_id);
74+
if let Some(&(_, lint)) = [
75+
(&BINOP_TRAITS, SUSPICIOUS_ARITHMETIC_IMPL),
76+
(&OP_ASSIGN_TRAITS, SUSPICIOUS_OP_ASSIGN_IMPL),
77+
]
78+
.iter()
79+
.find(|&(ts, _)| ts.iter().any(|&t| Ok(trait_id) == cx.tcx.lang_items().require(t)));
80+
if count_binops(&body.value) == 1;
81+
then {
14282
span_lint(
14383
cx,
144-
SUSPICIOUS_OP_ASSIGN_IMPL,
84+
lint,
14585
binop.span,
146-
&format!("suspicious use of binary operator in `{}` impl", impl_trait),
86+
&format!("suspicious use of `{}` in `{}` impl", binop.node.as_str(), cx.tcx.item_name(trait_id)),
14787
);
14888
}
14989
}
15090
}
15191
}
15292

153-
fn check_binop(
154-
cx: &LateContext<'_>,
155-
expr: &hir::Expr<'_>,
156-
binop: hir::BinOpKind,
157-
traits: &[&'static str],
158-
expected_ops: &[hir::BinOpKind],
159-
) -> Option<&'static str> {
160-
let mut trait_ids = vec![];
161-
let [krate, module] = paths::OPS_MODULE;
162-
163-
for &t in traits {
164-
let path = [krate, module, t];
165-
if let Some(trait_id) = get_trait_def_id(cx, &path) {
166-
trait_ids.push(trait_id);
167-
} else {
168-
return None;
169-
}
170-
}
171-
172-
// Get the actually implemented trait
173-
let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);
174-
175-
if_chain! {
176-
if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
177-
if let Some(idx) = trait_ids.iter().position(|&tid| tid == trait_ref.path.res.def_id());
178-
if binop != expected_ops[idx];
179-
then{
180-
return Some(traits[idx])
181-
}
182-
}
183-
184-
None
93+
fn count_binops(expr: &hir::Expr<'_>) -> u32 {
94+
let mut visitor = BinaryExprVisitor::default();
95+
visitor.visit_expr(expr);
96+
visitor.nb_binops
18597
}
18698

99+
#[derive(Default)]
187100
struct BinaryExprVisitor {
188101
nb_binops: u32,
189102
}

‎clippy_utils/src/higher.rs

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -11,31 +11,6 @@ use rustc_hir::{BorrowKind, Expr, ExprKind, StmtKind, UnOp};
1111
use rustc_lint::LateContext;
1212
use rustc_span::{sym, ExpnKind, Span, Symbol};
1313

14-
/// Converts a hir binary operator to the corresponding `ast` type.
15-
#[must_use]
16-
pub fn binop(op: hir::BinOpKind) -> ast::BinOpKind {
17-
match op {
18-
hir::BinOpKind::Eq => ast::BinOpKind::Eq,
19-
hir::BinOpKind::Ge => ast::BinOpKind::Ge,
20-
hir::BinOpKind::Gt => ast::BinOpKind::Gt,
21-
hir::BinOpKind::Le => ast::BinOpKind::Le,
22-
hir::BinOpKind::Lt => ast::BinOpKind::Lt,
23-
hir::BinOpKind::Ne => ast::BinOpKind::Ne,
24-
hir::BinOpKind::Or => ast::BinOpKind::Or,
25-
hir::BinOpKind::Add => ast::BinOpKind::Add,
26-
hir::BinOpKind::And => ast::BinOpKind::And,
27-
hir::BinOpKind::BitAnd => ast::BinOpKind::BitAnd,
28-
hir::BinOpKind::BitOr => ast::BinOpKind::BitOr,
29-
hir::BinOpKind::BitXor => ast::BinOpKind::BitXor,
30-
hir::BinOpKind::Div => ast::BinOpKind::Div,
31-
hir::BinOpKind::Mul => ast::BinOpKind::Mul,
32-
hir::BinOpKind::Rem => ast::BinOpKind::Rem,
33-
hir::BinOpKind::Shl => ast::BinOpKind::Shl,
34-
hir::BinOpKind::Shr => ast::BinOpKind::Shr,
35-
hir::BinOpKind::Sub => ast::BinOpKind::Sub,
36-
}
37-
}
38-
3914
/// Represent a range akin to `ast::ExprKind::Range`.
4015
#[derive(Debug, Copy, Clone)]
4116
pub struct Range<'a> {

‎clippy_utils/src/lib.rs

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1706,3 +1706,34 @@ pub fn is_test_module_or_function(tcx: TyCtxt<'_>, item: &Item<'_>) -> bool {
17061706

17071707
matches!(item.kind, ItemKind::Mod(..)) && item.ident.name.as_str().contains("test")
17081708
}
1709+
1710+
macro_rules! op_utils {
1711+
($($name:ident $assign:ident)*) => {
1712+
/// Binary operation traits like `LangItem::Add`
1713+
pub static BINOP_TRAITS: &[LangItem] = &[$(LangItem::$name,)*];
1714+
1715+
/// Operator-Assign traits like `LangItem::AddAssign`
1716+
pub static OP_ASSIGN_TRAITS: &[LangItem] = &[$(LangItem::$assign,)*];
1717+
1718+
/// Converts `BinOpKind::Add` to `(LangItem::Add, LangItem::AddAssign)`, for example
1719+
pub fn binop_traits(kind: hir::BinOpKind) -> Option<(LangItem, LangItem)> {
1720+
match kind {
1721+
$(hir::BinOpKind::$name => Some((LangItem::$name, LangItem::$assign)),)*
1722+
_ => None,
1723+
}
1724+
}
1725+
};
1726+
}
1727+
1728+
op_utils! {
1729+
Add AddAssign
1730+
Sub SubAssign
1731+
Mul MulAssign
1732+
Div DivAssign
1733+
Rem RemAssign
1734+
BitXor BitXorAssign
1735+
BitAnd BitAndAssign
1736+
BitOr BitOrAssign
1737+
Shl ShlAssign
1738+
Shr ShrAssign
1739+
}

‎clippy_utils/src/sugg.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ impl<'a> Sugg<'a> {
154154
| hir::ExprKind::Err => Sugg::NonParen(snippet),
155155
hir::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, snippet),
156156
hir::ExprKind::AssignOp(op, ..) => Sugg::BinOp(hirbinop2assignop(op), snippet),
157-
hir::ExprKind::Binary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(higher::binop(op.node)), snippet),
157+
hir::ExprKind::Binary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(op.node.into()), snippet),
158158
hir::ExprKind::Cast(..) => Sugg::BinOp(AssocOp::As, snippet),
159159
hir::ExprKind::Type(..) => Sugg::BinOp(AssocOp::Colon, snippet),
160160
}

0 commit comments

Comments
 (0)