From 10cf141ebb9c9856858d5f4cc36281b2b43d37b3 Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Sat, 11 Jan 2020 19:39:43 +0900 Subject: [PATCH] Apply review comments --- clippy_lints/src/matches.rs | 57 ++++++++++++++++++++------- clippy_lints/src/utils/author.rs | 9 ++--- clippy_lints/src/utils/hir_utils.rs | 7 +--- tests/ui/match_overlapping_arm.rs | 13 ++++++ tests/ui/match_overlapping_arm.stderr | 46 +++++++++++++++------ 5 files changed, 95 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 5aed9770d2e..fca8e1446a8 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1,4 +1,4 @@ -use crate::consts::{constant, Constant}; +use crate::consts::{constant, miri_to_const, Constant}; use crate::utils::paths; use crate::utils::sugg::Sugg; use crate::utils::{ @@ -444,7 +444,7 @@ fn check_match_bool(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>], e fn check_overlapping_arms<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ex: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>]) { if arms.len() >= 2 && cx.tables.expr_ty(ex).is_integral() { - let ranges = all_ranges(cx, arms); + let ranges = all_ranges(cx, arms, cx.tables.expr_ty(ex)); let type_ranges = type_ranges(&ranges); if !type_ranges.is_empty() { if let Some((start, end)) = overlapping(&type_ranges) { @@ -705,25 +705,52 @@ fn check_wild_in_or_pats(cx: &LateContext<'_, '_>, arms: &[Arm<'_>]) { } /// Gets all arms that are unbounded `PatRange`s. -fn all_ranges<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arms: &'tcx [Arm<'_>]) -> Vec> { +fn all_ranges<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + arms: &'tcx [Arm<'_>], + ty: Ty<'tcx>, +) -> Vec> { arms.iter() .flat_map(|arm| { if let Arm { ref pat, guard: None, .. } = *arm { - if let PatKind::Range(ref lhs, ref rhs, ref range_end) = pat.kind { - if let (Some(l), Some(r)) = (lhs, rhs) { - let lhs = constant(cx, cx.tables, l)?.0; - let rhs = constant(cx, cx.tables, r)?.0; - 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 PatKind::Range(ref lhs, ref rhs, range_end) = pat.kind { + match (lhs, rhs) { + (Some(lhs), Some(rhs)) => { + let lhs = constant(cx, cx.tables, lhs)?.0; + let rhs = constant(cx, cx.tables, rhs)?.0; + let rhs = match range_end { + RangeEnd::Included => Bound::Included(rhs), + RangeEnd::Excluded => Bound::Excluded(rhs), + }; + return Some(SpannedRange { + span: pat.span, + node: (lhs, rhs), + }); + }, + (None, Some(rhs)) => { + let lhs = miri_to_const(ty.numeric_min_val(cx.tcx)?)?; + let rhs = constant(cx, cx.tables, rhs)?.0; + let rhs = match range_end { + RangeEnd::Included => Bound::Included(rhs), + RangeEnd::Excluded => Bound::Excluded(rhs), + }; + return Some(SpannedRange { + span: pat.span, + node: (lhs, rhs), + }); + }, + (Some(lhs), None) => { + let lhs = constant(cx, cx.tables, lhs)?.0; + let rhs = miri_to_const(ty.numeric_max_val(cx.tcx)?)?; + return Some(SpannedRange { + span: pat.span, + node: (lhs, Bound::Excluded(rhs)), + }); + }, + _ => return None, } } diff --git a/clippy_lints/src/utils/author.rs b/clippy_lints/src/utils/author.rs index b9f11f79a37..cfbb32806da 100644 --- a/clippy_lints/src/utils/author.rs +++ b/clippy_lints/src/utils/author.rs @@ -12,6 +12,7 @@ use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, Pat, PatKind, QPath, Stmt, StmtKind, TyKind}; use rustc_session::declare_tool_lint; use syntax::ast::{Attribute, LitFloatType, LitKind}; +use syntax::walk_list; declare_clippy_lint! { /// **What it does:** Generates clippy code that detects the offending pattern @@ -617,13 +618,9 @@ impl<'tcx> Visitor<'tcx> for PrintVisitor { start_pat, end_pat, end_kind, current ); self.current = start_pat; - if let Some(expr) = start { - self.visit_expr(expr); - } + walk_list!(self, visit_expr, start); self.current = end_pat; - if let Some(expr) = end { - self.visit_expr(expr); - } + walk_list!(self, visit_expr, end); }, PatKind::Slice(ref start, ref middle, ref end) => { let start_pat = self.next("start"); diff --git a/clippy_lints/src/utils/hir_utils.rs b/clippy_lints/src/utils/hir_utils.rs index 00baad72e6d..01f719f4ab8 100644 --- a/clippy_lints/src/utils/hir_utils.rs +++ b/clippy_lints/src/utils/hir_utils.rs @@ -194,11 +194,8 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { (&PatKind::Tuple(ref l, ls), &PatKind::Tuple(ref r, rs)) => { ls == rs && over(l, r, |l, r| self.eq_pat(l, r)) }, - (&PatKind::Range(ref ls, ref le, ref li), &PatKind::Range(ref rs, ref re, ref ri)) => { - if let (Some(ls), Some(rs), Some(le), Some(re)) = (ls, rs, le, re) { - return self.eq_expr(ls, rs) && self.eq_expr(le, re) && (*li == *ri); - } - false + (&PatKind::Range(ref ls, ref le, li), &PatKind::Range(ref rs, ref re, ri)) => { + both(ls, rs, |a, b| self.eq_expr(a, b)) && both(le, re, |a, b| self.eq_expr(a, b)) && (li == ri) }, (&PatKind::Ref(ref le, ref lm), &PatKind::Ref(ref re, ref rm)) => lm == rm && self.eq_pat(le, re), (&PatKind::Slice(ref ls, ref li, ref le), &PatKind::Slice(ref rs, ref ri, ref re)) => { diff --git a/tests/ui/match_overlapping_arm.rs b/tests/ui/match_overlapping_arm.rs index a48de440652..ce7761119cb 100644 --- a/tests/ui/match_overlapping_arm.rs +++ b/tests/ui/match_overlapping_arm.rs @@ -1,4 +1,5 @@ #![feature(exclusive_range_pattern)] +#![feature(half_open_range_patterns)] #![warn(clippy::match_overlapping_arm)] #![allow(clippy::redundant_pattern_matching)] @@ -56,6 +57,18 @@ fn overlapping() { _ => (), } + match 42 { + 0.. => println!("0 .. 42"), + 3.. => println!("3 .. 42"), + _ => (), + } + + match 42 { + ..=23 => println!("0 ... 23"), + ..26 => println!("0 .. 26"), + _ => (), + } + if let None = Some(42) { // nothing } else if let None = Some(42) { diff --git a/tests/ui/match_overlapping_arm.stderr b/tests/ui/match_overlapping_arm.stderr index dcd42fca7b4..32dcefa86d8 100644 --- a/tests/ui/match_overlapping_arm.stderr +++ b/tests/ui/match_overlapping_arm.stderr @@ -1,63 +1,87 @@ error: some ranges overlap - --> $DIR/match_overlapping_arm.rs:11:9 + --> $DIR/match_overlapping_arm.rs:12:9 | LL | 0..=10 => println!("0 ... 10"), | ^^^^^^ | = note: `-D clippy::match-overlapping-arm` implied by `-D warnings` note: overlaps with this - --> $DIR/match_overlapping_arm.rs:12:9 + --> $DIR/match_overlapping_arm.rs:13:9 | LL | 0..=11 => println!("0 ... 11"), | ^^^^^^ error: some ranges overlap - --> $DIR/match_overlapping_arm.rs:17:9 + --> $DIR/match_overlapping_arm.rs:18:9 | LL | 0..=5 => println!("0 ... 5"), | ^^^^^ | note: overlaps with this - --> $DIR/match_overlapping_arm.rs:19:9 + --> $DIR/match_overlapping_arm.rs:20:9 | LL | FOO..=11 => println!("0 ... 11"), | ^^^^^^^^ error: some ranges overlap - --> $DIR/match_overlapping_arm.rs:25:9 + --> $DIR/match_overlapping_arm.rs:26:9 | LL | 0..=5 => println!("0 ... 5"), | ^^^^^ | note: overlaps with this - --> $DIR/match_overlapping_arm.rs:24:9 + --> $DIR/match_overlapping_arm.rs:25:9 | LL | 2 => println!("2"), | ^ error: some ranges overlap - --> $DIR/match_overlapping_arm.rs:31:9 + --> $DIR/match_overlapping_arm.rs:32:9 | LL | 0..=2 => println!("0 ... 2"), | ^^^^^ | note: overlaps with this - --> $DIR/match_overlapping_arm.rs:30:9 + --> $DIR/match_overlapping_arm.rs:31:9 | LL | 2 => println!("2"), | ^ error: some ranges overlap - --> $DIR/match_overlapping_arm.rs:54:9 + --> $DIR/match_overlapping_arm.rs:55:9 | LL | 0..11 => println!("0 .. 11"), | ^^^^^ | note: overlaps with this - --> $DIR/match_overlapping_arm.rs:55:9 + --> $DIR/match_overlapping_arm.rs:56:9 | LL | 0..=11 => println!("0 ... 11"), | ^^^^^^ -error: aborting due to 5 previous errors +error: some ranges overlap + --> $DIR/match_overlapping_arm.rs:61:9 + | +LL | 0.. => println!("0 .. 42"), + | ^^^ + | +note: overlaps with this + --> $DIR/match_overlapping_arm.rs:62:9 + | +LL | 3.. => println!("3 .. 42"), + | ^^^ + +error: some ranges overlap + --> $DIR/match_overlapping_arm.rs:67:9 + | +LL | ..=23 => println!("0 ... 23"), + | ^^^^^ + | +note: overlaps with this + --> $DIR/match_overlapping_arm.rs:68:9 + | +LL | ..26 => println!("0 .. 26"), + | ^^^^ + +error: aborting due to 7 previous errors