From 0f3345e8b2f839f1d3e0c8472537d7d954828ccd Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 13 Oct 2018 13:51:53 -0700 Subject: [PATCH 1/3] OUT_OF_BOUNDS_INDEXING fix #3102 false negative --- clippy_lints/src/indexing_slicing.rs | 70 +++++++++++++++++++--------- tests/ui/indexing_slicing.rs | 5 ++ tests/ui/indexing_slicing.stderr | 14 +++++- 3 files changed, 66 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index 984d725898d7..8b7a1f7882b7 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -111,17 +111,43 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { // Ranged indexes, i.e. &x[n..m], &x[n..], &x[..n] and &x[..] if let ty::Array(_, 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", - ); - } - return; + + match to_const_range(cx, range, size) { + (None, None) => {}, + (Some(start), None) => { + if start > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); + return; + } + }, + (None, Some(end)) => { + if end > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); + return; + } + }, + (Some(start), Some(end)) => { + if start > size || end > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); + } + // early return because both start and end are constant + return; + }, } } @@ -161,20 +187,20 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { } } -/// Returns an option containing a tuple with the start and end (exclusive) of -/// the range. +/// Returns a tuple of options with the start and end (exclusive) values of +/// the range. If the start or end is not constant, None is returned. fn to_const_range<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, range: Range<'_>, array_size: u128, -) -> Option<(u128, u128)> { +) -> (Option, Option) { let s = range .start .map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c)); let start = match s { - Some(Some(Constant::Int(x))) => x, - Some(_) => return None, - None => 0, + Some(Some(Constant::Int(x))) => Some(x), + Some(_) => None, + None => Some(0), }; let e = range @@ -182,13 +208,13 @@ fn to_const_range<'a, 'tcx>( .map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c)); let end = match e { Some(Some(Constant::Int(x))) => if range.limits == RangeLimits::Closed { - x + 1 + Some(x + 1) } else { - x + Some(x) }, - Some(_) => return None, - None => array_size, + Some(_) => None, + None => Some(array_size), }; - Some((start, end)) + (start, end) } diff --git a/tests/ui/indexing_slicing.rs b/tests/ui/indexing_slicing.rs index 6a32eb87491d..ff154091bb82 100644 --- a/tests/ui/indexing_slicing.rs +++ b/tests/ui/indexing_slicing.rs @@ -91,4 +91,9 @@ fn main() { x[M]; // Ok, should not produce stderr. v[N]; v[M]; + + // issue 3102 + let num = 1; + &x[num..10]; // should trigger out of bounds error + &x[10..num]; // should trigger out of bounds error } diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index 7d847c7a6734..c587269e3e57 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -267,5 +267,17 @@ error: indexing may panic. | = help: Consider using `.get(n)` or `.get_mut(n)` instead -error: aborting due to 37 previous errors +error: range is out of bounds + --> $DIR/indexing_slicing.rs:97:6 + | +97 | &x[num..10]; // should trigger out of bounds error + | ^^^^^^^^^^ + +error: range is out of bounds + --> $DIR/indexing_slicing.rs:98:6 + | +98 | &x[10..num]; // should trigger out of bounds error + | ^^^^^^^^^^ + +error: aborting due to 39 previous errors From 5c3928282699244f5e85a66f0cded09ea3dfbeda Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sun, 14 Oct 2018 07:49:28 -0700 Subject: [PATCH 2/3] out_of_bounds_indexing refactoring --- clippy_lints/src/indexing_slicing.rs | 65 +++++++++++++--------------- 1 file changed, 30 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index 8b7a1f7882b7..9f9c25f77281 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -108,46 +108,41 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { if let ExprKind::Index(ref array, ref index) = &expr.node { let ty = cx.tables.expr_ty(array); if let Some(range) = higher::range(cx, index) { + // Ranged indexes, i.e. &x[n..m], &x[n..], &x[..n] and &x[..] if let ty::Array(_, s) = ty.sty { let size: u128 = s.assert_usize(cx.tcx).unwrap().into(); - match to_const_range(cx, range, size) { - (None, None) => {}, - (Some(start), None) => { - if start > size { - utils::span_lint( - cx, - OUT_OF_BOUNDS_INDEXING, - expr.span, - "range is out of bounds", - ); - return; - } - }, - (None, Some(end)) => { - if end > size { - utils::span_lint( - cx, - OUT_OF_BOUNDS_INDEXING, - expr.span, - "range is out of bounds", - ); - return; - } - }, - (Some(start), Some(end)) => { - if start > size || end > size { - utils::span_lint( - cx, - OUT_OF_BOUNDS_INDEXING, - expr.span, - "range is out of bounds", - ); - } - // early return because both start and end are constant + let const_range = to_const_range(cx, range, size); + + if let (Some(start), _) = const_range { + if start > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); return; - }, + } + } + + if let (_, Some(end)) = const_range { + if end > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); + return; + } + } + + if let (Some(_), Some(_)) = const_range { + // early return because both start and end are constants + // and we have proven above that they are in bounds + return; } } From 66d3672b2696918a074d193813af1f74d1c48be9 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Mon, 15 Oct 2018 04:44:39 -0700 Subject: [PATCH 3/3] out_of_bounds_indexing improved reporting of out of bounds value --- clippy_lints/src/indexing_slicing.rs | 4 +- tests/ui/indexing_slicing.stderr | 68 ++++++++++++++-------------- 2 files changed, 36 insertions(+), 36 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index 9f9c25f77281..f960ab5958ce 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -120,7 +120,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { utils::span_lint( cx, OUT_OF_BOUNDS_INDEXING, - expr.span, + range.start.map_or(expr.span, |start| start.span), "range is out of bounds", ); return; @@ -132,7 +132,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { utils::span_lint( cx, OUT_OF_BOUNDS_INDEXING, - expr.span, + range.end.map_or(expr.span, |end| end.span), "range is out of bounds", ); return; diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index c587269e3e57..fafcb1bc4853 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -48,18 +48,18 @@ error: slicing may panic. = help: Consider using `.get(n..)` or .get_mut(n..)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:30:6 + --> $DIR/indexing_slicing.rs:30:11 | 30 | &x[..=4]; - | ^^^^^^^ + | ^ | = note: `-D clippy::out-of-bounds-indexing` implied by `-D warnings` error: range is out of bounds - --> $DIR/indexing_slicing.rs:31:6 + --> $DIR/indexing_slicing.rs:31:11 | 31 | &x[1..5]; - | ^^^^^^^ + | ^ error: slicing may panic. --> $DIR/indexing_slicing.rs:32:6 @@ -70,34 +70,34 @@ error: slicing may panic. = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:32:6 + --> $DIR/indexing_slicing.rs:32:8 | 32 | &x[5..][..10]; // Two lint reports, one for [5..] and another for [..10]. - | ^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:33:6 + --> $DIR/indexing_slicing.rs:33:8 | 33 | &x[5..]; - | ^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:34:6 + --> $DIR/indexing_slicing.rs:34:10 | 34 | &x[..5]; - | ^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:35:6 + --> $DIR/indexing_slicing.rs:35:8 | 35 | &x[5..].iter().map(|x| 2 * x).collect::>(); - | ^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:36:6 + --> $DIR/indexing_slicing.rs:36:12 | 36 | &x[0..=4]; - | ^^^^^^^^ + | ^ error: slicing may panic. --> $DIR/indexing_slicing.rs:37:6 @@ -148,46 +148,46 @@ error: slicing may panic. = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:60:6 + --> $DIR/indexing_slicing.rs:60:12 | 60 | &empty[1..5]; - | ^^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:61:6 + --> $DIR/indexing_slicing.rs:61:16 | 61 | &empty[0..=4]; - | ^^^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:62:6 + --> $DIR/indexing_slicing.rs:62:15 | 62 | &empty[..=4]; - | ^^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:63:6 + --> $DIR/indexing_slicing.rs:63:12 | 63 | &empty[1..]; - | ^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:64:6 + --> $DIR/indexing_slicing.rs:64:14 | 64 | &empty[..4]; - | ^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:65:6 + --> $DIR/indexing_slicing.rs:65:16 | 65 | &empty[0..=0]; - | ^^^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:66:6 + --> $DIR/indexing_slicing.rs:66:15 | 66 | &empty[..=0]; - | ^^^^^^^^^^^ + | ^ error: indexing may panic. --> $DIR/indexing_slicing.rs:74:5 @@ -230,10 +230,10 @@ error: slicing may panic. = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:78:6 + --> $DIR/indexing_slicing.rs:78:8 | 78 | &x[10..][..100]; // Two lint reports, one for [10..] and another for [..100]. - | ^^^^^^^ + | ^^ error: slicing may panic. --> $DIR/indexing_slicing.rs:79:6 @@ -268,16 +268,16 @@ error: indexing may panic. = help: Consider using `.get(n)` or `.get_mut(n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:97:6 + --> $DIR/indexing_slicing.rs:97:13 | 97 | &x[num..10]; // should trigger out of bounds error - | ^^^^^^^^^^ + | ^^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:98:6 + --> $DIR/indexing_slicing.rs:98:8 | 98 | &x[10..num]; // should trigger out of bounds error - | ^^^^^^^^^^ + | ^^ error: aborting due to 39 previous errors