Auto merge of rust-lang#5304 - sinkuu:redundant_clone_not_consumed, r=flip1995

bors · bors · commit 8485d40a3264 · 2020-03-12T20:22:31.000Z
Extend `redundant_clone` to the case that cloned value is not consumed Fixes rust-lang#5303. --- changelog: Extend `redundant_clone` to the case that cloned value is not consumed
diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs
@@ -6,7 +6,7 @@ use if_chain::if_chain;
 use matches::matches;
 use rustc::mir::{
     self, traversal,
-    visit::{MutatingUseContext, PlaceContext, Visitor as _},
+    visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor as _},
 };
 use rustc::ty::{self, fold::TypeVisitor, Ty};
 use rustc_data_structures::{fx::FxHashMap, transitive_relation::TransitiveRelation};
@@ -110,7 +110,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
                 continue;
             }
 
-            let (fn_def_id, arg, arg_ty, _) = unwrap_or_continue!(is_call_with_ref_arg(cx, mir, &terminator.kind));
+            let (fn_def_id, arg, arg_ty, clone_ret) =
+                unwrap_or_continue!(is_call_with_ref_arg(cx, mir, &terminator.kind));
 
             let from_borrow = match_def_path(cx, fn_def_id, &paths::CLONE_TRAIT_METHOD)
                 || match_def_path(cx, fn_def_id, &paths::TO_OWNED_METHOD)
@@ -132,16 +133,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
                 statement_index: bbdata.statements.len(),
             };
 
-            // Cloned local
-            let local = if from_borrow {
+            // `Local` to be cloned, and a local of `clone` call's destination
+            let (local, ret_local) = if from_borrow {
                 // `res = clone(arg)` can be turned into `res = move arg;`
                 // if `arg` is the only borrow of `cloned` at this point.
 
                 if cannot_move_out || !possible_borrower.only_borrowers(&[arg], cloned, loc) {
                     continue;
                 }
 
-                cloned
+                (cloned, clone_ret)
             } else {
                 // `arg` is a reference as it is `.deref()`ed in the previous block.
                 // Look into the predecessor block and find out the source of deref.
@@ -153,15 +154,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
                 let pred_terminator = mir[ps[0]].terminator();
 
                 // receiver of the `deref()` call
-                let pred_arg = if_chain! {
-                    if let Some((pred_fn_def_id, pred_arg, pred_arg_ty, Some(res))) =
+                let (pred_arg, deref_clone_ret) = if_chain! {
+                    if let Some((pred_fn_def_id, pred_arg, pred_arg_ty, res)) =
                         is_call_with_ref_arg(cx, mir, &pred_terminator.kind);
-                    if res.local == cloned;
+                    if res == cloned;
                     if match_def_path(cx, pred_fn_def_id, &paths::DEREF_TRAIT_METHOD);
                     if match_type(cx, pred_arg_ty, &paths::PATH_BUF)
                         || match_type(cx, pred_arg_ty, &paths::OS_STRING);
                     then {
-                        pred_arg
+                        (pred_arg, res)
                     } else {
                         continue;
                     }
@@ -188,25 +189,35 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
                     continue;
                 }
 
-                local
+                (local, deref_clone_ret)
             };
 
-            // `local` cannot be moved out if it is used later
-            let used_later = traversal::ReversePostorder::new(&mir, bb).skip(1).any(|(tbb, tdata)| {
-                // Give up on loops
-                if tdata.terminator().successors().any(|s| *s == bb) {
-                    return true;
-                }
+            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 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
+                    if (used && consumed) ||
+                        // Give up on loops
+                        tdata.terminator().successors().any(|s| *s == bb)
+                    {
+                        return (true, true);
+                    }
 
-                let mut vis = LocalUseVisitor {
-                    local,
-                    used_other_than_drop: false,
-                };
-                vis.visit_basic_block_data(tbb, tdata);
-                vis.used_other_than_drop
-            });
+                    let mut vis = LocalUseVisitor {
+                        used: (local, false),
+                        consumed_or_mutated: (ret_local, false),
+                    };
+                    vis.visit_basic_block_data(tbb, tdata);
+                    (used || vis.used.1, consumed || vis.consumed_or_mutated.1)
+                },
+            );
 
-            if !used_later {
+            if !used || !consumed_or_mutated {
                 let span = terminator.source_info.span;
                 let scope = terminator.source_info.scope;
                 let node = mir.source_scopes[scope]
@@ -240,10 +251,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
                                 String::new(),
                                 app,
                             );
-                            db.span_note(
-                                span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())),
-                                "this value is dropped without further use",
-                            );
+                            if used {
+                                db.span_note(
+                                    span,
+                                    "cloned value is neither consumed nor mutated",
+                                );
+                            } else {
+                                db.span_note(
+                                    span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())),
+                                    "this value is dropped without further use",
+                                );
+                            }
                         });
                     } else {
                         span_lint_hir(cx, REDUNDANT_CLONE, node, span, "redundant clone");
@@ -259,7 +277,7 @@ fn is_call_with_ref_arg<'tcx>(
     cx: &LateContext<'_, 'tcx>,
     mir: &'tcx mir::Body<'tcx>,
     kind: &'tcx mir::TerminatorKind<'tcx>,
-) -> Option<(def_id::DefId, mir::Local, Ty<'tcx>, Option<&'tcx mir::Place<'tcx>>)> {
+) -> Option<(def_id::DefId, mir::Local, Ty<'tcx>, mir::Local)> {
     if_chain! {
         if let mir::TerminatorKind::Call { func, args, destination, .. } = kind;
         if args.len() == 1;
@@ -268,7 +286,7 @@ fn is_call_with_ref_arg<'tcx>(
         if let (inner_ty, 1) = walk_ptrs_ty_depth(args[0].ty(&*mir, cx.tcx));
         if !is_copy(cx, inner_ty);
         then {
-            Some((def_id, *local, inner_ty, destination.as_ref().map(|(dest, _)| dest)))
+            Some((def_id, *local, inner_ty, destination.as_ref().map(|(dest, _)| dest)?.as_local()?))
         } else {
             None
         }
@@ -337,20 +355,15 @@ fn base_local_and_movability<'tcx>(
 }
 
 struct LocalUseVisitor {
-    local: mir::Local,
-    used_other_than_drop: bool,
+    used: (mir::Local, bool),
+    consumed_or_mutated: (mir::Local, bool),
 }
 
 impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
     fn visit_basic_block_data(&mut self, block: mir::BasicBlock, data: &mir::BasicBlockData<'tcx>) {
         let statements = &data.statements;
         for (statement_index, statement) in statements.iter().enumerate() {
             self.visit_statement(statement, mir::Location { block, statement_index });
-
-            // Once flagged, skip remaining statements
-            if self.used_other_than_drop {
-                return;
-            }
         }
 
         self.visit_terminator(
@@ -362,14 +375,23 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
         );
     }
 
-    fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext, _: mir::Location) {
-        match ctx {
-            PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_) => return,
-            _ => {},
+    fn visit_place(&mut self, place: &mir::Place<'tcx>, ctx: PlaceContext, _: mir::Location) {
+        let local = place.local;
+
+        if local == self.used.0
+            && !matches!(ctx, PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_))
+        {
+            self.used.1 = true;
         }
 
-        if *local == self.local {
-            self.used_other_than_drop = 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/format.fixed b/tests/ui/format.fixed
@@ -1,6 +1,6 @@
 // run-rustfix
 
-#![allow(clippy::print_literal)]
+#![allow(clippy::print_literal, clippy::redundant_clone)]
 #![warn(clippy::useless_format)]
 
 struct Foo(pub String);
diff --git a/tests/ui/format.rs b/tests/ui/format.rs
@@ -1,6 +1,6 @@
 // run-rustfix
 
-#![allow(clippy::print_literal)]
+#![allow(clippy::print_literal, clippy::redundant_clone)]
 #![warn(clippy::useless_format)]
 
 struct Foo(pub String);
diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed
@@ -50,6 +50,7 @@ fn main() {
     cannot_double_move(Alpha);
     cannot_move_from_type_with_drop();
     borrower_propagation();
+    not_consumed();
 }
 
 #[derive(Clone)]
@@ -136,3 +137,26 @@ fn borrower_propagation() {
         let _f = f.clone(); // ok
     }
 }
+
+fn not_consumed() {
+    let x = std::path::PathBuf::from("home");
+    let y = x.join("matthias");
+    // join() creates a new owned PathBuf, does not take a &mut to x variable, thus the .clone() is
+    // 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");
+
+    let t = Some(s);
+    // OK
+    if let Some(x) = t.clone() {
+        println!("{}", x);
+    }
+    if let Some(x) = t {
+        println!("{}", x);
+    }
+}
diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs
@@ -50,6 +50,7 @@ fn main() {
     cannot_double_move(Alpha);
     cannot_move_from_type_with_drop();
     borrower_propagation();
+    not_consumed();
 }
 
 #[derive(Clone)]
@@ -136,3 +137,26 @@ fn borrower_propagation() {
         let _f = f.clone(); // ok
     }
 }
+
+fn not_consumed() {
+    let x = std::path::PathBuf::from("home");
+    let y = x.clone().join("matthias");
+    // join() creates a new owned PathBuf, does not take a &mut to x variable, thus the .clone() is
+    // 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");
+
+    let t = Some(s);
+    // OK
+    if let Some(x) = t.clone() {
+        println!("{}", x);
+    }
+    if let Some(x) = t {
+        println!("{}", x);
+    }
+}
diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr
@@ -108,52 +108,64 @@ LL |     let _t = tup.0.clone();
    |              ^^^^^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:59:22
+  --> $DIR/redundant_clone.rs:60:22
    |
 LL |         (a.clone(), a.clone())
    |                      ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:59:21
+  --> $DIR/redundant_clone.rs:60:21
    |
 LL |         (a.clone(), a.clone())
    |                     ^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:119:15
+  --> $DIR/redundant_clone.rs:120:15
    |
 LL |     let _s = s.clone();
    |               ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:119:14
+  --> $DIR/redundant_clone.rs:120:14
    |
 LL |     let _s = s.clone();
    |              ^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:120:15
+  --> $DIR/redundant_clone.rs:121:15
    |
 LL |     let _t = t.clone();
    |               ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:120:14
+  --> $DIR/redundant_clone.rs:121:14
    |
 LL |     let _t = t.clone();
    |              ^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:130:19
+  --> $DIR/redundant_clone.rs:131:19
    |
 LL |         let _f = f.clone();
    |                   ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:130:18
+  --> $DIR/redundant_clone.rs:131:18
    |
 LL |         let _f = f.clone();
    |                  ^
 
-error: aborting due to 13 previous errors
+error: redundant clone
+  --> $DIR/redundant_clone.rs:143:14
+   |
+LL |     let y = x.clone().join("matthias");
+   |              ^^^^^^^^ help: remove this
+   |
+note: cloned value is neither consumed nor mutated
+  --> $DIR/redundant_clone.rs:143:13
+   |
+LL |     let y = x.clone().join("matthias");
+   |             ^^^^^^^^^
+
+error: aborting due to 14 previous errors
 
