From 3f112b1b8a42f2ceacb373bbb09b29d55523f7fe Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 11 Mar 2016 13:19:51 +0100 Subject: [PATCH 1/4] Fix punctuation in README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index a74de7c69983..556bfcf7eb24 100644 --- a/README.md +++ b/README.md @@ -234,7 +234,7 @@ And, in your `main.rs` or `lib.rs`: Both projects are independent and maintained by different people (even if some `clippy-service`'s contributions are authored by some `rust-clippy` members). -You can check it out this great service at [clippy.bashy.io](https://clippy.bashy.io/) +You can check it out this great service at [clippy.bashy.io](https://clippy.bashy.io/). ##License Licensed under [MPL](https://www.mozilla.org/MPL/2.0/). If you're having issues with the license, let me know and I'll try to change it to something more permissive. From a38958b8d94984ef15f27d808ac82279191ca19e Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 11 Mar 2016 20:27:33 +0100 Subject: [PATCH 2/4] Fix `unsugar_range` with `..` --- src/utils/mod.rs | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/src/utils/mod.rs b/src/utils/mod.rs index c626fcb8930c..625e8da197d5 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -681,6 +681,7 @@ pub fn camel_case_from(s: &str) -> usize { } /// Represents a range akin to `ast::ExprKind::Range`. +#[derive(Debug, Copy, Clone)] pub struct UnsugaredRange<'a> { pub start: Option<&'a Expr>, pub end: Option<&'a Expr>, @@ -711,24 +712,30 @@ pub fn unsugar_range(expr: &Expr) -> Option { Some(unwrap_unstable(expr)) } - if let ExprStruct(ref path, ref fields, None) = unwrap_unstable(&expr).node { - if match_path(path, &RANGE_FROM_PATH) { - Some(UnsugaredRange { start: get_field("start", fields), end: None, limits: RangeLimits::HalfOpen }) - } else if match_path(path, &RANGE_FULL_PATH) { - Some(UnsugaredRange { start: None, end: None, limits: RangeLimits::HalfOpen }) - } else if match_path(path, &RANGE_INCLUSIVE_NON_EMPTY_PATH) { - Some(UnsugaredRange { start: get_field("start", fields), end: get_field("end", fields), limits: RangeLimits::Closed }) - } else if match_path(path, &RANGE_PATH) { - Some(UnsugaredRange { start: get_field("start", fields), end: get_field("end", fields), limits: RangeLimits::HalfOpen }) - } else if match_path(path, &RANGE_TO_INCLUSIVE_PATH) { - Some(UnsugaredRange { start: None, end: get_field("end", fields), limits: RangeLimits::Closed }) - } else if match_path(path, &RANGE_TO_PATH) { - Some(UnsugaredRange { start: None, end: get_field("end", fields), limits: RangeLimits::HalfOpen }) - } else { - None + match unwrap_unstable(&expr).node { + ExprPath(None, ref path) => { + if match_path(path, &RANGE_FULL_PATH) { + Some(UnsugaredRange { start: None, end: None, limits: RangeLimits::HalfOpen }) + } else { + None + } } - } else { - None + ExprStruct(ref path, ref fields, None) => { + if match_path(path, &RANGE_FROM_PATH) { + Some(UnsugaredRange { start: get_field("start", fields), end: None, limits: RangeLimits::HalfOpen }) + } else if match_path(path, &RANGE_INCLUSIVE_NON_EMPTY_PATH) { + Some(UnsugaredRange { start: get_field("start", fields), end: get_field("end", fields), limits: RangeLimits::Closed }) + } else if match_path(path, &RANGE_PATH) { + Some(UnsugaredRange { start: get_field("start", fields), end: get_field("end", fields), limits: RangeLimits::HalfOpen }) + } else if match_path(path, &RANGE_TO_INCLUSIVE_PATH) { + Some(UnsugaredRange { start: None, end: get_field("end", fields), limits: RangeLimits::Closed }) + } else if match_path(path, &RANGE_TO_PATH) { + Some(UnsugaredRange { start: None, end: get_field("end", fields), limits: RangeLimits::HalfOpen }) + } else { + None + } + } + _ => None, } } From 87ef5f4d3b300c893cb404ab1f62816b34e0ea4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adolfo=20Ochagav=C3=ADa?= Date: Fri, 11 Mar 2016 10:51:16 +0100 Subject: [PATCH 3/4] Lint against indexing and slicing This can be useful to prevent panics in a codebase. ATM it is a pedantic lint, but in the future it should be added to the restricions group. --- README.md | 3 +- src/array_indexing.rs | 110 +++++++++++++++++++++++++-- src/lib.rs | 1 + tests/compile-fail/array_indexing.rs | 25 +++++- 4 files changed, 128 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 556bfcf7eb24..48a106e3591d 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to link with clippy-service](#link-with-clippy-service) ##Lints -There are 133 lints included in this crate: +There are 134 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -58,6 +58,7 @@ name [if_not_else](https://github.com/Manishearth/rust-clippy/wiki#if_not_else) | warn | finds if branches that could be swapped so no negation operation is necessary on the condition [if_same_then_else](https://github.com/Manishearth/rust-clippy/wiki#if_same_then_else) | warn | if with the same *then* and *else* blocks [ifs_same_cond](https://github.com/Manishearth/rust-clippy/wiki#ifs_same_cond) | warn | consecutive `ifs` with the same condition +[indexing_slicing](https://github.com/Manishearth/rust-clippy/wiki#indexing_slicing) | allow | indexing/slicing usage [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask) | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` [inline_always](https://github.com/Manishearth/rust-clippy/wiki#inline_always) | warn | `#[inline(always)]` is a bad idea in most cases [invalid_regex](https://github.com/Manishearth/rust-clippy/wiki#invalid_regex) | deny | finds invalid regular expressions in `Regex::new(_)` invocations diff --git a/src/array_indexing.rs b/src/array_indexing.rs index cfa52f390d29..abce04f0b774 100644 --- a/src/array_indexing.rs +++ b/src/array_indexing.rs @@ -3,7 +3,8 @@ use rustc::middle::const_eval::EvalHint::ExprTypeChecked; use rustc::middle::const_eval::{eval_const_expr_partial, ConstVal}; use rustc::middle::ty::TyArray; use rustc_front::hir::*; -use utils::span_lint; +use syntax::ast::RangeLimits; +use utils; /// **What it does:** Check for out of bounds array indexing with a constant index. /// @@ -17,6 +18,7 @@ use utils::span_lint; /// let x = [1,2,3,4]; /// ... /// x[9]; +/// &x[2..9]; /// ``` declare_lint! { pub OUT_OF_BOUNDS_INDEXING, @@ -24,28 +26,122 @@ declare_lint! { "out of bound constant indexing" } +/// **What it does:** Check for usage of indexing or slicing. +/// +/// **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. +/// +/// **Known problems:** Hopefully none. +/// +/// **Example:** +/// +/// ``` +/// ... +/// x[2]; +/// &x[0..2]; +/// ``` +declare_lint! { + pub INDEXING_SLICING, + Allow, + "indexing/slicing usage" +} + #[derive(Copy,Clone)] pub struct ArrayIndexing; impl LintPass for ArrayIndexing { fn get_lints(&self) -> LintArray { - lint_array!(OUT_OF_BOUNDS_INDEXING) + lint_array!(INDEXING_SLICING, OUT_OF_BOUNDS_INDEXING) } } impl LateLintPass for ArrayIndexing { fn check_expr(&mut self, cx: &LateContext, e: &Expr) { if let ExprIndex(ref array, ref index) = e.node { + // Array with known size can be checked statically let ty = cx.tcx.expr_ty(array); - if let TyArray(_, size) = ty.sty { - let index = eval_const_expr_partial(cx.tcx, &index, ExprTypeChecked, None); - if let Ok(ConstVal::Uint(index)) = index { - if size as u64 <= index { - span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "const index-expr is out of bounds"); + let size = size as u64; + + // Index is a constant uint + let const_index = eval_const_expr_partial(cx.tcx, &index, ExprTypeChecked, None); + if let Ok(ConstVal::Uint(const_index)) = const_index { + if size <= const_index { + utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "const index is out of bounds"); + utils::span_lint(cx, INDEXING_SLICING, e.span, "indexing may panic"); + } else { + // Index is within bounds + return; } } + + // Index is a constant range + if let Some(range) = utils::unsugar_range(index) { + let start = range.start.map(|start| + eval_const_expr_partial(cx.tcx, start, ExprTypeChecked, None)); + let end = range.end.map(|end| + eval_const_expr_partial(cx.tcx, end, ExprTypeChecked, None)); + + if let Some((start, end)) = to_const_range(start, end, range.limits, size) { + if start >= size && end >= size { + utils::span_lint(cx, + OUT_OF_BOUNDS_INDEXING, + e.span, + "range is out of bounds"); + utils::span_lint(cx, + INDEXING_SLICING, + e.span, + "slicing may panic"); + } else { + // Range is within bounds + return; + } + } + } + } + + if let Some(range) = utils::unsugar_range(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"); } } } } + +/// Returns an option containing a tuple with the start and end (exclusive) of the range +/// +/// Note: we assume the start and the end of the range are unsigned, since array slicing +/// works only on usize +fn to_const_range(start: Option>, + end: Option>, + limits: RangeLimits, + array_size: u64) + -> Option<(u64, u64)> { + let start = match start { + Some(Ok(ConstVal::Uint(x))) => x, + Some(_) => return None, + None => 0, + }; + + let end = match end { + Some(Ok(ConstVal::Uint(x))) => { + if limits == RangeLimits::Closed { + x + } else { + x - 1 + } + } + Some(_) => return None, + None => array_size - 1, + }; + + Some((start, end)) +} diff --git a/src/lib.rs b/src/lib.rs index 8c619a1eaf72..664819b97ecb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -181,6 +181,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box new_without_default::NewWithoutDefault); reg.register_lint_group("clippy_pedantic", vec![ + array_indexing::INDEXING_SLICING, enum_glob_use::ENUM_GLOB_USE, matches::SINGLE_MATCH_ELSE, methods::OPTION_UNWRAP_USED, diff --git a/tests/compile-fail/array_indexing.rs b/tests/compile-fail/array_indexing.rs index 1d9492bc0ab1..bcd25f5bf06d 100644 --- a/tests/compile-fail/array_indexing.rs +++ b/tests/compile-fail/array_indexing.rs @@ -1,6 +1,7 @@ -#![feature(plugin)] +#![feature(inclusive_range_syntax, plugin)] #![plugin(clippy)] +#![deny(indexing_slicing)] #![deny(out_of_bounds_indexing)] #![allow(no_effect)] @@ -8,6 +9,24 @@ fn main() { let x = [1,2,3,4]; x[0]; x[3]; - x[4]; //~ERROR: const index-expr is out of bounds - x[1 << 3]; //~ERROR: const index-expr is out of bounds + x[4]; //~ERROR: indexing may panic + //~^ ERROR: const index is out of bounds + x[1 << 3]; //~ERROR: indexing may panic + //~^ ERROR: const index is out of bounds + &x[1..5]; //~ERROR: slicing may panic + //~^ ERROR: range is out of bounds + &x[0..3]; + &x[0...4]; //~ERROR: slicing may panic + //~^ ERROR: range is out of bounds + &x[..]; + &x[1..]; + &x[..4]; + &x[..5]; //~ERROR: slicing may panic + //~^ ERROR: range is out of bounds + + let y = &x; + y[0]; //~ERROR: indexing may panic + &y[1..2]; //~ERROR: slicing may panic + &y[..]; + &y[0...4]; //~ERROR: slicing may panic } From 2f13c3bdefefb7947049ed51edde8db341994164 Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 11 Mar 2016 22:10:40 +0100 Subject: [PATCH 4/4] Small nits on INDEXING_SLICING --- src/array_indexing.rs | 28 ++++++++++------------------ src/lib.rs | 2 +- tests/compile-fail/array_indexing.rs | 15 +++++---------- 3 files changed, 16 insertions(+), 29 deletions(-) diff --git a/src/array_indexing.rs b/src/array_indexing.rs index abce04f0b774..274491b7c963 100644 --- a/src/array_indexing.rs +++ b/src/array_indexing.rs @@ -69,34 +69,26 @@ impl LateLintPass for ArrayIndexing { if let Ok(ConstVal::Uint(const_index)) = const_index { if size <= const_index { utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "const index is out of bounds"); - utils::span_lint(cx, INDEXING_SLICING, e.span, "indexing may panic"); - } else { - // Index is within bounds - return; } + + return; } // Index is a constant range if let Some(range) = utils::unsugar_range(index) { let start = range.start.map(|start| - eval_const_expr_partial(cx.tcx, start, ExprTypeChecked, None)); + eval_const_expr_partial(cx.tcx, start, ExprTypeChecked, None)).map(|v| v.ok()); let end = range.end.map(|end| - eval_const_expr_partial(cx.tcx, end, ExprTypeChecked, None)); + eval_const_expr_partial(cx.tcx, end, ExprTypeChecked, None)).map(|v| v.ok()); if let Some((start, end)) = to_const_range(start, end, range.limits, size) { - if start >= size && end >= size { + if start >= size || end >= size { utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "range is out of bounds"); - utils::span_lint(cx, - INDEXING_SLICING, - e.span, - "slicing may panic"); - } else { - // Range is within bounds - return; } + return; } } } @@ -120,19 +112,19 @@ impl LateLintPass for ArrayIndexing { /// /// Note: we assume the start and the end of the range are unsigned, since array slicing /// works only on usize -fn to_const_range(start: Option>, - end: Option>, +fn to_const_range(start: Option>, + end: Option>, limits: RangeLimits, array_size: u64) -> Option<(u64, u64)> { let start = match start { - Some(Ok(ConstVal::Uint(x))) => x, + Some(Some(ConstVal::Uint(x))) => x, Some(_) => return None, None => 0, }; let end = match end { - Some(Ok(ConstVal::Uint(x))) => { + Some(Some(ConstVal::Uint(x))) => { if limits == RangeLimits::Closed { x } else { diff --git a/src/lib.rs b/src/lib.rs index 664819b97ecb..4b9d5d8c9c4a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2,7 +2,7 @@ #![feature(rustc_private, collections)] #![feature(iter_arith)] #![feature(custom_attribute)] -#![allow(unknown_lints)] +#![allow(indexing_slicing, shadow_reuse, unknown_lints)] // this only exists to allow the "dogfood" integration test to work #[allow(dead_code)] diff --git a/tests/compile-fail/array_indexing.rs b/tests/compile-fail/array_indexing.rs index bcd25f5bf06d..14f3448a9f67 100644 --- a/tests/compile-fail/array_indexing.rs +++ b/tests/compile-fail/array_indexing.rs @@ -9,20 +9,15 @@ fn main() { let x = [1,2,3,4]; x[0]; x[3]; - x[4]; //~ERROR: indexing may panic - //~^ ERROR: const index is out of bounds - x[1 << 3]; //~ERROR: indexing may panic - //~^ ERROR: const index is out of bounds - &x[1..5]; //~ERROR: slicing may panic - //~^ ERROR: range is out of bounds + x[4]; //~ERROR: const index is out of bounds + x[1 << 3]; //~ERROR: const index is out of bounds + &x[1..5]; //~ERROR: range is out of bounds &x[0..3]; - &x[0...4]; //~ERROR: slicing may panic - //~^ ERROR: range is out of bounds + &x[0...4]; //~ERROR: range is out of bounds &x[..]; &x[1..]; &x[..4]; - &x[..5]; //~ERROR: slicing may panic - //~^ ERROR: range is out of bounds + &x[..5]; //~ERROR: range is out of bounds let y = &x; y[0]; //~ERROR: indexing may panic