From 9035264a8febac258d1def1d27978ab8cd1ff121 Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Fri, 10 Jan 2020 08:34:13 +0900 Subject: [PATCH] Add suggestion in `if_let_some_result` --- clippy_lints/src/ok_if_let.rs | 33 ++++++++++++++++++++++++++------- tests/ui/ok_if_let.fixed | 35 +++++++++++++++++++++++++++++++++++ tests/ui/ok_if_let.rs | 20 ++++++++++++++++---- tests/ui/ok_if_let.stderr | 28 +++++++++++++++++++++++++--- 4 files changed, 102 insertions(+), 14 deletions(-) create mode 100644 tests/ui/ok_if_let.fixed diff --git a/clippy_lints/src/ok_if_let.rs b/clippy_lints/src/ok_if_let.rs index 6145fae3156..9696951ac0c 100644 --- a/clippy_lints/src/ok_if_let.rs +++ b/clippy_lints/src/ok_if_let.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, 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}; @@ -40,18 +41,36 @@ 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 MatchSource::IfLetDesugar { contains_else_clause } = *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; 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, ""); + 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 mut sugg = format!( + "if let Ok({}) = {} {}", + some_expr_string, + // FIXME(JohnTitor): this trimming is hacky, probably can improve it + trimmed_ok.trim_matches('.'), + snippet(cx, body[0].span, ".."), + ); + if contains_else_clause { + sugg = format!("{} else {}", sugg, snippet(cx, body[1].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, - "Matching on `Some` with `ok()` is redundant", - &format!("Consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string)); + span_lint_and_sugg( + 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), + sugg, + applicability, + ); } } } diff --git a/tests/ui/ok_if_let.fixed b/tests/ui/ok_if_let.fixed new file mode 100644 index 00000000000..e7fdc972a19 --- /dev/null +++ b/tests/ui/ok_if_let.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 + } +} + +fn nested_some_no_else(x: &str) -> i32 { + { + if let Ok(y) = x.parse() { + return y; + }; + 0 + } +} + +fn main() { + let x = str_to_int("1"); + let y = str_to_int_ok("2"); + let z = nested_some_no_else("3"); + println!("{}{}{}", x, y, z); +} diff --git a/tests/ui/ok_if_let.rs b/tests/ui/ok_if_let.rs index 61db3113052..becadf3644f 100644 --- a/tests/ui/ok_if_let.rs +++ b/tests/ui/ok_if_let.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![warn(clippy::if_let_some_result)] fn str_to_int(x: &str) -> i32 { @@ -16,8 +18,18 @@ fn str_to_int_ok(x: &str) -> i32 { } } -fn main() { - let y = str_to_int("1"); - let z = str_to_int_ok("2"); - println!("{}{}", y, z); +fn nested_some_no_else(x: &str) -> i32 { + { + if let Some(y) = x.parse().ok() { + return y; + }; + 0 + } +} + +fn main() { + let x = str_to_int("1"); + let y = str_to_int_ok("2"); + let z = nested_some_no_else("3"); + println!("{}{}{}", x, y, z); } diff --git a/tests/ui/ok_if_let.stderr b/tests/ui/ok_if_let.stderr index e3e6c5c4634..4aa6057ba47 100644 --- a/tests/ui/ok_if_let.stderr +++ b/tests/ui/ok_if_let.stderr @@ -1,5 +1,5 @@ error: Matching on `Some` with `ok()` is redundant - --> $DIR/ok_if_let.rs:4:5 + --> $DIR/ok_if_let.rs:6:5 | LL | / if let Some(y) = x.parse().ok() { LL | | y @@ -9,7 +9,29 @@ 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 +help: Consider matching on `Ok(y)` and removing the call to `ok` instead + | +LL | if let Ok(y) = x.parse() { +LL | y +LL | } else { +LL | 0 +LL | } + | -error: aborting due to previous error +error: Matching on `Some` with `ok()` is redundant + --> $DIR/ok_if_let.rs:23:9 + | +LL | / if let Some(y) = x.parse().ok() { +LL | | return y; +LL | | }; + | |_________^ + | +help: Consider matching on `Ok(y)` and removing the call to `ok` instead + | +LL | if let Ok(y) = x.parse() { +LL | return y; +LL | }; + | + +error: aborting due to 2 previous errors