Auto merge of rust-lang#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: rust-lang#10488

Author: bors

clippy_lints/src/collection_is_never_read.rs

@@ -101,21 +101,34 @@ 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
         // have side effects, so consider them a read.
         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.

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

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