diff --git a/clippy_lints/src/array_indexing.rs b/clippy_lints/src/array_indexing.rs index 77aa5e83425..e7bb590e60d 100644 --- a/clippy_lints/src/array_indexing.rs +++ b/clippy_lints/src/array_indexing.rs @@ -1,3 +1,5 @@ +//! lint on indexing and slicing operations + use crate::consts::{constant, Constant}; use crate::utils::higher::Range; use crate::utils::{self, higher}; @@ -16,9 +18,14 @@ use syntax::ast::RangeLimits; /// **Example:** /// ```rust /// let x = [1,2,3,4]; -/// ... +/// +/// // Bad /// x[9]; /// &x[2..9]; +/// +/// // Good +/// x[0]; +/// x[3]; /// ``` declare_clippy_lint! { pub OUT_OF_BOUNDS_INDEXING, @@ -26,19 +33,29 @@ declare_clippy_lint! { "out of bounds constant indexing" } -/// **What it does:** Checks for usage of indexing or slicing. +/// **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 +/// bounds. /// -/// **Why is this bad?** Usually, this can be safely allowed. However, in some -/// domains such as kernel development, a panic can cause the whole operating -/// system to crash. +/// **Why is this bad?** Indexing and slicing can panic at runtime and there are +/// safe alternatives. /// /// **Known problems:** Hopefully none. /// /// **Example:** /// ```rust -/// ... +/// let x = vec![0; 5]; +/// // Bad /// x[2]; -/// &x[0..2]; +/// &x[2..100]; +/// &x[2..]; +/// &x[..100]; +/// +/// // Good +/// x.get(2) +/// x.get(2..100) +/// x.get(2..) +/// x.get(..100) /// ``` declare_clippy_lint! { pub INDEXING_SLICING, @@ -47,52 +64,105 @@ declare_clippy_lint! { } #[derive(Copy, Clone)] -pub struct ArrayIndexing; +pub struct IndexingSlicingPass; -impl LintPass for ArrayIndexing { +impl LintPass for IndexingSlicingPass { fn get_lints(&self) -> LintArray { lint_array!(INDEXING_SLICING, OUT_OF_BOUNDS_INDEXING) } } -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ArrayIndexing { - fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx hir::Expr) { - if let hir::ExprIndex(ref array, ref index) = e.node { - // Array with known size can be checked statically - let ty = cx.tables.expr_ty(array); - if let ty::TyArray(_, size) = ty.sty { - let size = size.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, e.span, "const index is out of bounds"); - } - - return; - } - - // Index is a constant range - if let Some(range) = higher::range(cx, index) { - if let Some((start, end)) = to_const_range(cx, range, size) { - if start > size || end > size { - utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "range is out of bounds"); +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { + if let ExprIndex(ref a, ref b) = &expr.node { + match &b.node { + // Both ExprStruct and ExprPath require this approach's checks + // on the `range` returned by `higher::range(cx, b)`. + // 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, b) { + let ty = cx.tables.expr_ty(a); + 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; + } + } } - return; + match (range.start, range.end) { + (None, Some(_)) => { + cx.span_lint( + INDEXING_SLICING, + expr.span, + "slicing may panic. Consider using \ + `.get(..n)`or `.get_mut(..n)` instead", + ); + } + (Some(_), None) => { + cx.span_lint( + INDEXING_SLICING, + expr.span, + "slicing may panic. Consider using \ + `.get(n..)` or .get_mut(n..)` instead", + ); + } + (Some(_), Some(_)) => { + cx.span_lint( + INDEXING_SLICING, + expr.span, + "slicing may panic. Consider using \ + `.get(n..m)` or `.get_mut(n..m)` instead", + ); + } + (None, None) => (), + } + } else { + cx.span_lint( + INDEXING_SLICING, + expr.span, + "indexing may panic. Consider using `.get(n)` or \ + `.get_mut(n)` instead", + ); } } - } - - if let Some(range) = higher::range(cx, index) { - // Full ranges are always valid - if range.start.is_none() && range.end.is_none() { - return; + ExprLit(_) => { + // [n] + let ty = cx.tables.expr_ty(a); + 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, b) { + 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. + } + } else { + cx.span_lint( + INDEXING_SLICING, + expr.span, + "indexing may panic. Consider using `.get(n)` or \ + `.get_mut(n)` instead", + ); + } } - - // Impossible to know if indexing or slicing is correct - utils::span_lint(cx, INDEXING_SLICING, e.span, "slicing may panic"); - } else { - utils::span_lint(cx, INDEXING_SLICING, e.span, "indexing may panic"); + _ => (), } } } @@ -100,15 +170,23 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ArrayIndexing { /// Returns an option containing a tuple with the start and end (exclusive) of /// the range. -fn to_const_range<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, range: Range, array_size: u128) -> Option<(u128, u128)> { - let s = range.start.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c)); +fn to_const_range<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + range: Range, + array_size: u128, +) -> Option<(u128, u128)> { + 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, }; - let e = range.end.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c)); + let e = range + .end + .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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a2f2ae8ab0c..bd89b37dc3a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -355,8 +355,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { ); reg.register_late_lint_pass(box escape::Pass{too_large_for_stack: conf.too_large_for_stack}); reg.register_early_lint_pass(box misc_early::MiscEarly); - reg.register_late_lint_pass(box array_indexing::ArrayIndexing); - reg.register_late_lint_pass(box panic_unimplemented::Pass); + reg.register_late_lint_pass(box array_indexing::IndexingSlicingPass); + reg.register_late_lint_pass(box panic::Pass); reg.register_late_lint_pass(box strings::StringLitAsBytes); reg.register_late_lint_pass(box derive::Derive); reg.register_late_lint_pass(box types::CharLitAsU8); diff --git a/tests/ui/array_indexing.rs b/tests/ui/array_indexing.rs index a01600edac7..2437df96bd1 100644 --- a/tests/ui/array_indexing.rs +++ b/tests/ui/array_indexing.rs @@ -1,18 +1,26 @@ #![feature(plugin)] - - #![warn(indexing_slicing)] #![warn(out_of_bounds_indexing)] #![allow(no_effect, unnecessary_operation)] fn main() { - let x = [1,2,3,4]; + let x = [1, 2, 3, 4]; + let index: usize = 1; + 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[4]; x[1 << 3]; &x[1..5]; + &x[1..][..5]; &x[0..3]; + &x[0..][..3]; &x[0..=4]; &x[..=4]; &x[..]; @@ -42,4 +50,11 @@ fn main() { &empty[..0]; &empty[1..]; &empty[..4]; + + let v = vec![0; 5]; + v[0]; + v[10]; + &v[10..100]; + &v[10..]; + &v[..100]; } diff --git a/tests/ui/array_indexing.stderr b/tests/ui/array_indexing.stderr index d730b012932..14ef73155a0 100644 --- a/tests/ui/array_indexing.stderr +++ b/tests/ui/array_indexing.stderr @@ -1,120 +1,222 @@ -error: const index is out of bounds - --> $DIR/array_indexing.rs:12:5 +error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead + --> $DIR/array_indexing.rs:11:5 | -12 | x[4]; +11 | x[index]; + | ^^^^^^^^ + | + = note: `-D indexing-slicing` implied by `-D warnings` + +error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead + --> $DIR/array_indexing.rs:12:6 + | +12 | &x[index_from..index_to]; + | ^^^^^^^^^^^^^^^^^^^^^^^ + +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:13:6 + | +13 | &x[index_from..][..index_to]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead + --> $DIR/array_indexing.rs:13:6 + | +13 | &x[index_from..][..index_to]; + | ^^^^^^^^^^^^^^^ + +error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead + --> $DIR/array_indexing.rs:14:6 + | +14 | &x[index..]; + | ^^^^^^^^^^ + +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:15:6 + | +15 | &x[..index]; + | ^^^^^^^^^^ + +error: const index is out of bounds + --> $DIR/array_indexing.rs:18:5 + | +18 | x[4]; | ^^^^ | = note: `-D out-of-bounds-indexing` implied by `-D warnings` -error: const index is out of bounds - --> $DIR/array_indexing.rs:13:5 - | -13 | x[1 << 3]; - | ^^^^^^^^^ - error: range is out of bounds - --> $DIR/array_indexing.rs:14:6 + --> $DIR/array_indexing.rs:20:6 | -14 | &x[1..5]; +20 | &x[1..5]; | ^^^^^^^ -error: range is out of bounds - --> $DIR/array_indexing.rs:16:6 +error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead + --> $DIR/array_indexing.rs:20:6 | -16 | &x[0..=4]; - | ^^^^^^^^ - -error: range is out of bounds - --> $DIR/array_indexing.rs:17:6 - | -17 | &x[..=4]; +20 | &x[1..5]; | ^^^^^^^ -error: range is out of bounds +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead --> $DIR/array_indexing.rs:21:6 | -21 | &x[5..]; +21 | &x[1..][..5]; + | ^^^^^^^^^^^ + +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:23:6 + | +23 | &x[0..][..3]; + | ^^^^^^^^^^^ + +error: range is out of bounds + --> $DIR/array_indexing.rs:25:6 + | +25 | &x[..=4]; + | ^^^^^^^ + +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:25:6 + | +25 | &x[..=4]; + | ^^^^^^^ + +error: range is out of bounds + --> $DIR/array_indexing.rs:29:6 + | +29 | &x[5..]; + | ^^^^^^ + +error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead + --> $DIR/array_indexing.rs:29:6 + | +29 | &x[5..]; | ^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:23:6 + --> $DIR/array_indexing.rs:31:6 | -23 | &x[..5]; +31 | &x[..5]; | ^^^^^^ -error: indexing may panic - --> $DIR/array_indexing.rs:26:5 +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:31:6 | -26 | y[0]; - | ^^^^ - | - = note: `-D indexing-slicing` implied by `-D warnings` +31 | &x[..5]; + | ^^^^^^ -error: slicing may panic - --> $DIR/array_indexing.rs:27:6 +error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead + --> $DIR/array_indexing.rs:34:5 | -27 | &y[1..2]; +34 | y[0]; + | ^^^^ + +error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead + --> $DIR/array_indexing.rs:35:6 + | +35 | &y[1..2]; | ^^^^^^^ -error: slicing may panic - --> $DIR/array_indexing.rs:29:6 +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:38:6 | -29 | &y[0..=4]; - | ^^^^^^^^ - -error: slicing may panic - --> $DIR/array_indexing.rs:30:6 - | -30 | &y[..=4]; +38 | &y[..=4]; | ^^^^^^^ error: const index is out of bounds - --> $DIR/array_indexing.rs:33:5 + --> $DIR/array_indexing.rs:41:5 | -33 | empty[0]; +41 | empty[0]; | ^^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:34:6 + --> $DIR/array_indexing.rs:42:6 | -34 | &empty[1..5]; +42 | &empty[1..5]; | ^^^^^^^^^^^ -error: range is out of bounds - --> $DIR/array_indexing.rs:35:6 +error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead + --> $DIR/array_indexing.rs:42:6 | -35 | &empty[0..=4]; - | ^^^^^^^^^^^^ - -error: range is out of bounds - --> $DIR/array_indexing.rs:36:6 - | -36 | &empty[..=4]; +42 | &empty[1..5]; | ^^^^^^^^^^^ -error: range is out of bounds - --> $DIR/array_indexing.rs:40:6 - | -40 | &empty[0..=0]; - | ^^^^^^^^^^^^ - -error: range is out of bounds - --> $DIR/array_indexing.rs:41:6 - | -41 | &empty[..=0]; - | ^^^^^^^^^^^ - -error: range is out of bounds - --> $DIR/array_indexing.rs:43:6 - | -43 | &empty[1..]; - | ^^^^^^^^^^ - error: range is out of bounds --> $DIR/array_indexing.rs:44:6 | -44 | &empty[..4]; +44 | &empty[..=4]; + | ^^^^^^^^^^^ + +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:44:6 + | +44 | &empty[..=4]; + | ^^^^^^^^^^^ + +error: range is out of bounds + --> $DIR/array_indexing.rs:49:6 + | +49 | &empty[..=0]; + | ^^^^^^^^^^^ + +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:49:6 + | +49 | &empty[..=0]; + | ^^^^^^^^^^^ + +error: range is out of bounds + --> $DIR/array_indexing.rs:51:6 + | +51 | &empty[1..]; | ^^^^^^^^^^ -error: aborting due to 19 previous errors +error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead + --> $DIR/array_indexing.rs:51:6 + | +51 | &empty[1..]; + | ^^^^^^^^^^ + +error: range is out of bounds + --> $DIR/array_indexing.rs:52:6 + | +52 | &empty[..4]; + | ^^^^^^^^^^ + +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:52:6 + | +52 | &empty[..4]; + | ^^^^^^^^^^ + +error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead + --> $DIR/array_indexing.rs:55:5 + | +55 | v[0]; + | ^^^^ + +error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead + --> $DIR/array_indexing.rs:56:5 + | +56 | v[10]; + | ^^^^^ + +error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead + --> $DIR/array_indexing.rs:57:6 + | +57 | &v[10..100]; + | ^^^^^^^^^^ + +error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead + --> $DIR/array_indexing.rs:58:6 + | +58 | &v[10..]; + | ^^^^^^^ + +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:59:6 + | +59 | &v[..100]; + | ^^^^^^^^ + +error: aborting due to 36 previous errors