Skip to content

Commit ee8d328

Browse files
oli-obkoli-obk
authored
Merge pull request #2413 from flip1995/assign_ops
Improved suggestion on misrefactored_assign_op lint
2 parents 8123495 + bd421cb commit ee8d328

File tree

3 files changed

+154
-31
lines changed

3 files changed

+154
-31
lines changed

‎clippy_lints/src/assign_ops.rs

+62-21
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use rustc::hir;
2+
use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
23
use rustc::lint::*;
34
use syntax::ast;
45
use utils::{get_trait_def_id, implements_trait, snippet_opt, span_lint_and_then, SpanlessEq};
@@ -87,19 +88,29 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps {
8788
});
8889
if let hir::ExprBinary(binop, ref l, ref r) = rhs.node {
8990
if op.node == binop.node {
90-
let lint = |assignee: &hir::Expr, rhs: &hir::Expr| {
91+
let lint = |assignee: &hir::Expr, rhs_other: &hir::Expr| {
9192
span_lint_and_then(
9293
cx,
9394
MISREFACTORED_ASSIGN_OP,
9495
expr.span,
9596
"variable appears on both sides of an assignment operation",
9697
|db| if let (Some(snip_a), Some(snip_r)) =
97-
(snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span))
98+
(snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span))
9899
{
100+
let a = &sugg::Sugg::hir(cx, assignee, "..");
101+
let r = &sugg::Sugg::hir(cx, rhs, "..");
102+
let long = format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r));
99103
db.span_suggestion(
100104
expr.span,
101-
"replace it with",
102-
format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
105+
&format!("Did you mean {} = {} {} {} or {}? Consider replacing it with",
106+
snip_a, snip_a, op.node.as_str(), snip_r,
107+
long),
108+
format!("{} {}= {}", snip_a, op.node.as_str(), snip_r)
109+
);
110+
db.span_suggestion(
111+
expr.span,
112+
"or",
113+
long
103114
);
104115
},
105116
);
@@ -189,23 +200,34 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps {
189200
);
190201
}
191202
};
192-
// a = a op b
193-
if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, l) {
194-
lint(assignee, r);
195-
}
196-
// a = b commutative_op a
197-
if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, r) {
198-
match op.node {
199-
hir::BiAdd |
200-
hir::BiMul |
201-
hir::BiAnd |
202-
hir::BiOr |
203-
hir::BiBitXor |
204-
hir::BiBitAnd |
205-
hir::BiBitOr => {
206-
lint(assignee, l);
207-
},
208-
_ => {},
203+
204+
let mut visitor = ExprVisitor {
205+
assignee: assignee,
206+
counter: 0,
207+
cx: cx
208+
};
209+
210+
walk_expr(&mut visitor, e);
211+
212+
if visitor.counter == 1 {
213+
// a = a op b
214+
if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, l) {
215+
lint(assignee, r);
216+
}
217+
// a = b commutative_op a
218+
if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, r) {
219+
match op.node {
220+
hir::BiAdd |
221+
hir::BiMul |
222+
hir::BiAnd |
223+
hir::BiOr |
224+
hir::BiBitXor |
225+
hir::BiBitAnd |
226+
hir::BiBitOr => {
227+
lint(assignee, l);
228+
},
229+
_ => {},
230+
}
209231
}
210232
}
211233
}
@@ -222,3 +244,22 @@ fn is_commutative(op: hir::BinOp_) -> bool {
222244
BiSub | BiDiv | BiRem | BiShl | BiShr | BiLt | BiLe | BiGe | BiGt => false,
223245
}
224246
}
247+
248+
struct ExprVisitor<'a, 'tcx: 'a> {
249+
assignee: &'a hir::Expr,
250+
counter: u8,
251+
cx: &'a LateContext<'a, 'tcx>,
252+
}
253+
254+
impl<'a, 'tcx: 'a> Visitor<'tcx> for ExprVisitor<'a, 'tcx> {
255+
fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
256+
if SpanlessEq::new(self.cx).ignore_fn().eq_expr(self.assignee, &expr) {
257+
self.counter += 1;
258+
}
259+
260+
walk_expr(self, expr);
261+
}
262+
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
263+
NestedVisitorMap::None
264+
}
265+
}

‎tests/ui/assign_ops2.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33

44
#[allow(unused_assignments)]
5-
#[warn(misrefactored_assign_op)]
5+
#[warn(misrefactored_assign_op, assign_op_pattern)]
66
fn main() {
77
let mut a = 5;
88
a += a + 1;
@@ -13,6 +13,10 @@ fn main() {
1313
a /= a / 2;
1414
a %= a % 5;
1515
a &= a & 1;
16+
a *= a * a;
17+
a = a * a * a;
18+
a = a * 42 * a;
19+
a = a * 2 + a;
1620
a -= 1 - a;
1721
a /= 5 / a;
1822
a %= 42 % a;

‎tests/ui/assign_ops2.stderr

+87-9
Original file line numberDiff line numberDiff line change
@@ -2,51 +2,129 @@ error: variable appears on both sides of an assignment operation
22
--> $DIR/assign_ops2.rs:8:5
33
|
44
8 | a += a + 1;
5-
| ^^^^^^^^^^ help: replace it with: `a += 1`
5+
| ^^^^^^^^^^
66
|
77
= note: `-D misrefactored-assign-op` implied by `-D warnings`
8+
help: Did you mean a = a + 1 or a = a + a + 1? Consider replacing it with
9+
|
10+
8 | a += 1;
11+
| ^^^^^^
12+
help: or
13+
|
14+
8 | a = a + a + 1;
15+
| ^^^^^^^^^^^^^
816

917
error: variable appears on both sides of an assignment operation
1018
--> $DIR/assign_ops2.rs:9:5
1119
|
1220
9 | a += 1 + a;
13-
| ^^^^^^^^^^ help: replace it with: `a += 1`
21+
| ^^^^^^^^^^
22+
help: Did you mean a = a + 1 or a = a + 1 + a? Consider replacing it with
23+
|
24+
9 | a += 1;
25+
| ^^^^^^
26+
help: or
27+
|
28+
9 | a = a + 1 + a;
29+
| ^^^^^^^^^^^^^
1430

1531
error: variable appears on both sides of an assignment operation
1632
--> $DIR/assign_ops2.rs:10:5
1733
|
1834
10 | a -= a - 1;
19-
| ^^^^^^^^^^ help: replace it with: `a -= 1`
35+
| ^^^^^^^^^^
36+
help: Did you mean a = a - 1 or a = a - (a - 1)? Consider replacing it with
37+
|
38+
10 | a -= 1;
39+
| ^^^^^^
40+
help: or
41+
|
42+
10 | a = a - (a - 1);
43+
| ^^^^^^^^^^^^^^^
2044

2145
error: variable appears on both sides of an assignment operation
2246
--> $DIR/assign_ops2.rs:11:5
2347
|
2448
11 | a *= a * 99;
25-
| ^^^^^^^^^^^ help: replace it with: `a *= 99`
49+
| ^^^^^^^^^^^
50+
help: Did you mean a = a * 99 or a = a * a * 99? Consider replacing it with
51+
|
52+
11 | a *= 99;
53+
| ^^^^^^^
54+
help: or
55+
|
56+
11 | a = a * a * 99;
57+
| ^^^^^^^^^^^^^^
2658

2759
error: variable appears on both sides of an assignment operation
2860
--> $DIR/assign_ops2.rs:12:5
2961
|
3062
12 | a *= 42 * a;
31-
| ^^^^^^^^^^^ help: replace it with: `a *= 42`
63+
| ^^^^^^^^^^^
64+
help: Did you mean a = a * 42 or a = a * 42 * a? Consider replacing it with
65+
|
66+
12 | a *= 42;
67+
| ^^^^^^^
68+
help: or
69+
|
70+
12 | a = a * 42 * a;
71+
| ^^^^^^^^^^^^^^
3272

3373
error: variable appears on both sides of an assignment operation
3474
--> $DIR/assign_ops2.rs:13:5
3575
|
3676
13 | a /= a / 2;
37-
| ^^^^^^^^^^ help: replace it with: `a /= 2`
77+
| ^^^^^^^^^^
78+
help: Did you mean a = a / 2 or a = a / (a / 2)? Consider replacing it with
79+
|
80+
13 | a /= 2;
81+
| ^^^^^^
82+
help: or
83+
|
84+
13 | a = a / (a / 2);
85+
| ^^^^^^^^^^^^^^^
3886

3987
error: variable appears on both sides of an assignment operation
4088
--> $DIR/assign_ops2.rs:14:5
4189
|
4290
14 | a %= a % 5;
43-
| ^^^^^^^^^^ help: replace it with: `a %= 5`
91+
| ^^^^^^^^^^
92+
help: Did you mean a = a % 5 or a = a % (a % 5)? Consider replacing it with
93+
|
94+
14 | a %= 5;
95+
| ^^^^^^
96+
help: or
97+
|
98+
14 | a = a % (a % 5);
99+
| ^^^^^^^^^^^^^^^
44100

45101
error: variable appears on both sides of an assignment operation
46102
--> $DIR/assign_ops2.rs:15:5
47103
|
48104
15 | a &= a & 1;
49-
| ^^^^^^^^^^ help: replace it with: `a &= 1`
105+
| ^^^^^^^^^^
106+
help: Did you mean a = a & 1 or a = a & a & 1? Consider replacing it with
107+
|
108+
15 | a &= 1;
109+
| ^^^^^^
110+
help: or
111+
|
112+
15 | a = a & a & 1;
113+
| ^^^^^^^^^^^^^
114+
115+
error: variable appears on both sides of an assignment operation
116+
--> $DIR/assign_ops2.rs:16:5
117+
|
118+
16 | a *= a * a;
119+
| ^^^^^^^^^^
120+
help: Did you mean a = a * a or a = a * a * a? Consider replacing it with
121+
|
122+
16 | a *= a;
123+
| ^^^^^^
124+
help: or
125+
|
126+
16 | a = a * a * a;
127+
| ^^^^^^^^^^^^^
50128

51-
error: aborting due to 8 previous errors
129+
error: aborting due to 9 previous errors
52130

0 commit comments

Comments
 (0)