diff --git a/clippy_lints/src/ok_if_let.rs b/clippy_lints/src/if_let_some_result.rs similarity index 54% rename from clippy_lints/src/ok_if_let.rs rename to clippy_lints/src/if_let_some_result.rs index 6145fae31562..a9826d586339 100644 --- a/clippy_lints/src/ok_if_let.rs +++ b/clippy_lints/src/if_let_some_result.rs @@ -1,5 +1,6 @@ -use crate::utils::{match_type, method_chain_args, paths, snippet, span_help_and_lint}; +use crate::utils::{match_type, method_chain_args, paths, snippet_with_applicability, span_lint_and_sugg}; use if_chain::if_chain; +use rustc_errors::Applicability; use rustc_hir::*; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -39,20 +40,32 @@ declare_lint_pass!(OkIfLet => [IF_LET_SOME_RESULT]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OkIfLet { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { if_chain! { //begin checking variables - if let ExprKind::Match(ref op, ref body, ref source) = expr.kind; //test if expr is a match - if let MatchSource::IfLetDesugar { .. } = *source; //test if it is an If Let - if let ExprKind::MethodCall(_, _, ref result_types) = op.kind; //check is expr.ok() has type Result.ok() + if let ExprKind::Match(ref op, ref body, source) = expr.kind; //test if expr is a match + if let MatchSource::IfLetDesugar { .. } = source; //test if it is an If Let + if let ExprKind::MethodCall(_, ok_span, ref result_types) = op.kind; //check is expr.ok() has type Result.ok() if let PatKind::TupleStruct(QPath::Resolved(_, ref x), ref y, _) = body[0].pat.kind; //get operation if method_chain_args(op, &["ok"]).is_some(); //test to see if using ok() methoduse std::marker::Sized; + let is_result_type = match_type(cx, cx.tables.expr_ty(&result_types[0]), &paths::RESULT); + if print::to_string(print::NO_ANN, |s| s.print_path(x, false)) == "Some" && is_result_type; then { - let is_result_type = match_type(cx, cx.tables.expr_ty(&result_types[0]), &paths::RESULT); - let some_expr_string = snippet(cx, y[0].span, ""); - if print::to_string(print::NO_ANN, |s| s.print_path(x, false)) == "Some" && is_result_type { - span_help_and_lint(cx, IF_LET_SOME_RESULT, expr.span, + let mut applicability = Applicability::MachineApplicable; + let some_expr_string = snippet_with_applicability(cx, y[0].span, "", &mut applicability); + let trimmed_ok = snippet_with_applicability(cx, op.span.until(ok_span), "", &mut applicability); + let sugg = format!( + "if let Ok({}) = {}", + some_expr_string, + trimmed_ok.trim().trim_end_matches('.'), + ); + span_lint_and_sugg( + cx, + IF_LET_SOME_RESULT, + expr.span.with_hi(op.span.hi()), "Matching on `Some` with `ok()` is redundant", - &format!("Consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string)); - } + &format!("Consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string), + sugg, + applicability, + ); } } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5fe0d937f2f9..497146772a51 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -210,6 +210,7 @@ pub mod functions; pub mod get_last_with_len; pub mod identity_conversion; pub mod identity_op; +pub mod if_let_some_result; pub mod if_not_else; pub mod implicit_return; pub mod indexing_slicing; @@ -263,7 +264,6 @@ pub mod new_without_default; pub mod no_effect; pub mod non_copy_const; pub mod non_expressive_names; -pub mod ok_if_let; pub mod open_options; pub mod overflow_check_conditional; pub mod panic_unimplemented; @@ -545,6 +545,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &get_last_with_len::GET_LAST_WITH_LEN, &identity_conversion::IDENTITY_CONVERSION, &identity_op::IDENTITY_OP, + &if_let_some_result::IF_LET_SOME_RESULT, &if_not_else::IF_NOT_ELSE, &implicit_return::IMPLICIT_RETURN, &indexing_slicing::INDEXING_SLICING, @@ -703,7 +704,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &non_expressive_names::JUST_UNDERSCORES_AND_DIGITS, &non_expressive_names::MANY_SINGLE_CHAR_NAMES, &non_expressive_names::SIMILAR_NAMES, - &ok_if_let::IF_LET_SOME_RESULT, &open_options::NONSENSICAL_OPEN_OPTIONS, &overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL, &panic_unimplemented::PANIC, @@ -904,7 +904,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box eval_order_dependence::EvalOrderDependence); store.register_late_pass(|| box missing_doc::MissingDoc::new()); store.register_late_pass(|| box missing_inline::MissingInline); - store.register_late_pass(|| box ok_if_let::OkIfLet); + store.register_late_pass(|| box if_let_some_result::OkIfLet); store.register_late_pass(|| box redundant_pattern_matching::RedundantPatternMatching); store.register_late_pass(|| box partialeq_ne_impl::PartialEqNeImpl); store.register_late_pass(|| box unused_io_amount::UnusedIoAmount); @@ -1153,6 +1153,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&get_last_with_len::GET_LAST_WITH_LEN), LintId::of(&identity_conversion::IDENTITY_CONVERSION), LintId::of(&identity_op::IDENTITY_OP), + LintId::of(&if_let_some_result::IF_LET_SOME_RESULT), LintId::of(&indexing_slicing::OUT_OF_BOUNDS_INDEXING), LintId::of(&infallible_destructuring_match::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&infinite_iter::INFINITE_ITER), @@ -1265,7 +1266,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST), LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS), LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES), - LintId::of(&ok_if_let::IF_LET_SOME_RESULT), LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), LintId::of(&panic_unimplemented::PANIC_PARAMS), @@ -1367,6 +1367,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING), LintId::of(&functions::DOUBLE_MUST_USE), LintId::of(&functions::MUST_USE_UNIT), + LintId::of(&if_let_some_result::IF_LET_SOME_RESULT), LintId::of(&infallible_destructuring_match::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&inherent_to_string::INHERENT_TO_STRING), LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY), @@ -1413,7 +1414,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&new_without_default::NEW_WITHOUT_DEFAULT), LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS), LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES), - LintId::of(&ok_if_let::IF_LET_SOME_RESULT), LintId::of(&panic_unimplemented::PANIC_PARAMS), LintId::of(&ptr::CMP_NULL), LintId::of(&ptr::PTR_ARG), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index eee16fcd3122..c6339daf2ebe 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -705,7 +705,7 @@ pub const ALL_LINTS: [Lint; 347] = [ group: "style", desc: "usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead", deprecation: None, - module: "ok_if_let", + module: "if_let_some_result", }, Lint { name: "if_not_else", diff --git a/tests/ui/if_let_some_result.fixed b/tests/ui/if_let_some_result.fixed new file mode 100644 index 000000000000..80505fd997f4 --- /dev/null +++ b/tests/ui/if_let_some_result.fixed @@ -0,0 +1,35 @@ +// run-rustfix + +#![warn(clippy::if_let_some_result)] + +fn str_to_int(x: &str) -> i32 { + if let Ok(y) = x.parse() { + y + } else { + 0 + } +} + +fn str_to_int_ok(x: &str) -> i32 { + if let Ok(y) = x.parse() { + y + } else { + 0 + } +} + +#[rustfmt::skip] +fn strange_some_no_else(x: &str) -> i32 { + { + if let Ok(y) = x . parse() { + return y; + }; + 0 + } +} + +fn main() { + let _ = str_to_int("1"); + let _ = str_to_int_ok("2"); + let _ = strange_some_no_else("3"); +} diff --git a/tests/ui/if_let_some_result.rs b/tests/ui/if_let_some_result.rs new file mode 100644 index 000000000000..ecac13574456 --- /dev/null +++ b/tests/ui/if_let_some_result.rs @@ -0,0 +1,35 @@ +// run-rustfix + +#![warn(clippy::if_let_some_result)] + +fn str_to_int(x: &str) -> i32 { + if let Some(y) = x.parse().ok() { + y + } else { + 0 + } +} + +fn str_to_int_ok(x: &str) -> i32 { + if let Ok(y) = x.parse() { + y + } else { + 0 + } +} + +#[rustfmt::skip] +fn strange_some_no_else(x: &str) -> i32 { + { + if let Some(y) = x . parse() . ok () { + return y; + }; + 0 + } +} + +fn main() { + let _ = str_to_int("1"); + let _ = str_to_int_ok("2"); + let _ = strange_some_no_else("3"); +} diff --git a/tests/ui/if_let_some_result.stderr b/tests/ui/if_let_some_result.stderr new file mode 100644 index 000000000000..334ccb010167 --- /dev/null +++ b/tests/ui/if_let_some_result.stderr @@ -0,0 +1,25 @@ +error: Matching on `Some` with `ok()` is redundant + --> $DIR/if_let_some_result.rs:6:5 + | +LL | if let Some(y) = x.parse().ok() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::if-let-some-result` implied by `-D warnings` +help: Consider matching on `Ok(y)` and removing the call to `ok` instead + | +LL | if let Ok(y) = x.parse() { + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: Matching on `Some` with `ok()` is redundant + --> $DIR/if_let_some_result.rs:24:9 + | +LL | if let Some(y) = x . parse() . ok () { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Consider matching on `Ok(y)` and removing the call to `ok` instead + | +LL | if let Ok(y) = x . parse() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors + diff --git a/tests/ui/ok_if_let.rs b/tests/ui/ok_if_let.rs deleted file mode 100644 index 61db31130527..000000000000 --- a/tests/ui/ok_if_let.rs +++ /dev/null @@ -1,23 +0,0 @@ -#![warn(clippy::if_let_some_result)] - -fn str_to_int(x: &str) -> i32 { - if let Some(y) = x.parse().ok() { - 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); -} diff --git a/tests/ui/ok_if_let.stderr b/tests/ui/ok_if_let.stderr deleted file mode 100644 index e3e6c5c46343..000000000000 --- a/tests/ui/ok_if_let.stderr +++ /dev/null @@ -1,15 +0,0 @@ -error: Matching on `Some` with `ok()` is redundant - --> $DIR/ok_if_let.rs:4:5 - | -LL | / if let Some(y) = x.parse().ok() { -LL | | y -LL | | } else { -LL | | 0 -LL | | } - | |_____^ - | - = note: `-D clippy::if-let-some-result` implied by `-D warnings` - = help: Consider matching on `Ok(y)` and removing the call to `ok` instead - -error: aborting due to previous error -