From 7216e83189e2458fd5d3a311f9d1a487cfee51d9 Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 01:14:10 +0100 Subject: [PATCH 1/8] Implement #471 --- src/lib.rs | 1 + src/matches.rs | 184 +++++++++++++++++++++++++++++++++- tests/compile-fail/matches.rs | 31 +++++- 3 files changed, 210 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 29b911a0cb24..2e05fd1fc431 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -172,6 +172,7 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::WHILE_LET_ON_ITERATOR, map_clone::MAP_CLONE, matches::MATCH_BOOL, + matches::MATCH_OVERLAPPING_ARM, matches::MATCH_REF_PATS, matches::SINGLE_MATCH, methods::OK_EXPECT, diff --git a/src/matches.rs b/src/matches.rs index 460893d93ab1..69a6fdf6016d 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -1,10 +1,14 @@ use rustc::lint::*; -use rustc_front::hir::*; +use rustc::middle::const_eval::ConstVal::{Int, Uint}; +use rustc::middle::const_eval::EvalHint::ExprTypeChecked; +use rustc::middle::const_eval::{eval_const_expr_partial, ConstVal}; use rustc::middle::ty; +use rustc_front::hir::*; +use std::cmp::Ordering; use syntax::ast::Lit_::LitBool; use syntax::codemap::Span; -use utils::{snippet, span_lint, span_help_and_lint, in_external_macro, expr_block}; +use utils::{snippet, span_lint, span_note_and_lint, span_help_and_lint, in_external_macro, expr_block}; /// **What it does:** This lint checks for matches with a single arm where an `if let` will usually suffice. It is `Warn` by default. /// @@ -22,6 +26,7 @@ use utils::{snippet, span_lint, span_help_and_lint, in_external_macro, expr_bloc declare_lint!(pub SINGLE_MATCH, Warn, "a match statement with a single nontrivial arm (i.e, where the other arm \ is `_ => {}`) is used; recommends `if let` instead"); + /// **What it does:** This lint checks for matches where all arms match a reference, suggesting to remove the reference and deref the matched expression instead. It is `Warn` by default. /// /// **Why is this bad?** It just makes the code less readable. That reference destructuring adds nothing to the code. @@ -40,6 +45,7 @@ declare_lint!(pub SINGLE_MATCH, Warn, declare_lint!(pub MATCH_REF_PATS, Warn, "a match has all arms prefixed with `&`; the match expression can be \ dereferenced instead"); + /// **What it does:** This lint checks for matches where match expression is a `bool`. It suggests to replace the expression with an `if...else` block. It is `Warn` by default. /// /// **Why is this bad?** It makes the code less readable. @@ -58,6 +64,25 @@ declare_lint!(pub MATCH_REF_PATS, Warn, declare_lint!(pub MATCH_BOOL, Warn, "a match on boolean expression; recommends `if..else` block instead"); +/// **What it does:** This lint checks for overlapping match arms. It is `Warn` by default. +/// +/// **Why is this bad?** It is likely to be an error and if not, makes the code less obvious. +/// +/// **Known problems:** None +/// +/// **Example:** +/// +/// ``` +/// let x = 5; +/// match x { +/// 1 ... 10 => println!("1 ... 10"), +/// 5 ... 15 => println!("5 ... 15"), +/// _ => (), +/// } +/// ``` +declare_lint!(pub MATCH_OVERLAPPING_ARM, Warn, + "overlapping match arms"); + #[allow(missing_copy_implementations)] pub struct MatchPass; @@ -150,6 +175,22 @@ impl LateLintPass for MatchPass { Consider using an if..else block"); } } + + // MATCH_OVERLAPPING_ARM + if arms.len() >= 2 { + let ranges = all_ranges(cx, arms); + let overlap = match type_ranges(&ranges) { + TypedRanges::IntRanges(ranges) => overlaping(&ranges).map(|(start, end)| (start.span, end.span)), + TypedRanges::UintRanges(ranges) => overlaping(&ranges).map(|(start, end)| (start.span, end.span)), + TypedRanges::None => None, + }; + + if let Some((start, end)) = overlap { + span_note_and_lint(cx, MATCH_OVERLAPPING_ARM, start, + "some ranges overlap", + end, "overlaps with this"); + } + } } if let ExprMatch(ref ex, ref arms, source) = expr.node { // check preconditions for MATCH_REF_PATS @@ -170,6 +211,77 @@ impl LateLintPass for MatchPass { } } +/// Get all arms that are unbounded PatRange-s. +fn all_ranges(cx: &LateContext, arms: &[Arm]) -> Vec> { + arms.iter() + .filter_map(|arm| { + if let Arm { ref pats, guard: None, .. } = *arm { + Some(pats.iter().filter_map(|pat| { + if_let_chain! {[ + let PatRange(ref lhs, ref rhs) = pat.node, + let Ok(lhs) = eval_const_expr_partial(cx.tcx, &lhs, ExprTypeChecked, None), + let Ok(rhs) = eval_const_expr_partial(cx.tcx, &rhs, ExprTypeChecked, None) + ], { + return Some(SpannedRange { span: pat.span, node: (lhs, rhs) }); + }} + + None + })) + } + else { + None + } + }) + .flat_map(IntoIterator::into_iter) + .collect() +} + +#[derive(Debug, Eq, PartialEq)] +struct SpannedRange { + span: Span, + node: (T, T), +} + +#[derive(Debug)] +enum TypedRanges { + IntRanges(Vec>), + UintRanges(Vec>), + None, +} + +/// Get all `Int` ranges or all `Uint` ranges. Mixed types are an error anyway and other types than +/// `Uint` and `Int` probably don't make sense. +fn type_ranges(ranges: &[SpannedRange]) -> TypedRanges { + if ranges.is_empty() { + TypedRanges::None + } + else { + match ranges[0].node { + (Int(_), Int(_)) => { + TypedRanges::IntRanges(ranges.iter().filter_map(|range| { + if let (Int(start), Int(end)) = range.node { + Some(SpannedRange { span: range.span, node: (start, end) }) + } + else { + None + } + }).collect()) + }, + (Uint(_), Uint(_)) => { + TypedRanges::UintRanges(ranges.iter().filter_map(|range| { + if let (Uint(start), Uint(end)) = range.node { + Some(SpannedRange { span: range.span, node: (start, end) }) + } + else { + None + } + }).collect()) + }, + _ => TypedRanges::None, + } + } +} + fn is_unit_expr(expr: &Expr) -> bool { match expr.node { ExprTup(ref v) if v.is_empty() => true, @@ -209,3 +321,71 @@ fn match_template(cx: &LateContext, } } } + +fn overlaping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, &SpannedRange)> + where T: Copy + Ord { + #[derive(Copy, Clone, Debug, Eq, PartialEq)] + enum Kind<'a, T: 'a> { + Start(T, &'a SpannedRange), + End(T, &'a SpannedRange), + } + + impl<'a, T: Copy> Kind<'a, T> { + fn range(&self) -> &'a SpannedRange { + match *self { + Kind::Start(_, r) | Kind::End(_, r) => r + } + } + + fn value(self) -> T { + match self { + Kind::Start(t, _) | Kind::End(t, _) => t + } + } + } + + impl<'a, T: Copy + Ord> PartialOrd for Kind<'a, T> { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } + } + + impl<'a, T: Copy + Ord> Ord for Kind<'a, T> { + fn cmp(&self, other: &Self) -> Ordering { + self.value().cmp(&other.value()) + } + } + + let mut values = Vec::with_capacity(2*ranges.len()); + + for r in ranges { + values.push(Kind::Start(r.node.0, &r)); + values.push(Kind::End(r.node.1, &r)); + } + + values.sort(); + + for (a, b) in values.iter().zip(values.iter().skip(1)) { + match (a, b) { + (&Kind::Start(_, ra), &Kind::End(_, rb)) => if ra.node != rb.node { return Some((ra, rb)) }, + (&Kind::End(a, _), &Kind::Start(b, _)) if a != b => (), + _ => return Some((&a.range(), &b.range())), + } + } + + None +} + +#[test] +fn test_overlapping() { + use syntax::codemap::DUMMY_SP; + + let sp = |s, e| SpannedRange { span: DUMMY_SP, node: (s, e) }; + + assert_eq!(None, overlaping::(&[])); + assert_eq!(None, overlaping(&[sp(1, 4)])); + assert_eq!(None, overlaping(&[sp(1, 4), sp(5, 6)])); + assert_eq!(None, overlaping(&[sp(1, 4), sp(5, 6), sp(10, 11)])); + assert_eq!(Some((&sp(1, 4), &sp(3, 6))), overlaping(&[sp(1, 4), sp(3, 6)])); + assert_eq!(Some((&sp(5, 6), &sp(6, 11))), overlaping(&[sp(1, 4), sp(5, 6), sp(6, 11)])); +} diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index ea3a48a94f5d..ab181901c9c1 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -51,17 +51,17 @@ fn match_bool() { true => 1, false => 0, }; - + match test { //~ ERROR you seem to be trying to match on a boolean expression true => (), false => { println!("Noooo!"); } }; - + match test { //~ ERROR you seem to be trying to match on a boolean expression false => { println!("Noooo!"); } _ => (), }; - + match test { //~ ERROR you seem to be trying to match on a boolean expression false => { println!("Noooo!"); } true => { println!("Yes!"); } @@ -70,7 +70,7 @@ fn match_bool() { // Not linted match option { 1 ... 10 => (), - 10 ... 20 => (), + 11 ... 20 => (), _ => (), }; } @@ -115,5 +115,28 @@ fn ref_pats() { } } +fn overlapping() { + const FOO : u64 = 2; + + match 42 { + 0 ... 10 => println!("0 ... 10"), //~ERROR + 0 ... 11 => println!("0 ... 10"), + _ => (), + } + + match 42 { + 0 ... 5 => println!("0 ... 10"), //~ERROR + 6 ... 7 => println!("6 ... 7"), + FOO ... 11 => println!("0 ... 10"), + _ => (), + } + + match 42 { + 0 ... 10 => println!("0 ... 10"), + 11 ... 50 => println!("0 ... 10"), + _ => (), + } +} + fn main() { } From 3373ea43c0ba0f226bc0a5e9bd578118fa564547 Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 02:06:18 +0100 Subject: [PATCH 2/8] Consider literal patterns in MATCH_OVERLAPPING_ARM --- src/matches.rs | 7 +++++++ tests/compile-fail/matches.rs | 8 +++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/matches.rs b/src/matches.rs index 69a6fdf6016d..227a5bdb2df8 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -225,6 +225,13 @@ fn all_ranges(cx: &LateContext, arms: &[Arm]) -> Vec> { return Some(SpannedRange { span: pat.span, node: (lhs, rhs) }); }} + if_let_chain! {[ + let PatLit(ref value) = pat.node, + let Ok(value) = eval_const_expr_partial(cx.tcx, &value, ExprTypeChecked, None) + ], { + return Some(SpannedRange { span: pat.span, node: (value.clone(), value) }); + }} + None })) } diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index ab181901c9c1..94d24746e4d0 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -125,12 +125,18 @@ fn overlapping() { } match 42 { - 0 ... 5 => println!("0 ... 10"), //~ERROR + 0 ... 5 => println!("0 ... 5"), //~ERROR 6 ... 7 => println!("6 ... 7"), FOO ... 11 => println!("0 ... 10"), _ => (), } + match 42 { + 2 => println!("2"), + 0 ... 5 => println!("0 ... 5"), //~ERROR + _ => (), + } + match 42 { 0 ... 10 => println!("0 ... 10"), 11 ... 50 => println!("0 ... 10"), From 0c8de9ed52ddc14e3797f89ab260a4e45be62418 Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 02:19:32 +0100 Subject: [PATCH 3/8] Split MatchPass::check_expr for dogfood --- src/matches.rs | 221 +++++++++++++++++++++++++------------------------ 1 file changed, 115 insertions(+), 106 deletions(-) diff --git a/src/matches.rs b/src/matches.rs index 227a5bdb2df8..78b39ac17918 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -96,117 +96,126 @@ impl LateLintPass for MatchPass { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { if in_external_macro(cx, expr.span) { return; } if let ExprMatch(ref ex, ref arms, MatchSource::Normal) = expr.node { - // check preconditions for SINGLE_MATCH - // only two arms - if arms.len() == 2 && - // both of the arms have a single pattern and no guard - arms[0].pats.len() == 1 && arms[0].guard.is_none() && - arms[1].pats.len() == 1 && arms[1].guard.is_none() && - // and the second pattern is a `_` wildcard: this is not strictly necessary, - // since the exhaustiveness check will ensure the last one is a catch-all, - // but in some cases, an explicit match is preferred to catch situations - // when an enum is extended, so we don't consider these cases - arms[1].pats[0].node == PatWild && - // we don't want any content in the second arm (unit or empty block) - is_unit_expr(&arms[1].body) && - // finally, MATCH_BOOL doesn't apply here - (cx.tcx.expr_ty(ex).sty != ty::TyBool || cx.current_level(MATCH_BOOL) == Allow) - { - span_help_and_lint(cx, SINGLE_MATCH, expr.span, - "you seem to be trying to use match for destructuring a \ - single pattern. Consider using `if let`", - &format!("try\nif let {} = {} {}", - snippet(cx, arms[0].pats[0].span, ".."), - snippet(cx, ex.span, ".."), - expr_block(cx, &arms[0].body, None, ".."))); - } - - // check preconditions for MATCH_BOOL - // type of expression == bool - if cx.tcx.expr_ty(ex).sty == ty::TyBool { - if arms.len() == 2 && arms[0].pats.len() == 1 { // no guards - let exprs = if let PatLit(ref arm_bool) = arms[0].pats[0].node { - if let ExprLit(ref lit) = arm_bool.node { - match lit.node { - LitBool(true) => Some((&*arms[0].body, &*arms[1].body)), - LitBool(false) => Some((&*arms[1].body, &*arms[0].body)), - _ => None, - } - } else { None } - } else { None }; - if let Some((ref true_expr, ref false_expr)) = exprs { - if !is_unit_expr(true_expr) { - if !is_unit_expr(false_expr) { - span_help_and_lint(cx, MATCH_BOOL, expr.span, - "you seem to be trying to match on a boolean expression. \ - Consider using an if..else block:", - &format!("try\nif {} {} else {}", - snippet(cx, ex.span, "b"), - expr_block(cx, true_expr, None, ".."), - expr_block(cx, false_expr, None, ".."))); - } else { - span_help_and_lint(cx, MATCH_BOOL, expr.span, - "you seem to be trying to match on a boolean expression. \ - Consider using an if..else block:", - &format!("try\nif {} {}", - snippet(cx, ex.span, "b"), - expr_block(cx, true_expr, None, ".."))); - } - } else if !is_unit_expr(false_expr) { - span_help_and_lint(cx, MATCH_BOOL, expr.span, - "you seem to be trying to match on a boolean expression. \ - Consider using an if..else block:", - &format!("try\nif !{} {}", - snippet(cx, ex.span, "b"), - expr_block(cx, false_expr, None, ".."))); - } else { - span_lint(cx, MATCH_BOOL, expr.span, - "you seem to be trying to match on a boolean expression. \ - Consider using an if..else block"); - } - } else { - span_lint(cx, MATCH_BOOL, expr.span, - "you seem to be trying to match on a boolean expression. \ - Consider using an if..else block"); - } - } else { - span_lint(cx, MATCH_BOOL, expr.span, - "you seem to be trying to match on a boolean expression. \ - Consider using an if..else block"); - } - } - - // MATCH_OVERLAPPING_ARM - if arms.len() >= 2 { - let ranges = all_ranges(cx, arms); - let overlap = match type_ranges(&ranges) { - TypedRanges::IntRanges(ranges) => overlaping(&ranges).map(|(start, end)| (start.span, end.span)), - TypedRanges::UintRanges(ranges) => overlaping(&ranges).map(|(start, end)| (start.span, end.span)), - TypedRanges::None => None, - }; - - if let Some((start, end)) = overlap { - span_note_and_lint(cx, MATCH_OVERLAPPING_ARM, start, - "some ranges overlap", - end, "overlaps with this"); - } - } + check_single_match(cx, ex, arms, expr); + check_match_bool(cx, ex, arms, expr); + check_overlapping_arms(cx, arms); } if let ExprMatch(ref ex, ref arms, source) = expr.node { - // check preconditions for MATCH_REF_PATS - if has_only_ref_pats(arms) { - if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node { - let template = match_template(cx, expr.span, source, "", inner); - span_lint(cx, MATCH_REF_PATS, expr.span, &format!( - "you don't need to add `&` to both the expression \ - and the patterns: use `{}`", template)); + check_match_ref_pats(cx, ex, arms, source, expr); + } + } +} + +fn check_single_match(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { + if arms.len() == 2 && + // both of the arms have a single pattern and no guard + arms[0].pats.len() == 1 && arms[0].guard.is_none() && + arms[1].pats.len() == 1 && arms[1].guard.is_none() && + // and the second pattern is a `_` wildcard: this is not strictly necessary, + // since the exhaustiveness check will ensure the last one is a catch-all, + // but in some cases, an explicit match is preferred to catch situations + // when an enum is extended, so we don't consider these cases + arms[1].pats[0].node == PatWild && + // we don't want any content in the second arm (unit or empty block) + is_unit_expr(&arms[1].body) && + // finally, MATCH_BOOL doesn't apply here + (cx.tcx.expr_ty(ex).sty != ty::TyBool || cx.current_level(MATCH_BOOL) == Allow) + { + span_help_and_lint(cx, SINGLE_MATCH, expr.span, + "you seem to be trying to use match for destructuring a \ + single pattern. Consider using `if let`", + &format!("try\nif let {} = {} {}", + snippet(cx, arms[0].pats[0].span, ".."), + snippet(cx, ex.span, ".."), + expr_block(cx, &arms[0].body, None, ".."))); + } +} + +fn check_match_bool(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { + // type of expression == bool + if cx.tcx.expr_ty(ex).sty == ty::TyBool { + if arms.len() == 2 && arms[0].pats.len() == 1 { // no guards + let exprs = if let PatLit(ref arm_bool) = arms[0].pats[0].node { + if let ExprLit(ref lit) = arm_bool.node { + match lit.node { + LitBool(true) => Some((&*arms[0].body, &*arms[1].body)), + LitBool(false) => Some((&*arms[1].body, &*arms[0].body)), + _ => None, + } + } else { None } + } else { None }; + if let Some((ref true_expr, ref false_expr)) = exprs { + if !is_unit_expr(true_expr) { + if !is_unit_expr(false_expr) { + span_help_and_lint(cx, MATCH_BOOL, expr.span, + "you seem to be trying to match on a boolean expression. \ + Consider using an if..else block:", + &format!("try\nif {} {} else {}", + snippet(cx, ex.span, "b"), + expr_block(cx, true_expr, None, ".."), + expr_block(cx, false_expr, None, ".."))); + } else { + span_help_and_lint(cx, MATCH_BOOL, expr.span, + "you seem to be trying to match on a boolean expression. \ + Consider using an if..else block:", + &format!("try\nif {} {}", + snippet(cx, ex.span, "b"), + expr_block(cx, true_expr, None, ".."))); + } + } else if !is_unit_expr(false_expr) { + span_help_and_lint(cx, MATCH_BOOL, expr.span, + "you seem to be trying to match on a boolean expression. \ + Consider using an if..else block:", + &format!("try\nif !{} {}", + snippet(cx, ex.span, "b"), + expr_block(cx, false_expr, None, ".."))); } else { - let template = match_template(cx, expr.span, source, "*", ex); - span_lint(cx, MATCH_REF_PATS, expr.span, &format!( - "instead of prefixing all patterns with `&`, you can dereference the \ - expression: `{}`", template)); + span_lint(cx, MATCH_BOOL, expr.span, + "you seem to be trying to match on a boolean expression. \ + Consider using an if..else block"); } + } else { + span_lint(cx, MATCH_BOOL, expr.span, + "you seem to be trying to match on a boolean expression. \ + Consider using an if..else block"); } + } else { + span_lint(cx, MATCH_BOOL, expr.span, + "you seem to be trying to match on a boolean expression. \ + Consider using an if..else block"); + } + } +} + +fn check_overlapping_arms(cx: &LateContext, arms: &[Arm]) { + if arms.len() >= 2 { + let ranges = all_ranges(cx, arms); + let overlap = match type_ranges(&ranges) { + TypedRanges::IntRanges(ranges) => overlaping(&ranges).map(|(start, end)| (start.span, end.span)), + TypedRanges::UintRanges(ranges) => overlaping(&ranges).map(|(start, end)| (start.span, end.span)), + TypedRanges::None => None, + }; + + if let Some((start, end)) = overlap { + span_note_and_lint(cx, MATCH_OVERLAPPING_ARM, start, + "some ranges overlap", + end, "overlaps with this"); + } + } +} + +fn check_match_ref_pats(cx: &LateContext, ex: &Expr, arms: &[Arm], source: MatchSource, expr: &Expr) { + if has_only_ref_pats(arms) { + if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node { + let template = match_template(cx, expr.span, source, "", inner); + span_lint(cx, MATCH_REF_PATS, expr.span, &format!( + "you don't need to add `&` to both the expression \ + and the patterns: use `{}`", template)); + } else { + let template = match_template(cx, expr.span, source, "*", ex); + span_lint(cx, MATCH_REF_PATS, expr.span, &format!( + "instead of prefixing all patterns with `&`, you can dereference the \ + expression: `{}`", template)); } } } From 1aa3956b8a84de9e2e81e9f10ad1f72a0a935918 Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 02:41:20 +0100 Subject: [PATCH 4/8] Update README --- README.md | 3 ++- src/matches.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index dc8fd37dfc9c..6d1b88688c15 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 85 lints included in this crate: +There are 86 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -40,6 +40,7 @@ name [linkedlist](https://github.com/Manishearth/rust-clippy/wiki#linkedlist) | warn | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a VecDeque [map_clone](https://github.com/Manishearth/rust-clippy/wiki#map_clone) | warn | using `.map(|x| x.clone())` to clone an iterator or option's contents (recommends `.cloned()` instead) [match_bool](https://github.com/Manishearth/rust-clippy/wiki#match_bool) | warn | a match on boolean expression; recommends `if..else` block instead +[match_overlapping_arm](https://github.com/Manishearth/rust-clippy/wiki#match_overlapping_arm) | warn | a match has overlapping arms [match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats) | warn | a match has all arms prefixed with `&`; the match expression can be dereferenced instead [min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max) | warn | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one) | warn | taking a number modulo 1, which always returns 0 diff --git a/src/matches.rs b/src/matches.rs index 78b39ac17918..1cdf76db663b 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -81,7 +81,7 @@ declare_lint!(pub MATCH_BOOL, Warn, /// } /// ``` declare_lint!(pub MATCH_OVERLAPPING_ARM, Warn, - "overlapping match arms"); + "a match has overlapping arms"); #[allow(missing_copy_implementations)] pub struct MatchPass; From 90efb7b76d401995eb054799dc55058aa66887c7 Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 11:25:32 +0100 Subject: [PATCH 5/8] Fix typo --- src/matches.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/matches.rs b/src/matches.rs index 1cdf76db663b..3ece02c03744 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -191,8 +191,8 @@ fn check_overlapping_arms(cx: &LateContext, arms: &[Arm]) { if arms.len() >= 2 { let ranges = all_ranges(cx, arms); let overlap = match type_ranges(&ranges) { - TypedRanges::IntRanges(ranges) => overlaping(&ranges).map(|(start, end)| (start.span, end.span)), - TypedRanges::UintRanges(ranges) => overlaping(&ranges).map(|(start, end)| (start.span, end.span)), + TypedRanges::IntRanges(ranges) => overlapping(&ranges).map(|(start, end)| (start.span, end.span)), + TypedRanges::UintRanges(ranges) => overlapping(&ranges).map(|(start, end)| (start.span, end.span)), TypedRanges::None => None, }; @@ -338,7 +338,7 @@ fn match_template(cx: &LateContext, } } -fn overlaping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, &SpannedRange)> +fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, &SpannedRange)> where T: Copy + Ord { #[derive(Copy, Clone, Debug, Eq, PartialEq)] enum Kind<'a, T: 'a> { @@ -398,10 +398,10 @@ fn test_overlapping() { let sp = |s, e| SpannedRange { span: DUMMY_SP, node: (s, e) }; - assert_eq!(None, overlaping::(&[])); - assert_eq!(None, overlaping(&[sp(1, 4)])); - assert_eq!(None, overlaping(&[sp(1, 4), sp(5, 6)])); - assert_eq!(None, overlaping(&[sp(1, 4), sp(5, 6), sp(10, 11)])); - assert_eq!(Some((&sp(1, 4), &sp(3, 6))), overlaping(&[sp(1, 4), sp(3, 6)])); - assert_eq!(Some((&sp(5, 6), &sp(6, 11))), overlaping(&[sp(1, 4), sp(5, 6), sp(6, 11)])); + 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)])); } From 2fd3093395d989bb8ada55440fb4101264e676e5 Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 11:25:44 +0100 Subject: [PATCH 6/8] Only run MATCH_OVERLAPPING_ARM on integral matches --- src/matches.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/matches.rs b/src/matches.rs index 3ece02c03744..053b6b072b51 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -98,7 +98,7 @@ impl LateLintPass for MatchPass { if let ExprMatch(ref ex, ref arms, MatchSource::Normal) = expr.node { check_single_match(cx, ex, arms, expr); check_match_bool(cx, ex, arms, expr); - check_overlapping_arms(cx, arms); + check_overlapping_arms(cx, ex, arms); } if let ExprMatch(ref ex, ref arms, source) = expr.node { check_match_ref_pats(cx, ex, arms, source, expr); @@ -187,8 +187,9 @@ fn check_match_bool(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { } } -fn check_overlapping_arms(cx: &LateContext, arms: &[Arm]) { - if arms.len() >= 2 { +fn check_overlapping_arms(cx: &LateContext, ex: &Expr, arms: &[Arm]) { + if arms.len() >= 2 && + cx.tcx.expr_ty(ex).is_integral() { let ranges = all_ranges(cx, arms); let overlap = match type_ranges(&ranges) { TypedRanges::IntRanges(ranges) => overlapping(&ranges).map(|(start, end)| (start.span, end.span)), From d01987a40bea852260f5d1442bb0631c0a893ec0 Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 11:26:35 +0100 Subject: [PATCH 7/8] Include error message in tests --- tests/compile-fail/matches.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index 94d24746e4d0..b569f9566efd 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -119,13 +119,13 @@ fn overlapping() { const FOO : u64 = 2; match 42 { - 0 ... 10 => println!("0 ... 10"), //~ERROR + 0 ... 10 => println!("0 ... 10"), //~ERROR: some ranges overlap 0 ... 11 => println!("0 ... 10"), _ => (), } match 42 { - 0 ... 5 => println!("0 ... 5"), //~ERROR + 0 ... 5 => println!("0 ... 5"), //~ERROR: some ranges overlap 6 ... 7 => println!("6 ... 7"), FOO ... 11 => println!("0 ... 10"), _ => (), @@ -133,7 +133,7 @@ fn overlapping() { match 42 { 2 => println!("2"), - 0 ... 5 => println!("0 ... 5"), //~ERROR + 0 ... 5 => println!("0 ... 5"), //~ERROR: some ranges overlap _ => (), } From 0fa8481ba390dc9b860123950717acbb34bd39fd Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 17:48:30 +0100 Subject: [PATCH 8/8] Put tests in tests folder --- src/matches.rs | 22 ++++------------------ tests/matches.rs | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 18 deletions(-) create mode 100644 tests/matches.rs diff --git a/src/matches.rs b/src/matches.rs index 053b6b072b51..e4172b40932f 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -254,9 +254,9 @@ fn all_ranges(cx: &LateContext, arms: &[Arm]) -> Vec> { } #[derive(Debug, Eq, PartialEq)] -struct SpannedRange { - span: Span, - node: (T, T), +pub struct SpannedRange { + pub span: Span, + pub node: (T, T), } #[derive(Debug)] @@ -339,7 +339,7 @@ fn match_template(cx: &LateContext, } } -fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, &SpannedRange)> +pub fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, &SpannedRange)> where T: Copy + Ord { #[derive(Copy, Clone, Debug, Eq, PartialEq)] enum Kind<'a, T: 'a> { @@ -392,17 +392,3 @@ fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, &Span None } - -#[test] -fn test_overlapping() { - use syntax::codemap::DUMMY_SP; - - let sp = |s, e| SpannedRange { span: DUMMY_SP, node: (s, e) }; - - 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)])); -} diff --git a/tests/matches.rs b/tests/matches.rs new file mode 100644 index 000000000000..03cc52817417 --- /dev/null +++ b/tests/matches.rs @@ -0,0 +1,20 @@ +#![allow(plugin_as_library)] +#![feature(rustc_private)] + +extern crate clippy; +extern crate syntax; + +#[test] +fn test_overlapping() { + use clippy::matches::overlapping; + use syntax::codemap::DUMMY_SP; + + let sp = |s, e| clippy::matches::SpannedRange { span: DUMMY_SP, node: (s, e) }; + + 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)])); +}