mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #8443 - Jarcho:match_cfg_arm, r=flip1995

Browse Source 
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
This commit is contained in:
bors 2022-02-21 08:46:42 +00:00
commit 9e605ef80f
15 changed files with 314 additions and 156 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
clippy_lints/src/matches/match_like_matches.rs
View File

@ -3,7 +3,7 @@ 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;
@ -11,7 +11,7 @@ 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,
@ -19,7 +19,7 @@ pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool
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)]),
@ -27,25 +27,28 @@ pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool
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!`

152
clippy_lints/src/matches/match_same_arms.rs
View File

@ -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");
}
},
);
}
}

19
clippy_lints/src/matches/match_single_binding.rs
View File

@ -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;
@ -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);

144
clippy_lints/src/matches/mod.rs
View File

@ -1,8 +1,11 @@
use clippy_utils::source::{snippet_opt, walk_span_to_context};
use clippy_utils::{meets_msrv, msrvs};
use rustc_hir::{Expr, ExprKind, Local, MatchSource, Pat};
use rustc_hir::{Arm, Expr, ExprKind, Local, MatchSource, Pat};
use rustc_lexer::{tokenize, TokenKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_semver::RustcVersion;
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::{Span, SpanData, SyntaxContext};
mod infalliable_detructuring_match;
mod match_as_ref;
@ -604,33 +607,39 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
return;
}
redundant_pattern_match::check(cx, expr);
if let ExprKind::Match(ex, arms, source) = expr.kind {
if !contains_cfg_arm(cx, expr, ex, arms) {
if source == MatchSource::Normal {
if !(meets_msrv(self.msrv.as_ref(), &msrvs::MATCHES_MACRO)
&& match_like_matches::check_match(cx, expr, ex, arms))
{
match_same_arms::check(cx, arms);
}
if meets_msrv(self.msrv.as_ref(), &msrvs::MATCHES_MACRO) {
if !match_like_matches::check(cx, expr) {
match_same_arms::check(cx, expr);
redundant_pattern_match::check_match(cx, expr, ex, arms);
single_match::check(cx, ex, arms, expr);
match_bool::check(cx, ex, arms, expr);
overlapping_arms::check(cx, ex, arms);
match_wild_enum::check(cx, ex, arms);
match_as_ref::check(cx, ex, arms, expr);
if self.infallible_destructuring_match_linted {
self.infallible_destructuring_match_linted = false;
} else {
match_single_binding::check(cx, ex, arms, expr);
}
}
match_ref_pats::check(cx, ex, arms.iter().map(|el| el.pat), expr);
}
} else {
match_same_arms::check(cx, expr);
}
if let ExprKind::Match(ex, arms, MatchSource::Normal) = expr.kind {
single_match::check(cx, ex, arms, expr);
match_bool::check(cx, ex, arms, expr);
overlapping_arms::check(cx, ex, arms);
// These don't depend on a relationship between multiple arms
match_wild_err_arm::check(cx, ex, arms);
match_wild_enum::check(cx, ex, arms);
match_as_ref::check(cx, ex, arms, expr);
wild_in_or_pats::check(cx, arms);
if self.infallible_destructuring_match_linted {
self.infallible_destructuring_match_linted = false;
} else {
match_single_binding::check(cx, ex, arms, expr);
} else {
if meets_msrv(self.msrv.as_ref(), &msrvs::MATCHES_MACRO) {
match_like_matches::check(cx, expr);
}
}
if let ExprKind::Match(ex, arms, _) = expr.kind {
match_ref_pats::check(cx, ex, arms.iter().map(|el| el.pat), expr);
redundant_pattern_match::check(cx, expr);
}
}
@ -644,3 +653,94 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
extract_msrv_attr!(LateContext);
}
/// Checks if there are any arms with a `#[cfg(..)]` attribute.
fn contains_cfg_arm(cx: &LateContext<'_>, e: &Expr<'_>, scrutinee: &Expr<'_>, arms: &[Arm<'_>]) -> bool {
let Some(scrutinee_span) = walk_span_to_context(scrutinee.span, SyntaxContext::root()) else {
// Shouldn't happen, but treat this as though a `cfg` attribute were found
return true;
};
let start = scrutinee_span.hi();
let mut arm_spans = arms.iter().map(|arm| {
let data = arm.span.data();
(data.ctxt == SyntaxContext::root()).then(|| (data.lo, data.hi))
});
let end = e.span.hi();
// Walk through all the non-code space before each match arm. The space trailing the final arm is
// handled after the `try_fold` e.g.
//
// match foo {
// _________^- everything between the scrutinee and arm1
//| arm1 => (),
//|---^___________^ everything before arm2
//| #[cfg(feature = "enabled")]
//| arm2 => some_code(),
//|---^____________________^ everything before arm3
//| // some comment about arm3
//| arm3 => some_code(),
//|---^____________________^ everything after arm3
//| #[cfg(feature = "disabled")]
//| arm4 = some_code(),
//|};
//|^
let found = arm_spans.try_fold(start, |start, range| {
let Some((end, next_start)) = range else {
// Shouldn't happen as macros can't expand to match arms, but treat this as though a `cfg` attribute were
// found.
return Err(());
};
let span = SpanData {
lo: start,
hi: end,
ctxt: SyntaxContext::root(),
parent: None,
}
.span();
(!span_contains_cfg(cx, span)).then(|| next_start).ok_or(())
});
match found {
Ok(start) => {
let span = SpanData {
lo: start,
hi: end,
ctxt: SyntaxContext::root(),
parent: None,
}
.span();
span_contains_cfg(cx, span)
},
Err(()) => true,
}
}
/// Checks if the given span contains a `#[cfg(..)]` attribute
fn span_contains_cfg(cx: &LateContext<'_>, s: Span) -> bool {
let Some(snip) = snippet_opt(cx, s) else {
// Assume true. This would require either an invalid span, or one which crosses file boundaries.
return true;
};
let mut pos = 0usize;
let mut iter = tokenize(&snip).map(|t| {
let start = pos;
pos += t.len;
(t.kind, start..pos)
});
// Search for the token sequence [`#`, `[`, `cfg`]
while iter.any(|(t, _)| matches!(t, TokenKind::Pound)) {
let mut iter = iter.by_ref().skip_while(|(t, _)| {
matches!(
t,
TokenKind::Whitespace | TokenKind::LineComment { .. } | TokenKind::BlockComment { .. }
)
});
if matches!(iter.next(), Some((TokenKind::OpenBracket, _)))
&& matches!(iter.next(), Some((TokenKind::Ident, range)) if &snip[range.clone()] == "cfg")
{
return true;
}
}
false
}

12
clippy_lints/src/matches/redundant_pattern_match.rs
View File

@ -12,13 +12,13 @@ use rustc_errors::Applicability;
use rustc_hir::LangItem::{OptionNone, PollPending};
use rustc_hir::{
intravisit::{walk_expr, Visitor},
Arm, Block, Expr, ExprKind, LangItem, MatchSource, Node, Pat, PatKind, QPath, UnOp,
Arm, Block, Expr, ExprKind, LangItem, Node, Pat, PatKind, QPath, UnOp,
};
use rustc_lint::LateContext;
use rustc_middle::ty::{self, subst::GenericArgKind, DefIdTree, Ty};
use rustc_span::sym;
pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if let Some(higher::IfLet {
if_else,
let_pat,
@ -27,11 +27,7 @@ pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
}) = higher::IfLet::hir(cx, expr)
{
find_sugg_for_if_let(cx, expr, let_pat, let_expr, "if", if_else.is_some());
}
if let ExprKind::Match(op, arms, MatchSource::Normal) = &expr.kind {
find_sugg_for_match(cx, expr, op, arms);
}
if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) {
} else if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) {
find_sugg_for_if_let(cx, expr, let_pat, let_expr, "while", false);
}
}
@ -304,7 +300,7 @@ fn find_sugg_for_if_let<'tcx>(
);
}
fn find_sugg_for_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, op: &Expr<'_>, arms: &[Arm<'_>]) {
pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, op: &Expr<'_>, arms: &[Arm<'_>]) {
if arms.len() == 2 {
let node_pair = (&arms[0].pat.kind, &arms[1].pat.kind);

10
tests/ui/match_as_ref.fixed
View File

@ -32,4 +32,12 @@ mod issue4437 {
}
}
fn main() {}
fn main() {
// Don't lint
let _ = match Some(0) {
#[cfg(feature = "foo")]
Some(ref x) if *x > 50 => None,
Some(ref x) => Some(x),
None => None,
};
}

10
tests/ui/match_as_ref.rs
View File

@ -41,4 +41,12 @@ mod issue4437 {
}
}
fn main() {}
fn main() {
// Don't lint
let _ = match Some(0) {
#[cfg(feature = "foo")]
Some(ref x) if *x > 50 => None,
Some(ref x) => Some(x),
None => None,
};
}

8
tests/ui/match_bool.rs
View File

@ -50,6 +50,14 @@ fn match_bool() {
11..=20 => 2,
_ => 3,
};
// Don't lint
let _ = match test {
#[cfg(feature = "foo")]
true if option == 5 => 10,
true => 0,
false => 1,
};
}
fn main() {}

15
tests/ui/match_expr_like_matches_macro.fixed
View File

@ -146,4 +146,19 @@ fn main() {
let _res = matches!(&val, &Some(ref _a));
fun(val);
}
{
enum E {
A,
B,
C,
}
let _ = match E::A {
E::B => true,
#[cfg(feature = "foo")]
E::A => true,
_ => false,
};
}
}

15
tests/ui/match_expr_like_matches_macro.rs
View File

@ -181,4 +181,19 @@ fn main() {
};
fun(val);
}
{
enum E {
A,
B,
C,
}
let _ = match E::A {
E::B => true,
#[cfg(feature = "foo")]
E::A => true,
_ => false,
};
}
}

10
tests/ui/match_same_arms2.rs
View File

@ -166,4 +166,12 @@ fn match_expr_like_matches_macro_priority() {
};
}
fn main() {}
fn main() {
let _ = match Some(0) {
Some(0) => 0,
Some(1) => 1,
#[cfg(feature = "foo")]
Some(2) => 2,
_ => 1,
};
}

8
tests/ui/match_single_binding.fixed
View File

@ -106,10 +106,8 @@ fn main() {
0 => println!("Array index start"),
_ => println!("Not an array index start"),
}
// False negative
// Lint
let x = 1;
match x {
// =>
_ => println!("Not an array index start"),
}
println!("Not an array index start");
}

3
tests/ui/match_single_binding.rs
View File

@ -118,7 +118,8 @@ fn main() {
0 => println!("Array index start"),
_ => println!("Not an array index start"),
}
// False negative
// Lint
let x = 1;
match x {
// =>

11
tests/ui/match_single_binding.stderr
View File

@ -167,5 +167,14 @@ LL + unwrapped
LL ~ })
|
error: aborting due to 11 previous errors
error: this match could be replaced by its body itself
--> $DIR/match_single_binding.rs:124:5
|
LL | / match x {
LL | | // =>
LL | | _ => println!("Not an array index start"),
LL | | }
| |_____^ help: consider using the match body instead: `println!("Not an array index start");`
error: aborting due to 12 previous errors

8
tests/ui/single_match.rs
View File

@ -234,4 +234,12 @@ macro_rules! single_match {
fn main() {
single_match!(5);
// Don't lint
let _ = match Some(0) {
#[cfg(feature = "foo")]
Some(10) => 11,
Some(x) => x,
_ => 0,
};
}