From f680eb164d8365160d6d43ca2589d95b97d5610b Mon Sep 17 00:00:00 2001 From: Chris Emerson Date: Mon, 18 Sep 2017 20:07:33 +0100 Subject: [PATCH] Update unnecessary_operation and no_effect to not suggest removing structs/enums wrappers when that type implements Drop as noted in #2061. --- clippy_lints/src/no_effect.rs | 39 ++-- tests/ui/no_effect.rs | 35 +++ tests/ui/no_effect.stderr | 394 +++++++++++++++++----------------- 3 files changed, 259 insertions(+), 209 deletions(-) diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index 1d5e51187..230c81193 100644 --- a/clippy_lints/src/no_effect.rs +++ b/clippy_lints/src/no_effect.rs @@ -40,12 +40,22 @@ declare_lint! { "outer expressions with no effect" } +/// Check whether this type implements Drop. +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, + } +} + fn has_no_effect(cx: &LateContext, expr: &Expr) -> bool { if in_macro(expr.span) { 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 +69,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 +78,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 +155,23 @@ fn reduce_expression<'a>(cx: &LateContext, expr: &'a Expr) -> Option 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, diff --git a/tests/ui/no_effect.rs b/tests/ui/no_effect.rs index bef1fad4f..4062b3688 100644 --- a/tests/ui/no_effect.rs +++ b/tests/ui/no_effect.rs @@ -16,6 +16,27 @@ 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) {} +} union Union { a: u8, @@ -24,6 +45,7 @@ union Union { 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 } @@ -61,6 +83,11 @@ fn main() { // 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 +108,12 @@ fn main() { [get_number(); 55]; [42; 55][get_number() as usize]; {get_number()}; + + // 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() }; } diff --git a/tests/ui/no_effect.stderr b/tests/ui/no_effect.stderr index 590b1eab4..c7c334546 100644 --- a/tests/ui/no_effect.stderr +++ b/tests/ui/no_effect.stderr @@ -1,270 +1,270 @@ error: statement with no effect - --> $DIR/no_effect.rs:34:5 + --> $DIR/no_effect.rs:56:5 | -34 | 0; +56 | 0; | ^^ | = note: `-D no-effect` implied by `-D warnings` -error: statement with no effect - --> $DIR/no_effect.rs:35:5 - | -35 | s2; - | ^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:36:5 - | -36 | Unit; - | ^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:37:5 - | -37 | Tuple(0); - | ^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:38:5 - | -38 | Struct { field: 0 }; - | ^^^^^^^^^^^^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:39:5 - | -39 | Struct { ..s }; - | ^^^^^^^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:40:5 - | -40 | Union { a: 0 }; - | ^^^^^^^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:41:5 - | -41 | Enum::Tuple(0); - | ^^^^^^^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:42:5 - | -42 | Enum::Struct { field: 0 }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:43:5 - | -43 | 5 + 6; - | ^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:44:5 - | -44 | *&42; - | ^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:45:5 - | -45 | &6; - | ^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:46:5 - | -46 | (5, 6, 7); - | ^^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:47:5 - | -47 | box 42; - | ^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:48:5 - | -48 | ..; - | ^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:49:5 - | -49 | 5..; - | ^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:50:5 - | -50 | ..5; - | ^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:51:5 - | -51 | 5..6; - | ^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:52:5 - | -52 | 5...6; - | ^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:53:5 - | -53 | [42, 55]; - | ^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:54:5 - | -54 | [42, 55][1]; - | ^^^^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:55:5 - | -55 | (42, 55).1; - | ^^^^^^^^^^^ - -error: statement with no effect - --> $DIR/no_effect.rs:56:5 - | -56 | [42; 55]; - | ^^^^^^^^^ - error: statement with no effect --> $DIR/no_effect.rs:57:5 | -57 | [42; 55][13]; - | ^^^^^^^^^^^^^ +57 | s2; + | ^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:58:5 + | +58 | Unit; + | ^^^^^ error: statement with no effect --> $DIR/no_effect.rs:59:5 | -59 | || x += 5; +59 | Tuple(0); + | ^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:60:5 + | +60 | Struct { field: 0 }; + | ^^^^^^^^^^^^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:61:5 + | +61 | Struct { ..s }; + | ^^^^^^^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:62:5 + | +62 | Union { a: 0 }; + | ^^^^^^^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:63:5 + | +63 | Enum::Tuple(0); + | ^^^^^^^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:64:5 + | +64 | Enum::Struct { field: 0 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:65:5 + | +65 | 5 + 6; + | ^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:66:5 + | +66 | *&42; + | ^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:67:5 + | +67 | &6; + | ^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:68:5 + | +68 | (5, 6, 7); + | ^^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:69:5 + | +69 | box 42; + | ^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:70:5 + | +70 | ..; + | ^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:71:5 + | +71 | 5..; + | ^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:72:5 + | +72 | ..5; + | ^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:73:5 + | +73 | 5..6; + | ^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:74:5 + | +74 | 5...6; + | ^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:75:5 + | +75 | [42, 55]; + | ^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:76:5 + | +76 | [42, 55][1]; + | ^^^^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:77:5 + | +77 | (42, 55).1; + | ^^^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:78:5 + | +78 | [42; 55]; + | ^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:79:5 + | +79 | [42; 55][13]; + | ^^^^^^^^^^^^^ + +error: statement with no effect + --> $DIR/no_effect.rs:81:5 + | +81 | || x += 5; | ^^^^^^^^^^ error: statement can be reduced - --> $DIR/no_effect.rs:65:5 + --> $DIR/no_effect.rs:92:5 | -65 | Tuple(get_number()); +92 | Tuple(get_number()); | ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` | = note: `-D unnecessary-operation` implied by `-D warnings` error: statement can be reduced - --> $DIR/no_effect.rs:66:5 + --> $DIR/no_effect.rs:93:5 | -66 | Struct { field: get_number() }; +93 | Struct { field: get_number() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:67:5 + --> $DIR/no_effect.rs:94:5 | -67 | Struct { ..get_struct() }; +94 | Struct { ..get_struct() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_struct();` error: statement can be reduced - --> $DIR/no_effect.rs:68:5 + --> $DIR/no_effect.rs:95:5 | -68 | Enum::Tuple(get_number()); +95 | Enum::Tuple(get_number()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:69:5 + --> $DIR/no_effect.rs:96:5 | -69 | Enum::Struct { field: get_number() }; +96 | Enum::Struct { field: get_number() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:70:5 + --> $DIR/no_effect.rs:97:5 | -70 | 5 + get_number(); +97 | 5 + get_number(); | ^^^^^^^^^^^^^^^^^ help: replace it with: `5;get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:71:5 + --> $DIR/no_effect.rs:98:5 | -71 | *&get_number(); +98 | *&get_number(); | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:72:5 + --> $DIR/no_effect.rs:99:5 | -72 | &get_number(); +99 | &get_number(); | ^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:73:5 - | -73 | (5, 6, get_number()); - | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `5;6;get_number();` + --> $DIR/no_effect.rs:100:5 + | +100 | (5, 6, get_number()); + | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `5;6;get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:74:5 - | -74 | box get_number(); - | ^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + --> $DIR/no_effect.rs:101:5 + | +101 | box get_number(); + | ^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:75:5 - | -75 | get_number()..; - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + --> $DIR/no_effect.rs:102:5 + | +102 | get_number()..; + | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:76:5 - | -76 | ..get_number(); - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + --> $DIR/no_effect.rs:103:5 + | +103 | ..get_number(); + | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:77:5 - | -77 | 5..get_number(); - | ^^^^^^^^^^^^^^^^ help: replace it with: `5;get_number();` + --> $DIR/no_effect.rs:104:5 + | +104 | 5..get_number(); + | ^^^^^^^^^^^^^^^^ help: replace it with: `5;get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:78:5 - | -78 | [42, get_number()]; - | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` + --> $DIR/no_effect.rs:105:5 + | +105 | [42, get_number()]; + | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:79:5 - | -79 | [42, 55][get_number() as usize]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42, 55];get_number() as usize;` + --> $DIR/no_effect.rs:106:5 + | +106 | [42, 55][get_number() as usize]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42, 55];get_number() as usize;` error: statement can be reduced - --> $DIR/no_effect.rs:80:5 - | -80 | (42, get_number()).1; - | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` + --> $DIR/no_effect.rs:107:5 + | +107 | (42, get_number()).1; + | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:81:5 - | -81 | [get_number(); 55]; - | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + --> $DIR/no_effect.rs:108:5 + | +108 | [get_number(); 55]; + | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: statement can be reduced - --> $DIR/no_effect.rs:82:5 - | -82 | [42; 55][get_number() as usize]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42; 55];get_number() as usize;` + --> $DIR/no_effect.rs:109:5 + | +109 | [42; 55][get_number() as usize]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42; 55];get_number() as usize;` error: statement can be reduced - --> $DIR/no_effect.rs:83:5 - | -83 | {get_number()}; - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + --> $DIR/no_effect.rs:110:5 + | +110 | {get_number()}; + | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` error: aborting due to 44 previous errors