Auto merge of #3518 - sinkuu:redundant_clone_tw, r=phansch

bors · bors · commit ada0b2b0950b · 2018-12-10T18:55:49.000Z
Lint redundant clone of fields

Makes `redundant_clone` warn on unnecessary `foo.field.clone()` sometimes (it can detect an unnecessary clone only if the base of projection, `foo` in this case, is not used at all after that). This is enough for cases like `returns_tuple().0.clone()`.
diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs
@@ -332,7 +332,7 @@ fn check_clippy_lint_names(cx: &LateContext<'_, '_>, items: &[NestedMetaItem]) {
                     &format!("unknown clippy lint: clippy::{}", name),
                     |db| {
                         if name.as_str().chars().any(|c| c.is_uppercase()) {
-                            let name_lower = name.as_str().to_lowercase().to_string();
+                            let name_lower = name.as_str().to_lowercase();
                             match lint_store.check_lint_name(
                                 &name_lower,
                                 Some(tool_name.as_str())
diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs
@@ -57,7 +57,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
     }
     match expr.node {
         ExprKind::Lit(..) | ExprKind::Closure(.., _) => true,
-        ExprKind::Path(..) => !has_drop(cx, expr),
+        ExprKind::Path(..) => !has_drop(cx, cx.tables.expr_ty(expr)),
         ExprKind::Index(ref a, ref b) | ExprKind::Binary(_, ref a, ref b) => {
             has_no_effect(cx, a) && has_no_effect(cx, b)
         },
@@ -70,7 +70,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
         | ExprKind::AddrOf(_, ref inner)
         | ExprKind::Box(ref inner) => has_no_effect(cx, inner),
         ExprKind::Struct(_, ref fields, ref base) => {
-            !has_drop(cx, expr)
+            !has_drop(cx, cx.tables.expr_ty(expr))
                 && fields.iter().all(|field| has_no_effect(cx, &field.expr))
                 && match *base {
                     Some(ref base) => has_no_effect(cx, base),
@@ -82,7 +82,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
                 let def = cx.tables.qpath_def(qpath, callee.hir_id);
                 match def {
                     Def::Struct(..) | Def::Variant(..) | Def::StructCtor(..) | Def::VariantCtor(..) => {
-                        !has_drop(cx, expr) && args.iter().all(|arg| has_no_effect(cx, arg))
+                        !has_drop(cx, cx.tables.expr_ty(expr)) && args.iter().all(|arg| has_no_effect(cx, arg))
                     },
                     _ => false,
                 }
@@ -161,7 +161,7 @@ fn reduce_expression<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option<Vec
         | ExprKind::AddrOf(_, ref inner)
         | ExprKind::Box(ref inner) => reduce_expression(cx, inner).or_else(|| Some(vec![inner])),
         ExprKind::Struct(_, ref fields, ref base) => {
-            if has_drop(cx, expr) {
+            if has_drop(cx, cx.tables.expr_ty(expr)) {
                 None
             } else {
                 Some(fields.iter().map(|f| &f.expr).chain(base).map(Deref::deref).collect())
@@ -172,7 +172,7 @@ fn reduce_expression<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option<Vec
                 let def = cx.tables.qpath_def(qpath, callee.hir_id);
                 match def {
                     Def::Struct(..) | Def::Variant(..) | Def::StructCtor(..) | Def::VariantCtor(..)
-                        if !has_drop(cx, expr) =>
+                        if !has_drop(cx, cx.tables.expr_ty(expr)) =>
                     {
                         Some(args.iter().collect())
                     },
diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs
@@ -12,7 +12,7 @@ use crate::rustc::hir::{def_id, Body, FnDecl};
 use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use crate::rustc::mir::{
     self, traversal,
-    visit::{MutatingUseContext, NonUseContext, PlaceContext, Visitor},
+    visit::{MutatingUseContext, PlaceContext, Visitor},
     TerminatorKind,
 };
 use crate::rustc::ty;
@@ -23,10 +23,11 @@ use crate::syntax::{
     source_map::{BytePos, Span},
 };
 use crate::utils::{
-    in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint_node, span_lint_node_and_then,
-    walk_ptrs_ty_depth,
+    has_drop, in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint_node,
+    span_lint_node_and_then, walk_ptrs_ty_depth,
 };
 use if_chain::if_chain;
+use matches::matches;
 use std::convert::TryFrom;
 
 macro_rules! unwrap_or_continue {
@@ -126,7 +127,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
             // _1 in MIR `{ _2 = &_1; clone(move _2); }` or `{ _2 = _1; to_path_buf(_2); } (from_deref)
             // In case of `from_deref`, `arg` is already a reference since it is `deref`ed in the previous
             // block.
-            let cloned = unwrap_or_continue!(find_stmt_assigns_to(arg, from_borrow, bbdata.statements.iter().rev()));
+            let (cloned, cannot_move_out) = unwrap_or_continue!(find_stmt_assigns_to(
+                cx,
+                mir,
+                arg,
+                from_borrow,
+                bbdata.statements.iter()
+            ));
+
+            if from_borrow && cannot_move_out {
+                continue;
+            }
 
             // _1 in MIR `{ _2 = &_1; _3 = deref(move _2); } -> { _4 = _3; to_path_buf(move _4); }`
             let referent = if from_deref {
@@ -150,7 +161,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
                     }
                 };
 
-                unwrap_or_continue!(find_stmt_assigns_to(pred_arg, true, mir[ps[0]].statements.iter().rev()))
+                let (local, cannot_move_out) = unwrap_or_continue!(find_stmt_assigns_to(
+                    cx,
+                    mir,
+                    pred_arg,
+                    true,
+                    mir[ps[0]].statements.iter()
+                ));
+                if cannot_move_out {
+                    continue;
+                }
+                local
             } else {
                 cloned
             };
@@ -227,27 +248,69 @@ fn is_call_with_ref_arg<'tcx>(
     }
 }
 
-/// Finds the first `to = (&)from`, and returns `Some(from)`.
+type CannotMoveOut = bool;
+
+/// Finds the first `to = (&)from`, and returns
+/// ``Some((from, [`true` if `from` cannot be moved out]))``.
 fn find_stmt_assigns_to<'a, 'tcx: 'a>(
+    cx: &LateContext<'_, 'tcx>,
+    mir: &mir::Mir<'tcx>,
     to: mir::Local,
     by_ref: bool,
-    mut stmts: impl Iterator<Item = &'a mir::Statement<'tcx>>,
-) -> Option<mir::Local> {
-    stmts.find_map(|stmt| {
-        if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind {
-            if *local == to {
-                if by_ref {
-                    if let mir::Rvalue::Ref(_, _, mir::Place::Local(r)) = **v {
-                        return Some(r);
-                    }
-                } else if let mir::Rvalue::Use(mir::Operand::Copy(mir::Place::Local(r))) = **v {
-                    return Some(r);
+    stmts: impl DoubleEndedIterator<Item = &'a mir::Statement<'tcx>>,
+) -> Option<(mir::Local, CannotMoveOut)> {
+    stmts
+        .rev()
+        .find_map(|stmt| {
+            if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind {
+                if *local == to {
+                    return Some(v);
                 }
             }
-        }
 
-        None
-    })
+            None
+        })
+        .and_then(|v| {
+            if by_ref {
+                if let mir::Rvalue::Ref(_, _, ref place) = **v {
+                    return base_local_and_movability(cx, mir, place);
+                }
+            } else if let mir::Rvalue::Use(mir::Operand::Copy(ref place)) = **v {
+                return base_local_and_movability(cx, mir, place);
+            }
+            None
+        })
+}
+
+/// Extracts and returns the undermost base `Local` of given `place`. Returns `place` itself
+/// if it is already a `Local`.
+///
+/// Also reports whether given `place` cannot be moved out.
+fn base_local_and_movability<'tcx>(
+    cx: &LateContext<'_, 'tcx>,
+    mir: &mir::Mir<'tcx>,
+    mut place: &mir::Place<'tcx>,
+) -> Option<(mir::Local, CannotMoveOut)> {
+    use rustc::mir::Place::*;
+
+    // Dereference. You cannot move things out from a borrowed value.
+    let mut deref = false;
+    // Accessing a field of an ADT that has `Drop`. Moving the field out will cause E0509.
+    let mut field = false;
+
+    loop {
+        match place {
+            Local(local) => return Some((*local, deref || field)),
+            Projection(proj) => {
+                place = &proj.base;
+                deref = deref || matches!(proj.elem, mir::ProjectionElem::Deref);
+                if !field && matches!(proj.elem, mir::ProjectionElem::Field(..)) {
+                    field = has_drop(cx, place.ty(&mir.local_decls, cx.tcx).to_ty(cx.tcx));
+                }
+            },
+            _ => return None,
+        }
+    }
 }
 
 struct LocalUseVisitor {
@@ -279,9 +342,7 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
 
     fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext<'tcx>, _: mir::Location) {
         match ctx {
-            PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(NonUseContext::StorageDead) => {
-                return;
-            },
+            PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_) => return,
             _ => {},
         }
 
diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs
@@ -266,9 +266,8 @@ pub fn implements_trait<'a, 'tcx>(
 }
 
 /// Check whether this type implements Drop.
-pub fn has_drop(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
-    let struct_ty = cx.tables.expr_ty(expr);
-    match struct_ty.ty_adt_def() {
+pub fn has_drop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
+    match ty.ty_adt_def() {
         Some(def) => def.has_dtor(cx.tcx),
         _ => false,
     }
diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs
@@ -34,14 +34,35 @@ fn main() {
 
     // Check that lint level works
     #[allow(clippy::redundant_clone)] let _ = String::new().to_string();
+
+    let tup = (String::from("foo"),);
+    let _ = tup.0.clone();
+
+    let tup_ref = &(String::from("foo"),);
+    let _s = tup_ref.0.clone(); // this `.clone()` cannot be removed
 }
 
 #[derive(Clone)]
 struct Alpha;
-fn double(a: Alpha) -> (Alpha, Alpha) {
-    if true {
+fn with_branch(a: Alpha, b: bool) -> (Alpha, Alpha) {
+    if b {
         (a.clone(), a.clone())
     } else {
         (Alpha, a)
     }
 }
+
+struct TypeWithDrop {
+    x: String,
+}
+
+impl Drop for TypeWithDrop {
+    fn drop(&mut self) {}
+}
+
+fn cannot_move_from_type_with_drop() -> String {
+    let s = TypeWithDrop {
+        x: String::new()
+    };
+    s.x.clone() // removing this `clone()` summons E0509
+}
diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr
@@ -96,16 +96,28 @@ note: this value is dropped without further use
    |             ^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:43:22
+  --> $DIR/redundant_clone.rs:39:18
    |
-43 |         (a.clone(), a.clone())
+39 |     let _ = tup.0.clone();
+   |                  ^^^^^^^^ help: remove this
+   |
+note: this value is dropped without further use
+  --> $DIR/redundant_clone.rs:39:13
+   |
+39 |     let _ = tup.0.clone();
+   |             ^^^^^
+
+error: redundant clone
+  --> $DIR/redundant_clone.rs:49:22
+   |
+49 |         (a.clone(), a.clone())
    |                      ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:43:21
+  --> $DIR/redundant_clone.rs:49:21
    |
-43 |         (a.clone(), a.clone())
+49 |         (a.clone(), a.clone())
    |                     ^
 
-error: aborting due to 9 previous errors
+error: aborting due to 10 previous errors
 

Original file line number	Diff line number	Diff line change
`@@ -266,9 +266,8 @@ pub fn implements_trait<'a, 'tcx>(`
`266`	`266`	`}`
`267`	`267`
`268`	`268`	`/// Check whether this type implements Drop.`
`269`		`-pub fn has_drop(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {`
`270`		`- let struct_ty = cx.tables.expr_ty(expr);`
`271`		`- match struct_ty.ty_adt_def() {`
	`269`	`+pub fn has_drop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {`
	`270`	`+ match ty.ty_adt_def() {`
`272`	`271`	`Some(def) => def.has_dtor(cx.tcx),`
`273`	`272`	`_ => false,`
`274`	`273`	`}`