Auto merge of #7004 - Jarcho:manual_map_if_then_else, r=camsteffen

Fix `manual_map` at the end of an if chain changelog: Fix `manual_map` suggestion at the end of an if chain
rust-lang · Mar 30, 2021 · 44bf60f · 44bf60f
2 parents 0552852 + fa689f8
commit 44bf60f
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 38 deletions.
diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs
@@ -2,7 +2,7 @@ use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF};
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
 use clippy_utils::ty::{can_partially_move_ty, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable};
-use clippy_utils::{is_allowed, is_else_clause_of_if_let_else, match_def_path, match_var, paths, peel_hir_expr_refs};
+use clippy_utils::{is_allowed, is_else_clause, match_def_path, match_var, paths, peel_hir_expr_refs};
 use rustc_ast::util::parser::PREC_POSTFIX;
 use rustc_errors::Applicability;
 use rustc_hir::{
@@ -181,8 +181,7 @@ impl LateLintPass<'_> for ManualMap {
                 expr.span,
                 "manual implementation of `Option::map`",
                 "try this",
-                if matches!(match_kind, MatchSource::IfLetDesugar { .. }) && is_else_clause_of_if_let_else(cx.tcx, expr)
-                {
+                if matches!(match_kind, MatchSource::IfLetDesugar { .. }) && is_else_clause(cx.tcx, expr) {
                     format!("{{ {}{}.map({}) }}", scrutinee_str, as_ref_str, body_str)
                 } else {
                     format!("{}{}.map({})", scrutinee_str, as_ref_str, body_str)

diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -797,22 +797,29 @@ pub fn get_parent_as_impl(tcx: TyCtxt<'_>, id: HirId) -> Option<&Impl<'_>> {
     }
 }
 
-/// Checks if the given expression is the else clause in the expression `if let .. {} else {}`
-pub fn is_else_clause_of_if_let_else(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
+/// Checks if the given expression is the else clause of either an `if` or `if let` expression.
+pub fn is_else_clause(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
     let map = tcx.hir();
     let mut iter = map.parent_iter(expr.hir_id);
-    let arm_id = match iter.next() {
-        Some((id, Node::Arm(..))) => id,
-        _ => return false,
-    };
     match iter.next() {
+        Some((arm_id, Node::Arm(..))) => matches!(
+            iter.next(),
+            Some((
+                _,
+                Node::Expr(Expr {
+                    kind: ExprKind::Match(_, [_, else_arm], MatchSource::IfLetDesugar { .. }),
+                    ..
+                })
+            ))
+            if else_arm.hir_id == arm_id
+        ),
         Some((
             _,
             Node::Expr(Expr {
-                kind: ExprKind::Match(_, [_, else_arm], kind),
+                kind: ExprKind::If(_, _, Some(else_expr)),
                 ..
             }),
-        )) => else_arm.hir_id == arm_id && matches!(kind, MatchSource::IfLetDesugar { .. }),
+        )) => else_expr.hir_id == expr.hir_id,
         _ => false,
     }
 }

diff --git a/tests/ui/manual_map_option.fixed b/tests/ui/manual_map_option.fixed
@@ -7,6 +7,7 @@
     clippy::map_identity,
     clippy::unit_arg,
     clippy::match_ref_pats,
+    clippy::redundant_pattern_matching,
     dead_code
 )]
 
@@ -130,7 +131,11 @@ fn main() {
     }
 
     // #6847
-    if Some(0).is_some() {
+    if let Some(_) = Some(0) {
+        Some(0)
+    } else { Some(0).map(|x| x + 1) };
+
+    if true {
         Some(0)
     } else { Some(0).map(|x| x + 1) };
 }
diff --git a/tests/ui/manual_map_option.rs b/tests/ui/manual_map_option.rs
@@ -7,6 +7,7 @@
     clippy::map_identity,
     clippy::unit_arg,
     clippy::match_ref_pats,
+    clippy::redundant_pattern_matching,
     dead_code
 )]
 
@@ -195,4 +196,12 @@ fn main() {
     } else {
         None
     };
+
+    if true {
+        Some(0)
+    } else if let Some(x) = Some(0) {
+        Some(x + 1)
+    } else {
+        None
+    };
 }
diff --git a/tests/ui/manual_map_option.stderr b/tests/ui/manual_map_option.stderr
@@ -1,5 +1,5 @@
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:14:5
+  --> $DIR/manual_map_option.rs:15:5
    |
 LL | /     match Some(0) {
 LL | |         Some(_) => Some(2),
@@ -10,7 +10,7 @@ LL | |     };
    = note: `-D clippy::manual-map` implied by `-D warnings`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:19:5
+  --> $DIR/manual_map_option.rs:20:5
    |
 LL | /     match Some(0) {
 LL | |         Some(x) => Some(x + 1),
@@ -19,7 +19,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| x + 1)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:24:5
+  --> $DIR/manual_map_option.rs:25:5
    |
 LL | /     match Some("") {
 LL | |         Some(x) => Some(x.is_empty()),
@@ -28,7 +28,7 @@ LL | |     };
    | |_____^ help: try this: `Some("").map(|x| x.is_empty())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:29:5
+  --> $DIR/manual_map_option.rs:30:5
    |
 LL | /     if let Some(x) = Some(0) {
 LL | |         Some(!x)
@@ -38,7 +38,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| !x)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:36:5
+  --> $DIR/manual_map_option.rs:37:5
    |
 LL | /     match Some(0) {
 LL | |         Some(x) => { Some(std::convert::identity(x)) }
@@ -47,7 +47,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(std::convert::identity)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:41:5
+  --> $DIR/manual_map_option.rs:42:5
    |
 LL | /     match Some(&String::new()) {
 LL | |         Some(x) => Some(str::len(x)),
@@ -56,7 +56,7 @@ LL | |     };
    | |_____^ help: try this: `Some(&String::new()).map(|x| str::len(x))`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:51:5
+  --> $DIR/manual_map_option.rs:52:5
    |
 LL | /     match &Some([0, 1]) {
 LL | |         Some(x) => Some(x[0]),
@@ -65,7 +65,7 @@ LL | |     };
    | |_____^ help: try this: `Some([0, 1]).as_ref().map(|x| x[0])`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:56:5
+  --> $DIR/manual_map_option.rs:57:5
    |
 LL | /     match &Some(0) {
 LL | |         &Some(x) => Some(x * 2),
@@ -74,7 +74,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| x * 2)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:61:5
+  --> $DIR/manual_map_option.rs:62:5
    |
 LL | /     match Some(String::new()) {
 LL | |         Some(ref x) => Some(x.is_empty()),
@@ -83,7 +83,7 @@ LL | |     };
    | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:66:5
+  --> $DIR/manual_map_option.rs:67:5
    |
 LL | /     match &&Some(String::new()) {
 LL | |         Some(x) => Some(x.len()),
@@ -92,7 +92,7 @@ LL | |     };
    | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:71:5
+  --> $DIR/manual_map_option.rs:72:5
    |
 LL | /     match &&Some(0) {
 LL | |         &&Some(x) => Some(x + x),
@@ -101,7 +101,7 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| x + x)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:84:9
+  --> $DIR/manual_map_option.rs:85:9
    |
 LL | /         match &mut Some(String::new()) {
 LL | |             Some(x) => Some(x.push_str("")),
@@ -110,7 +110,7 @@ LL | |         };
    | |_________^ help: try this: `Some(String::new()).as_mut().map(|x| x.push_str(""))`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:90:5
+  --> $DIR/manual_map_option.rs:91:5
    |
 LL | /     match &mut Some(String::new()) {
 LL | |         Some(ref x) => Some(x.len()),
@@ -119,7 +119,7 @@ LL | |     };
    | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:95:5
+  --> $DIR/manual_map_option.rs:96:5
    |
 LL | /     match &mut &Some(String::new()) {
 LL | |         Some(x) => Some(x.is_empty()),
@@ -128,7 +128,7 @@ LL | |     };
    | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:100:5
+  --> $DIR/manual_map_option.rs:101:5
    |
 LL | /     match Some((0, 1, 2)) {
 LL | |         Some((x, y, z)) => Some(x + y + z),
@@ -137,7 +137,7 @@ LL | |     };
    | |_____^ help: try this: `Some((0, 1, 2)).map(|(x, y, z)| x + y + z)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:105:5
+  --> $DIR/manual_map_option.rs:106:5
    |
 LL | /     match Some([1, 2, 3]) {
 LL | |         Some([first, ..]) => Some(first),
@@ -146,7 +146,7 @@ LL | |     };
    | |_____^ help: try this: `Some([1, 2, 3]).map(|[first, ..]| first)`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:110:5
+  --> $DIR/manual_map_option.rs:111:5
    |
 LL | /     match &Some((String::new(), "test")) {
 LL | |         Some((x, y)) => Some((y, x)),
@@ -155,7 +155,7 @@ LL | |     };
    | |_____^ help: try this: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:168:5
+  --> $DIR/manual_map_option.rs:169:5
    |
 LL | /     match Some(0) {
 LL | |         Some(x) => Some(vec![x]),
@@ -164,24 +164,27 @@ LL | |     };
    | |_____^ help: try this: `Some(0).map(|x| vec![x])`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:173:5
+  --> $DIR/manual_map_option.rs:174:5
    |
 LL | /     match option_env!("") {
 LL | |         Some(x) => Some(String::from(x)),
 LL | |         None => None,
 LL | |     };
    | |_____^ help: try this: `option_env!("").map(String::from)`
 
-error: redundant pattern matching, consider using `is_some()`
-  --> $DIR/manual_map_option.rs:191:12
-   |
-LL |     if let Some(_) = Some(0) {
-   |     -------^^^^^^^---------- help: try this: `if Some(0).is_some()`
+error: manual implementation of `Option::map`
+  --> $DIR/manual_map_option.rs:194:12
    |
-   = note: `-D clippy::redundant-pattern-matching` implied by `-D warnings`
+LL |       } else if let Some(x) = Some(0) {
+   |  ____________^
+LL | |         Some(x + 1)
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^ help: try this: `{ Some(0).map(|x| x + 1) }`
 
 error: manual implementation of `Option::map`
-  --> $DIR/manual_map_option.rs:193:12
+  --> $DIR/manual_map_option.rs:202:12
    |
 LL |       } else if let Some(x) = Some(0) {
    |  ____________^