From aebdf74e168c4eb269c99dde333272f0edf609e0 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 30 Jan 2017 12:30:16 +0100 Subject: [PATCH 1/3] correctly check exclusive range patterns for overlap --- clippy_lints/src/lib.rs | 1 + clippy_lints/src/matches.rs | 65 +++++++++++++++++++++++++++-------- tests/compile-fail/matches.rs | 25 ++++++++++++++ tests/matches.rs | 15 +++++--- 4 files changed, 86 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b6729d797..37e3953f7 100644 --- 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 index ab9819c53..519298c33 100644 --- 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,10 +362,14 @@ fn all_ranges(cx: &LateContext, arms: &[Arm]) -> Vec> { } .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) }); }} @@ -372,7 +377,7 @@ fn all_ranges(cx: &LateContext, arms: &[Arm]) -> Vec> { 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> { #[derive(Debug, Eq, PartialEq)] pub struct SpannedRange { pub span: Span, - pub node: (T, T), + pub node: (T, Bound), } type TypedRanges = Vec>; @@ -393,13 +398,26 @@ type TypedRanges = Vec>; /// `Uint` and `Int` probably don't make sense. fn type_ranges(ranges: &[SpannedRange]) -> 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(ranges: &[SpannedRange]) -> Option<(&SpannedRange, & #[derive(Copy, Clone, Debug, Eq, PartialEq)] enum Kind<'a, T: 'a> { Start(T, &'a SpannedRange), - End(T, &'a SpannedRange), + End(Bound, &'a SpannedRange), } impl<'a, T: Copy> Kind<'a, T> { @@ -454,9 +472,9 @@ pub fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, & } } - fn value(self) -> T { + fn value(self) -> Bound { match self { - Kind::Start(t, _) | + Kind::Start(t, _) => Bound::Included(t), Kind::End(t, _) => t, } } @@ -470,7 +488,24 @@ pub fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, & 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), + (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 +525,7 @@ pub fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, & 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 index 23dad0a99..61a1ec51d 100644 --- 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)] @@ -245,12 +246,36 @@ fn overlapping() { _ => (), } + match 42 { + 2 => println!("2"), //~NOTE overlaps with this + 0 ... 2 => println!("0 ... 5"), //~ERROR: some ranges overlap + _ => (), + } + match 42 { 0 ... 10 => println!("0 ... 10"), 11 ... 50 => println!("0 ... 10"), _ => (), } + match 42 { + 2 => println!("2"), + 0 .. 2 => println!("0 .. 2"), + _ => (), + } + + match 42 { + 0 .. 10 => println!("0 ... 10"), + 10 .. 50 => println!("0 ... 10"), + _ => (), + } + + match 42 { + 0 .. 11 => println!("0 ... 10"), //~ERROR: some ranges overlap + 0 ... 11 => println!("0 ... 10"), //~NOTE overlaps with this + _ => (), + } + if let None = Some(42) { // nothing } else if let None = Some(42) { diff --git a/tests/matches.rs b/tests/matches.rs index 1d9d3dedd..fade73aad 100644 --- 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::(&[])); - 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))])); } From d9ec55e695ff4c9f4e405c88f32c96fc21afafbd Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 31 Jan 2017 08:08:54 +0100 Subject: [PATCH 2/3] address nits --- clippy_lints/src/matches.rs | 1 + tests/compile-fail/matches.rs | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 519298c33..9cb557ba6 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -491,6 +491,7 @@ pub fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, & 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)) => { diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index 61a1ec51d..7f6bd73d2 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -229,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 _ => (), } @@ -248,13 +248,13 @@ fn overlapping() { match 42 { 2 => println!("2"), //~NOTE overlaps with this - 0 ... 2 => println!("0 ... 5"), //~ERROR: some ranges overlap + 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"), _ => (), } @@ -265,8 +265,8 @@ fn overlapping() { } match 42 { - 0 .. 10 => println!("0 ... 10"), - 10 .. 50 => println!("0 ... 10"), + 0 .. 10 => println!("0 .. 10"), + 10 .. 50 => println!("10 .. 50"), _ => (), } From 19f119caec2d7f36a7069ea33d23d5545fca0476 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 31 Jan 2017 11:19:49 +0100 Subject: [PATCH 3/3] fix println message in tests --- tests/compile-fail/matches.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index 7f6bd73d2..6cfb45e1f 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -271,8 +271,8 @@ fn overlapping() { } match 42 { - 0 .. 11 => println!("0 ... 10"), //~ERROR: some ranges overlap - 0 ... 11 => println!("0 ... 10"), //~NOTE overlaps with this + 0 .. 11 => println!("0 .. 11"), //~ERROR: some ranges overlap + 0 ... 11 => println!("0 ... 11"), //~NOTE overlaps with this _ => (), }