use the constant folder to generalize the lint a little bit and clean up the code. Add additional tests for things that should not be linted

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,
@@ -72,32 +73,25 @@ fn check_expr(&mut self, cx: &Context, expr: &Expr) {
                 }
             }
 
-            // if this for-loop is iterating over a two-sided range...
+            // 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 literals...
+                // ...and both sides are compile-time constant integers...
-                if let ExprLit(ref start_lit) = start_expr.node {
+                if let Some(Constant::ConstantInt(start_idx, _)) = constant_simple(start_expr) {
-                    if let ExprLit(ref stop_lit) = stop_expr.node {
+                    if let Some(Constant::ConstantInt(stop_idx, _)) = constant_simple(stop_expr) {
-                        // and they are both integers...
+                        // ...and the start index is greater than the stop index,
-                        if let LitInt(start_idx, _) = start_lit.node {
+                        // this loop will never run. This is often confusing for developers
-                            if let LitInt(stop_idx, _) = stop_lit.node {
+                        // who think that this will iterate from the larger value to the
-                                // and the start index is greater than the stop index,
+                        // smaller value.
-                                // this loop will never run. This is often confusing for developers
+                        if start_idx > stop_idx {
-                                // who think that this will iterate from the larger value to the
+                            span_help_and_lint(cx, REVERSE_RANGE_LOOP, expr.span,
-                                // smaller value.
+                                "this range is empty so this for loop will never run",
-                                if start_idx > stop_idx {
+                                &format!("Consider using `({}..{}).rev()` if you are attempting to \
-                                    span_lint(cx, REVERSE_RANGE_LOOP, expr.span, &format!(
+                                iterate over this range in reverse", stop_idx, start_idx));
-                                        "this range is empty and this for loop will never run. \
+                        } else if start_idx == stop_idx {
-                                         Consider using `({}..{}).rev()` if you are attempting to \
+                            // if they are equal, it's also problematic - this loop
-                                         iterate over this range in reverse", stop_idx, start_idx));
+                            // will never run.
-                                }
+                            span_lint(cx, REVERSE_RANGE_LOOP, expr.span,
-
+                                "this range is empty so this for loop will never run");
-                                // if they are equal, it's also problematic - this loop
-                                // will never run.
-                                if start_idx == stop_idx {
-                                    span_lint(cx, REVERSE_RANGE_LOOP, expr.span,
-                                        "this range is empty and this for loop will never run");
-                                }
-                            }
                         }
                     }
                 }

tests/compile-fail/for_loop.rs

@@ -34,11 +34,11 @@ fn main() {
         println!("{}", vec[i]);
     }
 
-    for i in 10..0 { //~ERROR this range is empty and this for loop will never run. Consider using `(0..10).rev()`
+    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 and this for loop will never run
+    for i in 5..5 { //~ERROR this range is empty so this for loop will never run
         println!("{}", i);
     }
 
@@ -46,6 +46,41 @@ fn main() {
         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`