From b37317b028cd0d4b60126e0bcf1402e60018f891 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 6 Jan 2022 02:54:35 -0500 Subject: [PATCH 1/5] Check if there are any overlapping patterns between equal arm bodies in `match_same_arm` --- clippy_lints/src/matches/match_same_arms.rs | 253 +++++++++++++++++++- tests/ui/match_same_arms2.rs | 13 + 2 files changed, 256 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs index d11dda57e6fd..6617cf4e47f8 100644 --- a/clippy_lints/src/matches/match_same_arms.rs +++ b/clippy_lints/src/matches/match_same_arms.rs @@ -1,19 +1,53 @@ 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, HirId, HirIdMap, HirIdSet, Pat, PatKind}; +use rustc_ast::ast::LitKind; +use rustc_hir::def_id::DefId; +use rustc_hir::{Arm, Expr, ExprKind, HirId, HirIdMap, HirIdSet, Pat, PatKind, RangeEnd}; use rustc_lint::LateContext; +use rustc_span::Symbol; use std::collections::hash_map::Entry; use super::MATCH_SAME_ARMS; -pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { +pub(super) 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 resolved_pats: Vec<_> = arms.iter().map(|a| ResolvedPat::from_pat(cx, a.pat)).collect(); + + // The furthast forwards a pattern can move without semantic changes + let forwards_blocking_idxs: Vec<_> = resolved_pats + .iter() + .enumerate() + .map(|(i, pat)| { + resolved_pats[i + 1..] + .iter() + .enumerate() + .find_map(|(j, other)| pat.can_also_match(other).then(|| i + 1 + j)) + .unwrap_or(resolved_pats.len()) + }) + .collect(); + + // The furthast backwards a pattern can move without semantic changes + let backwards_blocking_idxs: Vec<_> = resolved_pats + .iter() + .enumerate() + .map(|(i, pat)| { + resolved_pats[..i] + .iter() + .enumerate() + .rev() + .zip(forwards_blocking_idxs[..i].iter().copied().rev()) + .skip_while(|&(_, forward_block)| forward_block > i) + .find_map(|((j, other), forward_block)| (forward_block == i || pat.can_also_match(other)).then(|| j)) + .unwrap_or(0) + }) + .collect(); + 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); @@ -42,14 +76,16 @@ pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { } }; // 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()) + // If both arms overlap with an arm in between then these can't be merged either. + !(backwards_blocking_idxs[max_index] > min_index && forwards_blocking_idxs[min_index] < max_index) + && lhs.guard.is_none() + && rhs.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(); @@ -92,6 +128,203 @@ pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { } } +#[derive(Debug)] +enum ResolvedPat<'hir> { + Wild, + Struct(Option, Vec<(Symbol, ResolvedPat<'hir>)>), + Sequence(Option, Vec>, Option), + Or(Vec>), + Path(Option), + LitStr(Symbol), + LitBytes(&'hir [u8]), + LitInt(u128), + LitBool(bool), + Range(PatRange), +} + +#[derive(Debug)] +struct PatRange { + start: u128, + end: u128, + bounds: RangeEnd, +} +impl PatRange { + fn contains(&self, x: u128) -> bool { + x >= self.start + && match self.bounds { + RangeEnd::Included => x <= self.end, + RangeEnd::Excluded => x < self.end, + } + } + + fn overlaps(&self, other: &Self) -> bool { + !(self.is_empty() || other.is_empty()) + && match self.bounds { + RangeEnd::Included => self.end >= other.start, + RangeEnd::Excluded => self.end > other.start, + } + && match other.bounds { + RangeEnd::Included => self.start <= other.end, + RangeEnd::Excluded => self.start < other.end, + } + } + + fn is_empty(&self) -> bool { + match self.bounds { + RangeEnd::Included => false, + RangeEnd::Excluded => self.start == self.end, + } + } +} + +impl<'hir> ResolvedPat<'hir> { + fn from_pat(cx: &LateContext<'_>, pat: &'hir Pat<'_>) -> Self { + match pat.kind { + PatKind::Wild | PatKind::Binding(.., None) => Self::Wild, + PatKind::Binding(.., Some(pat)) | PatKind::Box(pat) | PatKind::Ref(pat, _) => Self::from_pat(cx, pat), + PatKind::Struct(ref path, fields, _) => { + let mut fields: Vec<_> = fields + .iter() + .map(|f| (f.ident.name, Self::from_pat(cx, f.pat))) + .collect(); + fields.sort_by_key(|&(name, _)| name); + Self::Struct(cx.qpath_res(path, pat.hir_id).opt_def_id(), fields) + }, + PatKind::TupleStruct(ref path, pats, wild_idx) => Self::Sequence( + cx.qpath_res(path, pat.hir_id).opt_def_id(), + pats.iter().map(|pat| Self::from_pat(cx, pat)).collect(), + wild_idx, + ), + PatKind::Or(pats) => Self::Or(pats.iter().map(|pat| Self::from_pat(cx, pat)).collect()), + PatKind::Path(ref path) => Self::Path(cx.qpath_res(path, pat.hir_id).opt_def_id()), + PatKind::Tuple(pats, wild_idx) => { + Self::Sequence(None, pats.iter().map(|pat| Self::from_pat(cx, pat)).collect(), wild_idx) + }, + PatKind::Lit(e) => match &e.kind { + ExprKind::Lit(lit) => match lit.node { + LitKind::Str(sym, _) => Self::LitStr(sym), + LitKind::ByteStr(ref bytes) => Self::LitBytes(&**bytes), + LitKind::Byte(val) => Self::LitInt(val.into()), + LitKind::Char(val) => Self::LitInt(val.into()), + LitKind::Int(val, _) => Self::LitInt(val), + LitKind::Bool(val) => Self::LitBool(val), + LitKind::Float(..) | LitKind::Err(_) => Self::Wild, + }, + _ => Self::Wild, + }, + PatKind::Range(start, end, bounds) => { + let start = match start { + None => 0, + Some(e) => match &e.kind { + ExprKind::Lit(lit) => match lit.node { + LitKind::Int(val, _) => val, + LitKind::Char(val) => val.into(), + LitKind::Byte(val) => val.into(), + _ => return Self::Wild, + }, + _ => return Self::Wild, + }, + }; + let (end, bounds) = match end { + None => (u128::MAX, RangeEnd::Included), + Some(e) => match &e.kind { + ExprKind::Lit(lit) => match lit.node { + LitKind::Int(val, _) => (val, bounds), + LitKind::Char(val) => (val.into(), bounds), + LitKind::Byte(val) => (val.into(), bounds), + _ => return Self::Wild, + }, + _ => return Self::Wild, + }, + }; + Self::Range(PatRange { start, end, bounds }) + }, + PatKind::Slice(pats, wild, pats2) => Self::Sequence( + None, + pats.iter() + .chain(pats2.iter()) + .map(|pat| Self::from_pat(cx, pat)) + .collect(), + wild.map(|_| pats.len()), + ), + } + } + + /// Checks if two patterns overlap in the values they can match assuming they are for the same + /// type. + fn can_also_match(&self, other: &Self) -> bool { + match (self, other) { + (Self::Wild, _) | (_, Self::Wild) => true, + (Self::Or(pats), other) | (other, Self::Or(pats)) => pats.iter().any(|pat| pat.can_also_match(other)), + (Self::Struct(lpath, lfields), Self::Struct(rpath, rfields)) => { + if lpath != rpath { + return false; + } + let mut rfields = rfields.iter(); + let mut rfield = match rfields.next() { + Some(x) => x, + None => return true, + }; + 'outer: for lfield in lfields { + loop { + if lfield.0 < rfield.0 { + continue 'outer; + } else if lfield.0 > rfield.0 { + rfield = match rfields.next() { + Some(x) => x, + None => return true, + }; + } else if !lfield.1.can_also_match(&rfield.1) { + return false; + } else { + rfield = match rfields.next() { + Some(x) => x, + None => return true, + }; + continue 'outer; + } + } + } + true + }, + (Self::Sequence(lpath, lpats, lwild_idx), Self::Sequence(rpath, rpats, rwild_idx)) => { + if lpath != rpath { + return false; + } + + let (lpats_start, lpats_end) = lwild_idx + .or(*rwild_idx) + .map_or((&**lpats, [].as_slice()), |idx| lpats.split_at(idx)); + let (rpats_start, rpats_end) = rwild_idx + .or(*lwild_idx) + .map_or((&**rpats, [].as_slice()), |idx| rpats.split_at(idx)); + + lpats_start + .iter() + .zip(rpats_start.iter()) + .all(|(lpat, rpat)| lpat.can_also_match(rpat)) + // `lpats_end` and `rpats_end` lengths may be disjointed, so start from the end and ignore any + // extras. + && lpats_end + .iter() + .rev() + .zip(rpats_end.iter().rev()) + .all(|(lpat, rpat)| lpat.can_also_match(rpat)) + }, + (Self::Path(x), Self::Path(y)) => x == y, + (Self::LitStr(x), Self::LitStr(y)) => x == y, + (Self::LitBytes(x), Self::LitBytes(y)) => x == y, + (Self::LitInt(x), Self::LitInt(y)) => x == y, + (Self::LitBool(x), Self::LitBool(y)) => x == y, + (Self::Range(x), Self::Range(y)) => x.overlaps(y), + (Self::Range(range), Self::LitInt(x)) | (Self::LitInt(x), Self::Range(range)) => range.contains(*x), + + // Todo: Lit* with Path, Range with Path, LitBytes with Sequence + _ => true, + } + } +} + fn pat_contains_local(pat: &Pat<'_>, id: HirId) -> bool { let mut result = false; pat.walk_short(|p| { diff --git a/tests/ui/match_same_arms2.rs b/tests/ui/match_same_arms2.rs index 67e1d518483c..6dc6c4172ee0 100644 --- a/tests/ui/match_same_arms2.rs +++ b/tests/ui/match_same_arms2.rs @@ -174,4 +174,17 @@ fn main() { Some(2) => 2, _ => 1, }; + + enum Foo { + X(u32), + Y(u32), + Z(u32), + } + + let _ = match Foo::X(0) { + Foo::X(0) => 1, + Foo::X(_) | Foo::Y(_) | Foo::Z(0) => 2, + Foo::Z(_) => 1, + _ => 0, + }; } From 5508f461b853345bdfbefe61fb4b7bccb433b302 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 6 Jan 2022 13:37:37 -0500 Subject: [PATCH 2/5] Use `DroplessArena` when allocating `ResolvedPat`s Fix tuple handling in `match_same_arms` --- clippy_lints/src/doc.rs | 15 ++- clippy_lints/src/lib.rs | 1 + clippy_lints/src/matches/match_same_arms.rs | 129 ++++++++++++-------- 3 files changed, 86 insertions(+), 59 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 16173580fd46..703aa458f44e 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -637,12 +637,6 @@ fn check_code(cx: &LateContext<'_>, text: &str, edition: Edition, span: Span) { loop { match parser.parse_item(ForceCollect::No) { Ok(Some(item)) => match &item.kind { - // Tests with one of these items are ignored - ItemKind::Static(..) - | ItemKind::Const(..) - | ItemKind::ExternCrate(..) - | ItemKind::ForeignMod(..) => return false, - // We found a main function ... ItemKind::Fn(box Fn { sig, body: Some(block), .. }) if item.ident.name == sym::main => { @@ -661,8 +655,13 @@ fn check_code(cx: &LateContext<'_>, text: &str, edition: Edition, span: Span) { return false; } }, - // Another function was found; this case is ignored too - ItemKind::Fn(..) => return false, + // Tests with one of these items are ignored + ItemKind::Static(..) + | ItemKind::Const(..) + | ItemKind::ExternCrate(..) + | ItemKind::ForeignMod(..) + // Another function was found; this case is ignored + | ItemKind::Fn(..) => return false, _ => {}, }, Ok(None) => break, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 504235d0d1ef..f2a079991444 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -23,6 +23,7 @@ // FIXME: switch to something more ergonomic here, once available. // (Currently there is no way to opt into sysroot crates without `extern crate`.) +extern crate rustc_arena; extern crate rustc_ast; extern crate rustc_ast_pretty; extern crate rustc_attr; diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs index 6617cf4e47f8..0afac347d080 100644 --- a/clippy_lints/src/matches/match_same_arms.rs +++ b/clippy_lints/src/matches/match_same_arms.rs @@ -1,10 +1,13 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; use clippy_utils::{path_to_local, search_same, SpanlessEq, SpanlessHash}; +use core::iter; +use rustc_arena::DroplessArena; use rustc_ast::ast::LitKind; use rustc_hir::def_id::DefId; use rustc_hir::{Arm, Expr, ExprKind, HirId, HirIdMap, HirIdSet, Pat, PatKind, RangeEnd}; use rustc_lint::LateContext; +use rustc_middle::ty; use rustc_span::Symbol; use std::collections::hash_map::Entry; @@ -17,7 +20,8 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { h.finish() }; - let resolved_pats: Vec<_> = arms.iter().map(|a| ResolvedPat::from_pat(cx, a.pat)).collect(); + let arena = DroplessArena::default(); + let resolved_pats: Vec<_> = arms.iter().map(|a| ResolvedPat::from_pat(cx, &arena, a.pat)).collect(); // The furthast forwards a pattern can move without semantic changes let forwards_blocking_idxs: Vec<_> = resolved_pats @@ -128,21 +132,22 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { } } -#[derive(Debug)] -enum ResolvedPat<'hir> { +#[derive(Clone, Copy)] +enum ResolvedPat<'hir, 'arena> { Wild, - Struct(Option, Vec<(Symbol, ResolvedPat<'hir>)>), - Sequence(Option, Vec>, Option), - Or(Vec>), + Struct(Option, &'arena [(Symbol, Self)]), + Tuple(Option, &'arena [Self]), + Or(&'arena [Self]), Path(Option), LitStr(Symbol), LitBytes(&'hir [u8]), LitInt(u128), LitBool(bool), Range(PatRange), + Slice(&'arena [Self], &'arena [Self], bool), } -#[derive(Debug)] +#[derive(Clone, Copy)] struct PatRange { start: u128, end: u128, @@ -177,28 +182,66 @@ impl PatRange { } } -impl<'hir> ResolvedPat<'hir> { - fn from_pat(cx: &LateContext<'_>, pat: &'hir Pat<'_>) -> Self { +#[allow(clippy::similar_names)] +impl<'hir, 'arena> ResolvedPat<'hir, 'arena> { + #[allow(clippy::too_many_lines)] + fn from_pat(cx: &LateContext<'_>, arena: &'arena DroplessArena, pat: &'hir Pat<'_>) -> Self { match pat.kind { PatKind::Wild | PatKind::Binding(.., None) => Self::Wild, - PatKind::Binding(.., Some(pat)) | PatKind::Box(pat) | PatKind::Ref(pat, _) => Self::from_pat(cx, pat), + PatKind::Binding(.., Some(pat)) | PatKind::Box(pat) | PatKind::Ref(pat, _) => { + Self::from_pat(cx, arena, pat) + }, PatKind::Struct(ref path, fields, _) => { - let mut fields: Vec<_> = fields - .iter() - .map(|f| (f.ident.name, Self::from_pat(cx, f.pat))) - .collect(); + let fields = + arena.alloc_from_iter(fields.iter().map(|f| (f.ident.name, Self::from_pat(cx, arena, f.pat)))); fields.sort_by_key(|&(name, _)| name); Self::Struct(cx.qpath_res(path, pat.hir_id).opt_def_id(), fields) }, - PatKind::TupleStruct(ref path, pats, wild_idx) => Self::Sequence( - cx.qpath_res(path, pat.hir_id).opt_def_id(), - pats.iter().map(|pat| Self::from_pat(cx, pat)).collect(), - wild_idx, - ), - PatKind::Or(pats) => Self::Or(pats.iter().map(|pat| Self::from_pat(cx, pat)).collect()), + PatKind::TupleStruct(ref path, pats, wild_idx) => { + let adt = match cx.typeck_results().pat_ty(pat).ty_adt_def() { + Some(x) => x, + None => return Self::Wild, + }; + let (var_id, variant) = if adt.is_enum() { + match cx.qpath_res(path, pat.hir_id).opt_def_id() { + Some(x) => (Some(x), adt.variant_with_ctor_id(x)), + None => return Self::Wild, + } + } else { + (None, adt.non_enum_variant()) + }; + let (front, back) = match wild_idx { + Some(i) => pats.split_at(i), + None => (pats, [].as_slice()), + }; + let pats = arena.alloc_from_iter( + front + .iter() + .map(|pat| Self::from_pat(cx, arena, pat)) + .chain(iter::repeat_with(|| Self::Wild).take(variant.fields.len() - pats.len())) + .chain(back.iter().map(|pat| Self::from_pat(cx, arena, pat))), + ); + Self::Tuple(var_id, pats) + }, + PatKind::Or(pats) => Self::Or(arena.alloc_from_iter(pats.iter().map(|pat| Self::from_pat(cx, arena, pat)))), PatKind::Path(ref path) => Self::Path(cx.qpath_res(path, pat.hir_id).opt_def_id()), PatKind::Tuple(pats, wild_idx) => { - Self::Sequence(None, pats.iter().map(|pat| Self::from_pat(cx, pat)).collect(), wild_idx) + let field_count = match cx.typeck_results().pat_ty(pat).kind() { + ty::Tuple(subs) => subs.len(), + _ => return Self::Wild, + }; + let (front, back) = match wild_idx { + Some(i) => pats.split_at(i), + None => (pats, [].as_slice()), + }; + let pats = arena.alloc_from_iter( + front + .iter() + .map(|pat| Self::from_pat(cx, arena, pat)) + .chain(iter::repeat_with(|| Self::Wild).take(field_count - pats.len())) + .chain(back.iter().map(|pat| Self::from_pat(cx, arena, pat))), + ); + Self::Tuple(None, pats) }, PatKind::Lit(e) => match &e.kind { ExprKind::Lit(lit) => match lit.node { @@ -239,13 +282,10 @@ impl<'hir> ResolvedPat<'hir> { }; Self::Range(PatRange { start, end, bounds }) }, - PatKind::Slice(pats, wild, pats2) => Self::Sequence( - None, - pats.iter() - .chain(pats2.iter()) - .map(|pat| Self::from_pat(cx, pat)) - .collect(), - wild.map(|_| pats.len()), + PatKind::Slice(front, wild_pat, back) => Self::Slice( + arena.alloc_from_iter(front.iter().map(|pat| Self::from_pat(cx, arena, pat))), + arena.alloc_from_iter(back.iter().map(|pat| Self::from_pat(cx, arena, pat))), + wild_pat.is_some(), ), } } @@ -253,9 +293,11 @@ impl<'hir> ResolvedPat<'hir> { /// Checks if two patterns overlap in the values they can match assuming they are for the same /// type. fn can_also_match(&self, other: &Self) -> bool { - match (self, other) { + match (*self, *other) { (Self::Wild, _) | (_, Self::Wild) => true, - (Self::Or(pats), other) | (other, Self::Or(pats)) => pats.iter().any(|pat| pat.can_also_match(other)), + (Self::Or(pats), ref other) | (ref other, Self::Or(pats)) => { + pats.iter().any(|pat| pat.can_also_match(other)) + }, (Self::Struct(lpath, lfields), Self::Struct(rpath, rfields)) => { if lpath != rpath { return false; @@ -287,28 +329,13 @@ impl<'hir> ResolvedPat<'hir> { } true }, - (Self::Sequence(lpath, lpats, lwild_idx), Self::Sequence(rpath, rpats, rwild_idx)) => { + (Self::Tuple(lpath, lpats), Self::Tuple(rpath, rpats)) => { if lpath != rpath { return false; } - - let (lpats_start, lpats_end) = lwild_idx - .or(*rwild_idx) - .map_or((&**lpats, [].as_slice()), |idx| lpats.split_at(idx)); - let (rpats_start, rpats_end) = rwild_idx - .or(*lwild_idx) - .map_or((&**rpats, [].as_slice()), |idx| rpats.split_at(idx)); - - lpats_start - .iter() - .zip(rpats_start.iter()) - .all(|(lpat, rpat)| lpat.can_also_match(rpat)) - // `lpats_end` and `rpats_end` lengths may be disjointed, so start from the end and ignore any - // extras. - && lpats_end + lpats .iter() - .rev() - .zip(rpats_end.iter().rev()) + .zip(rpats.iter()) .all(|(lpat, rpat)| lpat.can_also_match(rpat)) }, (Self::Path(x), Self::Path(y)) => x == y, @@ -316,10 +343,10 @@ impl<'hir> ResolvedPat<'hir> { (Self::LitBytes(x), Self::LitBytes(y)) => x == y, (Self::LitInt(x), Self::LitInt(y)) => x == y, (Self::LitBool(x), Self::LitBool(y)) => x == y, - (Self::Range(x), Self::Range(y)) => x.overlaps(y), - (Self::Range(range), Self::LitInt(x)) | (Self::LitInt(x), Self::Range(range)) => range.contains(*x), + (Self::Range(ref x), Self::Range(ref y)) => x.overlaps(y), + (Self::Range(ref range), Self::LitInt(x)) | (Self::LitInt(x), Self::Range(ref range)) => range.contains(x), - // Todo: Lit* with Path, Range with Path, LitBytes with Sequence + // Todo: Lit* with Path, Range with Path, LitBytes with Slice, Slice with Slice _ => true, } } From 4f8f4b4c82e4647a2464e364ff96cc1dc5cb8e9a Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 6 Jan 2022 14:54:57 -0500 Subject: [PATCH 3/5] Handle slice patterns in `match_same_arms` --- clippy_lints/src/matches/match_same_arms.rs | 53 ++++++++++++++------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs index 0afac347d080..39c1f0d0d018 100644 --- a/clippy_lints/src/matches/match_same_arms.rs +++ b/clippy_lints/src/matches/match_same_arms.rs @@ -21,27 +21,30 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { }; let arena = DroplessArena::default(); - let resolved_pats: Vec<_> = arms.iter().map(|a| ResolvedPat::from_pat(cx, &arena, a.pat)).collect(); + let normalized_pats: Vec<_> = arms + .iter() + .map(|a| NormalizedPat::from_pat(cx, &arena, a.pat)) + .collect(); // The furthast forwards a pattern can move without semantic changes - let forwards_blocking_idxs: Vec<_> = resolved_pats + let forwards_blocking_idxs: Vec<_> = normalized_pats .iter() .enumerate() .map(|(i, pat)| { - resolved_pats[i + 1..] + normalized_pats[i + 1..] .iter() .enumerate() .find_map(|(j, other)| pat.can_also_match(other).then(|| i + 1 + j)) - .unwrap_or(resolved_pats.len()) + .unwrap_or(normalized_pats.len()) }) .collect(); // The furthast backwards a pattern can move without semantic changes - let backwards_blocking_idxs: Vec<_> = resolved_pats + let backwards_blocking_idxs: Vec<_> = normalized_pats .iter() .enumerate() .map(|(i, pat)| { - resolved_pats[..i] + normalized_pats[..i] .iter() .enumerate() .rev() @@ -133,18 +136,18 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { } #[derive(Clone, Copy)] -enum ResolvedPat<'hir, 'arena> { +enum NormalizedPat<'a> { Wild, - Struct(Option, &'arena [(Symbol, Self)]), - Tuple(Option, &'arena [Self]), - Or(&'arena [Self]), + Struct(Option, &'a [(Symbol, Self)]), + Tuple(Option, &'a [Self]), + Or(&'a [Self]), Path(Option), LitStr(Symbol), - LitBytes(&'hir [u8]), + LitBytes(&'a [u8]), LitInt(u128), LitBool(bool), Range(PatRange), - Slice(&'arena [Self], &'arena [Self], bool), + Slice(&'a [Self], Option<&'a [Self]>), } #[derive(Clone, Copy)] @@ -183,9 +186,9 @@ impl PatRange { } #[allow(clippy::similar_names)] -impl<'hir, 'arena> ResolvedPat<'hir, 'arena> { +impl<'a> NormalizedPat<'a> { #[allow(clippy::too_many_lines)] - fn from_pat(cx: &LateContext<'_>, arena: &'arena DroplessArena, pat: &'hir Pat<'_>) -> Self { + fn from_pat(cx: &LateContext<'_>, arena: &'a DroplessArena, pat: &'a Pat<'_>) -> Self { match pat.kind { PatKind::Wild | PatKind::Binding(.., None) => Self::Wild, PatKind::Binding(.., Some(pat)) | PatKind::Box(pat) | PatKind::Ref(pat, _) => { @@ -284,8 +287,7 @@ impl<'hir, 'arena> ResolvedPat<'hir, 'arena> { }, PatKind::Slice(front, wild_pat, back) => Self::Slice( arena.alloc_from_iter(front.iter().map(|pat| Self::from_pat(cx, arena, pat))), - arena.alloc_from_iter(back.iter().map(|pat| Self::from_pat(cx, arena, pat))), - wild_pat.is_some(), + wild_pat.map(|_| &*arena.alloc_from_iter(back.iter().map(|pat| Self::from_pat(cx, arena, pat)))), ), } } @@ -345,6 +347,25 @@ impl<'hir, 'arena> ResolvedPat<'hir, 'arena> { (Self::LitBool(x), Self::LitBool(y)) => x == y, (Self::Range(ref x), Self::Range(ref y)) => x.overlaps(y), (Self::Range(ref range), Self::LitInt(x)) | (Self::LitInt(x), Self::Range(ref range)) => range.contains(x), + (Self::Slice(lpats, None), Self::Slice(rpats, None)) => { + lpats.len() == rpats.len() && lpats.iter().zip(rpats.iter()).all(|(x, y)| x.can_also_match(y)) + }, + (Self::Slice(pats, None), Self::Slice(front, Some(back))) + | (Self::Slice(front, Some(back)), Self::Slice(pats, None)) => { + if pats.len() < front.len() + back.len() { + return false; + } + pats[..front.len()] + .iter() + .zip(front.iter()) + .chain(pats[pats.len() - back.len()..].iter().zip(back.iter())) + .all(|(x, y)| x.can_also_match(y)) + }, + (Self::Slice(lfront, Some(lback)), Self::Slice(rfront, Some(rback))) => lfront + .iter() + .zip(rfront.iter()) + .chain(lback.iter().rev().zip(rback.iter().rev())) + .all(|(x, y)| x.can_also_match(y)), // Todo: Lit* with Path, Range with Path, LitBytes with Slice, Slice with Slice _ => true, From 08a7157a379ee7f794db386497f5cf1a4ee69fcc Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 21 Jan 2022 21:30:34 -0500 Subject: [PATCH 4/5] Improve message for `match_single_arms` --- clippy_lints/src/matches/match_same_arms.rs | 80 ++++--- tests/ui/match_same_arms.stderr | 165 +++++++-------- tests/ui/match_same_arms2.rs | 17 ++ tests/ui/match_same_arms2.stderr | 221 ++++++++++---------- 4 files changed, 249 insertions(+), 234 deletions(-) diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs index 39c1f0d0d018..5202df544f62 100644 --- a/clippy_lints/src/matches/match_same_arms.rs +++ b/clippy_lints/src/matches/match_same_arms.rs @@ -4,6 +4,7 @@ use clippy_utils::{path_to_local, search_same, SpanlessEq, SpanlessHash}; use core::iter; use rustc_arena::DroplessArena; use rustc_ast::ast::LitKind; +use rustc_errors::Applicability; use rustc_hir::def_id::DefId; use rustc_hir::{Arm, Expr, ExprKind, HirId, HirIdMap, HirIdSet, Pat, PatKind, RangeEnd}; use rustc_lint::LateContext; @@ -13,6 +14,7 @@ use std::collections::hash_map::Entry; use super::MATCH_SAME_ARMS; +#[allow(clippy::too_many_lines)] pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { let hash = |&(_, arm): &(usize, &Arm<'_>)| -> u64 { let mut h = SpanlessHash::new(cx); @@ -96,42 +98,52 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { }; 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… + for (&(i, arm1), &(j, arm2)) in search_same(&indexed_arms, hash, eq) { + if matches!(arm2.pat.kind, PatKind::Wild) { + span_lint_and_then( + cx, + MATCH_SAME_ARMS, + arm1.span, + "this match arm has an identical body to the `_` wildcard arm", + |diag| { + diag.span_suggestion( + arm1.span, + "try removing the arm", + String::new(), + Applicability::MaybeIncorrect, + ) + .help("or try changing either arm body") + .span_note(arm2.span, "`_` wildcard arm here"); + }, + ); + } else { + let back_block = backwards_blocking_idxs[j]; + let (keep_arm, move_arm) = if back_block < i || (back_block == 0 && forwards_blocking_idxs[i] <= j) { + (arm1, arm2) + } else { + (arm2, arm1) + }; - let lhs = snippet(cx, i.pat.span, ""); - let rhs = snippet(cx, j.pat.span, ""); + span_lint_and_then( + cx, + MATCH_SAME_ARMS, + keep_arm.span, + "this match arm has an identical body to another arm", + |diag| { + let move_pat_snip = snippet(cx, move_arm.pat.span, ""); + let keep_pat_snip = snippet(cx, keep_arm.pat.span, ""); - 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"); - } - }, - ); + diag.span_suggestion( + keep_arm.pat.span, + "try merging the arm patterns", + format!("{} | {}", keep_pat_snip, move_pat_snip), + Applicability::MaybeIncorrect, + ) + .help("or try changing either arm body") + .span_note(move_arm.span, "other arm here"); + }, + ); + } } } diff --git a/tests/ui/match_same_arms.stderr b/tests/ui/match_same_arms.stderr index 7752a8a6ff2b..b6d04263b37a 100644 --- a/tests/ui/match_same_arms.stderr +++ b/tests/ui/match_same_arms.stderr @@ -1,128 +1,121 @@ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms.rs:13:14 +error: this match arm has an identical body to the `_` wildcard arm + --> $DIR/match_same_arms.rs:11:9 | -LL | _ => 0, //~ ERROR match arms have same body - | ^ +LL | Abc::A => 0, + | ^^^^^^^^^^^ help: try removing the arm | = note: `-D clippy::match-same-arms` implied by `-D warnings` -note: same as this - --> $DIR/match_same_arms.rs:11:19 + = help: or try changing either arm body +note: `_` wildcard arm here + --> $DIR/match_same_arms.rs:13:9 | -LL | Abc::A => 0, - | ^ -note: `Abc::A` has the same arm body as the `_` wildcard, consider removing it - --> $DIR/match_same_arms.rs:11:19 - | -LL | Abc::A => 0, - | ^ +LL | _ => 0, //~ ERROR match arms have same body + | ^^^^^^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms.rs:18:20 - | -LL | (.., 3) => 42, //~ ERROR match arms have same body - | ^^ - | -note: same as this - --> $DIR/match_same_arms.rs:17:23 - | -LL | (1, .., 3) => 42, - | ^^ -help: consider refactoring into `(1, .., 3) | (.., 3)` +error: this match arm has an identical body to another arm --> $DIR/match_same_arms.rs:17:9 | LL | (1, .., 3) => 42, - | ^^^^^^^^^^ - = help: ...or consider changing the match arm bodies + | ----------^^^^^^ + | | + | help: try merging the arm patterns: `(1, .., 3) | (.., 3)` + | + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms.rs:18:9 + | +LL | (.., 3) => 42, //~ ERROR match arms have same body + | ^^^^^^^^^^^^^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms.rs:24:15 +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms.rs:24:9 | LL | 51 => 1, //~ ERROR match arms have same body - | ^ + | --^^^^^ + | | + | help: try merging the arm patterns: `51 | 42` | -note: same as this - --> $DIR/match_same_arms.rs:23:15 - | -LL | 42 => 1, - | ^ -help: consider refactoring into `42 | 51` + = help: or try changing either arm body +note: other arm here --> $DIR/match_same_arms.rs:23:9 | LL | 42 => 1, - | ^^ - = help: ...or consider changing the match arm bodies + | ^^^^^^^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms.rs:26:15 - | -LL | 52 => 2, //~ ERROR match arms have same body - | ^ - | -note: same as this - --> $DIR/match_same_arms.rs:25:15 - | -LL | 41 => 2, - | ^ -help: consider refactoring into `41 | 52` +error: this match arm has an identical body to another arm --> $DIR/match_same_arms.rs:25:9 | LL | 41 => 2, - | ^^ - = help: ...or consider changing the match arm bodies + | --^^^^^ + | | + | help: try merging the arm patterns: `41 | 52` + | + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms.rs:26:9 + | +LL | 52 => 2, //~ ERROR match arms have same body + | ^^^^^^^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms.rs:32:14 +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms.rs:32:9 | LL | 2 => 2, //~ ERROR 2nd matched arms have same body - | ^ - | -note: same as this - --> $DIR/match_same_arms.rs:31:14 + | -^^^^^ + | | + | help: try merging the arm patterns: `2 | 1` | -LL | 1 => 2, - | ^ -help: consider refactoring into `1 | 2` + = help: or try changing either arm body +note: other arm here --> $DIR/match_same_arms.rs:31:9 | LL | 1 => 2, - | ^ - = help: ...or consider changing the match arm bodies + | ^^^^^^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms.rs:33:14 +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms.rs:33:9 | LL | 3 => 2, //~ ERROR 3rd matched arms have same body - | ^ - | -note: same as this - --> $DIR/match_same_arms.rs:31:14 + | -^^^^^ + | | + | help: try merging the arm patterns: `3 | 1` | -LL | 1 => 2, - | ^ -help: consider refactoring into `1 | 3` + = help: or try changing either arm body +note: other arm here --> $DIR/match_same_arms.rs:31:9 | LL | 1 => 2, - | ^ - = help: ...or consider changing the match arm bodies + | ^^^^^^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms.rs:50:55 +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms.rs:32:9 | -LL | CommandInfo::External { name, .. } => name.to_string(), - | ^^^^^^^^^^^^^^^^ +LL | 2 => 2, //~ ERROR 2nd matched arms have same body + | -^^^^^ + | | + | help: try merging the arm patterns: `2 | 3` | -note: same as this - --> $DIR/match_same_arms.rs:49:54 + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms.rs:33:9 | -LL | CommandInfo::BuiltIn { name, .. } => name.to_string(), - | ^^^^^^^^^^^^^^^^ -help: consider refactoring into `CommandInfo::BuiltIn { name, .. } | CommandInfo::External { name, .. }` +LL | 3 => 2, //~ ERROR 3rd matched arms have same body + | ^^^^^^ + +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms.rs:50:17 + | +LL | CommandInfo::External { name, .. } => name.to_string(), + | ----------------------------------^^^^^^^^^^^^^^^^^^^^ + | | + | help: try merging the arm patterns: `CommandInfo::External { name, .. } | CommandInfo::BuiltIn { name, .. }` + | + = help: or try changing either arm body +note: other arm here --> $DIR/match_same_arms.rs:49:17 | LL | CommandInfo::BuiltIn { name, .. } => name.to_string(), - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = help: ...or consider changing the match arm bodies + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 7 previous errors +error: aborting due to 8 previous errors diff --git a/tests/ui/match_same_arms2.rs b/tests/ui/match_same_arms2.rs index 6dc6c4172ee0..fdd88f25529e 100644 --- a/tests/ui/match_same_arms2.rs +++ b/tests/ui/match_same_arms2.rs @@ -181,10 +181,27 @@ fn main() { Z(u32), } + // Don't lint. `Foo::X(0)` and `Foo::Z(_)` overlap with the arm in between. let _ = match Foo::X(0) { Foo::X(0) => 1, Foo::X(_) | Foo::Y(_) | Foo::Z(0) => 2, Foo::Z(_) => 1, _ => 0, }; + + // Suggest moving `Foo::Z(_)` up. + let _ = match Foo::X(0) { + Foo::X(0) => 1, + Foo::X(_) | Foo::Y(_) => 2, + Foo::Z(_) => 1, + _ => 0, + }; + + // Suggest moving `Foo::X(0)` down. + let _ = match Foo::X(0) { + Foo::X(0) => 1, + Foo::Y(_) | Foo::Z(0) => 2, + Foo::Z(_) => 1, + _ => 0, + }; } diff --git a/tests/ui/match_same_arms2.stderr b/tests/ui/match_same_arms2.stderr index e1ed12f93708..596cc8432b31 100644 --- a/tests/ui/match_same_arms2.stderr +++ b/tests/ui/match_same_arms2.stderr @@ -1,175 +1,138 @@ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms2.rs:20:14 +error: this match arm has an identical body to the `_` wildcard arm + --> $DIR/match_same_arms2.rs:11:9 | -LL | _ => { - | ______________^ -LL | | //~ ERROR match arms have same body +LL | / 42 => { LL | | foo(); LL | | let mut a = 42 + [23].len() as i32; +LL | | if true { ... | LL | | a LL | | }, - | |_________^ + | |_________^ help: try removing the arm | = note: `-D clippy::match-same-arms` implied by `-D warnings` -note: same as this - --> $DIR/match_same_arms2.rs:11:15 - | -LL | 42 => { - | _______________^ -LL | | foo(); -LL | | let mut a = 42 + [23].len() as i32; -LL | | if true { -... | -LL | | a -LL | | }, - | |_________^ -note: `42` has the same arm body as the `_` wildcard, consider removing it - --> $DIR/match_same_arms2.rs:11:15 + = help: or try changing either arm body +note: `_` wildcard arm here + --> $DIR/match_same_arms2.rs:20:9 | -LL | 42 => { - | _______________^ +LL | / _ => { +LL | | //~ ERROR match arms have same body LL | | foo(); LL | | let mut a = 42 + [23].len() as i32; -LL | | if true { ... | LL | | a LL | | }, | |_________^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms2.rs:34:15 +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms2.rs:34:9 | LL | 51 => foo(), //~ ERROR match arms have same body - | ^^^^^ - | -note: same as this - --> $DIR/match_same_arms2.rs:33:15 + | --^^^^^^^^^ + | | + | help: try merging the arm patterns: `51 | 42` | -LL | 42 => foo(), - | ^^^^^ -help: consider refactoring into `42 | 51` + = help: or try changing either arm body +note: other arm here --> $DIR/match_same_arms2.rs:33:9 | LL | 42 => foo(), - | ^^ - = help: ...or consider changing the match arm bodies + | ^^^^^^^^^^^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms2.rs:40:17 - | -LL | None => 24, //~ ERROR match arms have same body - | ^^ - | -note: same as this - --> $DIR/match_same_arms2.rs:39:20 - | -LL | Some(_) => 24, - | ^^ -help: consider refactoring into `Some(_) | None` +error: this match arm has an identical body to another arm --> $DIR/match_same_arms2.rs:39:9 | LL | Some(_) => 24, - | ^^^^^^^ - = help: ...or consider changing the match arm bodies - -error: this `match` has identical arm bodies - --> $DIR/match_same_arms2.rs:62:28 + | -------^^^^^^ + | | + | help: try merging the arm patterns: `Some(_) | None` | -LL | (None, Some(a)) => bar(a), //~ ERROR match arms have same body - | ^^^^^^ - | -note: same as this - --> $DIR/match_same_arms2.rs:61:28 + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms2.rs:40:9 | -LL | (Some(a), None) => bar(a), - | ^^^^^^ -help: consider refactoring into `(Some(a), None) | (None, Some(a))` +LL | None => 24, //~ ERROR match arms have same body + | ^^^^^^^^^^ + +error: this match arm has an identical body to another arm --> $DIR/match_same_arms2.rs:61:9 | LL | (Some(a), None) => bar(a), - | ^^^^^^^^^^^^^^^ - = help: ...or consider changing the match arm bodies - -error: this `match` has identical arm bodies - --> $DIR/match_same_arms2.rs:68:26 + | ---------------^^^^^^^^^^ + | | + | help: try merging the arm patterns: `(Some(a), None) | (None, Some(a))` | -LL | (.., Some(a)) => bar(a), //~ ERROR match arms have same body - | ^^^^^^ - | -note: same as this - --> $DIR/match_same_arms2.rs:67:26 + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms2.rs:62:9 | -LL | (Some(a), ..) => bar(a), - | ^^^^^^ -help: consider refactoring into `(Some(a), ..) | (.., Some(a))` +LL | (None, Some(a)) => bar(a), //~ ERROR match arms have same body + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this match arm has an identical body to another arm --> $DIR/match_same_arms2.rs:67:9 | LL | (Some(a), ..) => bar(a), - | ^^^^^^^^^^^^^ - = help: ...or consider changing the match arm bodies - -error: this `match` has identical arm bodies - --> $DIR/match_same_arms2.rs:102:29 - | -LL | (Ok(_), Some(x)) => println!("ok {}", x), - | ^^^^^^^^^^^^^^^^^^^^ + | -------------^^^^^^^^^^ + | | + | help: try merging the arm patterns: `(Some(a), ..) | (.., Some(a))` | -note: same as this - --> $DIR/match_same_arms2.rs:101:29 + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms2.rs:68:9 | -LL | (Ok(x), Some(_)) => println!("ok {}", x), - | ^^^^^^^^^^^^^^^^^^^^ -help: consider refactoring into `(Ok(x), Some(_)) | (Ok(_), Some(x))` +LL | (.., Some(a)) => bar(a), //~ ERROR match arms have same body + | ^^^^^^^^^^^^^^^^^^^^^^^ + +error: this match arm has an identical body to another arm --> $DIR/match_same_arms2.rs:101:9 | LL | (Ok(x), Some(_)) => println!("ok {}", x), - | ^^^^^^^^^^^^^^^^ - = help: ...or consider changing the match arm bodies - = note: this error originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info) + | ----------------^^^^^^^^^^^^^^^^^^^^^^^^ + | | + | help: try merging the arm patterns: `(Ok(x), Some(_)) | (Ok(_), Some(x))` + | + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms2.rs:102:9 + | +LL | (Ok(_), Some(x)) => println!("ok {}", x), + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms2.rs:117:18 +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms2.rs:117:9 | LL | Ok(_) => println!("ok"), - | ^^^^^^^^^^^^^^ + | -----^^^^^^^^^^^^^^^^^^ + | | + | help: try merging the arm patterns: `Ok(_) | Ok(3)` | -note: same as this - --> $DIR/match_same_arms2.rs:116:18 - | -LL | Ok(3) => println!("ok"), - | ^^^^^^^^^^^^^^ -help: consider refactoring into `Ok(3) | Ok(_)` + = help: or try changing either arm body +note: other arm here --> $DIR/match_same_arms2.rs:116:9 | LL | Ok(3) => println!("ok"), - | ^^^^^ - = help: ...or consider changing the match arm bodies - = note: this error originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info) + | ^^^^^^^^^^^^^^^^^^^^^^^ -error: this `match` has identical arm bodies - --> $DIR/match_same_arms2.rs:144:14 +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms2.rs:144:9 | LL | 1 => { - | ______________^ + | ^ help: try merging the arm patterns: `1 | 0` + | _________| + | | LL | | empty!(0); LL | | }, | |_________^ | -note: same as this - --> $DIR/match_same_arms2.rs:141:14 + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms2.rs:141:9 | -LL | 0 => { - | ______________^ +LL | / 0 => { LL | | empty!(0); LL | | }, | |_________^ -help: consider refactoring into `0 | 1` - --> $DIR/match_same_arms2.rs:141:9 - | -LL | 0 => { - | ^ - = help: ...or consider changing the match arm bodies error: match expression looks like `matches!` macro --> $DIR/match_same_arms2.rs:162:16 @@ -184,5 +147,35 @@ LL | | }; | = note: `-D clippy::match-like-matches-macro` implied by `-D warnings` -error: aborting due to 9 previous errors +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms2.rs:194:9 + | +LL | Foo::X(0) => 1, + | ---------^^^^^ + | | + | help: try merging the arm patterns: `Foo::X(0) | Foo::Z(_)` + | + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms2.rs:196:9 + | +LL | Foo::Z(_) => 1, + | ^^^^^^^^^^^^^^ + +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms2.rs:204:9 + | +LL | Foo::Z(_) => 1, + | ---------^^^^^ + | | + | help: try merging the arm patterns: `Foo::Z(_) | Foo::X(0)` + | + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms2.rs:202:9 + | +LL | Foo::X(0) => 1, + | ^^^^^^^^^^^^^^ + +error: aborting due to 11 previous errors From 773d20341ad8061df9acca781bab7a0fb38ed684 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Wed, 16 Mar 2022 23:55:38 -0400 Subject: [PATCH 5/5] Fix mixed enum variant kinds + code cleanup --- clippy_lints/src/matches/match_same_arms.rs | 113 +++++++++++--------- clippy_lints/src/write.rs | 9 +- tests/ui/match_same_arms2.rs | 23 ++++ tests/ui/match_same_arms2.stderr | 41 ++++--- 4 files changed, 123 insertions(+), 63 deletions(-) diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs index 5202df544f62..b8591fe0db0a 100644 --- a/clippy_lints/src/matches/match_same_arms.rs +++ b/clippy_lints/src/matches/match_same_arms.rs @@ -1,7 +1,9 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; use clippy_utils::{path_to_local, search_same, SpanlessEq, SpanlessHash}; +use core::cmp::Ordering; use core::iter; +use core::slice; use rustc_arena::DroplessArena; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; @@ -36,7 +38,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { normalized_pats[i + 1..] .iter() .enumerate() - .find_map(|(j, other)| pat.can_also_match(other).then(|| i + 1 + j)) + .find_map(|(j, other)| pat.has_overlapping_values(other).then(|| i + 1 + j)) .unwrap_or(normalized_pats.len()) }) .collect(); @@ -52,7 +54,9 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { .rev() .zip(forwards_blocking_idxs[..i].iter().copied().rev()) .skip_while(|&(_, forward_block)| forward_block > i) - .find_map(|((j, other), forward_block)| (forward_block == i || pat.can_also_match(other)).then(|| j)) + .find_map(|((j, other), forward_block)| { + (forward_block == i || pat.has_overlapping_values(other)).then(|| j) + }) .unwrap_or(0) }) .collect(); @@ -159,6 +163,10 @@ enum NormalizedPat<'a> { LitInt(u128), LitBool(bool), Range(PatRange), + /// A slice pattern. If the second value is `None`, then this matches an exact size. Otherwise + /// the first value contains everything before the `..` wildcard pattern, and the second value + /// contains everything afterwards. Note that either side, or both sides, may contain zero + /// patterns. Slice(&'a [Self], Option<&'a [Self]>), } @@ -178,23 +186,43 @@ impl PatRange { } fn overlaps(&self, other: &Self) -> bool { - !(self.is_empty() || other.is_empty()) - && match self.bounds { - RangeEnd::Included => self.end >= other.start, - RangeEnd::Excluded => self.end > other.start, - } - && match other.bounds { - RangeEnd::Included => self.start <= other.end, - RangeEnd::Excluded => self.start < other.end, - } + // Note: Empty ranges are impossible, so this is correct even though it would return true if an + // empty exclusive range were to reside within an inclusive range. + (match self.bounds { + RangeEnd::Included => self.end >= other.start, + RangeEnd::Excluded => self.end > other.start, + } && match other.bounds { + RangeEnd::Included => self.start <= other.end, + RangeEnd::Excluded => self.start < other.end, + }) } +} - fn is_empty(&self) -> bool { - match self.bounds { - RangeEnd::Included => false, - RangeEnd::Excluded => self.start == self.end, +/// Iterates over the pairs of fields with matching names. +fn iter_matching_struct_fields<'a>( + left: &'a [(Symbol, NormalizedPat<'a>)], + right: &'a [(Symbol, NormalizedPat<'a>)], +) -> impl Iterator, &'a NormalizedPat<'a>)> + 'a { + struct Iter<'a>( + slice::Iter<'a, (Symbol, NormalizedPat<'a>)>, + slice::Iter<'a, (Symbol, NormalizedPat<'a>)>, + ); + impl<'a> Iterator for Iter<'a> { + type Item = (&'a NormalizedPat<'a>, &'a NormalizedPat<'a>); + fn next(&mut self) -> Option { + // Note: all the fields in each slice are sorted by symbol value. + let mut left = self.0.next()?; + let mut right = self.1.next()?; + loop { + match left.0.cmp(&right.0) { + Ordering::Equal => return Some((&left.1, &right.1)), + Ordering::Less => left = self.0.next()?, + Ordering::Greater => right = self.1.next()?, + } + } } } + Iter(left.iter(), right.iter()) } #[allow(clippy::similar_names)] @@ -259,6 +287,7 @@ impl<'a> NormalizedPat<'a> { Self::Tuple(None, pats) }, PatKind::Lit(e) => match &e.kind { + // TODO: Handle negative integers. They're currently treated as a wild match. ExprKind::Lit(lit) => match lit.node { LitKind::Str(sym, _) => Self::LitStr(sym), LitKind::ByteStr(ref bytes) => Self::LitBytes(&**bytes), @@ -271,6 +300,7 @@ impl<'a> NormalizedPat<'a> { _ => Self::Wild, }, PatKind::Range(start, end, bounds) => { + // TODO: Handle negative integers. They're currently treated as a wild match. let start = match start { None => 0, Some(e) => match &e.kind { @@ -306,42 +336,17 @@ impl<'a> NormalizedPat<'a> { /// Checks if two patterns overlap in the values they can match assuming they are for the same /// type. - fn can_also_match(&self, other: &Self) -> bool { + fn has_overlapping_values(&self, other: &Self) -> bool { match (*self, *other) { (Self::Wild, _) | (_, Self::Wild) => true, (Self::Or(pats), ref other) | (ref other, Self::Or(pats)) => { - pats.iter().any(|pat| pat.can_also_match(other)) + pats.iter().any(|pat| pat.has_overlapping_values(other)) }, (Self::Struct(lpath, lfields), Self::Struct(rpath, rfields)) => { if lpath != rpath { return false; } - let mut rfields = rfields.iter(); - let mut rfield = match rfields.next() { - Some(x) => x, - None => return true, - }; - 'outer: for lfield in lfields { - loop { - if lfield.0 < rfield.0 { - continue 'outer; - } else if lfield.0 > rfield.0 { - rfield = match rfields.next() { - Some(x) => x, - None => return true, - }; - } else if !lfield.1.can_also_match(&rfield.1) { - return false; - } else { - rfield = match rfields.next() { - Some(x) => x, - None => return true, - }; - continue 'outer; - } - } - } - true + iter_matching_struct_fields(lfields, rfields).all(|(lpat, rpat)| lpat.has_overlapping_values(rpat)) }, (Self::Tuple(lpath, lpats), Self::Tuple(rpath, rpats)) => { if lpath != rpath { @@ -350,7 +355,7 @@ impl<'a> NormalizedPat<'a> { lpats .iter() .zip(rpats.iter()) - .all(|(lpat, rpat)| lpat.can_also_match(rpat)) + .all(|(lpat, rpat)| lpat.has_overlapping_values(rpat)) }, (Self::Path(x), Self::Path(y)) => x == y, (Self::LitStr(x), Self::LitStr(y)) => x == y, @@ -360,10 +365,12 @@ impl<'a> NormalizedPat<'a> { (Self::Range(ref x), Self::Range(ref y)) => x.overlaps(y), (Self::Range(ref range), Self::LitInt(x)) | (Self::LitInt(x), Self::Range(ref range)) => range.contains(x), (Self::Slice(lpats, None), Self::Slice(rpats, None)) => { - lpats.len() == rpats.len() && lpats.iter().zip(rpats.iter()).all(|(x, y)| x.can_also_match(y)) + lpats.len() == rpats.len() && lpats.iter().zip(rpats.iter()).all(|(x, y)| x.has_overlapping_values(y)) }, (Self::Slice(pats, None), Self::Slice(front, Some(back))) | (Self::Slice(front, Some(back)), Self::Slice(pats, None)) => { + // Here `pats` is an exact size match. If the combined lengths of `front` and `back` are greater + // then the minium length required will be greater than the length of `pats`. if pats.len() < front.len() + back.len() { return false; } @@ -371,15 +378,25 @@ impl<'a> NormalizedPat<'a> { .iter() .zip(front.iter()) .chain(pats[pats.len() - back.len()..].iter().zip(back.iter())) - .all(|(x, y)| x.can_also_match(y)) + .all(|(x, y)| x.has_overlapping_values(y)) }, (Self::Slice(lfront, Some(lback)), Self::Slice(rfront, Some(rback))) => lfront .iter() .zip(rfront.iter()) .chain(lback.iter().rev().zip(rback.iter().rev())) - .all(|(x, y)| x.can_also_match(y)), + .all(|(x, y)| x.has_overlapping_values(y)), + + // Enums can mix unit variants with tuple/struct variants. These can never overlap. + (Self::Path(_), Self::Tuple(..) | Self::Struct(..)) + | (Self::Tuple(..) | Self::Struct(..), Self::Path(_)) => false, + + // Tuples can be matched like a struct. + (Self::Tuple(x, _), Self::Struct(y, _)) | (Self::Struct(x, _), Self::Tuple(y, _)) => { + // TODO: check fields here. + x == y + }, - // Todo: Lit* with Path, Range with Path, LitBytes with Slice, Slice with Slice + // TODO: Lit* with Path, Range with Path, LitBytes with Slice _ => true, } } diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 532bd810a2e3..f3d818cc3485 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -581,14 +581,19 @@ impl Write { }; let replacement: String = match lit.token.kind { - LitKind::Integer | LitKind::Float | LitKind::Err => continue, LitKind::StrRaw(_) | LitKind::ByteStrRaw(_) if matches!(fmtstr.style, StrStyle::Raw(_)) => { lit.token.symbol.as_str().replace('{', "{{").replace('}', "}}") }, LitKind::Str | LitKind::ByteStr if matches!(fmtstr.style, StrStyle::Cooked) => { lit.token.symbol.as_str().replace('{', "{{").replace('}', "}}") }, - LitKind::StrRaw(_) | LitKind::Str | LitKind::ByteStrRaw(_) | LitKind::ByteStr => continue, + LitKind::StrRaw(_) + | LitKind::Str + | LitKind::ByteStrRaw(_) + | LitKind::ByteStr + | LitKind::Integer + | LitKind::Float + | LitKind::Err => continue, LitKind::Byte | LitKind::Char => match lit.token.symbol.as_str() { "\"" if matches!(fmtstr.style, StrStyle::Cooked) => "\\\"", "\"" if matches!(fmtstr.style, StrStyle::Raw(0)) => continue, diff --git a/tests/ui/match_same_arms2.rs b/tests/ui/match_same_arms2.rs index fdd88f25529e..dbfeb4379d51 100644 --- a/tests/ui/match_same_arms2.rs +++ b/tests/ui/match_same_arms2.rs @@ -204,4 +204,27 @@ fn main() { Foo::Z(_) => 1, _ => 0, }; + + // Don't lint. + let _ = match 0 { + -2 => 1, + -5..=50 => 2, + -150..=88 => 1, + _ => 3, + }; + + struct Bar { + x: u32, + y: u32, + z: u32, + } + + // Lint. + let _ = match None { + Some(Bar { x: 0, y: 5, .. }) => 1, + Some(Bar { y: 10, z: 0, .. }) => 2, + None => 50, + Some(Bar { y: 0, x: 5, .. }) => 1, + _ => 200, + }; } diff --git a/tests/ui/match_same_arms2.stderr b/tests/ui/match_same_arms2.stderr index 596cc8432b31..14a672ba2fec 100644 --- a/tests/ui/match_same_arms2.stderr +++ b/tests/ui/match_same_arms2.stderr @@ -40,33 +40,33 @@ LL | 42 => foo(), | ^^^^^^^^^^^ error: this match arm has an identical body to another arm - --> $DIR/match_same_arms2.rs:39:9 + --> $DIR/match_same_arms2.rs:40:9 | -LL | Some(_) => 24, - | -------^^^^^^ +LL | None => 24, //~ ERROR match arms have same body + | ----^^^^^^ | | - | help: try merging the arm patterns: `Some(_) | None` + | help: try merging the arm patterns: `None | Some(_)` | = help: or try changing either arm body note: other arm here - --> $DIR/match_same_arms2.rs:40:9 + --> $DIR/match_same_arms2.rs:39:9 | -LL | None => 24, //~ ERROR match arms have same body - | ^^^^^^^^^^ +LL | Some(_) => 24, + | ^^^^^^^^^^^^^ error: this match arm has an identical body to another arm - --> $DIR/match_same_arms2.rs:61:9 + --> $DIR/match_same_arms2.rs:62:9 | -LL | (Some(a), None) => bar(a), +LL | (None, Some(a)) => bar(a), //~ ERROR match arms have same body | ---------------^^^^^^^^^^ | | - | help: try merging the arm patterns: `(Some(a), None) | (None, Some(a))` + | help: try merging the arm patterns: `(None, Some(a)) | (Some(a), None)` | = help: or try changing either arm body note: other arm here - --> $DIR/match_same_arms2.rs:62:9 + --> $DIR/match_same_arms2.rs:61:9 | -LL | (None, Some(a)) => bar(a), //~ ERROR match arms have same body +LL | (Some(a), None) => bar(a), | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: this match arm has an identical body to another arm @@ -177,5 +177,20 @@ note: other arm here LL | Foo::X(0) => 1, | ^^^^^^^^^^^^^^ -error: aborting due to 11 previous errors +error: this match arm has an identical body to another arm + --> $DIR/match_same_arms2.rs:227:9 + | +LL | Some(Bar { y: 0, x: 5, .. }) => 1, + | ----------------------------^^^^^ + | | + | help: try merging the arm patterns: `Some(Bar { y: 0, x: 5, .. }) | Some(Bar { x: 0, y: 5, .. })` + | + = help: or try changing either arm body +note: other arm here + --> $DIR/match_same_arms2.rs:224:9 + | +LL | Some(Bar { x: 0, y: 5, .. }) => 1, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 12 previous errors