Skip to content

Commit 98c500c

Browse files
camsteffencamsteffen
committed
Factor BinOp utils
1 parent efbf7ca commit 98c500c

File tree

4 files changed

+95
-188
lines changed

4 files changed

+95
-188
lines changed

‎clippy_lints/src/assign_ops.rs

Lines changed: 27 additions & 64 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::{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 {

‎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/lib.rs

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

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

‎tests/ui/suspicious_arithmetic_impl.stderr

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,56 +1,56 @@
1-
error: suspicious use of binary operator in `Add` impl
1+
error: suspicious use of `-` in `Add` impl
22
--> $DIR/suspicious_arithmetic_impl.rs:13:20
33
|
44
LL | Foo(self.0 - other.0)
55
| ^
66
|
77
= note: `-D clippy::suspicious-arithmetic-impl` implied by `-D warnings`
88

9-
error: suspicious use of binary operator in `AddAssign` impl
9+
error: suspicious use of `-` in `AddAssign` impl
1010
--> $DIR/suspicious_arithmetic_impl.rs:19:23
1111
|
1212
LL | *self = *self - other;
1313
| ^
1414
|
1515
= note: `-D clippy::suspicious-op-assign-impl` implied by `-D warnings`
1616

17-
error: suspicious use of binary operator in `MulAssign` impl
17+
error: suspicious use of `/` in `MulAssign` impl
1818
--> $DIR/suspicious_arithmetic_impl.rs:32:16
1919
|
2020
LL | self.0 /= other.0;
2121
| ^^
2222

23-
error: suspicious use of binary operator in `Rem` impl
23+
error: suspicious use of `/` in `Rem` impl
2424
--> $DIR/suspicious_arithmetic_impl.rs:70:20
2525
|
2626
LL | Foo(self.0 / other.0)
2727
| ^
2828

29-
error: suspicious use of binary operator in `BitAnd` impl
29+
error: suspicious use of `|` in `BitAnd` impl
3030
--> $DIR/suspicious_arithmetic_impl.rs:78:20
3131
|
3232
LL | Foo(self.0 | other.0)
3333
| ^
3434

35-
error: suspicious use of binary operator in `BitOr` impl
35+
error: suspicious use of `^` in `BitOr` impl
3636
--> $DIR/suspicious_arithmetic_impl.rs:86:20
3737
|
3838
LL | Foo(self.0 ^ other.0)
3939
| ^
4040

41-
error: suspicious use of binary operator in `BitXor` impl
41+
error: suspicious use of `&` in `BitXor` impl
4242
--> $DIR/suspicious_arithmetic_impl.rs:94:20
4343
|
4444
LL | Foo(self.0 & other.0)
4545
| ^
4646

47-
error: suspicious use of binary operator in `Shl` impl
47+
error: suspicious use of `>>` in `Shl` impl
4848
--> $DIR/suspicious_arithmetic_impl.rs:102:20
4949
|
5050
LL | Foo(self.0 >> other.0)
5151
| ^^
5252

53-
error: suspicious use of binary operator in `Shr` impl
53+
error: suspicious use of `<<` in `Shr` impl
5454
--> $DIR/suspicious_arithmetic_impl.rs:110:20
5555
|
5656
LL | Foo(self.0 << other.0)

0 commit comments

Comments
 (0)