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

Extend `redundant_clone` to the case that cloned value is not consumed Fixes #5303. --- changelog: Extend `redundant_clone` to the case that cloned value is not consumed
2020-03-12 20:22:31 +00:00 · 2020-03-12 20:22:31 +00:00 · 8485d40a32
parent b064ea8096 d9ad33852c
commit 8485d40a32
6 changed files with 145 additions and 63 deletions
--- 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,8 +133,8 @@ 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.

@ -141,7 +142,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
                    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;

-                let mut vis = LocalUseVisitor {
-                    local,
-                    used_other_than_drop: false,
-                };
-                vis.visit_basic_block_data(tbb, tdata);
-                vis.used_other_than_drop
-            });
+            // 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);
+                    }

-            if !used_later {
+                    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 || !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,8 +355,8 @@ 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 {
@ -346,11 +364,6 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
        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;
+                },
+                _ => {},
+            }
        }
    }
 }
--- 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);
--- 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);
--- 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);
+    }
+}
--- 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);
+    }
+}
--- 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
-   |
-LL |     let _s = s.clone();
-   |               ^^^^^^^^ help: remove this
-   |
-note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:119:14
-   |
-LL |     let _s = s.clone();
-   |              ^
-
 error: redundant clone
  --> $DIR/redundant_clone.rs:120:15
   |
-LL |     let _t = t.clone();
+LL |     let _s = s.clone();
   |               ^^^^^^^^ help: remove this
   |
 note: this value is dropped without further use
  --> $DIR/redundant_clone.rs:120:14
   |
+LL |     let _s = s.clone();
+   |              ^
+
+error: redundant clone
+  --> $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: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