Revisiting indexing_slicing test cases

This commit contains a few changes. In an attempt to clarify which test cases should and should not produce stderr it became clear that some cases were being handled incorrectly. In order to address these test cases, a minor re-factor was made to the linting logic itself.

The re-factor was driven by edge case handling including a need for additional match conditions for `ExprCall` (`&x[0..=4]`) and `ExprBinary` (`x[1 << 3]`). Rather than attempt to account for each potential `Expr*` the code was re-factored into simply "if ranged index" and an "otherwise" conditions.

diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs
index 01f9f45c589119c65df02ed56c2d2fd93618fe50..b4e6414195e324aa885a483ab8cb1b0587725c60 100644 (file)
--- a/clippy_lints/src/indexing_slicing.rs
+++ b/clippy_lints/src/indexing_slicing.rs
@@ -1,8 +1,9 @@
 //! lint on indexing and slicing operations
 
 use crate::consts::{constant, Constant};
+use crate::utils;
+use crate::utils::higher;
 use crate::utils::higher::Range;
-use crate::utils::{self, higher};
 use rustc::hir::*;
 use rustc::lint::*;
 use rustc::ty;
@@ -97,89 +98,65 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
         if let ExprIndex(ref array, ref index) = &expr.node {
             let ty = cx.tables.expr_ty(array);
-            match &index.node {
-                // Both ExprStruct and ExprPath require this approach's checks
-                // on the `range` returned by `higher::range(cx, index)`.
-                // ExprStruct handles &x[n..m], &x[n..] and &x[..n].
-                // ExprPath handles &x[..] and x[var]
-                ExprStruct(..) | ExprPath(..) => {
-                    if let Some(range) = higher::range(cx, index) {
-                        if let ty::TyArray(_, s) = ty.sty {
-                            let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
-                            // Index is a constant range.
-                            if let Some((start, end)) = to_const_range(cx, range, size) {
-                                if start > size || end > size {
-                                    utils::span_lint(
-                                        cx,
-                                        OUT_OF_BOUNDS_INDEXING,
-                                        expr.span,
-                                        "range is out of bounds",
-                                    );
-                                } // Else range is in bounds, ok.
-
-                                return;
-                            }
+            if let Some(range) = higher::range(cx, index) {
+                // Ranged indexes, i.e. &x[n..m], &x[n..], &x[..n] and &x[..]
+                if let ty::TyArray(_, s) = ty.sty {
+                    let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
+                    // Index is a constant range.
+                    if let Some((start, end)) = to_const_range(cx, range, size) {
+                        if start > size || end > size {
+                            utils::span_lint(
+                                cx,
+                                OUT_OF_BOUNDS_INDEXING,
+                                expr.span,
+                                "range is out of bounds",
+                            );
                         }
-
-                        let help_msg = match (range.start, range.end) {
-                            (None, Some(_)) => {
-                                "Consider using `.get(..n)`or `.get_mut(..n)` instead"
-                            }
-                            (Some(_), None) => {
-                                "Consider using `.get(n..)` or .get_mut(n..)` instead"
-                            }
-                            (Some(_), Some(_)) => {
-                                "Consider using `.get(n..m)` or `.get_mut(n..m)` instead"
-                            }
-                            (None, None) => return, // [..] is ok.
-                        };
-
-                        utils::span_help_and_lint(
-                            cx,
-                            INDEXING_SLICING,
-                            expr.span,
-                            "slicing may panic.",
-                            help_msg,
-                        );
-                    } else {
-                        utils::span_help_and_lint(
-                            cx,
-                            INDEXING_SLICING,
-                            expr.span,
-                            "indexing may panic.",
-                            "Consider using `.get(n)` or `.get_mut(n)` instead",
-                        );
+                        return;
                     }
                 }
-                ExprLit(..) => {
-                    // [n]
-                    if let ty::TyArray(_, s) = ty.sty {
-                        let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
-                        // Index is a constant uint.
-                        if let Some((Constant::Int(const_index), _)) =
-                            constant(cx, cx.tables, index)
-                        {
-                            if size <= const_index {
-                                utils::span_lint(
-                                    cx,
-                                    OUT_OF_BOUNDS_INDEXING,
-                                    expr.span,
-                                    "const index is out of bounds",
-                                );
-                            }
-                            // Else index is in bounds, ok.
+
+                let help_msg = match (range.start, range.end) {
+                    (None, Some(_)) => "Consider using `.get(..n)`or `.get_mut(..n)` instead",
+                    (Some(_), None) => "Consider using `.get(n..)` or .get_mut(n..)` instead",
+                    (Some(_), Some(_)) => "Consider using `.get(n..m)` or `.get_mut(n..m)` instead",
+                    (None, None) => return, // [..] is ok.
+                };
+
+                utils::span_help_and_lint(
+                    cx,
+                    INDEXING_SLICING,
+                    expr.span,
+                    "slicing may panic.",
+                    help_msg,
+                );
+            } else {
+                // Catchall non-range index, i.e. [n] or [n << m]
+                if let ty::TyArray(_, s) = ty.sty {
+                    let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
+                    // Index is a constant uint.
+                    if let Some((Constant::Int(const_index), _)) = constant(cx, cx.tables, index) {
+                        if size <= const_index {
+                            utils::span_lint(
+                                cx,
+                                OUT_OF_BOUNDS_INDEXING,
+                                expr.span,
+                                "const index is out of bounds",
+                            );
                         }
-                    } else {
-                        utils::span_help_and_lint(
-                            cx,
-                            INDEXING_SLICING,
-                            expr.span,
-                            "indexing may panic.",
-                            "Consider using `.get(n)` or `.get_mut(n)` instead",
-                        );
+                        // Else index is in bounds, ok.
+
+                        return;
                     }
                 }
-                _ => (),
+
+                utils::span_help_and_lint(
+                    cx,
+                    INDEXING_SLICING,
+                    expr.span,
+                    "indexing may panic.",
+                    "Consider using `.get(n)` or `.get_mut(n)` instead",
+                );
             }
         }
     }

diff --git a/tests/ui/indexing_slicing.rs b/tests/ui/indexing_slicing.rs
index a22c9034346d612660a3241b393fc05b974d717e..16174afb1060366f95943ae2a5ed74f0b610827c 100644 (file)
--- a/tests/ui/indexing_slicing.rs
+++ b/tests/ui/indexing_slicing.rs
@@ -9,55 +9,63 @@ fn main() {
     let index_from: usize = 2;
     let index_to: usize = 3;
     x[index];
-    &x[index_from..index_to];
-    &x[index_from..][..index_to];
     &x[index..];
     &x[..index];
-    x[0];
-    x[3];
+    &x[index_from..index_to];
+    &x[index_from..][..index_to]; // Two lint reports, one for [index_from..] and another for [..index_to].
     x[4];
     x[1 << 3];
-    &x[1..5];
-    &x[1..][..5];
-    &x[0..3];
-    &x[0..][..3];
-    &x[0..].get(..3); // Ok
-    &x[0..=4];
     &x[..=4];
-    &x[..];
-    &x[1..];
-    &x[4..];
+    &x[1..5];
+    &x[5..][..10]; // Two lint reports, one for [5..] and another for [..10].
     &x[5..];
-    &x[..4];
     &x[..5];
     &x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>();
-    &x[2..].iter().map(|x| 2 * x).collect::<Vec<i32>>(); // Ok
+    &x[0..=4];
+    &x[0..][..3];
+    &x[1..][..5];
+
+    &x[4..]; // Ok, should not produce stderr.
+    &x[..4]; // Ok, should not produce stderr.
+    &x[..]; // Ok, should not produce stderr.
+    &x[1..]; // Ok, should not produce stderr.
+    &x[2..].iter().map(|x| 2 * x).collect::<Vec<i32>>(); // Ok, should not produce stderr.
+    &x[0..].get(..3); // Ok, should not produce stderr.
+    x[0]; // Ok, should not produce stderr.
+    x[3]; // Ok, should not produce stderr.
+    &x[0..3]; // Ok, should not produce stderr.
 
     let y = &x;
     y[0];
     &y[1..2];
-    &y[..];
     &y[0..=4];
     &y[..=4];
 
+    &y[..]; // Ok, should not produce stderr.
+
     let empty: [i8; 0] = [];
     empty[0];
     &empty[1..5];
     &empty[0..=4];
     &empty[..=4];
-    &empty[..];
-    &empty[0..];
-    &empty[0..0];
-    &empty[0..=0];
-    &empty[..=0];
-    &empty[..0];
     &empty[1..];
     &empty[..4];
+    &empty[0..=0];
+    &empty[..=0];
+
+    &empty[0..]; // Ok, should not produce stderr.
+    &empty[0..0]; // Ok, should not produce stderr.
+    &empty[..0]; // Ok, should not produce stderr.
+    &empty[..]; // Ok, should not produce stderr.
 
     let v = vec![0; 5];
     v[0];
     v[10];
+    v[1 << 3];
     &v[10..100];
+    &x[10..][..100]; // Two lint reports, one for [10..] and another for [..100].
     &v[10..];
     &v[..100];
+
+    &v[..]; // Ok, should not produce stderr.
 }

diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr
index 605a96e8b8c2dd99a25557c1fa3a9eaedc78690f..c9aefe0349a193928dfdf5f87b8845c2a32d136c 100644 (file)
--- a/tests/ui/indexing_slicing.stderr
+++ b/tests/ui/indexing_slicing.stderr
@@ -10,109 +10,135 @@ error: indexing may panic.
 error: slicing may panic.
   --> $DIR/indexing_slicing.rs:12:6
    |
-12 |     &x[index_from..index_to];
-   |      ^^^^^^^^^^^^^^^^^^^^^^^
+12 |     &x[index..];
+   |      ^^^^^^^^^^
    |
-   = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
+   = help: Consider using `.get(n..)` or .get_mut(n..)` instead
 
 error: slicing may panic.
   --> $DIR/indexing_slicing.rs:13:6
    |
-13 |     &x[index_from..][..index_to];
-   |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+13 |     &x[..index];
+   |      ^^^^^^^^^^
    |
    = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
 
 error: slicing may panic.
-  --> $DIR/indexing_slicing.rs:13:6
+  --> $DIR/indexing_slicing.rs:14:6
    |
-13 |     &x[index_from..][..index_to];
-   |      ^^^^^^^^^^^^^^^
+14 |     &x[index_from..index_to];
+   |      ^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: Consider using `.get(n..)` or .get_mut(n..)` instead
+   = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
 
 error: slicing may panic.
-  --> $DIR/indexing_slicing.rs:14:6
+  --> $DIR/indexing_slicing.rs:15:6
    |
-14 |     &x[index..];
-   |      ^^^^^^^^^^
+15 |     &x[index_from..][..index_to]; // Two lint reports, one for [index_from..] and another for [..index_to].
+   |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: Consider using `.get(n..)` or .get_mut(n..)` instead
+   = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
 
 error: slicing may panic.
   --> $DIR/indexing_slicing.rs:15:6
    |
-15 |     &x[..index];
-   |      ^^^^^^^^^^
+15 |     &x[index_from..][..index_to]; // Two lint reports, one for [index_from..] and another for [..index_to].
+   |      ^^^^^^^^^^^^^^^
    |
-   = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
+   = help: Consider using `.get(n..)` or .get_mut(n..)` instead
 
 error: const index is out of bounds
-  --> $DIR/indexing_slicing.rs:18:5
+  --> $DIR/indexing_slicing.rs:16:5
    |
-18 |     x[4];
+16 |     x[4];
    |     ^^^^
    |
    = note: `-D out-of-bounds-indexing` implied by `-D warnings`
 
+error: const index is out of bounds
+  --> $DIR/indexing_slicing.rs:17:5
+   |
+17 |     x[1 << 3];
+   |     ^^^^^^^^^
+
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:20:6
+  --> $DIR/indexing_slicing.rs:18:6
    |
-20 |     &x[1..5];
+18 |     &x[..=4];
    |      ^^^^^^^
 
-error: slicing may panic.
-  --> $DIR/indexing_slicing.rs:21:6
-   |
-21 |     &x[1..][..5];
-   |      ^^^^^^^^^^^
+error: range is out of bounds
+  --> $DIR/indexing_slicing.rs:19:6
    |
-   = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
+19 |     &x[1..5];
+   |      ^^^^^^^
 
 error: slicing may panic.
-  --> $DIR/indexing_slicing.rs:23:6
+  --> $DIR/indexing_slicing.rs:20:6
    |
-23 |     &x[0..][..3];
-   |      ^^^^^^^^^^^
+20 |     &x[5..][..10]; // Two lint reports, one for [5..] and another for [..10].
+   |      ^^^^^^^^^^^^
    |
    = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
 
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:26:6
+  --> $DIR/indexing_slicing.rs:20:6
    |
-26 |     &x[..=4];
-   |      ^^^^^^^
+20 |     &x[5..][..10]; // Two lint reports, one for [5..] and another for [..10].
+   |      ^^^^^^
 
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:30:6
+  --> $DIR/indexing_slicing.rs:21:6
    |
-30 |     &x[5..];
+21 |     &x[5..];
    |      ^^^^^^
 
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:32:6
+  --> $DIR/indexing_slicing.rs:22:6
    |
-32 |     &x[..5];
+22 |     &x[..5];
    |      ^^^^^^
 
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:33:6
+  --> $DIR/indexing_slicing.rs:23:6
    |
-33 |     &x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>();
+23 |     &x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>();
    |      ^^^^^^
 
+error: range is out of bounds
+  --> $DIR/indexing_slicing.rs:24:6
+   |
+24 |     &x[0..=4];
+   |      ^^^^^^^^
+
+error: slicing may panic.
+  --> $DIR/indexing_slicing.rs:25:6
+   |
+25 |     &x[0..][..3];
+   |      ^^^^^^^^^^^
+   |
+   = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
+
+error: slicing may panic.
+  --> $DIR/indexing_slicing.rs:26:6
+   |
+26 |     &x[1..][..5];
+   |      ^^^^^^^^^^^
+   |
+   = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
+
 error: indexing may panic.
-  --> $DIR/indexing_slicing.rs:37:5
+  --> $DIR/indexing_slicing.rs:39:5
    |
-37 |     y[0];
+39 |     y[0];
    |     ^^^^
    |
    = help: Consider using `.get(n)` or `.get_mut(n)` instead
 
 error: slicing may panic.
-  --> $DIR/indexing_slicing.rs:38:6
+  --> $DIR/indexing_slicing.rs:40:6
    |
-38 |     &y[1..2];
+40 |     &y[1..2];
    |      ^^^^^^^
    |
    = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
@@ -120,86 +146,128 @@ error: slicing may panic.
 error: slicing may panic.
   --> $DIR/indexing_slicing.rs:41:6
    |
-41 |     &y[..=4];
+41 |     &y[0..=4];
+   |      ^^^^^^^^
+   |
+   = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
+
+error: slicing may panic.
+  --> $DIR/indexing_slicing.rs:42:6
+   |
+42 |     &y[..=4];
    |      ^^^^^^^
    |
    = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
 
 error: const index is out of bounds
-  --> $DIR/indexing_slicing.rs:44:5
+  --> $DIR/indexing_slicing.rs:47:5
    |
-44 |     empty[0];
+47 |     empty[0];
    |     ^^^^^^^^
 
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:45:6
+  --> $DIR/indexing_slicing.rs:48:6
    |
-45 |     &empty[1..5];
+48 |     &empty[1..5];
    |      ^^^^^^^^^^^
 
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:47:6
+  --> $DIR/indexing_slicing.rs:49:6
    |
-47 |     &empty[..=4];
-   |      ^^^^^^^^^^^
+49 |     &empty[0..=4];
+   |      ^^^^^^^^^^^^
 
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:52:6
+  --> $DIR/indexing_slicing.rs:50:6
    |
-52 |     &empty[..=0];
+50 |     &empty[..=4];
    |      ^^^^^^^^^^^
 
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:54:6
+  --> $DIR/indexing_slicing.rs:51:6
    |
-54 |     &empty[1..];
+51 |     &empty[1..];
    |      ^^^^^^^^^^
 
 error: range is out of bounds
-  --> $DIR/indexing_slicing.rs:55:6
+  --> $DIR/indexing_slicing.rs:52:6
    |
-55 |     &empty[..4];
+52 |     &empty[..4];
    |      ^^^^^^^^^^
 
+error: range is out of bounds
+  --> $DIR/indexing_slicing.rs:53:6
+   |
+53 |     &empty[0..=0];
+   |      ^^^^^^^^^^^^
+
+error: range is out of bounds
+  --> $DIR/indexing_slicing.rs:54:6
+   |
+54 |     &empty[..=0];
+   |      ^^^^^^^^^^^
+
 error: indexing may panic.
-  --> $DIR/indexing_slicing.rs:58:5
+  --> $DIR/indexing_slicing.rs:62:5
    |
-58 |     v[0];
+62 |     v[0];
    |     ^^^^
    |
    = help: Consider using `.get(n)` or `.get_mut(n)` instead
 
 error: indexing may panic.
-  --> $DIR/indexing_slicing.rs:59:5
+  --> $DIR/indexing_slicing.rs:63:5
    |
-59 |     v[10];
+63 |     v[10];
    |     ^^^^^
    |
    = help: Consider using `.get(n)` or `.get_mut(n)` instead
 
+error: indexing may panic.
+  --> $DIR/indexing_slicing.rs:64:5
+   |
+64 |     v[1 << 3];
+   |     ^^^^^^^^^
+   |
+   = help: Consider using `.get(n)` or `.get_mut(n)` instead
+
 error: slicing may panic.
-  --> $DIR/indexing_slicing.rs:60:6
+  --> $DIR/indexing_slicing.rs:65:6
    |
-60 |     &v[10..100];
+65 |     &v[10..100];
    |      ^^^^^^^^^^
    |
    = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
 
 error: slicing may panic.
-  --> $DIR/indexing_slicing.rs:61:6
+  --> $DIR/indexing_slicing.rs:66:6
+   |
+66 |     &x[10..][..100]; // Two lint reports, one for [10..] and another for [..100].
+   |      ^^^^^^^^^^^^^^
+   |
+   = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
+
+error: range is out of bounds
+  --> $DIR/indexing_slicing.rs:66:6
+   |
+66 |     &x[10..][..100]; // Two lint reports, one for [10..] and another for [..100].
+   |      ^^^^^^^
+
+error: slicing may panic.
+  --> $DIR/indexing_slicing.rs:67:6
    |
-61 |     &v[10..];
+67 |     &v[10..];
    |      ^^^^^^^
    |
    = help: Consider using `.get(n..)` or .get_mut(n..)` instead
 
 error: slicing may panic.
-  --> $DIR/indexing_slicing.rs:62:6
+  --> $DIR/indexing_slicing.rs:68:6
    |
-62 |     &v[..100];
+68 |     &v[..100];
    |      ^^^^^^^^
    |
    = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
 
-error: aborting due to 28 previous errors
+error: aborting due to 38 previous errors