From fa689f865e4096a1788c38fe9fef0c05dc15b9be Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 29 Mar 2021 20:39:28 -0400 Subject: [PATCH] Fix `manual_map` at the end of an if chain --- clippy_lints/src/manual_map.rs | 5 ++- clippy_utils/src/lib.rs | 23 ++++++++----- tests/ui/manual_map_option.fixed | 7 +++- tests/ui/manual_map_option.rs | 9 +++++ tests/ui/manual_map_option.stderr | 55 ++++++++++++++++--------------- 5 files changed, 61 insertions(+), 38 deletions(-) diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs index ed157783b723..d6ef3aa1e778 100644 --- 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 index 9e4dcb600ed9..d6364625e704 100644 --- 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 index acb6a580cebd..5e26958041d3 100644 --- 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 index 3299e6177074..33eb81561050 100644 --- 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 index 048ccfb95824..cdc2c0e62a9b 100644 --- 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,7 +164,7 @@ 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)), @@ -172,16 +172,19 @@ 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 +error: manual implementation of `Option::map` + --> $DIR/manual_map_option.rs:194:12 | -LL | if let Some(_) = Some(0) { - | -------^^^^^^^---------- help: try this: `if Some(0).is_some()` - | - = 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) { | ____________^