Auto merge of #9214 - Jarcho:assign_op_prim, r=Manishearth

Check `assign_op_pattern` for conflicting borrows

fixes #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.

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
+}

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];
 }

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];
 }

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