Check if dropping an expression may have indirect side-effects (#14594)

It is not enough to check if an expression type implements `Drop` to
determine whether it can have a significant side-effect.

Also, add tests for unchecked cases which were explicitly handled in the
code, such as checking for side effect in a `struct`'s fields, or in its
base expression.

Fix rust-lang/rust-clippy#14592

changelog: [`no_effect_underscore_binding`]: do not propose to remove
the assignment of an object if dropping it might indirectly have a
visible side effect

3 files changed, 64 insertions, 5 deletions

diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs
index 7ab7976d569..02c48166131 100644
--- a/clippy_lints/src/no_effect.rs
+++ b/clippy_lints/src/no_effect.rs
@@ -221,10 +221,16 @@ fn is_operator_overridden(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     }
 }
 
+/// Checks if dropping `expr` might have a visible side effect.
+fn expr_ty_has_significant_drop(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    let ty = cx.typeck_results().expr_ty(expr);
+    ty.has_significant_drop(cx.tcx, cx.typing_env())
+}
+
 fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     match expr.kind {
         ExprKind::Lit(..) | ExprKind::Closure { .. } => true,
-        ExprKind::Path(..) => !has_drop(cx, cx.typeck_results().expr_ty(expr)),
+        ExprKind::Path(..) => !expr_ty_has_significant_drop(cx, expr),
         ExprKind::Index(a, b, _) | ExprKind::Binary(_, a, b) => has_no_effect(cx, a) && has_no_effect(cx, b),
         ExprKind::Array(v) | ExprKind::Tup(v) => v.iter().all(|val| has_no_effect(cx, val)),
         ExprKind::Repeat(inner, _)
@@ -233,8 +239,8 @@ fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
         | ExprKind::Unary(_, inner)
         | ExprKind::Field(inner, _)
         | ExprKind::AddrOf(_, _, inner) => has_no_effect(cx, inner),
-        ExprKind::Struct(_, fields, ref base) => {
-            !has_drop(cx, cx.typeck_results().expr_ty(expr))
+        ExprKind::Struct(_, fields, base) => {
+            !expr_ty_has_significant_drop(cx, expr)
                 && fields.iter().all(|field| has_no_effect(cx, field.expr))
                 && match &base {
                     StructTailExpr::None | StructTailExpr::DefaultFields(_) => true,
@@ -252,7 +258,7 @@ fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
                     Res::Def(DefKind::Struct | DefKind::Variant | DefKind::Ctor(..), ..)
                 );
                 if def_matched || is_range_literal(expr) {
-                    !has_drop(cx, cx.typeck_results().expr_ty(expr)) && args.iter().all(|arg| has_no_effect(cx, arg))
+                    !expr_ty_has_significant_drop(cx, expr) && args.iter().all(|arg| has_no_effect(cx, arg))
                 } else {
                     false
                 }
diff --git a/tests/ui/no_effect.rs b/tests/ui/no_effect.rs
index 703c2a3d984..4ab5bc9acde 100644
--- a/tests/ui/no_effect.rs
+++ b/tests/ui/no_effect.rs
@@ -221,3 +221,56 @@ fn main() {
     Cout << 142;
     -Cout;
 }
+
+fn issue14592() {
+    struct MyStruct {
+        _inner: MyInner,
+    }
+    struct MyInner {}
+
+    impl Drop for MyInner {
+        fn drop(&mut self) {
+            println!("dropping");
+        }
+    }
+
+    let x = MyStruct { _inner: MyInner {} };
+
+    let closure = || {
+        // Do not lint: dropping the assignment or assigning to `_` would
+        // change the output.
+        let _x = x;
+    };
+
+    println!("1");
+    closure();
+    println!("2");
+
+    struct Innocuous {
+        a: i32,
+    }
+
+    // Do not lint: one of the fields has a side effect.
+    let x = MyInner {};
+    let closure = || {
+        let _x = Innocuous {
+            a: {
+                x;
+                10
+            },
+        };
+    };
+
+    // Do not lint: the base has a side effect.
+    let x = MyInner {};
+    let closure = || {
+        let _x = Innocuous {
+            ..Innocuous {
+                a: {
+                    x;
+                    10
+                },
+            }
+        };
+    };
+}
diff --git a/tests/ui/single_range_in_vec_init.rs b/tests/ui/single_range_in_vec_init.rs
index c6c0cb347dc..25884450b08 100644
--- a/tests/ui/single_range_in_vec_init.rs
+++ b/tests/ui/single_range_in_vec_init.rs
@@ -1,6 +1,6 @@
 //@aux-build:proc_macros.rs
 //@no-rustfix: overlapping suggestions
-#![allow(clippy::no_effect, clippy::useless_vec, unused)]
+#![allow(clippy::no_effect, clippy::unnecessary_operation, clippy::useless_vec, unused)]
 #![warn(clippy::single_range_in_vec_init)]
 #![feature(generic_arg_infer)]