diff --git a/CHANGELOG.md b/CHANGELOG.md index 4be0eb868ae9..fc8464decfa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -250,6 +250,7 @@ All notable changes to this project will be documented in this file. [`for_loop_over_option`]: https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option [`for_loop_over_result`]: https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result [`identity_op`]: https://github.com/Manishearth/rust-clippy/wiki#identity_op +[`if_let_some_result`]: https://github.com/Manishearth/rust-clippy/wiki#if_let_some_result [`if_not_else`]: https://github.com/Manishearth/rust-clippy/wiki#if_not_else [`if_same_then_else`]: https://github.com/Manishearth/rust-clippy/wiki#if_same_then_else [`ifs_same_cond`]: https://github.com/Manishearth/rust-clippy/wiki#ifs_same_cond diff --git a/README.md b/README.md index 8b119ecf6274..45f236aba4e5 100644 --- a/README.md +++ b/README.md @@ -174,7 +174,7 @@ You can check out this great service at [clippy.bashy.io](https://clippy.bashy.i ## Lints -There are 172 lints included in this crate: +There are 173 lints included in this crate: name | default | triggers on ---------------------------------------------------------------------------------------------------------------------|---------|---------------------------------------------------------------------------------------------------------------------------------- @@ -231,6 +231,7 @@ name [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` +[if_let_some_result](https://github.com/Manishearth/rust-clippy/wiki#if_let_some_result) | warn | usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead [if_not_else](https://github.com/Manishearth/rust-clippy/wiki#if_not_else) | allow | `if` branches that could be swapped so no negation operation is necessary on the condition [if_same_then_else](https://github.com/Manishearth/rust-clippy/wiki#if_same_then_else) | warn | if with the same *then* and *else* blocks [ifs_same_cond](https://github.com/Manishearth/rust-clippy/wiki#ifs_same_cond) | warn | consecutive `ifs` with the same condition diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4cfd27c9453a..d4431c2e4c71 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -106,6 +106,7 @@ pub mod neg_multiply; pub mod new_without_default; pub mod no_effect; pub mod non_expressive_names; +pub mod ok_if_let; pub mod open_options; pub mod overflow_check_conditional; pub mod panic; @@ -254,6 +255,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box let_if_seq::LetIfSeq); reg.register_late_lint_pass(box eval_order_dependence::EvalOrderDependence); reg.register_late_lint_pass(box missing_doc::MissingDoc::new()); + reg.register_late_lint_pass(box ok_if_let::OkIfLetPass); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -404,6 +406,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { no_effect::NO_EFFECT, no_effect::UNNECESSARY_OPERATION, non_expressive_names::MANY_SINGLE_CHAR_NAMES, + ok_if_let::IF_LET_SOME_RESULT, open_options::NONSENSICAL_OPEN_OPTIONS, overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL, panic::PANIC_PARAMS, diff --git a/clippy_lints/src/ok_if_let.rs b/clippy_lints/src/ok_if_let.rs new file mode 100644 index 000000000000..94e06a2b39da --- /dev/null +++ b/clippy_lints/src/ok_if_let.rs @@ -0,0 +1,53 @@ +use rustc::lint::*; +use rustc::hir::*; +use utils::{paths, method_chain_args, span_help_and_lint, match_type, snippet}; + +/// **What it does:*** Checks for unnecessary `ok()` in if let. +/// +/// **Why is this bad?** Calling `ok()` in if let is unnecessary, instead match on `Ok(pat)` +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rustc +/// for result in iter { +/// if let Some(bench) = try!(result).parse().ok() { +/// vec.push(bench) +/// } +/// } +/// ``` +declare_lint! { + pub IF_LET_SOME_RESULT, + Warn, + "usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead" +} + +#[derive(Copy, Clone)] +pub struct OkIfLetPass; + +impl LintPass for OkIfLetPass { + fn get_lints(&self) -> LintArray { + lint_array!(IF_LET_SOME_RESULT) + } +} + +impl LateLintPass for OkIfLetPass { + fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { + if_let_chain! {[ //begin checking variables + let ExprMatch(ref op, ref body, ref source) = expr.node, //test if expr is a match + let MatchSource::IfLetDesugar { .. } = *source, //test if it is an If Let + let ExprMethodCall(_, _, ref result_types) = op.node, //check is expr.ok() has type Result.ok() + let PatKind::TupleStruct(ref x, ref y, _) = body[0].pats[0].node, //get operation + let Some(_) = method_chain_args(op, &["ok"]) //test to see if using ok() methoduse std::marker::Sized; + + ], { + let is_result_type = match_type(cx, cx.tcx.expr_ty(&result_types[0]), &paths::RESULT); + let some_expr_string = snippet(cx, y[0].span, ""); + if print::path_to_string(x) == "Some" && is_result_type { + span_help_and_lint(cx, IF_LET_SOME_RESULT, expr.span, + "Matching on `Some` with `ok()` is redundant", + &format!("Consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string)); + } + }} + } +} diff --git a/clippy_lints/src/zero_div_zero.rs b/clippy_lints/src/zero_div_zero.rs index 71221933d56c..64ec3e5eec70 100644 --- a/clippy_lints/src/zero_div_zero.rs +++ b/clippy_lints/src/zero_div_zero.rs @@ -38,8 +38,8 @@ impl LateLintPass for Pass { // do something like 0.0/(2.0 - 2.0), but it would be nice to warn on that case too. let Some(Constant::Float(ref lhs_value, lhs_width)) = constant_simple(left), let Some(Constant::Float(ref rhs_value, rhs_width)) = constant_simple(right), - let Some(0.0) = lhs_value.parse().ok(), - let Some(0.0) = rhs_value.parse().ok() + let Ok(0.0) = lhs_value.parse(), + let Ok(0.0) = rhs_value.parse() ], { // since we're about to suggest a use of std::f32::NaN or std::f64::NaN, // match the precision of the literals that are given. diff --git a/tests/compile-fail/ok_if_let.rs b/tests/compile-fail/ok_if_let.rs new file mode 100644 index 000000000000..3676f473bcd0 --- /dev/null +++ b/tests/compile-fail/ok_if_let.rs @@ -0,0 +1,27 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(if_let_some_result)] + +fn str_to_int(x: &str) -> i32 { + if let Some(y) = x.parse().ok() { + //~^ERROR Matching on `Some` with `ok()` is redundant + y + } else { + 0 + } +} + +fn str_to_int_ok(x: &str) -> i32 { + if let Ok(y) = x.parse() { + y + } else { + 0 + } +} + +fn main() { + let y = str_to_int("1"); + let z = str_to_int_ok("2"); + println!("{}{}", y, z); +}