Skip to content

Commit

Permalink
Auto merge of #12906 - lochetti:manual_unwrap_or_if_let, r=y21
Browse files Browse the repository at this point in the history 
Lint `manual_unwrap_or` for it let cases

This PR modifies `manual_unwrap_or` to lint for `if let` cases as well. This effort is part of the fixes desired by rust-lang/rust-clippy#12618

changelog:[`manual_unwrap_or`]: Lint for `if let` cases.
  • Loading branch information
@bors
bors committed Jun 17, 2024
2 parents 51c5eee + 70ca9a1 commit 9e54ff2
Show file tree
Hide file tree
Showing 13 changed files with 276 additions and 77 deletions.
111 changes: 79 additions & 32 deletions clippy_lints/src/matches/manual_unwrap_or.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,46 +3,78 @@ use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::{indent_of, reindent_multiline, snippet_opt};
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::usage::contains_return_break_continue_macro;
use clippy_utils::{is_res_lang_ctor, path_to_local_id, sugg};
use clippy_utils::{is_res_lang_ctor, path_to_local_id, peel_blocks, sugg};
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::LangItem::{OptionNone, ResultErr};
use rustc_hir::{Arm, Expr, PatKind};
use rustc_hir::{Arm, Expr, Pat, PatKind};
use rustc_lint::LateContext;
use rustc_middle::ty::Ty;
use rustc_span::sym;

use super::MANUAL_UNWRAP_OR;

pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, scrutinee: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>]) {
pub(super) fn check_match<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'tcx>,
scrutinee: &'tcx Expr<'_>,
arms: &'tcx [Arm<'_>],
) {
let ty = cx.typeck_results().expr_ty(scrutinee);
if let Some(ty_name) = if is_type_diagnostic_item(cx, ty, sym::Option) {
if let Some((or_arm, unwrap_arm)) = applicable_or_arm(cx, arms) {
check_and_lint(cx, expr, unwrap_arm.pat, scrutinee, unwrap_arm.body, or_arm.body, ty);
}
}

pub(super) fn check_if_let<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
let_pat: &'tcx Pat<'_>,
let_expr: &'tcx Expr<'_>,
then_expr: &'tcx Expr<'_>,
else_expr: &'tcx Expr<'_>,
) {
let ty = cx.typeck_results().expr_ty(let_expr);
check_and_lint(cx, expr, let_pat, let_expr, then_expr, peel_blocks(else_expr), ty);
}

fn check_and_lint<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
let_pat: &'tcx Pat<'_>,
let_expr: &'tcx Expr<'_>,
then_expr: &'tcx Expr<'_>,
else_expr: &'tcx Expr<'_>,
ty: Ty<'tcx>,
) {
if let PatKind::TupleStruct(ref qpath, [unwrap_pat], _) = let_pat.kind
&& let Res::Def(DefKind::Ctor(..), ctor_id) = cx.qpath_res(qpath, let_pat.hir_id)
&& let Some(variant_id) = cx.tcx.opt_parent(ctor_id)
&& (cx.tcx.lang_items().option_some_variant() == Some(variant_id)
|| cx.tcx.lang_items().result_ok_variant() == Some(variant_id))
&& let PatKind::Binding(_, binding_hir_id, ..) = unwrap_pat.kind
&& path_to_local_id(peel_blocks(then_expr), binding_hir_id)
&& cx.typeck_results().expr_adjustments(then_expr).is_empty()
&& let Some(ty_name) = find_type_name(cx, ty)
&& let Some(or_body_snippet) = snippet_opt(cx, else_expr.span)
&& let Some(indent) = indent_of(cx, expr.span)
&& constant_simple(cx, cx.typeck_results(), else_expr).is_some()
{
lint(cx, expr, let_expr, ty_name, or_body_snippet, indent);
}
}

fn find_type_name<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<&'static str> {
if is_type_diagnostic_item(cx, ty, sym::Option) {
Some("Option")
} else if is_type_diagnostic_item(cx, ty, sym::Result) {
Some("Result")
} else {
None
} && let Some(or_arm) = applicable_or_arm(cx, arms)
&& let Some(or_body_snippet) = snippet_opt(cx, or_arm.body.span)
&& let Some(indent) = indent_of(cx, expr.span)
&& constant_simple(cx, cx.typeck_results(), or_arm.body).is_some()
{
let reindented_or_body = reindent_multiline(or_body_snippet.into(), true, Some(indent));

let mut app = Applicability::MachineApplicable;
let suggestion = sugg::Sugg::hir_with_context(cx, scrutinee, expr.span.ctxt(), "..", &mut app).maybe_par();
span_lint_and_sugg(
cx,
MANUAL_UNWRAP_OR,
expr.span,
format!("this pattern reimplements `{ty_name}::unwrap_or`"),
"replace with",
format!("{suggestion}.unwrap_or({reindented_or_body})",),
app,
);
}
}

fn applicable_or_arm<'a>(cx: &LateContext<'_>, arms: &'a [Arm<'a>]) -> Option<&'a Arm<'a>> {
fn applicable_or_arm<'a>(cx: &LateContext<'_>, arms: &'a [Arm<'a>]) -> Option<(&'a Arm<'a>, &'a Arm<'a>)> {
if arms.len() == 2
&& arms.iter().all(|arm| arm.guard.is_none())
&& let Some((idx, or_arm)) = arms.iter().enumerate().find(|(_, arm)| match arm.pat.kind {
Expand All @@ -54,18 +86,33 @@ fn applicable_or_arm<'a>(cx: &LateContext<'_>, arms: &'a [Arm<'a>]) -> Option<&'
_ => false,
})
&& let unwrap_arm = &arms[1 - idx]
&& let PatKind::TupleStruct(ref qpath, [unwrap_pat], _) = unwrap_arm.pat.kind
&& let Res::Def(DefKind::Ctor(..), ctor_id) = cx.qpath_res(qpath, unwrap_arm.pat.hir_id)
&& let Some(variant_id) = cx.tcx.opt_parent(ctor_id)
&& (cx.tcx.lang_items().option_some_variant() == Some(variant_id)
|| cx.tcx.lang_items().result_ok_variant() == Some(variant_id))
&& let PatKind::Binding(_, binding_hir_id, ..) = unwrap_pat.kind
&& path_to_local_id(unwrap_arm.body, binding_hir_id)
&& cx.typeck_results().expr_adjustments(unwrap_arm.body).is_empty()
&& !contains_return_break_continue_macro(or_arm.body)
{
Some(or_arm)
Some((or_arm, unwrap_arm))
} else {
None
}
}

fn lint<'tcx>(
cx: &LateContext<'tcx>,
expr: &Expr<'tcx>,
scrutinee: &'tcx Expr<'_>,
ty_name: &str,
or_body_snippet: String,
indent: usize,
) {
let reindented_or_body = reindent_multiline(or_body_snippet.into(), true, Some(indent));

let mut app = Applicability::MachineApplicable;
let suggestion = sugg::Sugg::hir_with_context(cx, scrutinee, expr.span.ctxt(), "..", &mut app).maybe_par();
span_lint_and_sugg(
cx,
MANUAL_UNWRAP_OR,
expr.span,
format!("this pattern reimplements `{ty_name}::unwrap_or`"),
"replace with",
format!("{suggestion}.unwrap_or({reindented_or_body})",),
app,
);
}
10 changes: 9 additions & 1 deletion clippy_lints/src/matches/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1069,7 +1069,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
redundant_guards::check(cx, arms, &self.msrv);

if !in_constant(cx, expr.hir_id) {
manual_unwrap_or::check(cx, expr, ex, arms);
manual_unwrap_or::check_match(cx, expr, ex, arms);
manual_map::check_match(cx, expr, ex, arms);
manual_filter::check_match(cx, ex, arms, expr);
}
Expand Down Expand Up @@ -1097,6 +1097,14 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
);
}
if !in_constant(cx, expr.hir_id) {
manual_unwrap_or::check_if_let(
cx,
expr,
if_let.let_pat,
if_let.let_expr,
if_let.if_then,
else_expr,
);
manual_map::check_if_let(cx, expr, if_let.let_pat, if_let.let_expr, if_let.if_then, else_expr);
manual_filter::check_if_let(
cx,
Expand Down
52 changes: 52 additions & 0 deletions tests/ui/manual_unwrap_or.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,32 @@ fn option_unwrap_or() {
Some(s) => s,
None => &format!("{} {}!", "hello", "world"),
};

Some(1).unwrap_or(42);

//don't lint
if let Some(x) = Some(1) {
x + 1
} else {
42
};
if let Some(x) = Some(1) {
x
} else {
return;
};
for j in 0..4 {
if let Some(x) = Some(j) {
x
} else {
continue;
};
if let Some(x) = Some(j) {
x
} else {
break;
};
}
}

fn result_unwrap_or() {
Expand Down Expand Up @@ -138,6 +164,32 @@ fn result_unwrap_or() {
Ok(s) => s,
Err(s) => "Bob",
};

Ok::<i32, i32>(1).unwrap_or(42);

//don't lint
if let Ok(x) = Ok::<i32, i32>(1) {
x + 1
} else {
42
};
if let Ok(x) = Ok::<i32, i32>(1) {
x
} else {
return;
};
for j in 0..4 {
if let Ok(x) = Ok::<i32, i32>(j) {
x
} else {
continue;
};
if let Ok(x) = Ok::<i32, i32>(j) {
x
} else {
break;
};
}
}

// don't lint in const fn
Expand Down
60 changes: 60 additions & 0 deletions tests/ui/manual_unwrap_or.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,36 @@ fn option_unwrap_or() {
Some(s) => s,
None => &format!("{} {}!", "hello", "world"),
};

if let Some(x) = Some(1) {
x
} else {
42
};

//don't lint
if let Some(x) = Some(1) {
x + 1
} else {
42
};
if let Some(x) = Some(1) {
x
} else {
return;
};
for j in 0..4 {
if let Some(x) = Some(j) {
x
} else {
continue;
};
if let Some(x) = Some(j) {
x
} else {
break;
};
}
}

fn result_unwrap_or() {
Expand Down Expand Up @@ -177,6 +207,36 @@ fn result_unwrap_or() {
Ok(s) => s,
Err(s) => "Bob",
};

if let Ok(x) = Ok::<i32, i32>(1) {
x
} else {
42
};

//don't lint
if let Ok(x) = Ok::<i32, i32>(1) {
x + 1
} else {
42
};
if let Ok(x) = Ok::<i32, i32>(1) {
x
} else {
return;
};
for j in 0..4 {
if let Ok(x) = Ok::<i32, i32>(j) {
x
} else {
continue;
};
if let Ok(x) = Ok::<i32, i32>(j) {
x
} else {
break;
};
}
}

// don't lint in const fn
Expand Down
Loading

0 comments on commit 9e54ff2

Please sign in to comment.