Skip to content

Commit

Permalink
Fix manual_map at the end of an if chain
Browse files Browse the repository at this point in the history
  • Loading branch information
Jarcho committed Mar 30, 2021
1 parent 8e56a2b commit 46209c5
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 12 deletions.
5 changes: 2 additions & 3 deletions clippy_lints/src/manual_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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)
Expand Down
23 changes: 15 additions & 8 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
Expand Down
4 changes: 4 additions & 0 deletions tests/ui/manual_map_option.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -133,4 +133,8 @@ fn main() {
if Some(0).is_some() {
Some(0)
} else { Some(0).map(|x| x + 1) };

if true {
Some(0)
} else { Some(0).map(|x| x + 1) };
}
8 changes: 8 additions & 0 deletions tests/ui/manual_map_option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,12 @@ fn main() {
} else {
None
};

if true {
Some(0)
} else if let Some(x) = Some(0) {
Some(x + 1)
} else {
None
};
}
13 changes: 12 additions & 1 deletion tests/ui/manual_map_option.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -191,5 +191,16 @@ LL | | None
LL | | };
| |_____^ help: try this: `{ Some(0).map(|x| x + 1) }`

error: aborting due to 21 previous errors
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:201:12
|
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: aborting due to 22 previous errors

0 comments on commit 46209c5

Please sign in to comment.