From 8b59542acc9901a6568731541baa9f623c1991b3 Mon Sep 17 00:00:00 2001 From: Shea Newton Date: Thu, 14 Jun 2018 16:41:56 +0000 Subject: [PATCH] Second pass at addressing changes requested The changes reflected in this commit (requested in PR #2790) are as follows: - Extended `INDEXING_SLICING` documentation to include the array type so that it is clearer when indexing operations are allowed. - Variable `ty` defined identically in multiple scopes was moved to an outer scope so it's only defined once. - Added a missing return statement to ensure only one lint is triggered by a scenario. - Prettified match statement with a `let` clause. (I learned something new!) - Added `&x[5..].iter().map(|x| 2 * x).collect::>()` and `&x[2..].iter().map(|x| 2 * x).collect::>()` to the test cases. The first _should trigger the lint/stderr_ and the second _should not_. --- clippy_lints/src/indexing_slicing.rs | 55 ++++++++---- tests/ui/indexing_slicing.rs | 2 + tests/ui/indexing_slicing.stderr | 130 +++++++-------------------- 3 files changed, 70 insertions(+), 117 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index 1f2c7755eee..01f9f45c589 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -34,7 +34,7 @@ declare_clippy_lint! { } /// **What it does:** Checks for usage of indexing or slicing. Does not report -/// if we can tell that the indexing or slicing operations on an array are in +/// on arrays if we can tell that the indexing or slicing operations are in /// bounds. /// /// **Why is this bad?** Indexing and slicing can panic at runtime and there are @@ -44,7 +44,9 @@ declare_clippy_lint! { /// /// **Example:** /// ```rust +/// // Vector /// let x = vec![0; 5]; +/// /// // Bad /// x[2]; /// &x[2..100]; @@ -52,10 +54,29 @@ declare_clippy_lint! { /// &x[..100]; /// /// // Good -/// x.get(2) -/// x.get(2..100) -/// x.get(2..) -/// x.get(..100) +/// x.get(2); +/// x.get(2..100); +/// x.get(2..); +/// x.get(..100); +/// +/// // Array +/// let y = [0, 1, 2, 3]; +/// +/// // Bad +/// y[10]; +/// &y[10..100]; +/// &y[10..]; +/// &y[..100]; +/// +/// // Good +/// y[2]; +/// &y[2..]; +/// &y[..2]; +/// &y[0..3]; +/// y.get(10); +/// y.get(10..100); +/// y.get(10..); +/// y.get(..100); /// ``` declare_clippy_lint! { pub INDEXING_SLICING, @@ -75,6 +96,7 @@ impl LintPass for IndexingSlicing { 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)`. @@ -82,7 +104,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { // ExprPath handles &x[..] and x[var] ExprStruct(..) | ExprPath(..) => { if let Some(range) = higher::range(cx, index) { - let ty = cx.tables.expr_ty(array); if let ty::TyArray(_, s) = ty.sty { let size: u128 = s.assert_usize(cx.tcx).unwrap().into(); // Index is a constant range. @@ -94,27 +115,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { expr.span, "range is out of bounds", ); - } else { - // Range is in bounds, ok. - return; - } + } // Else range is in bounds, ok. + + return; } } - let help_msg; - match (range.start, range.end) { + let help_msg = match (range.start, range.end) { (None, Some(_)) => { - help_msg = "Consider using `.get(..n)`or `.get_mut(..n)` instead"; + "Consider using `.get(..n)`or `.get_mut(..n)` instead" } (Some(_), None) => { - help_msg = "Consider using `.get(n..)` or .get_mut(n..)` instead"; + "Consider using `.get(n..)` or .get_mut(n..)` instead" } (Some(_), Some(_)) => { - help_msg = - "Consider using `.get(n..m)` or `.get_mut(n..m)` instead"; + "Consider using `.get(n..m)` or `.get_mut(n..m)` instead" } - (None, None) => return, // [..] is ok - } + (None, None) => return, // [..] is ok. + }; utils::span_help_and_lint( cx, @@ -135,7 +153,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { } ExprLit(..) => { // [n] - let ty = cx.tables.expr_ty(array); if let ty::TyArray(_, s) = ty.sty { let size: u128 = s.assert_usize(cx.tcx).unwrap().into(); // Index is a constant uint. diff --git a/tests/ui/indexing_slicing.rs b/tests/ui/indexing_slicing.rs index 913063b8dd5..a22c9034346 100644 --- a/tests/ui/indexing_slicing.rs +++ b/tests/ui/indexing_slicing.rs @@ -30,6 +30,8 @@ fn main() { &x[5..]; &x[..4]; &x[..5]; + &x[5..].iter().map(|x| 2 * x).collect::>(); + &x[2..].iter().map(|x| 2 * x).collect::>(); // Ok let y = &x; y[0]; diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index 642817d9e94..605a96e8b8c 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -61,14 +61,6 @@ error: range is out of bounds 20 | &x[1..5]; | ^^^^^^^ -error: slicing may panic. - --> $DIR/indexing_slicing.rs:20:6 - | -20 | &x[1..5]; - | ^^^^^^^ - | - = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead - error: slicing may panic. --> $DIR/indexing_slicing.rs:21:6 | @@ -91,181 +83,123 @@ error: range is out of bounds 26 | &x[..=4]; | ^^^^^^^ -error: slicing may panic. - --> $DIR/indexing_slicing.rs:26:6 - | -26 | &x[..=4]; - | ^^^^^^^ - | - = help: Consider using `.get(..n)`or `.get_mut(..n)` instead - error: range is out of bounds --> $DIR/indexing_slicing.rs:30:6 | 30 | &x[5..]; | ^^^^^^ -error: slicing may panic. - --> $DIR/indexing_slicing.rs:30:6 - | -30 | &x[5..]; - | ^^^^^^ - | - = help: Consider using `.get(n..)` or .get_mut(n..)` instead - error: range is out of bounds --> $DIR/indexing_slicing.rs:32:6 | 32 | &x[..5]; | ^^^^^^ -error: slicing may panic. - --> $DIR/indexing_slicing.rs:32:6 +error: range is out of bounds + --> $DIR/indexing_slicing.rs:33:6 | -32 | &x[..5]; +33 | &x[5..].iter().map(|x| 2 * x).collect::>(); | ^^^^^^ - | - = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: indexing may panic. - --> $DIR/indexing_slicing.rs:35:5 + --> $DIR/indexing_slicing.rs:37:5 | -35 | y[0]; +37 | y[0]; | ^^^^ | = help: Consider using `.get(n)` or `.get_mut(n)` instead error: slicing may panic. - --> $DIR/indexing_slicing.rs:36:6 + --> $DIR/indexing_slicing.rs:38:6 | -36 | &y[1..2]; +38 | &y[1..2]; | ^^^^^^^ | = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead error: slicing may panic. - --> $DIR/indexing_slicing.rs:39:6 + --> $DIR/indexing_slicing.rs:41:6 | -39 | &y[..=4]; +41 | &y[..=4]; | ^^^^^^^ | = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: const index is out of bounds - --> $DIR/indexing_slicing.rs:42:5 + --> $DIR/indexing_slicing.rs:44:5 | -42 | empty[0]; +44 | empty[0]; | ^^^^^^^^ -error: range is out of bounds - --> $DIR/indexing_slicing.rs:43:6 - | -43 | &empty[1..5]; - | ^^^^^^^^^^^ - -error: slicing may panic. - --> $DIR/indexing_slicing.rs:43:6 - | -43 | &empty[1..5]; - | ^^^^^^^^^^^ - | - = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead - error: range is out of bounds --> $DIR/indexing_slicing.rs:45:6 | -45 | &empty[..=4]; +45 | &empty[1..5]; | ^^^^^^^^^^^ -error: slicing may panic. - --> $DIR/indexing_slicing.rs:45:6 - | -45 | &empty[..=4]; - | ^^^^^^^^^^^ - | - = help: Consider using `.get(..n)`or `.get_mut(..n)` instead - error: range is out of bounds - --> $DIR/indexing_slicing.rs:50:6 + --> $DIR/indexing_slicing.rs:47:6 | -50 | &empty[..=0]; +47 | &empty[..=4]; | ^^^^^^^^^^^ -error: slicing may panic. - --> $DIR/indexing_slicing.rs:50:6 - | -50 | &empty[..=0]; - | ^^^^^^^^^^^ - | - = help: Consider using `.get(..n)`or `.get_mut(..n)` instead - error: range is out of bounds --> $DIR/indexing_slicing.rs:52:6 | -52 | &empty[1..]; - | ^^^^^^^^^^ - -error: slicing may panic. - --> $DIR/indexing_slicing.rs:52:6 - | -52 | &empty[1..]; - | ^^^^^^^^^^ - | - = help: Consider using `.get(n..)` or .get_mut(n..)` instead +52 | &empty[..=0]; + | ^^^^^^^^^^^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:53:6 + --> $DIR/indexing_slicing.rs:54:6 | -53 | &empty[..4]; +54 | &empty[1..]; | ^^^^^^^^^^ -error: slicing may panic. - --> $DIR/indexing_slicing.rs:53:6 +error: range is out of bounds + --> $DIR/indexing_slicing.rs:55:6 | -53 | &empty[..4]; +55 | &empty[..4]; | ^^^^^^^^^^ - | - = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: indexing may panic. - --> $DIR/indexing_slicing.rs:56:5 + --> $DIR/indexing_slicing.rs:58:5 | -56 | v[0]; +58 | v[0]; | ^^^^ | = help: Consider using `.get(n)` or `.get_mut(n)` instead error: indexing may panic. - --> $DIR/indexing_slicing.rs:57:5 + --> $DIR/indexing_slicing.rs:59:5 | -57 | v[10]; +59 | v[10]; | ^^^^^ | = help: Consider using `.get(n)` or `.get_mut(n)` instead error: slicing may panic. - --> $DIR/indexing_slicing.rs:58:6 + --> $DIR/indexing_slicing.rs:60:6 | -58 | &v[10..100]; +60 | &v[10..100]; | ^^^^^^^^^^ | = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead error: slicing may panic. - --> $DIR/indexing_slicing.rs:59:6 + --> $DIR/indexing_slicing.rs:61:6 | -59 | &v[10..]; +61 | &v[10..]; | ^^^^^^^ | = help: Consider using `.get(n..)` or .get_mut(n..)` instead error: slicing may panic. - --> $DIR/indexing_slicing.rs:60:6 + --> $DIR/indexing_slicing.rs:62:6 | -60 | &v[..100]; +62 | &v[..100]; | ^^^^^^^^ | = help: Consider using `.get(..n)`or `.get_mut(..n)` instead -error: aborting due to 36 previous errors +error: aborting due to 28 previous errors