Skip to content

Commit

Permalink
Auto merge of #88163 - camsteffen:collapsible-match-fix, r=Manishearth
Browse files Browse the repository at this point in the history
Fix clippy::collapsible_match with let expressions

This fixes rust-lang/rust-clippy#7575 which is a regression from #80357. I am fixing the bug here instead of in the clippy repo (if that's okay) because a) the regression has not been synced yet and b) I would like to land the fix on nightly asap.

The fix is basically to re-generalize `match` and `if let` for the lint implementation (they were split because `if let` no longer desugars to `match` in the HIR).

Also fixes rust-lang/rust-clippy#7586 and fixes rust-lang/rust-clippy#7591
cc `@rust-lang/clippy`
`@xFrednet` do you want to review this?
  • Loading branch information
bors committed Aug 22, 2021
2 parents 1eb187c + ada9282 commit 7481e6d
Show file tree
Hide file tree
Showing 17 changed files with 136 additions and 150 deletions.
157 changes: 78 additions & 79 deletions src/tools/clippy/clippy_lints/src/collapsible_match.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::visitors::LocalUsedVisitor;
use clippy_utils::{higher, is_lang_ctor, path_to_local, peel_ref_operators, SpanlessEq};
use clippy_utils::{higher, is_lang_ctor, is_unit_expr, path_to_local, peel_ref_operators, SpanlessEq};
use if_chain::if_chain;
use rustc_hir::LangItem::OptionNone;
use rustc_hir::{Expr, ExprKind, Guard, HirId, Pat, PatKind, StmtKind};
use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, MatchSource, Pat, PatKind, StmtKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::{MultiSpan, Span};
Expand Down Expand Up @@ -49,104 +49,87 @@ declare_lint_pass!(CollapsibleMatch => [COLLAPSIBLE_MATCH]);

impl<'tcx> LateLintPass<'tcx> for CollapsibleMatch {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if let Some(higher::IfLet {
let_pat,
if_then,
if_else,
..
}) = higher::IfLet::hir(expr)
{
check_arm(cx, if_then, None, let_pat, if_else);

check_if_let(cx, if_then, let_pat);
}

if let ExprKind::Match(_expr, arms, _source) = expr.kind {
if let Some(wild_arm) = arms.iter().rfind(|arm| is_wild_like(cx, &arm.pat.kind, &arm.guard)) {
for arm in arms {
check_arm(cx, arm.body, arm.guard.as_ref(), arm.pat, Some(wild_arm.body));
match IfLetOrMatch::parse(cx, expr) {
Some(IfLetOrMatch::Match(_, arms, _)) => {
if let Some(els_arm) = arms.iter().rfind(|arm| arm_is_wild_like(cx, arm)) {
for arm in arms {
check_arm(cx, true, arm.pat, arm.body, arm.guard.as_ref(), Some(els_arm.body));
}
}
}

if let Some(first_arm) = arms.get(0) {
check_if_let(cx, &first_arm.body, &first_arm.pat);
Some(IfLetOrMatch::IfLet(_, pat, body, els)) => {
check_arm(cx, false, pat, body, None, els);
}
None => {}
}
}
}

fn check_arm<'tcx>(
cx: &LateContext<'tcx>,
outer_block: &'tcx Expr<'tcx>,
outer_guard: Option<&Guard<'tcx>>,
outer_is_match: bool,
outer_pat: &'tcx Pat<'tcx>,
wild_outer_block: Option<&'tcx Expr<'tcx>>,
outer_then_body: &'tcx Expr<'tcx>,
outer_guard: Option<&'tcx Guard<'tcx>>,
outer_else_body: Option<&'tcx Expr<'tcx>>
) {
let expr = strip_singleton_blocks(outer_block);
let inner_expr = strip_singleton_blocks(outer_then_body);
if_chain! {
if let ExprKind::Match(expr_in, arms_inner, _) = expr.kind;
// the outer arm pattern and the inner match
if expr_in.span.ctxt() == outer_pat.span.ctxt();
// there must be no more than two arms in the inner match for this lint
if arms_inner.len() == 2;
// no if guards on the inner match
if arms_inner.iter().all(|arm| arm.guard.is_none());
if let Some(inner) = IfLetOrMatch::parse(cx, inner_expr);
if let Some((inner_scrutinee, inner_then_pat, inner_else_body)) = match inner {
IfLetOrMatch::IfLet(scrutinee, pat, _, els) => Some((scrutinee, pat, els)),
IfLetOrMatch::Match(scrutinee, arms, ..) => if_chain! {
// if there are more than two arms, collapsing would be non-trivial
if arms.len() == 2 && arms.iter().all(|a| a.guard.is_none());
// one of the arms must be "wild-like"
if let Some(wild_idx) = arms.iter().rposition(|a| arm_is_wild_like(cx, a));
then {
let (then, els) = (&arms[1 - wild_idx], &arms[wild_idx]);
Some((scrutinee, then.pat, Some(els.body)))
} else {
None
}
},
};
if outer_pat.span.ctxt() == inner_scrutinee.span.ctxt();
// match expression must be a local binding
// match <local> { .. }
if let Some(binding_id) = path_to_local(peel_ref_operators(cx, expr_in));
// one of the branches must be "wild-like"
if let Some(wild_inner_arm_idx) = arms_inner.iter().rposition(|arm_inner| is_wild_like(cx, &arm_inner.pat.kind, &arm_inner.guard));
let (wild_inner_arm, non_wild_inner_arm) =
(&arms_inner[wild_inner_arm_idx], &arms_inner[1 - wild_inner_arm_idx]);
if !pat_contains_or(non_wild_inner_arm.pat);
if let Some(binding_id) = path_to_local(peel_ref_operators(cx, inner_scrutinee));
if !pat_contains_or(inner_then_pat);
// the binding must come from the pattern of the containing match arm
// ..<local>.. => match <local> { .. }
if let Some(binding_span) = find_pat_binding(outer_pat, binding_id);
// the "wild-like" branches must be equal
if wild_outer_block.map(|el| SpanlessEq::new(cx).eq_expr(wild_inner_arm.body, el)).unwrap_or(true);
// the "else" branches must be equal
if match (outer_else_body, inner_else_body) {
(None, None) => true,
(None, Some(e)) | (Some(e), None) => is_unit_expr(e),
(Some(a), Some(b)) => SpanlessEq::new(cx).eq_expr(a, b),
};
// the binding must not be used in the if guard
let mut used_visitor = LocalUsedVisitor::new(cx, binding_id);
if match outer_guard {
None => true,
Some(Guard::If(expr) | Guard::IfLet(_, expr)) => !used_visitor.check_expr(expr),
if outer_guard.map_or(true, |(Guard::If(e) | Guard::IfLet(_, e))| !used_visitor.check_expr(e));
// ...or anywhere in the inner expression
if match inner {
IfLetOrMatch::IfLet(_, _, body, els) => {
!used_visitor.check_expr(body) && els.map_or(true, |e| !used_visitor.check_expr(e))
},
IfLetOrMatch::Match(_, arms, ..) => !arms.iter().any(|arm| used_visitor.check_arm(arm)),
};
// ...or anywhere in the inner match
if !arms_inner.iter().any(|arm| used_visitor.check_arm(arm));
then {
span_lint_and_then(
cx,
COLLAPSIBLE_MATCH,
expr.span,
"unnecessary nested match",
|diag| {
let mut help_span = MultiSpan::from_spans(vec![binding_span, non_wild_inner_arm.pat.span]);
help_span.push_span_label(binding_span, "replace this binding".into());
help_span.push_span_label(non_wild_inner_arm.pat.span, "with this pattern".into());
diag.span_help(help_span, "the outer pattern can be modified to include the inner pattern");
},
let msg = format!(
"this `{}` can be collapsed into the outer `{}`",
if matches!(inner, IfLetOrMatch::Match(..)) { "match" } else { "if let" },
if outer_is_match { "match" } else { "if let" },
);
}
}
}

fn check_if_let<'tcx>(cx: &LateContext<'tcx>, outer_expr: &'tcx Expr<'tcx>, outer_pat: &'tcx Pat<'tcx>) {
let block_inner = strip_singleton_blocks(outer_expr);
if_chain! {
if let Some(higher::IfLet { if_then: inner_if_then, let_expr: inner_let_expr, let_pat: inner_let_pat, .. }) = higher::IfLet::hir(block_inner);
if let Some(binding_id) = path_to_local(peel_ref_operators(cx, inner_let_expr));
if let Some(binding_span) = find_pat_binding(outer_pat, binding_id);
let mut used_visitor = LocalUsedVisitor::new(cx, binding_id);
if !used_visitor.check_expr(inner_if_then);
then {
span_lint_and_then(
cx,
COLLAPSIBLE_MATCH,
block_inner.span,
"unnecessary nested `if let` or `match`",
inner_expr.span,
&msg,
|diag| {
let mut help_span = MultiSpan::from_spans(vec![binding_span, inner_let_pat.span]);
let mut help_span = MultiSpan::from_spans(vec![binding_span, inner_then_pat.span]);
help_span.push_span_label(binding_span, "replace this binding".into());
help_span.push_span_label(inner_let_pat.span, "with this pattern".into());
help_span.push_span_label(inner_then_pat.span, "with this pattern".into());
diag.span_help(help_span, "the outer pattern can be modified to include the inner pattern");
},
);
Expand All @@ -168,14 +151,30 @@ fn strip_singleton_blocks<'hir>(mut expr: &'hir Expr<'hir>) -> &'hir Expr<'hir>
expr
}

/// A "wild-like" pattern is wild ("_") or `None`.
/// For this lint to apply, both the outer and inner patterns
/// must have "wild-like" branches that can be combined.
fn is_wild_like(cx: &LateContext<'_>, pat_kind: &PatKind<'_>, arm_guard: &Option<Guard<'_>>) -> bool {
if arm_guard.is_some() {
enum IfLetOrMatch<'hir> {
Match(&'hir Expr<'hir>, &'hir [Arm<'hir>], MatchSource),
/// scrutinee, pattern, then block, else block
IfLet(&'hir Expr<'hir>, &'hir Pat<'hir>, &'hir Expr<'hir>, Option<&'hir Expr<'hir>>),
}

impl<'hir> IfLetOrMatch<'hir> {
fn parse(cx: &LateContext<'_>, expr: &Expr<'hir>) -> Option<Self> {
match expr.kind {
ExprKind::Match(expr, arms, source) => Some(Self::Match(expr, arms, source)),
_ => higher::IfLet::hir(cx, expr).map(|higher::IfLet { let_expr, let_pat, if_then, if_else }| {
Self::IfLet(let_expr, let_pat, if_then, if_else)
})
}
}
}

/// A "wild-like" arm has a wild (`_`) or `None` pattern and no guard. Such arms can be "collapsed"
/// into a single wild arm without any significant loss in semantics or readability.
fn arm_is_wild_like(cx: &LateContext<'_>, arm: &Arm<'_>) -> bool {
if arm.guard.is_some() {
return false;
}
match pat_kind {
match arm.pat.kind {
PatKind::Binding(..) | PatKind::Wild => true,
PatKind::Path(ref qpath) => is_lang_ctor(cx, qpath, OptionNone),
_ => false,
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/if_let_mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl<'tcx> LateLintPass<'tcx> for IfLetMutex {
if_then,
if_else: Some(if_else),
..
}) = higher::IfLet::hir(expr)
}) = higher::IfLet::hir(cx, expr)
{
op_visit.visit_expr(let_expr);
if op_visit.mutex_lock_called {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/if_let_some_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ declare_lint_pass!(OkIfLet => [IF_LET_SOME_RESULT]);
impl<'tcx> LateLintPass<'tcx> for OkIfLet {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if_chain! { //begin checking variables
if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(expr);
if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(cx, expr);
if let ExprKind::MethodCall(_, ok_span, ref result_types, _) = let_expr.kind; //check is expr.ok() has type Result<T,E>.ok(, _)
if let PatKind::TupleStruct(QPath::Resolved(_, ref x), ref y, _) = let_pat.kind; //get operation
if method_chain_args(let_expr, &["ok"]).is_some(); //test to see if using ok() methoduse std::marker::Sized;
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/loops/manual_flatten.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ pub(super) fn check<'tcx>(

if_chain! {
if let Some(inner_expr) = inner_expr;
if let Some(higher::IfLet { let_pat, let_expr, if_else: None, .. }) = higher::IfLet::hir(inner_expr);
if let Some(higher::IfLet { let_pat, let_expr, if_else: None, .. }) = higher::IfLet::hir(cx, inner_expr);
// Ensure match_expr in `if let` statement is the same as the pat from the for-loop
if let PatKind::Binding(_, pat_hir_id, _, _) = pat.kind;
if path_to_local_id(let_expr, pat_hir_id);
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/loops/while_let_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_block: &'
let_expr,
if_else: Some(if_else),
..
}) = higher::IfLet::hir(inner)
}) = higher::IfLet::hir(cx, inner)
{
if is_simple_break_expr(if_else) {
could_be_while_let(cx, expr, let_pat, let_expr);
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/manual_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl LateLintPass<'_> for ManualMap {
let_expr,
if_then,
if_else: Some(if_else),
}) = higher::IfLet::hir(expr)
}) = higher::IfLet::hir(cx, expr)
{
manage_lint(cx, expr, (&let_pat.kind, if_then), (&PatKind::Wild, if_else), let_expr);
}
Expand Down
20 changes: 6 additions & 14 deletions src/tools/clippy/clippy_lints/src/matches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ use clippy_utils::sugg::Sugg;
use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, match_type, peel_mid_ty_refs};
use clippy_utils::visitors::LocalUsedVisitor;
use clippy_utils::{
get_parent_expr, in_macro, is_expn_of, is_lang_ctor, is_lint_allowed, is_refutable, is_wild, meets_msrv, msrvs,
path_to_local, path_to_local_id, peel_hir_pat_refs, peel_n_hir_expr_refs, recurse_or_patterns, remove_blocks,
strip_pat_refs,
get_parent_expr, in_macro, is_expn_of, is_lang_ctor, is_lint_allowed, is_refutable, is_unit_expr, is_wild,
meets_msrv, msrvs, path_to_local, path_to_local_id, peel_hir_pat_refs, peel_n_hir_expr_refs, recurse_or_patterns,
remove_blocks, strip_pat_refs,
};
use clippy_utils::{paths, search_same, SpanlessEq, SpanlessHash};
use core::array;
Expand Down Expand Up @@ -634,7 +634,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
if let ExprKind::Match(ref ex, ref arms, _) = expr.kind {
check_match_ref_pats(cx, ex, arms.iter().map(|el| el.pat), expr);
}
if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(expr) {
if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(cx, expr) {
check_match_ref_pats(cx, let_expr, once(let_pat), expr);
}
}
Expand Down Expand Up @@ -1298,7 +1298,7 @@ fn check_match_like_matches<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>)
let_expr,
if_then,
if_else: Some(if_else),
}) = higher::IfLet::hir(expr)
}) = higher::IfLet::hir(cx, expr)
{
return find_matches_sugg(
cx,
Expand Down Expand Up @@ -1672,14 +1672,6 @@ fn type_ranges(ranges: &[SpannedRange<Constant>]) -> TypedRanges {
.collect()
}

fn is_unit_expr(expr: &Expr<'_>) -> bool {
match expr.kind {
ExprKind::Tup(v) if v.is_empty() => true,
ExprKind::Block(b, _) if b.stmts.is_empty() && b.expr.is_none() => true,
_ => false,
}
}

// Checks if arm has the form `None => None`
fn is_none_arm(cx: &LateContext<'_>, arm: &Arm<'_>) -> bool {
matches!(arm.pat.kind, PatKind::Path(ref qpath) if is_lang_ctor(cx, qpath, OptionNone))
Expand Down Expand Up @@ -1835,7 +1827,7 @@ mod redundant_pattern_match {
let_pat,
let_expr,
..
}) = higher::IfLet::ast(cx, expr)
}) = higher::IfLet::hir(cx, expr)
{
find_sugg_for_if_let(cx, expr, let_pat, let_expr, "if", if_else.is_some())
}
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/option_if_let_else.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: boo
fn detect_option_if_let_else<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<OptionIfLetElseOccurence> {
if_chain! {
if !in_macro(expr.span); // Don't lint macros, because it behaves weirdly
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(expr);
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(cx, expr);
if !is_else_clause(cx.tcx, expr);
if !is_result_ok(cx, let_expr); // Don't lint on Result::ok because a different lint does it already
if let PatKind::TupleStruct(struct_qpath, [inner_pat], _) = &let_pat.kind;
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/question_mark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl QuestionMark {

fn check_if_let_some_and_early_return_none(cx: &LateContext<'_>, expr: &Expr<'_>) {
if_chain! {
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(expr);
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(cx, expr);
if Self::is_option(cx, let_expr);

if let PatKind::TupleStruct(ref path1, fields, None) = let_pat.kind;
Expand Down
34 changes: 9 additions & 25 deletions src/tools/clippy/clippy_utils/src/higher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,7 @@ pub struct IfLet<'hir> {
}

impl<'hir> IfLet<'hir> {
#[inline]
pub fn ast(cx: &LateContext<'tcx>, expr: &Expr<'hir>) -> Option<Self> {
let rslt = Self::hir(expr)?;
Self::is_not_within_while_context(cx, expr)?;
Some(rslt)
}

#[inline]
pub const fn hir(expr: &Expr<'hir>) -> Option<Self> {
pub fn hir(cx: &LateContext<'_>, expr: &Expr<'hir>) -> Option<Self> {
if let ExprKind::If(
Expr {
kind: ExprKind::Let(let_pat, let_expr, _),
Expand All @@ -97,6 +89,14 @@ impl<'hir> IfLet<'hir> {
if_else,
) = expr.kind
{
let hir = cx.tcx.hir();
let mut iter = hir.parent_iter(expr.hir_id);
if let Some((_, Node::Block(Block { stmts: [], .. }))) = iter.next() {
if let Some((_, Node::Expr(Expr { kind: ExprKind::Loop(_, _, LoopSource::While, _), .. }))) = iter.next() {
// while loop desugar
return None;
}
}
return Some(Self {
let_pat,
let_expr,
Expand All @@ -106,22 +106,6 @@ impl<'hir> IfLet<'hir> {
}
None
}

#[inline]
fn is_not_within_while_context(cx: &LateContext<'tcx>, expr: &Expr<'hir>) -> Option<()> {
let hir = cx.tcx.hir();
let parent = hir.get_parent_node(expr.hir_id);
let parent_parent = hir.get_parent_node(parent);
let parent_parent_node = hir.get(parent_parent);
if let Node::Expr(Expr {
kind: ExprKind::Loop(_, _, LoopSource::While, _),
..
}) = parent_parent_node
{
return None;
}
Some(())
}
}

pub struct IfOrIfLet<'hir> {
Expand Down
4 changes: 4 additions & 0 deletions src/tools/clippy/clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,10 @@ pub fn in_macro(span: Span) -> bool {
}
}

pub fn is_unit_expr(expr: &Expr<'_>) -> bool {
matches!(expr.kind, ExprKind::Block(Block { stmts: [], expr: None, .. }, _) | ExprKind::Tup([]))
}

/// Checks if given pattern is a wildcard (`_`)
pub fn is_wild(pat: &Pat<'_>) -> bool {
matches!(pat.kind, PatKind::Wild)
Expand Down
Loading

0 comments on commit 7481e6d

Please sign in to comment.