From aca64b8df77670c6661b7174d7edc16133442162 Mon Sep 17 00:00:00 2001
From: Shotaro Yamada <sinkuu@sinkuu.xyz>
Date: Fri, 13 Mar 2020 01:25:18 +0900
Subject: [PATCH] Check for mutation

---
 clippy_lints/src/redundant_clone.rs | 25 ++++++++++++++++---------
 tests/ui/redundant_clone.fixed      |  5 +++++
 tests/ui/redundant_clone.rs         |  5 +++++
 tests/ui/redundant_clone.stderr     |  2 +-
 4 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs
index 7c1a70408465..624fe227ca3d 100644
--- a/clippy_lints/src/redundant_clone.rs
+++ b/clippy_lints/src/redundant_clone.rs
@@ -195,8 +195,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
             let is_temp = mir_read_only.local_kind(ret_local) == mir::LocalKind::Temp;
 
             // 1. `local` can be moved out if it is not used later.
-            // 2. If `ret_local` is a temporary and is not consumed, we can remove this `clone` call anyway.
-            let (used, consumed) = traversal::ReversePostorder::new(&mir, bb).skip(1).fold(
+            // 2. If `ret_local` is a temporary and is neither consumed nor mutated, we can remove this `clone`
+            // call anyway.
+            let (used, consumed_or_mutated) = traversal::ReversePostorder::new(&mir, bb).skip(1).fold(
                 (false, !is_temp),
                 |(used, consumed), (tbb, tdata)| {
                     // Short-circuit
@@ -209,14 +210,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
 
                     let mut vis = LocalUseVisitor {
                         used: (local, false),
-                        consumed: (ret_local, false),
+                        consumed_or_mutated: (ret_local, false),
                     };
                     vis.visit_basic_block_data(tbb, tdata);
-                    (used || vis.used.1, consumed || vis.consumed.1)
+                    (used || vis.used.1, consumed || vis.consumed_or_mutated.1)
                 },
             );
 
-            if !used || !consumed {
+            if !used || !consumed_or_mutated {
                 let span = terminator.source_info.span;
                 let scope = terminator.source_info.scope;
                 let node = mir.source_scopes[scope]
@@ -253,7 +254,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
                             if used {
                                 db.span_note(
                                     span,
-                                    "cloned value is not consumed",
+                                    "cloned value is neither consumed nor mutated",
                                 );
                             } else {
                                 db.span_note(
@@ -355,7 +356,7 @@ fn base_local_and_movability<'tcx>(
 
 struct LocalUseVisitor {
     used: (mir::Local, bool),
-    consumed: (mir::Local, bool),
+    consumed_or_mutated: (mir::Local, bool),
 }
 
 impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
@@ -381,8 +382,14 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
             self.used.1 = true;
         }
 
-        if *local == self.consumed.0 && matches!(ctx, PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)) {
-            self.consumed.1 = true;
+        if *local == self.consumed_or_mutated.0 {
+            match ctx {
+                PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)
+                | PlaceContext::MutatingUse(MutatingUseContext::Borrow) => {
+                    self.consumed_or_mutated.1 = true;
+                },
+                _ => {},
+            }
         }
     }
 }
diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed
index 99d97f9cc3ce..17734b04aba4 100644
--- a/tests/ui/redundant_clone.fixed
+++ b/tests/ui/redundant_clone.fixed
@@ -145,4 +145,9 @@ fn not_consumed() {
     // redundant. (It also does not consume the PathBuf)
 
     println!("x: {:?}, y: {:?}", x, y);
+
+    let mut s = String::new();
+    s.clone().push_str("foo"); // OK, removing this `clone()` will change the behavior.
+    s.push_str("bar");
+    assert_eq!(s, "bar");
 }
diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs
index 58cd05c67cf0..aee6855eea94 100644
--- a/tests/ui/redundant_clone.rs
+++ b/tests/ui/redundant_clone.rs
@@ -145,4 +145,9 @@ fn not_consumed() {
     // redundant. (It also does not consume the PathBuf)
 
     println!("x: {:?}, y: {:?}", x, y);
+
+    let mut s = String::new();
+    s.clone().push_str("foo"); // OK, removing this `clone()` will change the behavior.
+    s.push_str("bar");
+    assert_eq!(s, "bar");
 }
diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr
index 04fe74e9175b..9c27812b9cdc 100644
--- a/tests/ui/redundant_clone.stderr
+++ b/tests/ui/redundant_clone.stderr
@@ -161,7 +161,7 @@ error: redundant clone
 LL |     let y = x.clone().join("matthias");
    |              ^^^^^^^^ help: remove this
    |
-note: cloned value is not consumed
+note: cloned value is neither consumed nor mutated
   --> $DIR/redundant_clone.rs:143:13
    |
 LL |     let y = x.clone().join("matthias");