diff --git a/README.md b/README.md index 813d18683d5e..ae7113579017 100644 --- a/README.md +++ b/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 diff --git a/src/lib.rs b/src/lib.rs index ddaa3bf490c2..9cb9eb30561a 100755 --- a/src/lib.rs +++ b/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, diff --git a/src/loops.rs b/src/loops.rs index d1a3e4ac7ab6..3b7194812178 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -25,13 +25,16 @@ declare_lint!{ pub UNUSED_COLLECT, Warn, "`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 +72,37 @@ impl LintPass for LoopsPass { } } + // 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... + if let ExprLit(ref start_lit) = start_expr.node { + if let ExprLit(ref stop_lit) = stop_expr.node { + // and they are both integers... + if let LitInt(start_idx, _) = start_lit.node { + if let LitInt(stop_idx, _) = stop_lit.node { + // 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_lint(cx, REVERSE_RANGE_LOOP, expr.span, &format!( + "this range is empty and this for loop will never run. \ + Consider using `({}..{}).rev()` if you are attempting to \ + iterate over this range in reverse", stop_idx, start_idx)); + } + + // 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"); + } + } + } + } + } + } + if let ExprMethodCall(ref method, _, ref args) = arg.node { // just the receiver, no arguments if args.len() == 1 { @@ -126,7 +160,7 @@ impl LintPass for LoopsPass { 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. \ diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index c8d1d383c78c..d84b70025b82 100755 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -14,7 +14,7 @@ impl Unrelated { } } -#[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,18 @@ 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()` + println!("{}", i); + } + + for i in 5..5 { //~ERROR this range is empty and 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 _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`