Merge pull request #2067 from jugglerchris/no_effect_with_drop

Fix #2061 (don't suggest removing types which implement Drop)

Author: oli-obk

clippy_lints/src/no_effect.rs

@@ -1,7 +1,7 @@
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::hir::def::Def;
 use rustc::hir::{BiAnd, BiOr, BlockCheckMode, Expr, Expr_, Stmt, StmtSemi, UnsafeSource};
-use utils::{in_macro, snippet_opt, span_lint, span_lint_and_sugg};
+use utils::{in_macro, snippet_opt, span_lint, span_lint_and_sugg, has_drop};
 use std::ops::Deref;
 
 /// **What it does:** Checks for statements which have no effect.
@@ -45,7 +45,8 @@ fn has_no_effect(cx: &LateContext, expr: &Expr) -> bool {
         return false;
     }
     match expr.node {
-        Expr_::ExprLit(..) | Expr_::ExprClosure(.., _) | Expr_::ExprPath(..) => true,
+        Expr_::ExprLit(..) | Expr_::ExprClosure(.., _) => true,
+        Expr_::ExprPath(..) => !has_drop(cx, expr),
         Expr_::ExprIndex(ref a, ref b) | Expr_::ExprBinary(_, ref a, ref b) => {
             has_no_effect(cx, a) && has_no_effect(cx, b)
         },
@@ -59,7 +60,7 @@ fn has_no_effect(cx: &LateContext, expr: &Expr) -> bool {
         Expr_::ExprAddrOf(_, ref inner) |
         Expr_::ExprBox(ref inner) => has_no_effect(cx, inner),
         Expr_::ExprStruct(_, ref fields, ref base) => {
-            fields.iter().all(|field| has_no_effect(cx, &field.expr)) && match *base {
+            !has_drop(cx, expr) && fields.iter().all(|field| has_no_effect(cx, &field.expr)) && match *base {
                 Some(ref base) => has_no_effect(cx, base),
                 None => true,
             }
@@ -68,7 +69,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(..) => {
-                    args.iter().all(|arg| has_no_effect(cx, arg))
+                    !has_drop(cx, expr) && args.iter().all(|arg| has_no_effect(cx, arg))
                 },
                 _ => false,
             }
@@ -145,18 +146,23 @@ fn reduce_expression<'a>(cx: &LateContext, expr: &'a Expr) -> Option<Vec<&'a Exp
         Expr_::ExprTupField(ref inner, _) |
         Expr_::ExprAddrOf(_, ref inner) |
         Expr_::ExprBox(ref inner) => reduce_expression(cx, inner).or_else(|| Some(vec![inner])),
-        Expr_::ExprStruct(_, ref fields, ref base) => Some(
-            fields
-                .iter()
-                .map(|f| &f.expr)
-                .chain(base)
-                .map(Deref::deref)
-                .collect(),
-        ),
+        Expr_::ExprStruct(_, ref fields, ref base) => {
+            if has_drop(cx, expr) {
+                None
+            } else {
+                Some(
+                    fields
+                        .iter()
+                        .map(|f| &f.expr)
+                        .chain(base)
+                        .map(Deref::deref)
+                        .collect())
+            }
+        },
         Expr_::ExprCall(ref callee, ref args) => if let Expr_::ExprPath(ref qpath) = callee.node {
             let def = cx.tables.qpath_def(qpath, callee.hir_id);
             match def {
-                Def::Struct(..) | Def::Variant(..) | Def::StructCtor(..) | Def::VariantCtor(..) => {
+                Def::Struct(..) | Def::Variant(..) | Def::StructCtor(..) | Def::VariantCtor(..) if !has_drop(cx, expr) => {
                     Some(args.iter().collect())
                 },
                 _ => None,

clippy_lints/src/utils/mod.rs

@@ -358,6 +358,15 @@ 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() {
+        Some(def) => def.has_dtor(cx.tcx),
+        _ => false,
+    }
+}
+
 /// Resolve the definition of a node from its `HirId`.
 pub fn resolve_node(cx: &LateContext, qpath: &QPath, id: HirId) -> def::Def {
     cx.tables.qpath_def(qpath, id)

tests/ui/no_effect.rs

@@ -16,14 +16,38 @@ enum Enum {
     Tuple(i32),
     Struct { field: i32 },
 }
-
+struct DropUnit;
+impl Drop for DropUnit {
+    fn drop(&mut self) {}
+}
+struct DropStruct {
+    field: i32
+}
+impl Drop for DropStruct {
+    fn drop(&mut self) {}
+}
+struct DropTuple(i32);
+impl Drop for DropTuple {
+    fn drop(&mut self) {}
+}
+enum DropEnum {
+    Tuple(i32),
+    Struct { field: i32 },
+}
+impl Drop for DropEnum {
+    fn drop(&mut self) {}
+}
+struct FooString {
+    s: String,
+}
 union Union {
     a: u8,
     b: f64,
 }
 
 fn get_number() -> i32 { 0 }
 fn get_struct() -> Struct { Struct { field: 0 } }
+fn get_drop_struct() -> DropStruct { DropStruct { field: 0 } }
 
 unsafe fn unsafe_fn() -> i32 { 0 }
 
@@ -57,10 +81,17 @@ fn main() {
     [42; 55][13];
     let mut x = 0;
     || x += 5;
+    let s: String = "foo".into();
+    FooString { s: s };
 
     // Do not warn
     get_number();
     unsafe { unsafe_fn() };
+    DropUnit;
+    DropStruct { field: 0 };
+    DropTuple(0);
+    DropEnum::Tuple(0);
+    DropEnum::Struct { field: 0 };
 
     Tuple(get_number());
     Struct { field: get_number() };
@@ -81,4 +112,13 @@ fn main() {
     [get_number(); 55];
     [42; 55][get_number() as usize];
     {get_number()};
+    FooString { s: String::from("blah"), };
+
+    // Do not warn
+    DropTuple(get_number());
+    DropStruct { field: get_number() };
+    DropStruct { field: get_number() };
+    DropStruct { ..get_drop_struct() };
+    DropEnum::Tuple(get_number());
+    DropEnum::Struct { field: get_number() };
 }