Skip to content

Commit

Permalink
Fix mixed enum variant kinds + code cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarcho
Jarcho committed Mar 17, 2022
1 parent 08a7157 commit b51f3c1
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 63 deletions.
111 changes: 63 additions & 48 deletions clippy_lints/src/matches/match_same_arms.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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]>),
}

Expand All @@ -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<Item = (&'a NormalizedPat<'a>, &'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<Self::Item> {
// 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)]
Expand Down Expand Up @@ -306,42 +334,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 {
Expand All @@ -350,7 +353,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,
Expand All @@ -360,26 +363,38 @@ 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;
}
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))
.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,
}
}
Expand Down
9 changes: 7 additions & 2 deletions clippy_lints/src/write.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
23 changes: 23 additions & 0 deletions tests/ui/match_same_arms2.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
41 changes: 28 additions & 13 deletions tests/ui/match_same_arms2.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

0 comments on commit b51f3c1

Please sign in to comment.