diff --git a/README.md b/README.md index 212d868c6988..ec1dd7f6dbb3 100644 --- a/README.md +++ b/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 102 lints included in this crate: +There are 104 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -37,6 +37,8 @@ name [extend_from_slice](https://github.com/Manishearth/rust-clippy/wiki#extend_from_slice) | warn | `.extend_from_slice(_)` is a faster way to extend a Vec by a slice [filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next) | warn | using `filter(p).next()`, which is more succinctly expressed as `.find(p)` [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp) | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) +[for_loop_over_option](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option) | warn | for-looping over an `Option`, which is more clearly expressed as an `if let` +[for_loop_over_result](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result) | warn | for-looping over a `Result`, which is more clearly expressed as an `if let` [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1` [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask) | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` [inline_always](https://github.com/Manishearth/rust-clippy/wiki#inline_always) | warn | `#[inline(always)]` is a bad idea in most cases diff --git a/src/lib.rs b/src/lib.rs index 90625e4bb5ca..82bd2d39a129 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -194,6 +194,8 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::EMPTY_LOOP, loops::EXPLICIT_COUNTER_LOOP, loops::EXPLICIT_ITER_LOOP, + loops::FOR_LOOP_OVER_OPTION, + loops::FOR_LOOP_OVER_RESULT, loops::ITER_NEXT_LOOP, loops::NEEDLESS_RANGE_LOOP, loops::REVERSE_RANGE_LOOP, diff --git a/src/loops.rs b/src/loops.rs index 699e6e525dab..74ff9edcb6b7 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -11,7 +11,7 @@ use std::collections::{HashSet, HashMap}; 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, get_enclosing_block}; -use utils::{HASHMAP_PATH, VEC_PATH, LL_PATH}; +use utils::{HASHMAP_PATH, VEC_PATH, LL_PATH, OPTION_PATH, RESULT_PATH}; /// **What it does:** This lint checks for looping over the range of `0..len` of some collection just to get the values by index. It is `Warn` by default. /// @@ -48,6 +48,26 @@ declare_lint!{ pub EXPLICIT_ITER_LOOP, Warn, declare_lint!{ pub ITER_NEXT_LOOP, Warn, "for-looping over `_.next()` which is probably not intended" } +/// **What it does:** This lint checks for `for` loops over `Option` values. It is `Warn` by default. +/// +/// **Why is this bad?** Readability. This is more clearly expressed as an `if let`. +/// +/// **Known problems:** None +/// +/// **Example:** `for x in option { .. }`. This should be `if let Some(x) = option { .. }`. +declare_lint!{ pub FOR_LOOP_OVER_OPTION, Warn, + "for-looping over an `Option`, which is more clearly expressed as an `if let`" } + +/// **What it does:** This lint checks for `for` loops over `Result` values. It is `Warn` by default. +/// +/// **Why is this bad?** Readability. This is more clearly expressed as an `if let`. +/// +/// **Known problems:** None +/// +/// **Example:** `for x in result { .. }`. This should be `if let Ok(x) = result { .. }`. +declare_lint!{ pub FOR_LOOP_OVER_RESULT, Warn, + "for-looping over a `Result`, which is more clearly expressed as an `if let`" } + /// **What it does:** This lint detects `loop + match` combinations that are easier written as a `while let` loop. It is `Warn` by default. /// /// **Why is this bad?** The `while let` loop is usually shorter and more readable @@ -248,7 +268,7 @@ impl LateLintPass for LoopsPass { fn check_for_loop(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &Expr) { check_for_loop_range(cx, pat, arg, body, expr); check_for_loop_reverse_range(cx, arg, expr); - check_for_loop_explicit_iter(cx, arg, expr); + check_for_loop_arg(cx, pat, arg, expr); check_for_loop_explicit_counter(cx, arg, body, expr); } @@ -373,7 +393,8 @@ fn check_for_loop_reverse_range(cx: &LateContext, arg: &Expr, expr: &Expr) { } } -fn check_for_loop_explicit_iter(cx: &LateContext, arg: &Expr, expr: &Expr) { +fn check_for_loop_arg(cx: &LateContext, pat: &Pat, arg: &Expr, expr: &Expr) { + let mut next_loop_linted = false; // whether or not ITER_NEXT_LOOP lint was used if let ExprMethodCall(ref method, _, ref args) = arg.node { // just the receiver, no arguments if args.len() == 1 { @@ -401,10 +422,40 @@ fn check_for_loop_explicit_iter(cx: &LateContext, arg: &Expr, expr: &Expr) { expr.span, "you are iterating over `Iterator::next()` which is an Option; this will compile but is \ probably not what you want"); + next_loop_linted = true; } } } + if !next_loop_linted { + check_arg_type(cx, pat, arg); + } +} +/// Check for `for` loops over `Option`s and `Results` +fn check_arg_type(cx: &LateContext, pat: &Pat, arg: &Expr) { + let ty = cx.tcx.expr_ty(arg); + if match_type(cx, ty, &OPTION_PATH) { + span_help_and_lint( + cx, + FOR_LOOP_OVER_OPTION, + arg.span, + &format!("for loop over `{0}`, which is an `Option`. This is more readably written as \ + an `if let` statement.", snippet(cx, arg.span, "_")), + &format!("consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`", + snippet(cx, pat.span, "_"), snippet(cx, arg.span, "_")) + ); + } + else if match_type(cx, ty, &RESULT_PATH) { + span_help_and_lint( + cx, + FOR_LOOP_OVER_RESULT, + arg.span, + &format!("for loop over `{0}`, which is a `Result`. This is more readably written as \ + an `if let` statement.", snippet(cx, arg.span, "_")), + &format!("consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`", + snippet(cx, pat.span, "_"), snippet(cx, arg.span, "_")) + ); + } } fn check_for_loop_explicit_counter(cx: &LateContext, arg: &Expr, body: &Expr, expr: &Expr) { diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 37ecd290175b..1fcbbf54d1f2 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -3,6 +3,75 @@ use std::collections::*; +#[deny(clippy)] +fn for_loop_over_option_and_result() { + let option = Some(1); + let result = option.ok_or("x not found"); + let v = vec![0,1,2]; + + // check FOR_LOOP_OVER_OPTION lint + + for x in option { + //~^ ERROR for loop over `option`, which is an `Option`. + //~| HELP consider replacing `for x in option` with `if let Some(x) = option` + println!("{}", x); + } + + // check FOR_LOOP_OVER_RESULT lint + + for x in result { + //~^ ERROR for loop over `result`, which is a `Result`. + //~| HELP consider replacing `for x in result` with `if let Ok(x) = result` + println!("{}", x); + } + + for x in option.ok_or("x not found") { + //~^ ERROR for loop over `option.ok_or("x not found")`, which is a `Result`. + //~| HELP consider replacing `for x in option.ok_or("x not found")` with `if let Ok(x) = option.ok_or("x not found")` + println!("{}", x); + } + + // make sure LOOP_OVER_NEXT lint takes precedence when next() is the last call in the chain + + for x in v.iter().next() { + //~^ ERROR you are iterating over `Iterator::next()` which is an Option + println!("{}", x); + } + + // make sure we lint when next() is not the last call in the chain + + for x in v.iter().next().and(Some(0)) { + //~^ ERROR for loop over `v.iter().next().and(Some(0))`, which is an `Option` + //~| HELP consider replacing `for x in v.iter().next().and(Some(0))` with `if let Some(x) = v.iter().next().and(Some(0))` + println!("{}", x); + } + + for x in v.iter().next().ok_or("x not found") { + //~^ ERROR for loop over `v.iter().next().ok_or("x not found")`, which is a `Result` + //~| HELP consider replacing `for x in v.iter().next().ok_or("x not found")` with `if let Ok(x) = v.iter().next().ok_or("x not found")` + println!("{}", x); + } + + // check for false positives + + // for loop false positive + for x in v { + println!("{}", x); + } + + // while let false positive for Option + while let Some(x) = option { + println!("{}", x); + break; + } + + // while let false positive for Option + while let Ok(x) = result { + println!("{}", x); + break; + } +} + struct Unrelated(Vec); impl Unrelated { fn next(&self) -> std::slice::Iter { @@ -209,4 +278,6 @@ fn main() { let mut index = 0; for _v in &vec { index += 1 } println!("index: {}", index); + + for_loop_over_option_and_result(); }