From 7af0c678559ef75e9ec6359ec3e05d76dfc355f5 Mon Sep 17 00:00:00 2001 From: Shea Newton Date: Tue, 22 May 2018 21:56:02 -0700 Subject: [PATCH 1/7] Extend `indexing_slicing` lint Hey there clippy team! I've made some assumptions in this PR and I'm not at all certain they'll look like the right approach to you. I'm looking forward to any feedback or revision requests you have, thanks! Prior to this commit the `indexing_slicing` lint was limited to indexing/slicing operations on arrays. This meant that the scope of a really useful lint didn't include vectors. In order to include vectors in the `indexing_slicing` lint a few steps were taken. The `array_indexing.rs` source file in `clippy_lints` was renamed to `indexing_slicing.rs` to more accurately reflect the lint's new scope. The `OUT_OF_BOUNDS_INDEXING` lint persists through these changes so if we can know that a constant index or slice on an array is in bounds no lint is triggered. The `array_indexing` tests in the `tests/ui` directory were also extended and moved to `indexing_slicing.rs` and `indexing_slicing.stderr`. The `indexing_slicing` lint was moved to the `clippy_pedantic` lint group. A specific "Consider using" string was added to each of the `indexing_slicing` lint reports. At least one of the test scenarios might look peculiar and I'll leave it up to y'all to decide if it's palatable. It's the result of indexing the array `x` after `let x = [1, 2, 3, 4];` ``` error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)`instead --> $DIR/indexing_slicing.rs:23:6 | 23 | &x[0..][..3]; | ^^^^^^^^^^^ ``` The error string reports only on the second half's range-to, because the range-from is in bounds! Again, thanks for taking a look. Closes #2536 --- clippy_lints/src/array_indexing.rs | 168 ++++++++++++++++------ clippy_lints/src/lib.rs | 4 +- tests/ui/array_indexing.rs | 21 ++- tests/ui/array_indexing.stderr | 216 +++++++++++++++++++++-------- 4 files changed, 302 insertions(+), 107 deletions(-) diff --git a/clippy_lints/src/array_indexing.rs b/clippy_lints/src/array_indexing.rs index 77aa5e834258..e7bb590e60df 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"); +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; + } + } + } + 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", + ); } - - 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"); + 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. } - return; + } 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; - } - - // 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 a2f2ae8ab0c8..bd89b37dc3ae 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 a01600edac77..2437df96bd10 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 d730b012932a..14ef73155a0d 100644 --- a/tests/ui/array_indexing.stderr +++ b/tests/ui/array_indexing.stderr @@ -1,120 +1,222 @@ +error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead + --> $DIR/array_indexing.rs:11:5 + | +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:12:5 + --> $DIR/array_indexing.rs:18:5 | -12 | x[4]; +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 +error: range is out of bounds + --> $DIR/array_indexing.rs:20:6 | -13 | x[1 << 3]; - | ^^^^^^^^^ +20 | &x[1..5]; + | ^^^^^^^ -error: range is out of bounds - --> $DIR/array_indexing.rs:14:6 +error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead + --> $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)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:21:6 | -16 | &x[0..=4]; - | ^^^^^^^^ +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:17:6 + --> $DIR/array_indexing.rs:25:6 | -17 | &x[..=4]; +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:21:6 + --> $DIR/array_indexing.rs:29:6 | -21 | &x[5..]; +29 | &x[5..]; | ^^^^^^ -error: range is out of bounds - --> $DIR/array_indexing.rs:23:6 +error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead + --> $DIR/array_indexing.rs:29:6 | -23 | &x[..5]; +29 | &x[5..]; | ^^^^^^ -error: indexing may panic - --> $DIR/array_indexing.rs:26:5 +error: range is out of bounds + --> $DIR/array_indexing.rs:31:6 | -26 | y[0]; - | ^^^^ +31 | &x[..5]; + | ^^^^^^ + +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:31:6 | - = 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 - --> $DIR/array_indexing.rs:29:6 +error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead + --> $DIR/array_indexing.rs:35:6 | -29 | &y[0..=4]; - | ^^^^^^^^ +35 | &y[1..2]; + | ^^^^^^^ -error: slicing may panic - --> $DIR/array_indexing.rs:30:6 +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:38: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]; - | ^^^^^^^^^^^^ +42 | &empty[1..5]; + | ^^^^^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:36:6 + --> $DIR/array_indexing.rs:44:6 | -36 | &empty[..=4]; +44 | &empty[..=4]; | ^^^^^^^^^^^ -error: range is out of bounds - --> $DIR/array_indexing.rs:40:6 +error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead + --> $DIR/array_indexing.rs:44:6 | -40 | &empty[0..=0]; - | ^^^^^^^^^^^^ +44 | &empty[..=4]; + | ^^^^^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:41:6 + --> $DIR/array_indexing.rs:49:6 | -41 | &empty[..=0]; +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:43:6 + --> $DIR/array_indexing.rs:51:6 | -43 | &empty[1..]; +51 | &empty[1..]; + | ^^^^^^^^^^ + +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:44:6 + --> $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 | -44 | &empty[..4]; +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: aborting due to 19 previous errors +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 From 5b759efa4c9702aa095f1564e9cfa76046abf2b1 Mon Sep 17 00:00:00 2001 From: Shea Newton Date: Tue, 22 May 2018 22:02:07 -0700 Subject: [PATCH 2/7] Rename instances of `array_indexing` This commit renames instances of `array_indexing` to `indexing_slicing` and moves the `indexing_slicing` lint to the `clippy_pedantic` group. The justification for this commit's changes are detailed in the previous commit's message. --- ...{array_indexing.rs => indexing_slicing.rs} | 2 +- clippy_lints/src/lib.rs | 15 ++-- ...{array_indexing.rs => indexing_slicing.rs} | 0 ...ndexing.stderr => indexing_slicing.stderr} | 72 +++++++++---------- 4 files changed, 44 insertions(+), 45 deletions(-) rename clippy_lints/src/{array_indexing.rs => indexing_slicing.rs} (99%) rename tests/ui/{array_indexing.rs => indexing_slicing.rs} (100%) rename tests/ui/{array_indexing.stderr => indexing_slicing.stderr} (76%) diff --git a/clippy_lints/src/array_indexing.rs b/clippy_lints/src/indexing_slicing.rs similarity index 99% rename from clippy_lints/src/array_indexing.rs rename to clippy_lints/src/indexing_slicing.rs index e7bb590e60df..d7f2a37c5fec 100644 --- a/clippy_lints/src/array_indexing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -3,7 +3,7 @@ use crate::consts::{constant, Constant}; use crate::utils::higher::Range; use crate::utils::{self, higher}; -use rustc::hir; +use rustc::hir::*; use rustc::lint::*; use rustc::ty; use syntax::ast::RangeLimits; diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bd89b37dc3ae..e261fe417f7e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -6,7 +6,7 @@ #![feature(stmt_expr_attributes)] #![feature(range_contains)] #![feature(macro_vis_matcher)] -#![allow(unknown_lints, indexing_slicing, shadow_reuse, missing_docs_in_private_items)] +#![allow(unknown_lints, shadow_reuse, missing_docs_in_private_items)] #![recursion_limit = "256"] #![allow(stable_features)] #![feature(iterator_find_map)] @@ -99,7 +99,6 @@ pub mod utils; // begin lints modules, do not remove this comment, it’s used in `update_lints` pub mod approx_const; pub mod arithmetic; -pub mod array_indexing; pub mod assign_ops; pub mod attrs; pub mod bit_mask; @@ -139,6 +138,7 @@ pub mod identity_conversion; pub mod identity_op; pub mod if_let_redundant_pattern_matching; pub mod if_not_else; +pub mod indexing_slicing; pub mod infallible_destructuring_match; pub mod infinite_iter; pub mod inherent_impl; @@ -355,8 +355,7 @@ 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::IndexingSlicingPass); - reg.register_late_lint_pass(box panic::Pass); + reg.register_late_lint_pass(box panic_unimplemented::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); @@ -432,12 +431,11 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box unwrap::Pass); reg.register_late_lint_pass(box duration_subsec::DurationSubsec); reg.register_late_lint_pass(box default_trait_access::DefaultTraitAccess); - + reg.register_late_lint_pass(box indexing_slicing::IndexingSlicingPass); reg.register_lint_group("clippy_restriction", vec![ arithmetic::FLOAT_ARITHMETIC, arithmetic::INTEGER_ARITHMETIC, - array_indexing::INDEXING_SLICING, assign_ops::ASSIGN_OPS, else_if_without_else::ELSE_IF_WITHOUT_ELSE, inherent_impl::MULTIPLE_INHERENT_IMPL, @@ -468,6 +466,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { enum_variants::PUB_ENUM_VARIANT_NAMES, enum_variants::STUTTER, if_not_else::IF_NOT_ELSE, + indexing_slicing::INDEXING_SLICING, infinite_iter::MAYBE_INFINITE_ITER, items_after_statements::ITEMS_AFTER_STATEMENTS, matches::SINGLE_MATCH_ELSE, @@ -500,7 +499,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_lint_group("clippy", vec![ approx_const::APPROX_CONSTANT, - array_indexing::OUT_OF_BOUNDS_INDEXING, + indexing_slicing::OUT_OF_BOUNDS_INDEXING, assign_ops::ASSIGN_OP_PATTERN, assign_ops::MISREFACTORED_ASSIGN_OP, attrs::DEPRECATED_SEMVER, @@ -863,7 +862,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_lint_group("clippy_correctness", vec![ approx_const::APPROX_CONSTANT, - array_indexing::OUT_OF_BOUNDS_INDEXING, + indexing_slicing::OUT_OF_BOUNDS_INDEXING, attrs::DEPRECATED_SEMVER, attrs::USELESS_ATTRIBUTE, bit_mask::BAD_BIT_MASK, diff --git a/tests/ui/array_indexing.rs b/tests/ui/indexing_slicing.rs similarity index 100% rename from tests/ui/array_indexing.rs rename to tests/ui/indexing_slicing.rs diff --git a/tests/ui/array_indexing.stderr b/tests/ui/indexing_slicing.stderr similarity index 76% rename from tests/ui/array_indexing.stderr rename to tests/ui/indexing_slicing.stderr index 14ef73155a0d..30231a31d191 100644 --- a/tests/ui/array_indexing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -1,5 +1,5 @@ error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead - --> $DIR/array_indexing.rs:11:5 + --> $DIR/indexing_slicing.rs:11:5 | 11 | x[index]; | ^^^^^^^^ @@ -7,37 +7,37 @@ error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead = 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 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.rs:15:6 | 15 | &x[..index]; | ^^^^^^^^^^ error: const index is out of bounds - --> $DIR/array_indexing.rs:18:5 + --> $DIR/indexing_slicing.rs:18:5 | 18 | x[4]; | ^^^^ @@ -45,175 +45,175 @@ error: const index is out of bounds = note: `-D out-of-bounds-indexing` implied by `-D warnings` error: range is out of bounds - --> $DIR/array_indexing.rs:20:6 + --> $DIR/indexing_slicing.rs:20:6 | 20 | &x[1..5]; | ^^^^^^^ error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead - --> $DIR/array_indexing.rs:20:6 + --> $DIR/indexing_slicing.rs:20:6 | 20 | &x[1..5]; | ^^^^^^^ error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/array_indexing.rs:21:6 + --> $DIR/indexing_slicing.rs:21:6 | 21 | &x[1..][..5]; | ^^^^^^^^^^^ error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/array_indexing.rs:23:6 + --> $DIR/indexing_slicing.rs:23:6 | 23 | &x[0..][..3]; | ^^^^^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:25:6 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.rs:25:6 | 25 | &x[..=4]; | ^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:29:6 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.rs:29:6 | 29 | &x[5..]; | ^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:31:6 + --> $DIR/indexing_slicing.rs:31:6 | 31 | &x[..5]; | ^^^^^^ error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/array_indexing.rs:31:6 + --> $DIR/indexing_slicing.rs:31:6 | 31 | &x[..5]; | ^^^^^^ error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead - --> $DIR/array_indexing.rs:34:5 + --> $DIR/indexing_slicing.rs:34:5 | 34 | y[0]; | ^^^^ error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead - --> $DIR/array_indexing.rs:35:6 + --> $DIR/indexing_slicing.rs:35:6 | 35 | &y[1..2]; | ^^^^^^^ error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/array_indexing.rs:38:6 + --> $DIR/indexing_slicing.rs:38:6 | 38 | &y[..=4]; | ^^^^^^^ error: const index is out of bounds - --> $DIR/array_indexing.rs:41:5 + --> $DIR/indexing_slicing.rs:41:5 | 41 | empty[0]; | ^^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:42:6 + --> $DIR/indexing_slicing.rs:42:6 | 42 | &empty[1..5]; | ^^^^^^^^^^^ error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead - --> $DIR/array_indexing.rs:42:6 + --> $DIR/indexing_slicing.rs:42:6 | 42 | &empty[1..5]; | ^^^^^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:44:6 + --> $DIR/indexing_slicing.rs:44:6 | 44 | &empty[..=4]; | ^^^^^^^^^^^ error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/array_indexing.rs:44:6 + --> $DIR/indexing_slicing.rs:44:6 | 44 | &empty[..=4]; | ^^^^^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:49:6 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.rs:49:6 | 49 | &empty[..=0]; | ^^^^^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:51:6 + --> $DIR/indexing_slicing.rs:51:6 | 51 | &empty[1..]; | ^^^^^^^^^^ error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead - --> $DIR/array_indexing.rs:51:6 + --> $DIR/indexing_slicing.rs:51:6 | 51 | &empty[1..]; | ^^^^^^^^^^ error: range is out of bounds - --> $DIR/array_indexing.rs:52:6 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.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 + --> $DIR/indexing_slicing.rs:59:6 | 59 | &v[..100]; | ^^^^^^^^ From a7c0ff3fa676aaa3e6d27a413c302abb0eba9805 Mon Sep 17 00:00:00 2001 From: Shea Newton Date: Wed, 13 Jun 2018 23:28:57 +0000 Subject: [PATCH 3/7] This commit represents an attempt to address changes requested in the process of reviewing PR #2790. The changes reflected in this commit are as follows: - Revised `IndexingSlicingPass` struct name to IndexingSlicing for consistency with the rest of the code base. - Revised match arm condition to use `(..)` shorthand in favor of `(_, _, _)`. - Restored a couple telling variable names. - Calls to `cx.span_lint` were revised to use `utils::span_help_and_lint`. - Took a stab at refactoring some generalizable calls to `utils::span_help_and_lint` to minimize duplicate code. - Revised INDEXING_SLICING declaration to pedantic rather than restriction. - Added `&x[0..].get(..3)` to the test cases. --- clippy_lints/src/indexing_slicing.rs | 76 +++++----- clippy_lints/src/lib.rs | 2 +- tests/ui/indexing_slicing.rs | 1 + tests/ui/indexing_slicing.stderr | 199 +++++++++++++++++---------- 4 files changed, 164 insertions(+), 114 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index d7f2a37c5fec..1f2c7755eee8 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -59,30 +59,30 @@ declare_clippy_lint! { /// ``` declare_clippy_lint! { pub INDEXING_SLICING, - restriction, + pedantic, "indexing/slicing usage" } #[derive(Copy, Clone)] -pub struct IndexingSlicingPass; +pub struct IndexingSlicing; -impl LintPass for IndexingSlicingPass { +impl LintPass for IndexingSlicing { fn get_lints(&self) -> LintArray { lint_array!(INDEXING_SLICING, OUT_OF_BOUNDS_INDEXING) } } -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass { +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { - if let ExprIndex(ref a, ref b) = &expr.node { - match &b.node { + if let ExprIndex(ref array, ref index) = &expr.node { + match &index.node { // Both ExprStruct and ExprPath require this approach's checks - // on the `range` returned by `higher::range(cx, b)`. + // 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, b) { - let ty = cx.tables.expr_ty(a); + 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. @@ -100,49 +100,48 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass { } } } + + let help_msg; 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", - ); + help_msg = "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", - ); + help_msg = "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", - ); + help_msg = + "Consider using `.get(n..m)` or `.get_mut(n..m)` instead"; } - (None, None) => (), + (None, None) => return, // [..] is ok } + + utils::span_help_and_lint( + cx, + INDEXING_SLICING, + expr.span, + "slicing may panic.", + help_msg, + ); } else { - cx.span_lint( + utils::span_help_and_lint( + cx, INDEXING_SLICING, expr.span, - "indexing may panic. Consider using `.get(n)` or \ - `.get_mut(n)` instead", + "indexing may panic.", + "Consider using `.get(n)` or `.get_mut(n)` instead", ); } } - ExprLit(_) => { + ExprLit(..) => { // [n] - let ty = cx.tables.expr_ty(a); + 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. - if let Some((Constant::Int(const_index), _)) = constant(cx, cx.tables, b) { + if let Some((Constant::Int(const_index), _)) = + constant(cx, cx.tables, index) + { if size <= const_index { utils::span_lint( cx, @@ -154,11 +153,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass { // Else index is in bounds, ok. } } else { - cx.span_lint( + utils::span_help_and_lint( + cx, INDEXING_SLICING, expr.span, - "indexing may panic. Consider using `.get(n)` or \ - `.get_mut(n)` instead", + "indexing may panic.", + "Consider using `.get(n)` or `.get_mut(n)` instead", ); } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index e261fe417f7e..621b21429a98 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -431,7 +431,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box unwrap::Pass); reg.register_late_lint_pass(box duration_subsec::DurationSubsec); reg.register_late_lint_pass(box default_trait_access::DefaultTraitAccess); - reg.register_late_lint_pass(box indexing_slicing::IndexingSlicingPass); + reg.register_late_lint_pass(box indexing_slicing::IndexingSlicing); reg.register_lint_group("clippy_restriction", vec![ arithmetic::FLOAT_ARITHMETIC, diff --git a/tests/ui/indexing_slicing.rs b/tests/ui/indexing_slicing.rs index 2437df96bd10..913063b8dd54 100644 --- a/tests/ui/indexing_slicing.rs +++ b/tests/ui/indexing_slicing.rs @@ -21,6 +21,7 @@ fn main() { &x[1..][..5]; &x[0..3]; &x[0..][..3]; + &x[0..].get(..3); // Ok &x[0..=4]; &x[..=4]; &x[..]; diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index 30231a31d191..642817d9e940 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -1,40 +1,51 @@ -error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead +error: indexing may panic. --> $DIR/indexing_slicing.rs:11:5 | 11 | x[index]; | ^^^^^^^^ | = note: `-D indexing-slicing` implied by `-D warnings` + = help: Consider using `.get(n)` or `.get_mut(n)` instead -error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead +error: slicing may panic. --> $DIR/indexing_slicing.rs:12:6 | 12 | &x[index_from..index_to]; | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead -error: slicing may panic. 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]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead -error: slicing may panic. 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]; | ^^^^^^^^^^^^^^^ + | + = help: Consider using `.get(n..)` or .get_mut(n..)` instead -error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead +error: slicing may panic. --> $DIR/indexing_slicing.rs:14:6 | 14 | &x[index..]; | ^^^^^^^^^^ + | + = help: Consider using `.get(n..)` or .get_mut(n..)` instead -error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead +error: slicing may panic. --> $DIR/indexing_slicing.rs:15:6 | 15 | &x[..index]; | ^^^^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: const index is out of bounds --> $DIR/indexing_slicing.rs:18:5 @@ -50,173 +61,211 @@ error: range is out of bounds 20 | &x[1..5]; | ^^^^^^^ -error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead +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. Consider using `.get(..n)`or `.get_mut(..n)` instead +error: slicing may panic. --> $DIR/indexing_slicing.rs:21:6 | 21 | &x[1..][..5]; | ^^^^^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead -error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead +error: slicing may panic. --> $DIR/indexing_slicing.rs:23:6 | 23 | &x[0..][..3]; | ^^^^^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:25:6 + --> $DIR/indexing_slicing.rs:26:6 | -25 | &x[..=4]; +26 | &x[..=4]; | ^^^^^^^ -error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/indexing_slicing.rs:25:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:26:6 | -25 | &x[..=4]; +26 | &x[..=4]; | ^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:29:6 + --> $DIR/indexing_slicing.rs:30:6 | -29 | &x[5..]; +30 | &x[5..]; | ^^^^^^ -error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead - --> $DIR/indexing_slicing.rs:29:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:30:6 | -29 | &x[5..]; +30 | &x[5..]; | ^^^^^^ + | + = help: Consider using `.get(n..)` or .get_mut(n..)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:31:6 + --> $DIR/indexing_slicing.rs:32:6 | -31 | &x[..5]; +32 | &x[..5]; | ^^^^^^ -error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/indexing_slicing.rs:31:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:32:6 | -31 | &x[..5]; +32 | &x[..5]; | ^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead -error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead - --> $DIR/indexing_slicing.rs:34:5 +error: indexing may panic. + --> $DIR/indexing_slicing.rs:35:5 | -34 | y[0]; +35 | y[0]; | ^^^^ + | + = help: Consider using `.get(n)` or `.get_mut(n)` instead -error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead - --> $DIR/indexing_slicing.rs:35:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:36:6 | -35 | &y[1..2]; +36 | &y[1..2]; | ^^^^^^^ + | + = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead -error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/indexing_slicing.rs:38:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:39:6 | -38 | &y[..=4]; +39 | &y[..=4]; | ^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: const index is out of bounds - --> $DIR/indexing_slicing.rs:41:5 + --> $DIR/indexing_slicing.rs:42:5 | -41 | empty[0]; +42 | empty[0]; | ^^^^^^^^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:42:6 + --> $DIR/indexing_slicing.rs:43:6 | -42 | &empty[1..5]; +43 | &empty[1..5]; | ^^^^^^^^^^^ -error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead - --> $DIR/indexing_slicing.rs:42:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:43:6 | -42 | &empty[1..5]; +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:44:6 + --> $DIR/indexing_slicing.rs:45:6 | -44 | &empty[..=4]; +45 | &empty[..=4]; | ^^^^^^^^^^^ -error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/indexing_slicing.rs:44:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:45:6 | -44 | &empty[..=4]; +45 | &empty[..=4]; | ^^^^^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:49:6 + --> $DIR/indexing_slicing.rs:50:6 | -49 | &empty[..=0]; +50 | &empty[..=0]; | ^^^^^^^^^^^ -error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/indexing_slicing.rs:49:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:50:6 | -49 | &empty[..=0]; +50 | &empty[..=0]; | ^^^^^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:51:6 + --> $DIR/indexing_slicing.rs:52:6 | -51 | &empty[1..]; +52 | &empty[1..]; | ^^^^^^^^^^ -error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead - --> $DIR/indexing_slicing.rs:51:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:52:6 | -51 | &empty[1..]; +52 | &empty[1..]; | ^^^^^^^^^^ + | + = help: Consider using `.get(n..)` or .get_mut(n..)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:52:6 + --> $DIR/indexing_slicing.rs:53:6 | -52 | &empty[..4]; +53 | &empty[..4]; | ^^^^^^^^^^ -error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/indexing_slicing.rs:52:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:53:6 | -52 | &empty[..4]; +53 | &empty[..4]; | ^^^^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead -error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead - --> $DIR/indexing_slicing.rs:55:5 +error: indexing may panic. + --> $DIR/indexing_slicing.rs:56:5 | -55 | v[0]; +56 | v[0]; | ^^^^ + | + = help: Consider using `.get(n)` or `.get_mut(n)` instead -error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead - --> $DIR/indexing_slicing.rs:56:5 +error: indexing may panic. + --> $DIR/indexing_slicing.rs:57:5 | -56 | v[10]; +57 | v[10]; | ^^^^^ + | + = help: Consider using `.get(n)` or `.get_mut(n)` instead -error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead - --> $DIR/indexing_slicing.rs:57:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:58:6 | -57 | &v[10..100]; +58 | &v[10..100]; | ^^^^^^^^^^ + | + = help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead -error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead - --> $DIR/indexing_slicing.rs:58:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:59:6 | -58 | &v[10..]; +59 | &v[10..]; | ^^^^^^^ + | + = help: Consider using `.get(n..)` or .get_mut(n..)` instead -error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead - --> $DIR/indexing_slicing.rs:59:6 +error: slicing may panic. + --> $DIR/indexing_slicing.rs:60:6 | -59 | &v[..100]; +60 | &v[..100]; | ^^^^^^^^ + | + = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: aborting due to 36 previous errors From 8b59542acc9901a6568731541baa9f623c1991b3 Mon Sep 17 00:00:00 2001 From: Shea Newton Date: Thu, 14 Jun 2018 16:41:56 +0000 Subject: [PATCH 4/7] 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 1f2c7755eee8..01f9f45c5891 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 913063b8dd54..a22c9034346d 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 642817d9e940..605a96e8b8c2 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 - | -50 | &empty[..=0]; - | ^^^^^^^^^^^ - -error: slicing may panic. - --> $DIR/indexing_slicing.rs:50:6 + --> $DIR/indexing_slicing.rs:47:6 | -50 | &empty[..=0]; +47 | &empty[..=4]; | ^^^^^^^^^^^ - | - = 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 From 4ec439bef0124a01dd71ea0d5f441066690f33ec Mon Sep 17 00:00:00 2001 From: Shea Newton Date: Thu, 14 Jun 2018 20:04:37 +0000 Subject: [PATCH 5/7] 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. --- clippy_lints/src/indexing_slicing.rs | 133 ++++++++---------- tests/ui/indexing_slicing.rs | 52 ++++--- tests/ui/indexing_slicing.stderr | 202 ++++++++++++++++++--------- 3 files changed, 220 insertions(+), 167 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index 01f9f45c5891..b4e6414195e3 100644 --- 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 a22c9034346d..16174afb1060 100644 --- 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::>(); - &x[2..].iter().map(|x| 2 * x).collect::>(); // 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::>(); // 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 605a96e8b8c2..c9aefe0349a1 100644 --- 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::>(); +23 | &x[5..].iter().map(|x| 2 * x).collect::>(); | ^^^^^^ +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 From e63f5dfedbb004afc6e9ca303b12f1c8f8cbea0d Mon Sep 17 00:00:00 2001 From: Shea Newton Date: Fri, 15 Jun 2018 15:54:38 +0000 Subject: [PATCH 6/7] Add tests that index with a `const` value. In this commit tests were added to ensure that tests with a `const` index behaved as expected. In order to minimize the changes to the test's corresponding `stderr`, the tests were appended to the end of the file. --- tests/ui/indexing_slicing.rs | 11 +++++++++++ tests/ui/indexing_slicing.stderr | 24 +++++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/tests/ui/indexing_slicing.rs b/tests/ui/indexing_slicing.rs index 16174afb1060..301658415d61 100644 --- a/tests/ui/indexing_slicing.rs +++ b/tests/ui/indexing_slicing.rs @@ -68,4 +68,15 @@ fn main() { &v[..100]; &v[..]; // Ok, should not produce stderr. + + // + // Continue tests at end function to minimize the changes to this file's corresponding stderr. + // + + const N: usize = 15; // Out of bounds + const M: usize = 3; // In bounds + x[N]; + x[M]; // Ok, should not produce stderr. + v[N]; + v[M]; } diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index c9aefe0349a1..2546d62bfc68 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -269,5 +269,27 @@ error: slicing may panic. | = help: Consider using `.get(..n)`or `.get_mut(..n)` instead -error: aborting due to 38 previous errors +error: const index is out of bounds + --> $DIR/indexing_slicing.rs:78:5 + | +78 | x[N]; + | ^^^^ + +error: indexing may panic. + --> $DIR/indexing_slicing.rs:80:5 + | +80 | v[N]; + | ^^^^ + | + = help: Consider using `.get(n)` or `.get_mut(n)` instead + +error: indexing may panic. + --> $DIR/indexing_slicing.rs:81:5 + | +81 | v[M]; + | ^^^^ + | + = help: Consider using `.get(n)` or `.get_mut(n)` instead + +error: aborting due to 41 previous errors From c479b3bc2856a2c2362cd17b95e28caaaffe0908 Mon Sep 17 00:00:00 2001 From: Shea Newton Date: Tue, 19 Jun 2018 21:30:43 +0000 Subject: [PATCH 7/7] Removing lint for constant `usize` array indexing This commit removes the logic in this PR that linted out-of-bounds constant `usize` indexing on arrays. That case is already handled by rustc's `const_err` lint. Beyond removing the linting logic, the test file and its associated stderr were updated to verify that const `usize` indexing operations on arrays are no longer handled by this `indexing_slicing` lint. --- clippy_lints/src/indexing_slicing.rs | 24 ++++++---------------- tests/ui/indexing_slicing.rs | 8 ++++---- tests/ui/indexing_slicing.stderr | 30 +++------------------------- 3 files changed, 13 insertions(+), 49 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index b4e6414195e3..7dd72a5383cd 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -34,9 +34,9 @@ declare_clippy_lint! { "out of bounds constant indexing" } -/// **What it does:** Checks for usage of indexing or slicing. Does not report -/// on arrays if we can tell that the indexing or slicing operations are in -/// bounds. +/// **What it does:** Checks for usage of indexing or slicing. Arrays are special cased, this lint +/// does report on arrays if we can tell that slicing operations are in bounds and does not +/// lint on constant `usize` indexing on arrays because that is handled by rustc's `const_err` lint. /// /// **Why is this bad?** Indexing and slicing can panic at runtime and there are /// safe alternatives. @@ -64,13 +64,11 @@ declare_clippy_lint! { /// 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]; @@ -132,20 +130,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { ); } 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(); + if let ty::TyArray(..) = ty.sty { // 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. - + if let Some(..) = constant(cx, cx.tables, index) { + // Let rustc's `const_err` lint handle constant `usize` indexing on arrays. return; } } diff --git a/tests/ui/indexing_slicing.rs b/tests/ui/indexing_slicing.rs index 301658415d61..e39dc92367c8 100644 --- a/tests/ui/indexing_slicing.rs +++ b/tests/ui/indexing_slicing.rs @@ -13,8 +13,8 @@ fn main() { &x[..index]; &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[4]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. + x[1 << 3]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. &x[..=4]; &x[1..5]; &x[5..][..10]; // Two lint reports, one for [5..] and another for [..10]. @@ -44,7 +44,7 @@ fn main() { &y[..]; // Ok, should not produce stderr. let empty: [i8; 0] = []; - empty[0]; + empty[0]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. &empty[1..5]; &empty[0..=4]; &empty[..=4]; @@ -75,7 +75,7 @@ fn main() { const N: usize = 15; // Out of bounds const M: usize = 3; // In bounds - x[N]; + x[N]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. x[M]; // Ok, should not produce stderr. v[N]; v[M]; diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index 2546d62bfc68..ee11dce6d1c2 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -47,25 +47,13 @@ error: slicing may panic. | = help: Consider using `.get(n..)` or .get_mut(n..)` instead -error: const index is out of bounds - --> $DIR/indexing_slicing.rs:16:5 - | -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:18:6 | 18 | &x[..=4]; | ^^^^^^^ + | + = note: `-D out-of-bounds-indexing` implied by `-D warnings` error: range is out of bounds --> $DIR/indexing_slicing.rs:19:6 @@ -159,12 +147,6 @@ error: slicing may panic. | = help: Consider using `.get(..n)`or `.get_mut(..n)` instead -error: const index is out of bounds - --> $DIR/indexing_slicing.rs:47:5 - | -47 | empty[0]; - | ^^^^^^^^ - error: range is out of bounds --> $DIR/indexing_slicing.rs:48:6 | @@ -269,12 +251,6 @@ error: slicing may panic. | = help: Consider using `.get(..n)`or `.get_mut(..n)` instead -error: const index is out of bounds - --> $DIR/indexing_slicing.rs:78:5 - | -78 | x[N]; - | ^^^^ - error: indexing may panic. --> $DIR/indexing_slicing.rs:80:5 | @@ -291,5 +267,5 @@ error: indexing may panic. | = help: Consider using `.get(n)` or `.get_mut(n)` instead -error: aborting due to 41 previous errors +error: aborting due to 37 previous errors