Skip to content

Commit 05a51e5

Browse files
borsbors
committed
Auto merge of rust-lang#9214 - Jarcho:assign_op_prim, r=Manishearth
Check `assign_op_pattern` for conflicting borrows fixes rust-lang#9180 changelog: [`assign_op_pattern`](https://rust-lang.github.io/rust-clippy/master/#assign_op_pattern): Don't lint when the suggestion would cause borrowck errors.
2 parents fa3c293 + a2f9b93 commit 05a51e5

File tree

4 files changed

+124
-10
lines changed

4 files changed

+124
-10
lines changed

‎clippy_lints/src/operators/assign_op_pattern.rs

+80
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@ use rustc_errors::Applicability;
88
use rustc_hir as hir;
99
use rustc_hir::intravisit::{walk_expr, Visitor};
1010
use rustc_lint::LateContext;
11+
use rustc_middle::mir::FakeReadCause;
12+
use rustc_middle::ty::BorrowKind;
13+
use rustc_trait_selection::infer::TyCtxtInferExt;
14+
use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
1115

1216
use super::ASSIGN_OP_PATTERN;
1317

@@ -29,6 +33,16 @@ pub(super) fn check<'tcx>(
2933
.map_or(true, |t| t.path.res.def_id() != trait_id);
3034
if implements_trait(cx, ty, trait_id, &[rty.into()]);
3135
then {
36+
// Primitive types execute assign-ops right-to-left. Every other type is left-to-right.
37+
if !(ty.is_primitive() && rty.is_primitive()) {
38+
// TODO: This will have false negatives as it doesn't check if the borrows are
39+
// actually live at the end of their respective expressions.
40+
let mut_borrows = mut_borrows_in_expr(cx, assignee);
41+
let imm_borrows = imm_borrows_in_expr(cx, rhs);
42+
if mut_borrows.iter().any(|id| imm_borrows.contains(id)) {
43+
return;
44+
}
45+
}
3246
span_lint_and_then(
3347
cx,
3448
ASSIGN_OP_PATTERN,
@@ -99,3 +113,69 @@ impl<'a, 'tcx> Visitor<'tcx> for ExprVisitor<'a, 'tcx> {
99113
walk_expr(self, expr);
100114
}
101115
}
116+
117+
fn imm_borrows_in_expr(cx: &LateContext<'_>, e: &hir::Expr<'_>) -> hir::HirIdSet {
118+
struct S(hir::HirIdSet);
119+
impl Delegate<'_> for S {
120+
fn borrow(&mut self, place: &PlaceWithHirId<'_>, _: hir::HirId, kind: BorrowKind) {
121+
if matches!(kind, BorrowKind::ImmBorrow | BorrowKind::UniqueImmBorrow) {
122+
self.0.insert(match place.place.base {
123+
PlaceBase::Local(id) => id,
124+
PlaceBase::Upvar(id) => id.var_path.hir_id,
125+
_ => return,
126+
});
127+
}
128+
}
129+
130+
fn consume(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {}
131+
fn mutate(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {}
132+
fn fake_read(&mut self, _: &PlaceWithHirId<'_>, _: FakeReadCause, _: hir::HirId) {}
133+
fn copy(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {}
134+
}
135+
136+
let mut s = S(hir::HirIdSet::default());
137+
cx.tcx.infer_ctxt().enter(|infcx| {
138+
let mut v = ExprUseVisitor::new(
139+
&mut s,
140+
&infcx,
141+
cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap()),
142+
cx.param_env,
143+
cx.typeck_results(),
144+
);
145+
v.consume_expr(e);
146+
});
147+
s.0
148+
}
149+
150+
fn mut_borrows_in_expr(cx: &LateContext<'_>, e: &hir::Expr<'_>) -> hir::HirIdSet {
151+
struct S(hir::HirIdSet);
152+
impl Delegate<'_> for S {
153+
fn borrow(&mut self, place: &PlaceWithHirId<'_>, _: hir::HirId, kind: BorrowKind) {
154+
if matches!(kind, BorrowKind::MutBorrow) {
155+
self.0.insert(match place.place.base {
156+
PlaceBase::Local(id) => id,
157+
PlaceBase::Upvar(id) => id.var_path.hir_id,
158+
_ => return,
159+
});
160+
}
161+
}
162+
163+
fn consume(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {}
164+
fn mutate(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {}
165+
fn fake_read(&mut self, _: &PlaceWithHirId<'_>, _: FakeReadCause, _: hir::HirId) {}
166+
fn copy(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {}
167+
}
168+
169+
let mut s = S(hir::HirIdSet::default());
170+
cx.tcx.infer_ctxt().enter(|infcx| {
171+
let mut v = ExprUseVisitor::new(
172+
&mut s,
173+
&infcx,
174+
cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap()),
175+
cx.param_env,
176+
cx.typeck_results(),
177+
);
178+
v.consume_expr(e);
179+
});
180+
s.0
181+
}

‎tests/ui/assign_ops.fixed

+11
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// run-rustfix
22

3+
use core::num::Wrapping;
4+
35
#[allow(dead_code, unused_assignments)]
46
#[warn(clippy::assign_op_pattern)]
57
fn main() {
@@ -18,4 +20,13 @@ fn main() {
1820
a = 6 << a;
1921
let mut s = String::new();
2022
s += "bla";
23+
24+
// Issue #9180
25+
let mut a = Wrapping(0u32);
26+
a += Wrapping(1u32);
27+
let mut v = vec![0u32, 1u32];
28+
v[0] += v[1];
29+
let mut v = vec![Wrapping(0u32), Wrapping(1u32)];
30+
v[0] = v[0] + v[1];
31+
let _ = || v[0] = v[0] + v[1];
2132
}

‎tests/ui/assign_ops.rs

+11
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// run-rustfix
22

3+
use core::num::Wrapping;
4+
35
#[allow(dead_code, unused_assignments)]
46
#[warn(clippy::assign_op_pattern)]
57
fn main() {
@@ -18,4 +20,13 @@ fn main() {
1820
a = 6 << a;
1921
let mut s = String::new();
2022
s = s + "bla";
23+
24+
// Issue #9180
25+
let mut a = Wrapping(0u32);
26+
a = a + Wrapping(1u32);
27+
let mut v = vec![0u32, 1u32];
28+
v[0] = v[0] + v[1];
29+
let mut v = vec![Wrapping(0u32), Wrapping(1u32)];
30+
v[0] = v[0] + v[1];
31+
let _ = || v[0] = v[0] + v[1];
2132
}

‎tests/ui/assign_ops.stderr

+22-10
Original file line numberDiff line numberDiff line change
@@ -1,58 +1,70 @@
11
error: manual implementation of an assign operation
2-
--> $DIR/assign_ops.rs:7:5
2+
--> $DIR/assign_ops.rs:9:5
33
|
44
LL | a = a + 1;
55
| ^^^^^^^^^ help: replace it with: `a += 1`
66
|
77
= note: `-D clippy::assign-op-pattern` implied by `-D warnings`
88

99
error: manual implementation of an assign operation
10-
--> $DIR/assign_ops.rs:8:5
10+
--> $DIR/assign_ops.rs:10:5
1111
|
1212
LL | a = 1 + a;
1313
| ^^^^^^^^^ help: replace it with: `a += 1`
1414

1515
error: manual implementation of an assign operation
16-
--> $DIR/assign_ops.rs:9:5
16+
--> $DIR/assign_ops.rs:11:5
1717
|
1818
LL | a = a - 1;
1919
| ^^^^^^^^^ help: replace it with: `a -= 1`
2020

2121
error: manual implementation of an assign operation
22-
--> $DIR/assign_ops.rs:10:5
22+
--> $DIR/assign_ops.rs:12:5
2323
|
2424
LL | a = a * 99;
2525
| ^^^^^^^^^^ help: replace it with: `a *= 99`
2626

2727
error: manual implementation of an assign operation
28-
--> $DIR/assign_ops.rs:11:5
28+
--> $DIR/assign_ops.rs:13:5
2929
|
3030
LL | a = 42 * a;
3131
| ^^^^^^^^^^ help: replace it with: `a *= 42`
3232

3333
error: manual implementation of an assign operation
34-
--> $DIR/assign_ops.rs:12:5
34+
--> $DIR/assign_ops.rs:14:5
3535
|
3636
LL | a = a / 2;
3737
| ^^^^^^^^^ help: replace it with: `a /= 2`
3838

3939
error: manual implementation of an assign operation
40-
--> $DIR/assign_ops.rs:13:5
40+
--> $DIR/assign_ops.rs:15:5
4141
|
4242
LL | a = a % 5;
4343
| ^^^^^^^^^ help: replace it with: `a %= 5`
4444

4545
error: manual implementation of an assign operation
46-
--> $DIR/assign_ops.rs:14:5
46+
--> $DIR/assign_ops.rs:16:5
4747
|
4848
LL | a = a & 1;
4949
| ^^^^^^^^^ help: replace it with: `a &= 1`
5050

5151
error: manual implementation of an assign operation
52-
--> $DIR/assign_ops.rs:20:5
52+
--> $DIR/assign_ops.rs:22:5
5353
|
5454
LL | s = s + "bla";
5555
| ^^^^^^^^^^^^^ help: replace it with: `s += "bla"`
5656

57-
error: aborting due to 9 previous errors
57+
error: manual implementation of an assign operation
58+
--> $DIR/assign_ops.rs:26:5
59+
|
60+
LL | a = a + Wrapping(1u32);
61+
| ^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a += Wrapping(1u32)`
62+
63+
error: manual implementation of an assign operation
64+
--> $DIR/assign_ops.rs:28:5
65+
|
66+
LL | v[0] = v[0] + v[1];
67+
| ^^^^^^^^^^^^^^^^^^ help: replace it with: `v[0] += v[1]`
68+
69+
error: aborting due to 11 previous errors
5870

0 commit comments

Comments
 (0)