Skip to content

Commit

Permalink
Auto merge of #8443 - Jarcho:match_cfg_arm, r=flip1995
Browse files Browse the repository at this point in the history 
Don't lint `match` expressions with `cfg`ed arms

Somehow there are no open issues related to this for any of the affected lints. At least none that I could fine from a quick search.

changelog: Don't lint `match` expressions with `cfg`ed arms in many cases
  • Loading branch information
@bors
bors committed Feb 21, 2022
2 parents 29ee5e2 + 78345b4 commit 9e605ef
Show file tree
Hide file tree
Showing 15 changed files with 314 additions and 156 deletions.
45 changes: 24 additions & 21 deletions clippy_lints/src/matches/match_like_matches.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,49 +3,52 @@ use clippy_utils::source::snippet_with_applicability;
use clippy_utils::{higher, is_wild};
use rustc_ast::{Attribute, LitKind};
use rustc_errors::Applicability;
use rustc_hir::{BorrowKind, Expr, ExprKind, Guard, MatchSource, Pat};
use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, Guard, Pat};
use rustc_lint::LateContext;
use rustc_middle::ty;
use rustc_span::source_map::Spanned;

use super::MATCH_LIKE_MATCHES_MACRO;

/// Lint a `match` or `if let .. { .. } else { .. }` expr that could be replaced by `matches!`
pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if let Some(higher::IfLet {
let_pat,
let_expr,
if_then,
if_else: Some(if_else),
}) = higher::IfLet::hir(cx, expr)
{
return find_matches_sugg(
find_matches_sugg(
cx,
let_expr,
IntoIterator::into_iter([(&[][..], Some(let_pat), if_then, None), (&[][..], None, if_else, None)]),
expr,
true,
);
}
}

if let ExprKind::Match(scrut, arms, MatchSource::Normal) = expr.kind {
return find_matches_sugg(
cx,
scrut,
arms.iter().map(|arm| {
(
cx.tcx.hir().attrs(arm.hir_id),
Some(arm.pat),
arm.body,
arm.guard.as_ref(),
)
}),
expr,
false,
);
}

false
pub(super) fn check_match<'tcx>(
cx: &LateContext<'tcx>,
e: &'tcx Expr<'_>,
scrutinee: &'tcx Expr<'_>,
arms: &'tcx [Arm<'tcx>],
) -> bool {
find_matches_sugg(
cx,
scrutinee,
arms.iter().map(|arm| {
(
cx.tcx.hir().attrs(arm.hir_id),
Some(arm.pat),
arm.body,
arm.guard.as_ref(),
)
}),
e,
false,
)
}

/// Lint a `match` or `if let` for replacement by `matches!`
Expand Down
152 changes: 75 additions & 77 deletions clippy_lints/src/matches/match_same_arms.rs
View file
Original file line number Diff line number Diff line change
@@ -1,96 +1,94 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::snippet;
use clippy_utils::{path_to_local, search_same, SpanlessEq, SpanlessHash};
use rustc_hir::{Arm, Expr, ExprKind, HirId, HirIdMap, HirIdSet, MatchSource, Pat, PatKind};
use rustc_hir::{Arm, Expr, HirId, HirIdMap, HirIdSet, Pat, PatKind};
use rustc_lint::LateContext;
use std::collections::hash_map::Entry;

use super::MATCH_SAME_ARMS;

pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
if let ExprKind::Match(_, arms, MatchSource::Normal) = expr.kind {
let hash = |&(_, arm): &(usize, &Arm<'_>)| -> u64 {
let mut h = SpanlessHash::new(cx);
h.hash_expr(arm.body);
h.finish()
};
pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) {
let hash = |&(_, arm): &(usize, &Arm<'_>)| -> u64 {
let mut h = SpanlessHash::new(cx);
h.hash_expr(arm.body);
h.finish()
};

let eq = |&(lindex, lhs): &(usize, &Arm<'_>), &(rindex, rhs): &(usize, &Arm<'_>)| -> bool {
let min_index = usize::min(lindex, rindex);
let max_index = usize::max(lindex, rindex);
let eq = |&(lindex, lhs): &(usize, &Arm<'_>), &(rindex, rhs): &(usize, &Arm<'_>)| -> bool {
let min_index = usize::min(lindex, rindex);
let max_index = usize::max(lindex, rindex);

let mut local_map: HirIdMap<HirId> = HirIdMap::default();
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
if_chain! {
if let Some(a_id) = path_to_local(a);
if let Some(b_id) = path_to_local(b);
let entry = match local_map.entry(a_id) {
Entry::Vacant(entry) => entry,
// check if using the same bindings as before
Entry::Occupied(entry) => return *entry.get() == b_id,
};
// the names technically don't have to match; this makes the lint more conservative
if cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id);
if cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b);
if pat_contains_local(lhs.pat, a_id);
if pat_contains_local(rhs.pat, b_id);
then {
entry.insert(b_id);
true
} else {
false
}
let mut local_map: HirIdMap<HirId> = HirIdMap::default();
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
if_chain! {
if let Some(a_id) = path_to_local(a);
if let Some(b_id) = path_to_local(b);
let entry = match local_map.entry(a_id) {
Entry::Vacant(entry) => entry,
// check if using the same bindings as before
Entry::Occupied(entry) => return *entry.get() == b_id,
};
// the names technically don't have to match; this makes the lint more conservative
if cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id);
if cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b);
if pat_contains_local(lhs.pat, a_id);
if pat_contains_local(rhs.pat, b_id);
then {
entry.insert(b_id);
true
} else {
false
}
};
// Arms with a guard are ignored, those can’t always be merged together
// This is also the case for arms in-between each there is an arm with a guard
(min_index..=max_index).all(|index| arms[index].guard.is_none())
&& SpanlessEq::new(cx)
.expr_fallback(eq_fallback)
.eq_expr(lhs.body, rhs.body)
// these checks could be removed to allow unused bindings
&& bindings_eq(lhs.pat, local_map.keys().copied().collect())
&& bindings_eq(rhs.pat, local_map.values().copied().collect())
}
};
// Arms with a guard are ignored, those can’t always be merged together
// This is also the case for arms in-between each there is an arm with a guard
(min_index..=max_index).all(|index| arms[index].guard.is_none())
&& SpanlessEq::new(cx)
.expr_fallback(eq_fallback)
.eq_expr(lhs.body, rhs.body)
// these checks could be removed to allow unused bindings
&& bindings_eq(lhs.pat, local_map.keys().copied().collect())
&& bindings_eq(rhs.pat, local_map.values().copied().collect())
};

let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
span_lint_and_then(
cx,
MATCH_SAME_ARMS,
j.body.span,
"this `match` has identical arm bodies",
|diag| {
diag.span_note(i.body.span, "same as this");
let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
span_lint_and_then(
cx,
MATCH_SAME_ARMS,
j.body.span,
"this `match` has identical arm bodies",
|diag| {
diag.span_note(i.body.span, "same as this");

// Note: this does not use `span_suggestion` on purpose:
// there is no clean way
// to remove the other arm. Building a span and suggest to replace it to ""
// makes an even more confusing error message. Also in order not to make up a
// span for the whole pattern, the suggestion is only shown when there is only
// one pattern. The user should know about `|` if they are already using it…
// Note: this does not use `span_suggestion` on purpose:
// there is no clean way
// to remove the other arm. Building a span and suggest to replace it to ""
// makes an even more confusing error message. Also in order not to make up a
// span for the whole pattern, the suggestion is only shown when there is only
// one pattern. The user should know about `|` if they are already using it…

let lhs = snippet(cx, i.pat.span, "<pat1>");
let rhs = snippet(cx, j.pat.span, "<pat2>");
let lhs = snippet(cx, i.pat.span, "<pat1>");
let rhs = snippet(cx, j.pat.span, "<pat2>");

if let PatKind::Wild = j.pat.kind {
// if the last arm is _, then i could be integrated into _
// note that i.pat cannot be _, because that would mean that we're
// hiding all the subsequent arms, and rust won't compile
diag.span_note(
i.body.span,
&format!(
"`{}` has the same arm body as the `_` wildcard, consider removing it",
lhs
),
);
} else {
diag.span_help(i.pat.span, &format!("consider refactoring into `{} | {}`", lhs, rhs,))
.help("...or consider changing the match arm bodies");
}
},
);
}
if let PatKind::Wild = j.pat.kind {
// if the last arm is _, then i could be integrated into _
// note that i.pat cannot be _, because that would mean that we're
// hiding all the subsequent arms, and rust won't compile
diag.span_note(
i.body.span,
&format!(
"`{}` has the same arm body as the `_` wildcard, consider removing it",
lhs
),
);
} else {
diag.span_help(i.pat.span, &format!("consider refactoring into `{} | {}`", lhs, rhs,))
.help("...or consider changing the match arm bodies");
}
},
);
}
}

Expand Down
19 changes: 1 addition & 18 deletions clippy_lints/src/matches/match_single_binding.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::{indent_of, snippet_block, snippet_opt, snippet_with_applicability};
use clippy_utils::source::{indent_of, snippet_block, snippet_with_applicability};
use clippy_utils::sugg::Sugg;
use clippy_utils::{get_parent_expr, is_refutable, peel_blocks};
use rustc_errors::Applicability;
Expand All @@ -14,23 +14,6 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e
return;
}

// HACK:
// This is a hack to deal with arms that are excluded by macros like `#[cfg]`. It is only used here
// to prevent false positives as there is currently no better way to detect if code was excluded by
// a macro. See PR #6435
if_chain! {
if let Some(match_snippet) = snippet_opt(cx, expr.span);
if let Some(arm_snippet) = snippet_opt(cx, arms[0].span);
if let Some(ex_snippet) = snippet_opt(cx, ex.span);
let rest_snippet = match_snippet.replace(&arm_snippet, "").replace(&ex_snippet, "");
if rest_snippet.contains("=>");
then {
// The code it self contains another thick arrow "=>"
// -> Either another arm or a comment
return;
}
}

let matched_vars = ex.span;
let bind_names = arms[0].pat.span;
let match_body = peel_blocks(arms[0].body);
Expand Down
Loading

0 comments on commit 9e605ef

Please sign in to comment.