Auto merge of #10492 - schubart:collection_is_never_read_unit_type, r=Manishearth

`collection_is_never_read`: Handle unit type

changelog: [`collection_is_never_read`]: Fix false negative
fixes: #10488

3 files changed, 44 insertions, 14 deletions

diff --git a/clippy_lints/src/collection_is_never_read.rs b/clippy_lints/src/collection_is_never_read.rs
index 15a5b7dd274..5e2eb5789f6 100644
--- a/clippy_lints/src/collection_is_never_read.rs
+++ b/clippy_lints/src/collection_is_never_read.rs
@@ -101,9 +101,9 @@ fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Bloc
             return ControlFlow::Continue(());
         }
 
-        // Method call on `id` in a statement ignores any return value, so it's not a read access:
+        // Look for method call with receiver `id`. It might be a non-read access:
         //
-        // id.foo(...); // Not reading `id`.
+        // id.foo(args)
         //
         // Only assuming this for "official" methods defined on the type. For methods defined in extension
         // traits (identified as local, based on the orphan rule), pessimistically assume that they might
@@ -111,11 +111,24 @@ fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Bloc
         if let Some(Node::Expr(parent)) = get_parent_node(cx.tcx, expr.hir_id)
             && let ExprKind::MethodCall(_, receiver, _, _) = parent.kind
             && path_to_local_id(receiver, id)
-            && let Some(Node::Stmt(..)) = get_parent_node(cx.tcx, parent.hir_id)
             && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(parent.hir_id)
             && !method_def_id.is_local()
         {
-            return ControlFlow::Continue(());
+            // The method call is a statement, so the return value is not used. That's not a read access:
+            //
+            // id.foo(args);
+            if let Some(Node::Stmt(..)) = get_parent_node(cx.tcx, parent.hir_id) {
+                return ControlFlow::Continue(());
+            }
+
+            // The method call is not a statement, so its return value is used somehow but its type is the
+            // unit type, so this is not a real read access. Examples:
+            //
+            // let y = x.clear();
+            // println!("{:?}", x.clear());
+            if cx.typeck_results().expr_ty(parent).is_unit() {
+                return ControlFlow::Continue(());
+            }
         }
 
         // Any other access to `id` is a read access. Stop searching.
diff --git a/tests/ui/collection_is_never_read.rs b/tests/ui/collection_is_never_read.rs
index ca20031bfbe..01259a983ab 100644
--- a/tests/ui/collection_is_never_read.rs
+++ b/tests/ui/collection_is_never_read.rs
@@ -84,13 +84,18 @@ fn shadowing_2() {
 }
 
 #[allow(clippy::let_unit_value)]
-fn fake_read() {
-    let mut x = vec![1, 2, 3]; // Ok
+fn fake_read_1() {
+    let mut x = vec![1, 2, 3]; // WARNING
     x.reverse();
-    // `collection_is_never_read` gets fooled, but other lints should catch this.
     let _: () = x.clear();
 }
 
+fn fake_read_2() {
+    let mut x = vec![1, 2, 3]; // WARNING
+    x.reverse();
+    println!("{:?}", x.push(5));
+}
+
 fn assignment() {
     let mut x = vec![1, 2, 3]; // WARNING
     let y = vec![4, 5, 6]; // Ok
diff --git a/tests/ui/collection_is_never_read.stderr b/tests/ui/collection_is_never_read.stderr
index f5dea96116f..cf51a53686f 100644
--- a/tests/ui/collection_is_never_read.stderr
+++ b/tests/ui/collection_is_never_read.stderr
@@ -25,40 +25,52 @@ LL |     let mut x = HashMap::new(); // WARNING
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: collection is never read
-  --> $DIR/collection_is_never_read.rs:95:5
+  --> $DIR/collection_is_never_read.rs:88:5
    |
 LL |     let mut x = vec![1, 2, 3]; // WARNING
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: collection is never read
-  --> $DIR/collection_is_never_read.rs:102:5
+  --> $DIR/collection_is_never_read.rs:94:5
    |
 LL |     let mut x = vec![1, 2, 3]; // WARNING
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: collection is never read
-  --> $DIR/collection_is_never_read.rs:119:5
+  --> $DIR/collection_is_never_read.rs:100:5
+   |
+LL |     let mut x = vec![1, 2, 3]; // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:107:5
+   |
+LL |     let mut x = vec![1, 2, 3]; // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:124:5
    |
 LL |     let mut x = HashSet::new(); // WARNING
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: collection is never read
-  --> $DIR/collection_is_never_read.rs:133:5
+  --> $DIR/collection_is_never_read.rs:138:5
    |
 LL |     let x = vec![1, 2, 3]; // WARNING
    |     ^^^^^^^^^^^^^^^^^^^^^^
 
 error: collection is never read
-  --> $DIR/collection_is_never_read.rs:169:5
+  --> $DIR/collection_is_never_read.rs:174:5
    |
 LL |     let mut s = String::new();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: collection is never read
-  --> $DIR/collection_is_never_read.rs:182:5
+  --> $DIR/collection_is_never_read.rs:187:5
    |
 LL |     let mut s = String::from("Hello, World!");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 10 previous errors
+error: aborting due to 12 previous errors