Merge pull request #331 from swgillespie/empty-range

implement iterating over an empty range lint as described in #330

README.md

@@ -6,7 +6,7 @@ A collection of lints that give helpful tips to newbies and catch oversights.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 56 lints included in this crate:
+There are 57 lints included in this crate:
 
 name                                                                                                   | default | meaning
 -------------------------------------------------------------------------------------------------------|---------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -48,6 +48,7 @@ name
 [redundant_closure](https://github.com/Manishearth/rust-clippy/wiki#redundant_closure)                 | warn    | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`)
 [redundant_pattern](https://github.com/Manishearth/rust-clippy/wiki#redundant_pattern)                 | warn    | using `name @ _` in a pattern
 [result_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#result_unwrap_used)               | allow   | using `Result.unwrap()`, which might be better handled
+[reverse_range_loop](https://github.com/Manishearth/rust-clippy/wiki#reverse_range_loop)               | warn    | Iterating over an empty range, such as `10..0` or `5..5`
 [shadow_reuse](https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse)                           | allow   | rebinding a name to an expression that re-uses the original value, e.g. `let x = x + 1`
 [shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same)                             | allow   | rebinding a name to itself, e.g. `let mut x = &mut x`
 [shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated)                   | allow   | The name is re-bound without even using the original value

src/lib.rs

@@ -122,6 +122,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         loops::EXPLICIT_ITER_LOOP,
         loops::ITER_NEXT_LOOP,
         loops::NEEDLESS_RANGE_LOOP,
+        loops::REVERSE_RANGE_LOOP,
         loops::UNUSED_COLLECT,
         loops::WHILE_LET_LOOP,
         matches::MATCH_REF_PATS,

src/loops.rs

@@ -3,6 +3,7 @@
 use reexport::*;
 use rustc_front::visit::{Visitor, walk_expr};
 use rustc::middle::ty;
+use consts::{constant_simple, Constant};
 use std::collections::HashSet;
 
 use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type,
@@ -25,13 +26,16 @@
                "`collect()`ing an iterator without using the result; this is usually better \
                 written as a for loop" }
 
+declare_lint!{ pub REVERSE_RANGE_LOOP, Warn,
+               "Iterating over an empty range, such as `10..0` or `5..5`" }
+
 #[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)
+                    WHILE_LET_LOOP, UNUSED_COLLECT, REVERSE_RANGE_LOOP)
     }
 
     fn check_expr(&mut self, cx: &Context, expr: &Expr) {
@@ -69,6 +73,30 @@ fn check_expr(&mut self, cx: &Context, expr: &Expr) {
                 }
             }
 
+            // if this for loop is iterating over a two-sided range...
+            if let ExprRange(Some(ref start_expr), Some(ref stop_expr)) = arg.node {
+                // ...and both sides are compile-time constant integers...
+                if let Some(Constant::ConstantInt(start_idx, _)) = constant_simple(start_expr) {
+                    if let Some(Constant::ConstantInt(stop_idx, _)) = constant_simple(stop_expr) {
+                        // ...and the start index is greater than the stop index,
+                        // this loop will never run. This is often confusing for developers
+                        // who think that this will iterate from the larger value to the
+                        // smaller value.
+                        if start_idx > stop_idx {
+                            span_help_and_lint(cx, REVERSE_RANGE_LOOP, expr.span,
+                                "this range is empty so this for loop will never run",
+                                &format!("Consider using `({}..{}).rev()` if you are attempting to \
+                                iterate over this range in reverse", stop_idx, start_idx));
+                        } else if start_idx == stop_idx {
+                            // if they are equal, it's also problematic - this loop
+                            // will never run.
+                            span_lint(cx, REVERSE_RANGE_LOOP, expr.span,
+                                "this range is empty so this for loop will never run");
+                        }
+                    }
+                }
+            }
+
             if let ExprMethodCall(ref method, _, ref args) = arg.node {
                 // just the receiver, no arguments
                 if args.len() == 1 {
@@ -126,7 +154,7 @@ fn check_expr(&mut self, cx: &Context, expr: &Expr) {
     fn check_stmt(&mut self, cx: &Context, stmt: &Stmt) {
         if let StmtSemi(ref expr, _) = stmt.node {
             if let ExprMethodCall(ref method, _, ref args) = expr.node {
-                if args.len() == 1 && method.node.name == "collect" && 
+                if args.len() == 1 && method.node.name == "collect" &&
                         match_trait_method(cx, expr, &["core", "iter", "Iterator"]) {
                     span_lint(cx, UNUSED_COLLECT, expr.span, &format!(
                         "you are collect()ing an iterator and throwing away the result. \

tests/compile-fail/for_loop.rs

@@ -14,7 +14,7 @@ fn iter(&self) -> std::slice::Iter<u8> {
     }
 }
 
-#[deny(needless_range_loop, explicit_iter_loop, iter_next_loop)]
+#[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop)]
 #[deny(unused_collect)]
 #[allow(linkedlist)]
 fn main() {
@@ -34,6 +34,53 @@ fn main() {
         println!("{}", vec[i]);
     }
 
+    for i in 10..0 { //~ERROR this range is empty so this for loop will never run
+        println!("{}", i);
+    }
+
+    for i in 5..5 { //~ERROR this range is empty so this for loop will never run
+        println!("{}", i);
+    }
+
+    for i in 0..10 { // not an error, the start index is less than the end index
+        println!("{}", i);
+    }
+
+    for i in (10..0).rev() { // not an error, this is an established idiom for looping backwards on a range
+        println!("{}", i);
+    }
+
+    for i in (10..0).map(|x| x * 2) { // not an error, it can't be known what arbitrary methods do to a range
+        println!("{}", i);
+    }
+
+    // testing that the empty range lint folds constants
+    for i in 10..5+4 { //~ERROR this range is empty so this for loop will never run
+        println!("{}", i);
+    }
+
+    for i in (5+2)..(3-1) { //~ERROR this range is empty so this for loop will never run
+        println!("{}", i);
+    }
+
+    for i in (5+2)..(8-1) { //~ERROR this range is empty so this for loop will never run
+        println!("{}", i);
+    }
+
+    for i in (2*2)..(2*3) { // no error, 4..6 is fine
+        println!("{}", i);
+    }
+
+    let x = 42;
+    for i in x..10 { // no error, not constant-foldable
+        println!("{}", i);
+    }
+
+    /*
+    for i in (10..0).map(|x| x * 2) {
+        println!("{}", i);
+    }*/
+
     for _v in vec.iter() { } //~ERROR it is more idiomatic to loop over `&vec`
     for _v in vec.iter_mut() { } //~ERROR it is more idiomatic to loop over `&mut vec`