From 1c15428339094c344ae149c8e9027b6474abd9eb Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 13:39:28 -0700 Subject: [PATCH 01/10] removed formatting changes --- clippy_lints/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fd42d91f41d9..4fe9f0b25bea 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, @@ -405,6 +407,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { no_effect::UNNECESSARY_OPERATION, non_expressive_names::MANY_SINGLE_CHAR_NAMES, open_options::NONSENSICAL_OPEN_OPTIONS, + ok_if_let::IF_LET_SOME_RESULT, overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL, panic::PANIC_PARAMS, precedence::PRECEDENCE, From ab9435a6d4334181c09bd56d55cfdb58b175f606 Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 13:42:11 -0700 Subject: [PATCH 02/10] ran update --- CHANGELOG.md | 1 + README.md | 3 ++- clippy_lints/src/lib.rs | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cca4245bfa73..4ad00aafcc6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -244,6 +244,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 7d6ed96681a5..372f7c633f82 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(x)` statements is unnecessary, match on `Ok(expr)` 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 4fe9f0b25bea..12cf1a68568b 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -406,8 +406,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { no_effect::NO_EFFECT, no_effect::UNNECESSARY_OPERATION, non_expressive_names::MANY_SINGLE_CHAR_NAMES, - open_options::NONSENSICAL_OPEN_OPTIONS, ok_if_let::IF_LET_SOME_RESULT, + open_options::NONSENSICAL_OPEN_OPTIONS, overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL, panic::PANIC_PARAMS, precedence::PRECEDENCE, From d152ca04bdacf1f278ba81991b4845eda33de9fd Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 13:44:23 -0700 Subject: [PATCH 03/10] updated zero_div_zero.rs --- clippy_lints/src/zero_div_zero.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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. From c18dc13f6e9ac3adced57c2410aa7b0f6bbeeb63 Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 13:48:52 -0700 Subject: [PATCH 04/10] added file again --- clippy_lints/src/ok_if_let.rs | 53 +++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 clippy_lints/src/ok_if_let.rs diff --git a/clippy_lints/src/ok_if_let.rs b/clippy_lints/src/ok_if_let.rs new file mode 100644 index 000000000000..2bb162685efc --- /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}; + +/// **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(x` +/// +/// **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(x)` statements is unnecessary, match on `Ok(expr)` 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 { contains_else_clause: _ } = *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 = print::pat_to_string(&y[0]); + 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)); + } + }} + } +} From 72a653d8d441e592d0f66d2639da47732bf8b5ee Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 13:49:29 -0700 Subject: [PATCH 05/10] readded files --- tests/compile-fail/ok_if_let.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 tests/compile-fail/ok_if_let.rs 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); +} From 1188f59102464ef229a72ce21f31535f1ba815d6 Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 13:53:10 -0700 Subject: [PATCH 06/10] fixed struct elison --- clippy_lints/src/ok_if_let.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/ok_if_let.rs b/clippy_lints/src/ok_if_let.rs index 2bb162685efc..f77fec0a43ef 100644 --- a/clippy_lints/src/ok_if_let.rs +++ b/clippy_lints/src/ok_if_let.rs @@ -35,7 +35,7 @@ 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 { contains_else_clause: _ } = *source, //test if it is an If Let + 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; From 357311476465fb0dbe3db4746e1d42ad9a0c485c Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 14:15:24 -0700 Subject: [PATCH 07/10] replaced pat_to_string with snippet --- clippy_lints/src/ok_if_let.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/ok_if_let.rs b/clippy_lints/src/ok_if_let.rs index f77fec0a43ef..c2069d98e770 100644 --- a/clippy_lints/src/ok_if_let.rs +++ b/clippy_lints/src/ok_if_let.rs @@ -1,10 +1,10 @@ use rustc::lint::*; use rustc::hir::*; -use utils::{paths, method_chain_args, span_help_and_lint, match_type}; +use utils::{paths, method_chain_args, span_help_and_lint, match_type, snippet_opt}; /// **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(x` +/// **Why is this bad?** Calling `ok()` in if let is unnecessary, instead match on `Ok(pat)` /// /// **Known problems:** None. /// @@ -19,7 +19,7 @@ use utils::{paths, method_chain_args, span_help_and_lint, match_type}; declare_lint! { pub IF_LET_SOME_RESULT, Warn, - "usage of `ok()` in `if let Some(x)` statements is unnecessary, match on `Ok(expr)` instead" + "usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead" } #[derive(Copy, Clone)] @@ -38,11 +38,11 @@ impl LateLintPass for OkIfLetPass { 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(some_expr_string) = snippet_opt(cx, y[0].span), 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 = print::pat_to_string(&y[0]); 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", From a57f21945bfbff83293a7d495d53faebc449a0a7 Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 14:17:22 -0700 Subject: [PATCH 08/10] ran update --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 372f7c633f82..b8a74fa39aea 100644 --- a/README.md +++ b/README.md @@ -231,7 +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(x)` statements is unnecessary, match on `Ok(expr)` instead +[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 From 97e9299a1024fcce1875f90b872194dae55f1329 Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 14:26:41 -0700 Subject: [PATCH 09/10] fixed array bounds checking --- clippy_lints/src/ok_if_let.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/ok_if_let.rs b/clippy_lints/src/ok_if_let.rs index c2069d98e770..a0c11bbb4ab0 100644 --- a/clippy_lints/src/ok_if_let.rs +++ b/clippy_lints/src/ok_if_let.rs @@ -38,11 +38,16 @@ impl LateLintPass for OkIfLetPass { 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(some_expr_string) = snippet_opt(cx, y[0].span), 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 mut some_expr_string = String::from(""); + if y.len() > 0 { + if let Some(x) = snippet_opt(cx, y[0].span) { + some_expr_string = x; + } + } 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", From c8aa35e15040ff30f4b5058c6afdd0184df653ba Mon Sep 17 00:00:00 2001 From: Kit Freddura Date: Sun, 2 Oct 2016 14:38:31 -0700 Subject: [PATCH 10/10] replaced snippet_opt with snippet for more terse code --- clippy_lints/src/ok_if_let.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/ok_if_let.rs b/clippy_lints/src/ok_if_let.rs index a0c11bbb4ab0..94e06a2b39da 100644 --- a/clippy_lints/src/ok_if_let.rs +++ b/clippy_lints/src/ok_if_let.rs @@ -1,6 +1,6 @@ use rustc::lint::*; use rustc::hir::*; -use utils::{paths, method_chain_args, span_help_and_lint, match_type, snippet_opt}; +use utils::{paths, method_chain_args, span_help_and_lint, match_type, snippet}; /// **What it does:*** Checks for unnecessary `ok()` in if let. /// @@ -42,12 +42,7 @@ impl LateLintPass for OkIfLetPass { ], { let is_result_type = match_type(cx, cx.tcx.expr_ty(&result_types[0]), &paths::RESULT); - let mut some_expr_string = String::from(""); - if y.len() > 0 { - if let Some(x) = snippet_opt(cx, y[0].span) { - some_expr_string = x; - } - } + 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",