Auto merge of #5559 - alex-700:fix-while-let-on-iterator-fp, r=flip1995

Fix FP on while-let-on-iterator - fix `is_refutable` for slice patterns - fix `is_refutable` for bindings - add some TODO-s for cases, which can not be fixed easily fixes #3780 changelog: fix FP on while-let-on-iterator for arrays and bindings
rust-lang · May 2, 2020 · 60538d6 · 60538d6
2 parents cc9088f + d0c1f8a
commit 60538d6
Show file tree

Hide file tree

Showing 4 changed files with 160 additions and 18 deletions.
diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs
@@ -933,6 +933,7 @@ pub fn is_ctor_or_promotable_const_function(cx: &LateContext<'_, '_>, expr: &Exp
 }
 
 /// Returns `true` if a pattern is refutable.
+// TODO: should be implemented using rustc/mir_build/hair machinery
 pub fn is_refutable(cx: &LateContext<'_, '_>, pat: &Pat<'_>) -> bool {
     fn is_enum_variant(cx: &LateContext<'_, '_>, qpath: &QPath<'_>, id: HirId) -> bool {
         matches!(
@@ -946,27 +947,34 @@ pub fn is_refutable(cx: &LateContext<'_, '_>, pat: &Pat<'_>) -> bool {
     }
 
     match pat.kind {
-        PatKind::Binding(..) | PatKind::Wild => false,
+        PatKind::Wild => false,
+        PatKind::Binding(_, _, _, pat) => pat.map_or(false, |pat| is_refutable(cx, pat)),
         PatKind::Box(ref pat) | PatKind::Ref(ref pat, _) => is_refutable(cx, pat),
         PatKind::Lit(..) | PatKind::Range(..) => true,
         PatKind::Path(ref qpath) => is_enum_variant(cx, qpath, pat.hir_id),
-        PatKind::Or(ref pats) | PatKind::Tuple(ref pats, _) => are_refutable(cx, pats.iter().map(|pat| &**pat)),
+        PatKind::Or(ref pats) => {
+            // TODO: should be the honest check, that pats is exhaustive set
+            are_refutable(cx, pats.iter().map(|pat| &**pat))
+        },
+        PatKind::Tuple(ref pats, _) => are_refutable(cx, pats.iter().map(|pat| &**pat)),
         PatKind::Struct(ref qpath, ref fields, _) => {
-            if is_enum_variant(cx, qpath, pat.hir_id) {
-                true
-            } else {
-                are_refutable(cx, fields.iter().map(|field| &*field.pat))
-            }
+            is_enum_variant(cx, qpath, pat.hir_id) || are_refutable(cx, fields.iter().map(|field| &*field.pat))
         },
         PatKind::TupleStruct(ref qpath, ref pats, _) => {
-            if is_enum_variant(cx, qpath, pat.hir_id) {
-                true
-            } else {
-                are_refutable(cx, pats.iter().map(|pat| &**pat))
-            }
+            is_enum_variant(cx, qpath, pat.hir_id) || are_refutable(cx, pats.iter().map(|pat| &**pat))
         },
         PatKind::Slice(ref head, ref middle, ref tail) => {
-            are_refutable(cx, head.iter().chain(middle).chain(tail.iter()).map(|pat| &**pat))
+            match &cx.tables.node_type(pat.hir_id).kind {
+                ty::Slice(..) => {
+                    // [..] is the only irrefutable slice pattern.
+                    !head.is_empty() || middle.is_none() || !tail.is_empty()
+                },
+                ty::Array(..) => are_refutable(cx, head.iter().chain(middle).chain(tail.iter()).map(|pat| &**pat)),
+                _ => {
+                    // unreachable!()
+                    true
+                },
+            }
         },
     }
 }

diff --git a/tests/ui/while_let_on_iterator.fixed b/tests/ui/while_let_on_iterator.fixed
@@ -2,6 +2,7 @@
 
 #![warn(clippy::while_let_on_iterator)]
 #![allow(clippy::never_loop, unreachable_code, unused_mut)]
+#![feature(or_patterns)]
 
 fn base() {
     let mut iter = 1..20;
@@ -77,6 +78,62 @@ fn refutable() {
     // */
 }
 
+fn refutable2() {
+    // Issue 3780
+    {
+        let v = vec![1, 2, 3];
+        let mut it = v.windows(2);
+        while let Some([x, y]) = it.next() {
+            println!("x: {}", x);
+            println!("y: {}", y);
+        }
+
+        let mut it = v.windows(2);
+        while let Some([x, ..]) = it.next() {
+            println!("x: {}", x);
+        }
+
+        let mut it = v.windows(2);
+        while let Some([.., y]) = it.next() {
+            println!("y: {}", y);
+        }
+
+        let mut it = v.windows(2);
+        for [..] in it {}
+
+        let v = vec![[1], [2], [3]];
+        let mut it = v.iter();
+        while let Some([1]) = it.next() {}
+
+        let mut it = v.iter();
+        for [_x] in it {}
+    }
+
+    // binding
+    {
+        let v = vec![1, 2, 3];
+        let mut it = v.iter();
+        while let Some(x @ 1) = it.next() {
+            println!("{}", x);
+        }
+
+        let v = vec![[1], [2], [3]];
+        let mut it = v.iter();
+        for x @ [_] in it {
+            println!("{:?}", x);
+        }
+    }
+
+    // false negative
+    {
+        let v = vec![1, 2, 3];
+        let mut it = v.iter().map(Some);
+        while let Some(Some(_) | None) = it.next() {
+            println!("1");
+        }
+    }
+}
+
 fn nested_loops() {
     let a = [42, 1337];
     let mut y = a.iter();
@@ -152,6 +209,7 @@ fn issue1654() {
 fn main() {
     base();
     refutable();
+    refutable2();
     nested_loops();
     issue1121();
     issue2965();

diff --git a/tests/ui/while_let_on_iterator.rs b/tests/ui/while_let_on_iterator.rs
@@ -2,6 +2,7 @@
 
 #![warn(clippy::while_let_on_iterator)]
 #![allow(clippy::never_loop, unreachable_code, unused_mut)]
+#![feature(or_patterns)]
 
 fn base() {
     let mut iter = 1..20;
@@ -77,6 +78,62 @@ fn refutable() {
     // */
 }
 
+fn refutable2() {
+    // Issue 3780
+    {
+        let v = vec![1, 2, 3];
+        let mut it = v.windows(2);
+        while let Some([x, y]) = it.next() {
+            println!("x: {}", x);
+            println!("y: {}", y);
+        }
+
+        let mut it = v.windows(2);
+        while let Some([x, ..]) = it.next() {
+            println!("x: {}", x);
+        }
+
+        let mut it = v.windows(2);
+        while let Some([.., y]) = it.next() {
+            println!("y: {}", y);
+        }
+
+        let mut it = v.windows(2);
+        while let Some([..]) = it.next() {}
+
+        let v = vec![[1], [2], [3]];
+        let mut it = v.iter();
+        while let Some([1]) = it.next() {}
+
+        let mut it = v.iter();
+        while let Some([_x]) = it.next() {}
+    }
+
+    // binding
+    {
+        let v = vec![1, 2, 3];
+        let mut it = v.iter();
+        while let Some(x @ 1) = it.next() {
+            println!("{}", x);
+        }
+
+        let v = vec![[1], [2], [3]];
+        let mut it = v.iter();
+        while let Some(x @ [_]) = it.next() {
+            println!("{:?}", x);
+        }
+    }
+
+    // false negative
+    {
+        let v = vec![1, 2, 3];
+        let mut it = v.iter().map(Some);
+        while let Some(Some(_) | None) = it.next() {
+            println!("1");
+        }
+    }
+}
+
 fn nested_loops() {
     let a = [42, 1337];
     let mut y = a.iter();
@@ -152,6 +209,7 @@ fn issue1654() {
 fn main() {
     base();
     refutable();
+    refutable2();
     nested_loops();
     issue1121();
     issue2965();

diff --git a/tests/ui/while_let_on_iterator.stderr b/tests/ui/while_let_on_iterator.stderr
@@ -1,28 +1,46 @@
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:8:5
+  --> $DIR/while_let_on_iterator.rs:9:5
    |
 LL |     while let Option::Some(x) = iter.next() {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in iter`
    |
    = note: `-D clippy::while-let-on-iterator` implied by `-D warnings`
 
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:13:5
+  --> $DIR/while_let_on_iterator.rs:14:5
    |
 LL |     while let Some(x) = iter.next() {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in iter`
 
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:18:5
+  --> $DIR/while_let_on_iterator.rs:19:5
    |
 LL |     while let Some(_) = iter.next() {}
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in iter`
 
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:97:9
+  --> $DIR/while_let_on_iterator.rs:102:9
+   |
+LL |         while let Some([..]) = it.next() {}
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for [..] in it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:109:9
+   |
+LL |         while let Some([_x]) = it.next() {}
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for [_x] in it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:122:9
+   |
+LL |         while let Some(x @ [_]) = it.next() {
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x @ [_] in it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:154:9
    |
 LL |         while let Some(_) = y.next() {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in y`
 
-error: aborting due to 4 previous errors
+error: aborting due to 7 previous errors