Merge pull request #396 from fhartwig/while-let-for

Suggest for loop instead of while-let when looping over iterators

Author: llogiq

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 68 lints included in this crate:
+There are 69 lints included in this crate:
 
 name                                                                                                   | default | meaning
 -------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -74,6 +74,7 @@ name
 [unstable_as_slice](https://github.com/Manishearth/rust-clippy/wiki#unstable_as_slice)                 | warn    | as_slice is not stable and can be replaced by & v[..]see https://github.com/rust-lang/rust/issues/27729
 [unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect)                       | warn    | `collect()`ing an iterator without using the result; this is usually better written as a for loop
 [while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop)                       | warn    | `loop { if let { ... } else break }` can be written as a `while let` loop
+[while_let_on_iterator](https://github.com/Manishearth/rust-clippy/wiki#while_let_on_iterator)         | warn    | using a while-let loop instead of a for loop on an iterator
 [wrong_pub_self_convention](https://github.com/Manishearth/rust-clippy/wiki#wrong_pub_self_convention) | allow   | defining a public method named with an established prefix (like "into_") that takes `self` with the wrong convention
 [wrong_self_convention](https://github.com/Manishearth/rust-clippy/wiki#wrong_self_convention)         | warn    | defining a method named with an established prefix (like "into_") that takes `self` with the wrong convention
 [zero_divided_by_zero](https://github.com/Manishearth/rust-clippy/wiki#zero_divided_by_zero)           | warn    | usage of `0.0 / 0.0` to obtain NaN instead of std::f32::NaN or std::f64::NaN

src/lib.rs

@@ -138,6 +138,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         loops::REVERSE_RANGE_LOOP,
         loops::UNUSED_COLLECT,
         loops::WHILE_LET_LOOP,
+        loops::WHILE_LET_ON_ITERATOR,
         matches::MATCH_BOOL,
         matches::MATCH_REF_PATS,
         matches::SINGLE_MATCH,

src/loops.rs

@@ -11,7 +11,8 @@ use std::collections::{HashSet,HashMap};
 use syntax::ast::Lit_::*;
 
 use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type,
-            in_external_macro, expr_block, span_help_and_lint, is_integer_literal};
+            in_external_macro, expr_block, span_help_and_lint, is_integer_literal,
+            get_enclosing_block};
 use utils::{VEC_PATH, LL_PATH};
 
 declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn,
@@ -38,14 +39,17 @@ declare_lint!{ pub EXPLICIT_COUNTER_LOOP, Warn,
 
 declare_lint!{ pub EMPTY_LOOP, Warn, "empty `loop {}` detected" }
 
+declare_lint!{ pub WHILE_LET_ON_ITERATOR, Warn, "using a while-let loop instead of a for loop on an iterator" }
+
 #[derive(Copy, Clone)]
 pub struct LoopsPass;
 
 impl LintPass for LoopsPass {
     fn get_lints(&self) -> LintArray {
         lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP,
                     WHILE_LET_LOOP, UNUSED_COLLECT, REVERSE_RANGE_LOOP,
-                    EXPLICIT_COUNTER_LOOP, EMPTY_LOOP)
+                    EXPLICIT_COUNTER_LOOP, EMPTY_LOOP,
+                    WHILE_LET_ON_ITERATOR)
     }
 }
 
@@ -228,6 +232,28 @@ impl LateLintPass for LoopsPass {
                 }
             }
         }
+        if let ExprMatch(ref match_expr, ref arms, MatchSource::WhileLetDesugar) = expr.node {
+            let pat = &arms[0].pats[0].node;
+            if let (&PatEnum(ref path, Some(ref pat_args)),
+                    &ExprMethodCall(method_name, _, ref method_args)) =
+                        (pat, &match_expr.node) {
+                let iter_expr = &method_args[0];
+                if let Some(lhs_constructor) = path.segments.last() {
+                    if method_name.node.as_str() == "next" &&
+                            match_trait_method(cx, match_expr, &["core", "iter", "Iterator"]) &&
+                            lhs_constructor.identifier.name.as_str() == "Some" &&
+                            !is_iterator_used_after_while_let(cx, iter_expr) {
+                        let iterator = snippet(cx, method_args[0].span, "_");
+                        let loop_var = snippet(cx, pat_args[0].span, "_");
+                        span_help_and_lint(cx, WHILE_LET_ON_ITERATOR, expr.span,
+                                           "this loop could be written as a `for` loop",
+                                           &format!("try\nfor {} in {} {{...}}",
+                                                    loop_var,
+                                                    iterator));
+                    }
+                }
+            }
+        }
     }
 
     fn check_stmt(&mut self, cx: &LateContext, stmt: &Stmt) {
@@ -300,6 +326,46 @@ impl<'v, 't> Visitor<'v> for VarVisitor<'v, 't> {
     }
 }
 
+fn is_iterator_used_after_while_let(cx: &LateContext, iter_expr: &Expr) -> bool {
+    let def_id = match var_def_id(cx, iter_expr) {
+        Some(id) => id,
+        None => return false
+    };
+    let mut visitor = VarUsedAfterLoopVisitor {
+        cx: cx,
+        def_id: def_id,
+        iter_expr_id: iter_expr.id,
+        past_while_let: false,
+        var_used_after_while_let: false
+    };
+    if let Some(enclosing_block) = get_enclosing_block(cx, def_id) {
+        walk_block(&mut visitor, enclosing_block);
+    }
+    visitor.var_used_after_while_let
+}
+
+struct VarUsedAfterLoopVisitor<'v, 't: 'v> {
+    cx: &'v LateContext<'v, 't>,
+    def_id: NodeId,
+    iter_expr_id: NodeId,
+    past_while_let: bool,
+    var_used_after_while_let: bool
+}
+
+impl <'v, 't> Visitor<'v> for VarUsedAfterLoopVisitor<'v, 't> {
+    fn visit_expr(&mut self, expr: &'v Expr) {
+        if self.past_while_let {
+            if Some(self.def_id) == var_def_id(self.cx, expr) {
+                self.var_used_after_while_let = true;
+            }
+        } else if self.iter_expr_id == expr.id {
+            self.past_while_let = true;
+        }
+        walk_expr(self, expr);
+    }
+}
+
+
 /// Return true if the type of expr is one that provides IntoIterator impls
 /// for &T and &mut T, such as Vec.
 fn is_ref_iterable_type(cx: &LateContext, e: &Expr) -> bool {

src/utils.rs

@@ -255,6 +255,20 @@ pub fn get_parent_expr<'c>(cx: &'c LateContext, e: &Expr) -> Option<&'c Expr> {
         if let NodeExpr(parent) = node { Some(parent) } else { None } )
 }
 
+#[allow(needless_lifetimes)] // workaround for https://github.com/Manishearth/rust-clippy/issues/417
+pub fn get_enclosing_block<'c>(cx: &'c LateContext, node: NodeId) -> Option<&'c Block> {
+    let map = &cx.tcx.map;
+    let enclosing_node = map.get_enclosing_scope(node)
+                            .and_then(|enclosing_id| map.find(enclosing_id));
+    if let Some(node) = enclosing_node {
+        match node {
+            NodeBlock(ref block) => Some(block),
+            NodeItem(&Item{ node: ItemFn(_, _, _, _, _, ref block), .. }) => Some(block),
+            _ => None
+        }
+    } else { None }
+}
+
 #[cfg(not(feature="structured_logging"))]
 pub fn span_lint<T: LintContext>(cx: &T, lint: &'static Lint, sp: Span, msg: &str) {
     cx.span_lint(lint, sp, msg);

tests/compile-fail/while_loop.rs

@@ -1,7 +1,7 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 
-#![deny(while_let_loop, empty_loop)]
+#![deny(while_let_loop, empty_loop, while_let_on_iterator)]
 #![allow(dead_code, unused)]
 
 fn main() {
@@ -53,6 +53,50 @@ fn main() {
     while let Some(x) = y { // no error, obviously
         println!("{}", x);
     }
+
+    let mut iter = 1..20;
+    while let Option::Some(x) = iter.next() { //~ERROR this loop could be written as a `for` loop
+        println!("{}", x);
+    }
+
+    let mut iter = 1..20;
+    while let Some(x) = iter.next() { //~ERROR this loop could be written as a `for` loop
+        println!("{}", x);
+    }
+
+    let mut iter = 1..20;
+    while let Some(_) = iter.next() {} //~ERROR this loop could be written as a `for` loop
+
+    let mut iter = 1..20;
+    while let None = iter.next() {} // this is fine (if nonsensical)
+
+    let mut iter = 1..20;
+    if let Some(x) = iter.next() { // also fine
+        println!("{}", x)
+    }
+
+    // the following shouldn't warn because it can't be written with a for loop
+    let mut iter = 1u32..20;
+    while let Some(x) = iter.next() {
+        println!("next: {:?}", iter.next())
+    }
+
+    // neither can this
+    let mut iter = 1u32..20;
+    while let Some(x) = iter.next() {
+        println!("next: {:?}", iter.next());
+    }
+
+    // or this
+    let mut iter = 1u32..20;
+    while let Some(x) = iter.next() {break;}
+    println!("Remaining iter {:?}", iter);
+
+    // or this
+    let mut iter = 1u32..20;
+    while let Some(x) = iter.next() {
+        iter = 1..20;
+    }
 }
 
 // regression test (#360)