Auto merge of rust-lang#5692 - ebroto:5689_N_dotdot_N, r=yaahc

reversed_empty_ranges: avoid linting N..N except in for loop arguments changelog: [`reversed_empty_ranges`]: avoid linting N..N except in for loop arguments r? @yaahc Fixes rust-lang#5689
Dylan-DPC-zz · Jun 8, 2020 · ff0993c · ff0993c
2 parents f947644 + a664ce7
commit ff0993c
Show file tree

Hide file tree

Showing 6 changed files with 51 additions and 66 deletions.
diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs
@@ -241,14 +241,26 @@ fn check_inclusive_range_minus_one(cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
 }
 
 fn check_reversed_empty_range(cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
-    fn inside_indexing_expr<'a>(cx: &'a LateContext<'_, '_>, expr: &Expr<'_>) -> Option<&'a Expr<'a>> {
-        match get_parent_expr(cx, expr) {
-            parent_expr @ Some(Expr {
+    fn inside_indexing_expr(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
+        matches!(
+            get_parent_expr(cx, expr),
+            Some(Expr {
                 kind: ExprKind::Index(..),
                 ..
-            }) => parent_expr,
-            _ => None,
+            })
+        )
+    }
+
+    fn is_for_loop_arg(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
+        let mut cur_expr = expr;
+        while let Some(parent_expr) = get_parent_expr(cx, cur_expr) {
+            match higher::for_loop(parent_expr) {
+                Some((_, args, _)) if args.hir_id == expr.hir_id => return true,
+                _ => cur_expr = parent_expr,
+            }
         }
+
+        false
     }
 
     fn is_empty_range(limits: RangeLimits, ordering: Ordering) -> bool {
@@ -267,34 +279,18 @@ fn check_reversed_empty_range(cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
         if let Some(ordering) = Constant::partial_cmp(cx.tcx, ty, &start_idx, &end_idx);
         if is_empty_range(limits, ordering);
         then {
-            if let Some(parent_expr) = inside_indexing_expr(cx, expr) {
-                let (reason, outcome) = if ordering == Ordering::Equal {
-                    ("empty", "always yield an empty slice")
-                } else {
-                    ("reversed", "panic at run-time")
-                };
-
-                span_lint_and_then(
-                    cx,
-                    REVERSED_EMPTY_RANGES,
-                    expr.span,
-                    &format!("this range is {} and using it to index a slice will {}", reason, outcome),
-                    |diag| {
-                        if_chain! {
-                            if ordering == Ordering::Equal;
-                            if let ty::Slice(slice_ty) = cx.tables.expr_ty(parent_expr).kind;
-                            then {
-                                diag.span_suggestion(
-                                    parent_expr.span,
-                                    "if you want an empty slice, use",
-                                    format!("[] as &[{}]", slice_ty),
-                                    Applicability::MaybeIncorrect
-                                );
-                            }
-                        }
-                    }
-                );
-            } else {
+            if inside_indexing_expr(cx, expr) {
+                // Avoid linting `N..N` as it has proven to be useful, see #5689 and #5628 ...
+                if ordering != Ordering::Equal {
+                    span_lint(
+                        cx,
+                        REVERSED_EMPTY_RANGES,
+                        expr.span,
+                        "this range is reversed and using it to index a slice will panic at run-time",
+                    );
+                }
+            // ... except in for loop arguments for backwards compatibility with `reverse_range_loop`
+            } else if ordering != Ordering::Equal || is_for_loop_arg(cx, expr) {
                 span_lint_and_then(
                     cx,
                     REVERSED_EMPTY_RANGES,

diff --git a/tests/ui/reversed_empty_ranges_fixable.fixed b/tests/ui/reversed_empty_ranges_fixable.fixed
@@ -4,8 +4,6 @@
 const ANSWER: i32 = 42;
 
 fn main() {
-    let arr = [1, 2, 3, 4, 5];
-
     // These should be linted:
 
     (21..=42).rev().for_each(|x| println!("{}", x));
@@ -14,16 +12,18 @@ fn main() {
     for _ in (-42..=-21).rev() {}
     for _ in (21u32..42u32).rev() {}
 
-    let _ = &[] as &[i32];
-
     // These should be ignored as they are not empty ranges:
 
     (21..=42).for_each(|x| println!("{}", x));
     (21..42).for_each(|x| println!("{}", x));
 
+    let arr = [1, 2, 3, 4, 5];
     let _ = &arr[1..=3];
     let _ = &arr[1..3];
 
     for _ in 21..=42 {}
     for _ in 21..42 {}
+
+    // This range is empty but should be ignored, see issue #5689
+    let _ = &arr[0..0];
 }
diff --git a/tests/ui/reversed_empty_ranges_fixable.rs b/tests/ui/reversed_empty_ranges_fixable.rs
@@ -4,8 +4,6 @@
 const ANSWER: i32 = 42;
 
 fn main() {
-    let arr = [1, 2, 3, 4, 5];
-
     // These should be linted:
 
     (42..=21).for_each(|x| println!("{}", x));
@@ -14,16 +12,18 @@ fn main() {
     for _ in -21..=-42 {}
     for _ in 42u32..21u32 {}
 
-    let _ = &arr[3..3];
-
     // These should be ignored as they are not empty ranges:
 
     (21..=42).for_each(|x| println!("{}", x));
     (21..42).for_each(|x| println!("{}", x));
 
+    let arr = [1, 2, 3, 4, 5];
     let _ = &arr[1..=3];
     let _ = &arr[1..3];
 
     for _ in 21..=42 {}
     for _ in 21..42 {}
+
+    // This range is empty but should be ignored, see issue #5689
+    let _ = &arr[0..0];
 }
diff --git a/tests/ui/reversed_empty_ranges_fixable.stderr b/tests/ui/reversed_empty_ranges_fixable.stderr
@@ -1,5 +1,5 @@
 error: this range is empty so it will yield no values
-  --> $DIR/reversed_empty_ranges_fixable.rs:11:5
+  --> $DIR/reversed_empty_ranges_fixable.rs:9:5
    |
 LL |     (42..=21).for_each(|x| println!("{}", x));
    |     ^^^^^^^^^
@@ -11,7 +11,7 @@ LL |     (21..=42).rev().for_each(|x| println!("{}", x));
    |     ^^^^^^^^^^^^^^^
 
 error: this range is empty so it will yield no values
-  --> $DIR/reversed_empty_ranges_fixable.rs:12:13
+  --> $DIR/reversed_empty_ranges_fixable.rs:10:13
    |
 LL |     let _ = (ANSWER..21).filter(|x| x % 2 == 0).take(10).collect::<Vec<_>>();
    |             ^^^^^^^^^^^^
@@ -22,7 +22,7 @@ LL |     let _ = (21..ANSWER).rev().filter(|x| x % 2 == 0).take(10).collect::<Ve
    |             ^^^^^^^^^^^^^^^^^^
 
 error: this range is empty so it will yield no values
-  --> $DIR/reversed_empty_ranges_fixable.rs:14:14
+  --> $DIR/reversed_empty_ranges_fixable.rs:12:14
    |
 LL |     for _ in -21..=-42 {}
    |              ^^^^^^^^^
@@ -33,7 +33,7 @@ LL |     for _ in (-42..=-21).rev() {}
    |              ^^^^^^^^^^^^^^^^^
 
 error: this range is empty so it will yield no values
-  --> $DIR/reversed_empty_ranges_fixable.rs:15:14
+  --> $DIR/reversed_empty_ranges_fixable.rs:13:14
    |
 LL |     for _ in 42u32..21u32 {}
    |              ^^^^^^^^^^^^
@@ -43,11 +43,5 @@ help: consider using the following if you are attempting to iterate over this ra
 LL |     for _ in (21u32..42u32).rev() {}
    |              ^^^^^^^^^^^^^^^^^^^^
 
-error: this range is empty and using it to index a slice will always yield an empty slice
-  --> $DIR/reversed_empty_ranges_fixable.rs:17:18
-   |
-LL |     let _ = &arr[3..3];
-   |              ----^^^^- help: if you want an empty slice, use: `[] as &[i32]`
-
-error: aborting due to 5 previous errors
+error: aborting due to 4 previous errors
 
diff --git a/tests/ui/reversed_empty_ranges_unfixable.rs b/tests/ui/reversed_empty_ranges_unfixable.rs
@@ -4,11 +4,12 @@ const ANSWER: i32 = 42;
 const SOME_NUM: usize = 3;
 
 fn main() {
-    let _ = (42 + 10..42 + 10).map(|x| x / 2).find(|&x| x == 21);
-
     let arr = [1, 2, 3, 4, 5];
     let _ = &arr[3usize..=1usize];
     let _ = &arr[SOME_NUM..1];
 
     for _ in ANSWER..ANSWER {}
+
+    // Should not be linted, see issue #5689
+    let _ = (42 + 10..42 + 10).map(|x| x / 2).find(|&x| x == 21);
 }
diff --git a/tests/ui/reversed_empty_ranges_unfixable.stderr b/tests/ui/reversed_empty_ranges_unfixable.stderr
@@ -1,28 +1,22 @@
-error: this range is empty so it will yield no values
-  --> $DIR/reversed_empty_ranges_unfixable.rs:7:13
-   |
-LL |     let _ = (42 + 10..42 + 10).map(|x| x / 2).find(|&x| x == 21);
-   |             ^^^^^^^^^^^^^^^^^^
-   |
-   = note: `-D clippy::reversed-empty-ranges` implied by `-D warnings`
-
 error: this range is reversed and using it to index a slice will panic at run-time
-  --> $DIR/reversed_empty_ranges_unfixable.rs:10:18
+  --> $DIR/reversed_empty_ranges_unfixable.rs:8:18
    |
 LL |     let _ = &arr[3usize..=1usize];
    |                  ^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::reversed-empty-ranges` implied by `-D warnings`
 
 error: this range is reversed and using it to index a slice will panic at run-time
-  --> $DIR/reversed_empty_ranges_unfixable.rs:11:18
+  --> $DIR/reversed_empty_ranges_unfixable.rs:9:18
    |
 LL |     let _ = &arr[SOME_NUM..1];
    |                  ^^^^^^^^^^^
 
 error: this range is empty so it will yield no values
-  --> $DIR/reversed_empty_ranges_unfixable.rs:13:14
+  --> $DIR/reversed_empty_ranges_unfixable.rs:11:14
    |
 LL |     for _ in ANSWER..ANSWER {}
    |              ^^^^^^^^^^^^^^
 
-error: aborting due to 4 previous errors
+error: aborting due to 3 previous errors