From: Tim Nielens Date: Sat, 17 Oct 2020 23:11:59 +0000 (+0200) Subject: manual_unwrap_or / support Result::unwrap_or X-Git-Url: https://git.lizzy.rs/?a=commitdiff_plain;h=915ce3608724e6c900d1b5eb4412cac2fcace33a;p=rust.git manual_unwrap_or / support Result::unwrap_or --- diff --git a/clippy_lints/src/manual_unwrap_or.rs b/clippy_lints/src/manual_unwrap_or.rs index ddb8cc25077..f3f1e31abde 100644 --- a/clippy_lints/src/manual_unwrap_or.rs +++ b/clippy_lints/src/manual_unwrap_or.rs @@ -2,7 +2,7 @@ use crate::utils; use if_chain::if_chain; use rustc_errors::Applicability; -use rustc_hir::{def, Arm, Expr, ExprKind, PatKind, QPath}; +use rustc_hir::{def, Arm, Expr, ExprKind, Pat, PatKind, QPath}; use rustc_lint::LintContext; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::lint::in_external_macro; @@ -10,7 +10,7 @@ declare_clippy_lint! { /// **What it does:** - /// Finds patterns that reimplement `Option::unwrap_or`. + /// Finds patterns that reimplement `Option::unwrap_or` or `Result::unwrap_or`. /// /// **Why is this bad?** /// Concise code helps focusing on behavior instead of boilerplate. @@ -33,7 +33,7 @@ /// ``` pub MANUAL_UNWRAP_OR, complexity, - "finds patterns that can be encoded more concisely with `Option::unwrap_or`" + "finds patterns that can be encoded more concisely with `Option::unwrap_or` or `Result::unwrap_or`" } declare_lint_pass!(ManualUnwrapOr => [MANUAL_UNWRAP_OR]); @@ -43,32 +43,50 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { if in_external_macro(cx.sess(), expr.span) { return; } - lint_option_unwrap_or_case(cx, expr); + lint_manual_unwrap_or(cx, expr); } } -fn lint_option_unwrap_or_case<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - fn applicable_none_arm<'a>(arms: &'a [Arm<'a>]) -> Option<&'a Arm<'a>> { +#[derive(Copy, Clone)] +enum Case { + Option, + Result, +} + +impl Case { + fn unwrap_fn_path(&self) -> &str { + match self { + Case::Option => "Option::unwrap_or", + Case::Result => "Result::unwrap_or", + } + } +} + +fn lint_manual_unwrap_or<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + fn applicable_or_arm<'a>(arms: &'a [Arm<'a>]) -> Option<&'a Arm<'a>> { if_chain! { if arms.len() == 2; if arms.iter().all(|arm| arm.guard.is_none()); - if let Some((idx, none_arm)) = arms.iter().enumerate().find(|(_, arm)| - if let PatKind::Path(ref qpath) = arm.pat.kind { - utils::match_qpath(qpath, &utils::paths::OPTION_NONE) - } else { - false + if let Some((idx, or_arm)) = arms.iter().enumerate().find(|(_, arm)| + match arm.pat.kind { + PatKind::Path(ref some_qpath) => + utils::match_qpath(some_qpath, &utils::paths::OPTION_NONE), + PatKind::TupleStruct(ref err_qpath, &[Pat { kind: PatKind::Wild, .. }], _) => + utils::match_qpath(err_qpath, &utils::paths::RESULT_ERR), + _ => false, } ); - let some_arm = &arms[1 - idx]; - if let PatKind::TupleStruct(ref some_qpath, &[some_binding], _) = some_arm.pat.kind; - if utils::match_qpath(some_qpath, &utils::paths::OPTION_SOME); - if let PatKind::Binding(_, binding_hir_id, ..) = some_binding.kind; - if let ExprKind::Path(QPath::Resolved(_, body_path)) = some_arm.body.kind; + let unwrap_arm = &arms[1 - idx]; + if let PatKind::TupleStruct(ref unwrap_qpath, &[unwrap_pat], _) = unwrap_arm.pat.kind; + if utils::match_qpath(unwrap_qpath, &utils::paths::OPTION_SOME) + || utils::match_qpath(unwrap_qpath, &utils::paths::RESULT_OK); + if let PatKind::Binding(_, binding_hir_id, ..) = unwrap_pat.kind; + if let ExprKind::Path(QPath::Resolved(_, body_path)) = unwrap_arm.body.kind; if let def::Res::Local(body_path_hir_id) = body_path.res; if body_path_hir_id == binding_hir_id; - if !utils::usage::contains_return_break_continue_macro(none_arm.body); + if !utils::usage::contains_return_break_continue_macro(or_arm.body); then { - Some(none_arm) + Some(or_arm) } else { None } @@ -78,24 +96,35 @@ fn applicable_none_arm<'a>(arms: &'a [Arm<'a>]) -> Option<&'a Arm<'a>> { if_chain! { if let ExprKind::Match(scrutinee, match_arms, _) = expr.kind; let ty = cx.typeck_results().expr_ty(scrutinee); - if utils::is_type_diagnostic_item(cx, ty, sym!(option_type)); - if let Some(none_arm) = applicable_none_arm(match_arms); + if let Some(case) = if utils::is_type_diagnostic_item(cx, ty, sym!(option_type)) { + Some(Case::Option) + } else if utils::is_type_diagnostic_item(cx, ty, sym!(result_type)) { + Some(Case::Result) + } else { + None + }; + if let Some(or_arm) = applicable_or_arm(match_arms); if let Some(scrutinee_snippet) = utils::snippet_opt(cx, scrutinee.span); - if let Some(none_body_snippet) = utils::snippet_opt(cx, none_arm.body.span); + if let Some(or_body_snippet) = utils::snippet_opt(cx, or_arm.body.span); if let Some(indent) = utils::indent_of(cx, expr.span); - if constant_simple(cx, cx.typeck_results(), none_arm.body).is_some(); + if constant_simple(cx, cx.typeck_results(), or_arm.body).is_some(); then { - let reindented_none_body = - utils::reindent_multiline(none_body_snippet.into(), true, Some(indent)); + let reindented_or_body = + utils::reindent_multiline(or_body_snippet.into(), true, Some(indent)); + let wrap_in_parens = !matches!(scrutinee, Expr { kind: ExprKind::Call(..), .. }); + let l_paren = if wrap_in_parens { "(" } else { "" }; + let r_paren = if wrap_in_parens { ")" } else { "" }; utils::span_lint_and_sugg( cx, MANUAL_UNWRAP_OR, expr.span, - "this pattern reimplements `Option::unwrap_or`", + &format!("this pattern reimplements `{}`", case.unwrap_fn_path()), "replace with", format!( - "{}.unwrap_or({})", + "{}{}{}.unwrap_or({})", + l_paren, scrutinee_snippet, - reindented_none_body, + r_paren, + reindented_or_body, ), Applicability::MachineApplicable, ); diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 6301d623a2b..b930d9aedcf 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1183,7 +1183,7 @@ Lint { name: "manual_unwrap_or", group: "complexity", - desc: "finds patterns that can be encoded more concisely with `Option::unwrap_or`", + desc: "finds patterns that can be encoded more concisely with `Option::unwrap_or` or `Result::unwrap_or`", deprecation: None, module: "manual_unwrap_or", }, diff --git a/tests/ui/manual_unwrap_or.fixed b/tests/ui/manual_unwrap_or.fixed index a8736f1e6ef..ceb8985d3d5 100644 --- a/tests/ui/manual_unwrap_or.fixed +++ b/tests/ui/manual_unwrap_or.fixed @@ -1,7 +1,7 @@ // run-rustfix #![allow(dead_code)] -fn unwrap_or() { +fn option_unwrap_or() { // int case Some(1).unwrap_or(42); @@ -65,4 +65,46 @@ fn unwrap_or() { }; } +fn result_unwrap_or() { + // int case + (Ok(1) as Result).unwrap_or(42); + + // int case reversed + (Ok(1) as Result).unwrap_or(42); + + // richer none expr + (Ok(1) as Result).unwrap_or(1 + 42); + + // multiline case + #[rustfmt::skip] + (Ok(1) as Result).unwrap_or({ + 42 + 42 + + 42 + 42 + 42 + + 42 + 42 + 42 + }); + + // string case + (Ok("Bob") as Result<&str, &str>).unwrap_or("Alice"); + + // don't lint + match Ok(1) as Result { + Ok(i) => i + 2, + Err(_) => 42, + }; + match Ok(1) as Result { + Ok(i) => i, + Err(_) => return, + }; + for j in 0..4 { + match Ok(j) as Result { + Ok(i) => i, + Err(_) => continue, + }; + match Ok(j) as Result { + Ok(i) => i, + Err(_) => break, + }; + } +} + fn main() {} diff --git a/tests/ui/manual_unwrap_or.rs b/tests/ui/manual_unwrap_or.rs index bede8cffc32..beca1de0ed1 100644 --- a/tests/ui/manual_unwrap_or.rs +++ b/tests/ui/manual_unwrap_or.rs @@ -1,7 +1,7 @@ // run-rustfix #![allow(dead_code)] -fn unwrap_or() { +fn option_unwrap_or() { // int case match Some(1) { Some(i) => i, @@ -80,4 +80,61 @@ fn unwrap_or() { }; } +fn result_unwrap_or() { + // int case + match Ok(1) as Result { + Ok(i) => i, + Err(_) => 42, + }; + + // int case reversed + match Ok(1) as Result { + Err(_) => 42, + Ok(i) => i, + }; + + // richer none expr + match Ok(1) as Result { + Ok(i) => i, + Err(_) => 1 + 42, + }; + + // multiline case + #[rustfmt::skip] + match Ok(1) as Result { + Ok(i) => i, + Err(_) => { + 42 + 42 + + 42 + 42 + 42 + + 42 + 42 + 42 + } + }; + + // string case + match Ok("Bob") as Result<&str, &str> { + Ok(i) => i, + Err(_) => "Alice", + }; + + // don't lint + match Ok(1) as Result { + Ok(i) => i + 2, + Err(_) => 42, + }; + match Ok(1) as Result { + Ok(i) => i, + Err(_) => return, + }; + for j in 0..4 { + match Ok(j) as Result { + Ok(i) => i, + Err(_) => continue, + }; + match Ok(j) as Result { + Ok(i) => i, + Err(_) => break, + }; + } +} + fn main() {} diff --git a/tests/ui/manual_unwrap_or.stderr b/tests/ui/manual_unwrap_or.stderr index 674f2952635..5d465666caf 100644 --- a/tests/ui/manual_unwrap_or.stderr +++ b/tests/ui/manual_unwrap_or.stderr @@ -57,5 +57,62 @@ LL | | None => "Alice", LL | | }; | |_____^ help: replace with: `Some("Bob").unwrap_or("Alice")` -error: aborting due to 5 previous errors +error: this pattern reimplements `Result::unwrap_or` + --> $DIR/manual_unwrap_or.rs:85:5 + | +LL | / match Ok(1) as Result { +LL | | Ok(i) => i, +LL | | Err(_) => 42, +LL | | }; + | |_____^ help: replace with: `(Ok(1) as Result).unwrap_or(42)` + +error: this pattern reimplements `Result::unwrap_or` + --> $DIR/manual_unwrap_or.rs:91:5 + | +LL | / match Ok(1) as Result { +LL | | Err(_) => 42, +LL | | Ok(i) => i, +LL | | }; + | |_____^ help: replace with: `(Ok(1) as Result).unwrap_or(42)` + +error: this pattern reimplements `Result::unwrap_or` + --> $DIR/manual_unwrap_or.rs:97:5 + | +LL | / match Ok(1) as Result { +LL | | Ok(i) => i, +LL | | Err(_) => 1 + 42, +LL | | }; + | |_____^ help: replace with: `(Ok(1) as Result).unwrap_or(1 + 42)` + +error: this pattern reimplements `Result::unwrap_or` + --> $DIR/manual_unwrap_or.rs:104:5 + | +LL | / match Ok(1) as Result { +LL | | Ok(i) => i, +LL | | Err(_) => { +LL | | 42 + 42 +... | +LL | | } +LL | | }; + | |_____^ + | +help: replace with + | +LL | (Ok(1) as Result).unwrap_or({ +LL | 42 + 42 +LL | + 42 + 42 + 42 +LL | + 42 + 42 + 42 +LL | }); + | + +error: this pattern reimplements `Result::unwrap_or` + --> $DIR/manual_unwrap_or.rs:114:5 + | +LL | / match Ok("Bob") as Result<&str, &str> { +LL | | Ok(i) => i, +LL | | Err(_) => "Alice", +LL | | }; + | |_____^ help: replace with: `(Ok("Bob") as Result<&str, &str>).unwrap_or("Alice")` + +error: aborting due to 10 previous errors