Merge pull request #1490 from Manishearth/excl_range_pat_overlap

oli-obk · web-flow · commit 909ef37f0827 · 2017-01-31T12:40:21.000+01:00
correctly check exclusive range patterns for overlap
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -10,6 +10,7 @@
 #![feature(stmt_expr_attributes)]
 #![feature(repeat_str)]
 #![feature(conservative_impl_trait)]
+#![feature(collections_bound)]
 
 #![allow(indexing_slicing, shadow_reuse, unknown_lints, missing_docs_in_private_items)]
 #![allow(needless_lifetimes)]
diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs
@@ -6,6 +6,7 @@ use rustc_const_eval::EvalHint::ExprTypeChecked;
 use rustc_const_eval::ConstContext;
 use rustc_const_math::ConstInt;
 use std::cmp::Ordering;
+use std::collections::Bound;
 use syntax::ast::LitKind;
 use syntax::codemap::Span;
 use utils::paths;
@@ -361,18 +362,22 @@ fn all_ranges(cx: &LateContext, arms: &[Arm]) -> Vec<SpannedRange<ConstVal>> {
                 }
                 .filter_map(|pat| {
                     if_let_chain! {[
-                    let PatKind::Range(ref lhs, ref rhs, _) = pat.node,
+                    let PatKind::Range(ref lhs, ref rhs, ref range_end) = pat.node,
                     let Ok(lhs) = constcx.eval(lhs, ExprTypeChecked),
                     let Ok(rhs) = constcx.eval(rhs, ExprTypeChecked)
                 ], {
+                    let rhs = match *range_end {
+                        RangeEnd::Included => Bound::Included(rhs),
+                        RangeEnd::Excluded => Bound::Excluded(rhs),
+                    };
                     return Some(SpannedRange { span: pat.span, node: (lhs, rhs) });
                 }}
 
                     if_let_chain! {[
                     let PatKind::Lit(ref value) = pat.node,
                     let Ok(value) = constcx.eval(value, ExprTypeChecked)
                 ], {
-                    return Some(SpannedRange { span: pat.span, node: (value.clone(), value) });
+                    return Some(SpannedRange { span: pat.span, node: (value.clone(), Bound::Included(value)) });
                 }}
 
                     None
@@ -384,7 +389,7 @@ fn all_ranges(cx: &LateContext, arms: &[Arm]) -> Vec<SpannedRange<ConstVal>> {
 #[derive(Debug, Eq, PartialEq)]
 pub struct SpannedRange<T> {
     pub span: Span,
-    pub node: (T, T),
+    pub node: (T, Bound<T>),
 }
 
 type TypedRanges = Vec<SpannedRange<ConstInt>>;
@@ -393,13 +398,26 @@ type TypedRanges = Vec<SpannedRange<ConstInt>>;
 /// `Uint` and `Int` probably don't make sense.
 fn type_ranges(ranges: &[SpannedRange<ConstVal>]) -> TypedRanges {
     ranges.iter()
-        .filter_map(|range| if let (ConstVal::Integral(start), ConstVal::Integral(end)) = range.node {
-            Some(SpannedRange {
-                span: range.span,
-                node: (start, end),
-            })
-        } else {
-            None
+        .filter_map(|range| match range.node {
+            (ConstVal::Integral(start), Bound::Included(ConstVal::Integral(end))) => {
+                Some(SpannedRange {
+                    span: range.span,
+                    node: (start, Bound::Included(end)),
+                })
+            },
+            (ConstVal::Integral(start), Bound::Excluded(ConstVal::Integral(end))) => {
+                Some(SpannedRange {
+                    span: range.span,
+                    node: (start, Bound::Excluded(end)),
+                })
+            },
+            (ConstVal::Integral(start), Bound::Unbounded) => {
+                Some(SpannedRange {
+                    span: range.span,
+                    node: (start, Bound::Unbounded),
+                })
+            },
+            _ => None,
         })
         .collect()
 }
@@ -443,7 +461,7 @@ pub fn overlapping<T>(ranges: &[SpannedRange<T>]) -> Option<(&SpannedRange<T>, &
     #[derive(Copy, Clone, Debug, Eq, PartialEq)]
     enum Kind<'a, T: 'a> {
         Start(T, &'a SpannedRange<T>),
-        End(T, &'a SpannedRange<T>),
+        End(Bound<T>, &'a SpannedRange<T>),
     }
 
     impl<'a, T: Copy> Kind<'a, T> {
@@ -454,9 +472,9 @@ pub fn overlapping<T>(ranges: &[SpannedRange<T>]) -> Option<(&SpannedRange<T>, &
             }
         }
 
-        fn value(self) -> T {
+        fn value(self) -> Bound<T> {
             match self {
-                Kind::Start(t, _) |
+                Kind::Start(t, _) => Bound::Included(t),
                 Kind::End(t, _) => t,
             }
         }
@@ -470,7 +488,25 @@ pub fn overlapping<T>(ranges: &[SpannedRange<T>]) -> Option<(&SpannedRange<T>, &
 
     impl<'a, T: Copy + Ord> Ord for Kind<'a, T> {
         fn cmp(&self, other: &Self) -> Ordering {
-            self.value().cmp(&other.value())
+            match (self.value(), other.value()) {
+                (Bound::Included(a), Bound::Included(b)) |
+                (Bound::Excluded(a), Bound::Excluded(b)) => a.cmp(&b),
+                // Range patterns cannot be unbounded (yet)
+                (Bound::Unbounded, _) |
+                (_, Bound::Unbounded) => unimplemented!(),
+                (Bound::Included(a), Bound::Excluded(b)) => {
+                    match a.cmp(&b) {
+                        Ordering::Equal => Ordering::Greater,
+                        other => other,
+                    }
+                },
+                (Bound::Excluded(a), Bound::Included(b)) => {
+                    match a.cmp(&b) {
+                        Ordering::Equal => Ordering::Less,
+                        other => other,
+                    }
+                },
+            }
         }
     }
 
@@ -490,7 +526,7 @@ pub fn overlapping<T>(ranges: &[SpannedRange<T>]) -> Option<(&SpannedRange<T>, &
                     return Some((ra, rb));
                 }
             },
-            (&Kind::End(a, _), &Kind::Start(b, _)) if a != b => (),
+            (&Kind::End(a, _), &Kind::Start(b, _)) if a != Bound::Included(b) => (),
             _ => return Some((a.range(), b.range())),
         }
     }
diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs
@@ -1,4 +1,5 @@
 #![feature(plugin)]
+#![feature(exclusive_range_pattern)]
 
 #![plugin(clippy)]
 #![deny(clippy)]
@@ -228,14 +229,14 @@ fn overlapping() {
 
     match 42 {
         0 ... 10 => println!("0 ... 10"), //~ERROR: some ranges overlap
-        0 ... 11 => println!("0 ... 10"), //~NOTE overlaps with this
+        0 ... 11 => println!("0 ... 11"), //~NOTE overlaps with this
         _ => (),
     }
 
     match 42 {
         0 ... 5 => println!("0 ... 5"), //~ERROR: some ranges overlap
         6 ... 7 => println!("6 ... 7"),
-        FOO ... 11 => println!("0 ... 10"), //~NOTE overlaps with this
+        FOO ... 11 => println!("0 ... 11"), //~NOTE overlaps with this
         _ => (),
     }
 
@@ -245,9 +246,33 @@ fn overlapping() {
         _ => (),
     }
 
+    match 42 {
+        2 => println!("2"), //~NOTE overlaps with this
+        0 ... 2 => println!("0 ... 2"), //~ERROR: some ranges overlap
+        _ => (),
+    }
+
     match 42 {
         0 ... 10 => println!("0 ... 10"),
-        11 ... 50 => println!("0 ... 10"),
+        11 ... 50 => println!("11 ... 50"),
+        _ => (),
+    }
+
+    match 42 {
+        2 => println!("2"),
+        0 .. 2 => println!("0 .. 2"),
+        _ => (),
+    }
+
+    match 42 {
+        0 .. 10 => println!("0 .. 10"),
+        10 .. 50 => println!("10 .. 50"),
+        _ => (),
+    }
+
+    match 42 {
+        0 .. 11 => println!("0 .. 11"), //~ERROR: some ranges overlap
+        0 ... 11 => println!("0 ... 11"), //~NOTE overlaps with this
         _ => (),
     }
 
diff --git a/tests/matches.rs b/tests/matches.rs
@@ -1,7 +1,9 @@
 #![feature(rustc_private)]
+#![feature(collections_bound)]
 
 extern crate clippy_lints;
 extern crate syntax;
+use std::collections::Bound;
 
 #[test]
 fn test_overlapping() {
@@ -16,9 +18,12 @@ fn test_overlapping() {
     };
 
     assert_eq!(None, overlapping::<u8>(&[]));
-    assert_eq!(None, overlapping(&[sp(1, 4)]));
-    assert_eq!(None, overlapping(&[sp(1, 4), sp(5, 6)]));
-    assert_eq!(None, overlapping(&[sp(1, 4), sp(5, 6), sp(10, 11)]));
-    assert_eq!(Some((&sp(1, 4), &sp(3, 6))), overlapping(&[sp(1, 4), sp(3, 6)]));
-    assert_eq!(Some((&sp(5, 6), &sp(6, 11))), overlapping(&[sp(1, 4), sp(5, 6), sp(6, 11)]));
+    assert_eq!(None, overlapping(&[sp(1, Bound::Included(4))]));
+    assert_eq!(None, overlapping(&[sp(1, Bound::Included(4)), sp(5, Bound::Included(6))]));
+    assert_eq!(None,
+               overlapping(&[sp(1, Bound::Included(4)), sp(5, Bound::Included(6)), sp(10, Bound::Included(11))]));
+    assert_eq!(Some((&sp(1, Bound::Included(4)), &sp(3, Bound::Included(6)))),
+               overlapping(&[sp(1, Bound::Included(4)), sp(3, Bound::Included(6))]));
+    assert_eq!(Some((&sp(5, Bound::Included(6)), &sp(6, Bound::Included(11)))),
+               overlapping(&[sp(1, Bound::Included(4)), sp(5, Bound::Included(6)), sp(6, Bound::Included(11))]));
 }
