diff --git a/clippy_lints/src/operators/assign_op_pattern.rs b/clippy_lints/src/operators/assign_op_pattern.rs index 979e0a66707..945a09a647c 100644 --- a/clippy_lints/src/operators/assign_op_pattern.rs +++ b/clippy_lints/src/operators/assign_op_pattern.rs @@ -8,6 +8,10 @@ use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::intravisit::{walk_expr, Visitor}; use rustc_lint::LateContext; +use rustc_middle::mir::FakeReadCause; +use rustc_middle::ty::BorrowKind; +use rustc_trait_selection::infer::TyCtxtInferExt; +use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; use super::ASSIGN_OP_PATTERN; @@ -29,6 +33,16 @@ pub(super) fn check<'tcx>( .map_or(true, |t| t.path.res.def_id() != trait_id); if implements_trait(cx, ty, trait_id, &[rty.into()]); then { + // Primitive types execute assign-ops right-to-left. Every other type is left-to-right. + if !(ty.is_primitive() && rty.is_primitive()) { + // TODO: This will have false negatives as it doesn't check if the borrows are + // actually live at the end of their respective expressions. + let mut_borrows = mut_borrows_in_expr(cx, assignee); + let imm_borrows = imm_borrows_in_expr(cx, rhs); + if mut_borrows.iter().any(|id| imm_borrows.contains(id)) { + return; + } + } span_lint_and_then( cx, ASSIGN_OP_PATTERN, @@ -99,3 +113,69 @@ impl<'a, 'tcx> Visitor<'tcx> for ExprVisitor<'a, 'tcx> { walk_expr(self, expr); } } + +fn imm_borrows_in_expr(cx: &LateContext<'_>, e: &hir::Expr<'_>) -> hir::HirIdSet { + struct S(hir::HirIdSet); + impl Delegate<'_> for S { + fn borrow(&mut self, place: &PlaceWithHirId<'_>, _: hir::HirId, kind: BorrowKind) { + if matches!(kind, BorrowKind::ImmBorrow | BorrowKind::UniqueImmBorrow) { + self.0.insert(match place.place.base { + PlaceBase::Local(id) => id, + PlaceBase::Upvar(id) => id.var_path.hir_id, + _ => return, + }); + } + } + + fn consume(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {} + fn mutate(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {} + fn fake_read(&mut self, _: &PlaceWithHirId<'_>, _: FakeReadCause, _: hir::HirId) {} + fn copy(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {} + } + + let mut s = S(hir::HirIdSet::default()); + cx.tcx.infer_ctxt().enter(|infcx| { + let mut v = ExprUseVisitor::new( + &mut s, + &infcx, + cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap()), + cx.param_env, + cx.typeck_results(), + ); + v.consume_expr(e); + }); + s.0 +} + +fn mut_borrows_in_expr(cx: &LateContext<'_>, e: &hir::Expr<'_>) -> hir::HirIdSet { + struct S(hir::HirIdSet); + impl Delegate<'_> for S { + fn borrow(&mut self, place: &PlaceWithHirId<'_>, _: hir::HirId, kind: BorrowKind) { + if matches!(kind, BorrowKind::MutBorrow) { + self.0.insert(match place.place.base { + PlaceBase::Local(id) => id, + PlaceBase::Upvar(id) => id.var_path.hir_id, + _ => return, + }); + } + } + + fn consume(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {} + fn mutate(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {} + fn fake_read(&mut self, _: &PlaceWithHirId<'_>, _: FakeReadCause, _: hir::HirId) {} + fn copy(&mut self, _: &PlaceWithHirId<'_>, _: hir::HirId) {} + } + + let mut s = S(hir::HirIdSet::default()); + cx.tcx.infer_ctxt().enter(|infcx| { + let mut v = ExprUseVisitor::new( + &mut s, + &infcx, + cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap()), + cx.param_env, + cx.typeck_results(), + ); + v.consume_expr(e); + }); + s.0 +} diff --git a/tests/ui/assign_ops.fixed b/tests/ui/assign_ops.fixed index 52b1b3afe16..da034b51cfd 100644 --- a/tests/ui/assign_ops.fixed +++ b/tests/ui/assign_ops.fixed @@ -1,5 +1,7 @@ // run-rustfix +use core::num::Wrapping; + #[allow(dead_code, unused_assignments)] #[warn(clippy::assign_op_pattern)] fn main() { @@ -18,4 +20,13 @@ fn main() { a = 6 << a; let mut s = String::new(); s += "bla"; + + // Issue #9180 + let mut a = Wrapping(0u32); + a += Wrapping(1u32); + let mut v = vec![0u32, 1u32]; + v[0] += v[1]; + let mut v = vec![Wrapping(0u32), Wrapping(1u32)]; + v[0] = v[0] + v[1]; + let _ = || v[0] = v[0] + v[1]; } diff --git a/tests/ui/assign_ops.rs b/tests/ui/assign_ops.rs index 527a46b2c2b..337bb02c8a6 100644 --- a/tests/ui/assign_ops.rs +++ b/tests/ui/assign_ops.rs @@ -1,5 +1,7 @@ // run-rustfix +use core::num::Wrapping; + #[allow(dead_code, unused_assignments)] #[warn(clippy::assign_op_pattern)] fn main() { @@ -18,4 +20,13 @@ fn main() { a = 6 << a; let mut s = String::new(); s = s + "bla"; + + // Issue #9180 + let mut a = Wrapping(0u32); + a = a + Wrapping(1u32); + let mut v = vec![0u32, 1u32]; + v[0] = v[0] + v[1]; + let mut v = vec![Wrapping(0u32), Wrapping(1u32)]; + v[0] = v[0] + v[1]; + let _ = || v[0] = v[0] + v[1]; } diff --git a/tests/ui/assign_ops.stderr b/tests/ui/assign_ops.stderr index 3486bd8da4d..63a938ab4b4 100644 --- a/tests/ui/assign_ops.stderr +++ b/tests/ui/assign_ops.stderr @@ -1,5 +1,5 @@ error: manual implementation of an assign operation - --> $DIR/assign_ops.rs:7:5 + --> $DIR/assign_ops.rs:9:5 | LL | a = a + 1; | ^^^^^^^^^ help: replace it with: `a += 1` @@ -7,52 +7,64 @@ LL | a = a + 1; = note: `-D clippy::assign-op-pattern` implied by `-D warnings` error: manual implementation of an assign operation - --> $DIR/assign_ops.rs:8:5 + --> $DIR/assign_ops.rs:10:5 | LL | a = 1 + a; | ^^^^^^^^^ help: replace it with: `a += 1` error: manual implementation of an assign operation - --> $DIR/assign_ops.rs:9:5 + --> $DIR/assign_ops.rs:11:5 | LL | a = a - 1; | ^^^^^^^^^ help: replace it with: `a -= 1` error: manual implementation of an assign operation - --> $DIR/assign_ops.rs:10:5 + --> $DIR/assign_ops.rs:12:5 | LL | a = a * 99; | ^^^^^^^^^^ help: replace it with: `a *= 99` error: manual implementation of an assign operation - --> $DIR/assign_ops.rs:11:5 + --> $DIR/assign_ops.rs:13:5 | LL | a = 42 * a; | ^^^^^^^^^^ help: replace it with: `a *= 42` error: manual implementation of an assign operation - --> $DIR/assign_ops.rs:12:5 + --> $DIR/assign_ops.rs:14:5 | LL | a = a / 2; | ^^^^^^^^^ help: replace it with: `a /= 2` error: manual implementation of an assign operation - --> $DIR/assign_ops.rs:13:5 + --> $DIR/assign_ops.rs:15:5 | LL | a = a % 5; | ^^^^^^^^^ help: replace it with: `a %= 5` error: manual implementation of an assign operation - --> $DIR/assign_ops.rs:14:5 + --> $DIR/assign_ops.rs:16:5 | LL | a = a & 1; | ^^^^^^^^^ help: replace it with: `a &= 1` error: manual implementation of an assign operation - --> $DIR/assign_ops.rs:20:5 + --> $DIR/assign_ops.rs:22:5 | LL | s = s + "bla"; | ^^^^^^^^^^^^^ help: replace it with: `s += "bla"` -error: aborting due to 9 previous errors +error: manual implementation of an assign operation + --> $DIR/assign_ops.rs:26:5 + | +LL | a = a + Wrapping(1u32); + | ^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a += Wrapping(1u32)` + +error: manual implementation of an assign operation + --> $DIR/assign_ops.rs:28:5 + | +LL | v[0] = v[0] + v[1]; + | ^^^^^^^^^^^^^^^^^^ help: replace it with: `v[0] += v[1]` + +error: aborting due to 11 previous errors