Merge branch 'aochagavia-indexing_slicing'

Author: mcarton

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
@@ -234,7 +235,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.

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,35 +18,122 @@ use utils::span_lint;
 /// let x = [1,2,3,4];
 /// ...
 /// x[9];
+/// &x[2..9];
 /// ```
 declare_lint! {
     pub OUT_OF_BOUNDS_INDEXING,
     Deny,
     "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");
+                    }
+
+                    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)).map(|v| v.ok());
+                    let end = range.end.map(|end|
+                        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 {
+                            utils::span_lint(cx,
+                                             OUT_OF_BOUNDS_INDEXING,
+                                             e.span,
+                                             "range is out of 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<Option<ConstVal>>,
+                    end: Option<Option<ConstVal>>,
+                    limits: RangeLimits,
+                    array_size: u64)
+                    -> Option<(u64, u64)> {
+    let start = match start {
+        Some(Some(ConstVal::Uint(x))) => x,
+        Some(_) => return None,
+        None => 0,
+    };
+
+    let end = match end {
+        Some(Some(ConstVal::Uint(x))) => {
+            if limits == RangeLimits::Closed {
+                x
+            } else {
+                x - 1
+            }
+        }
+        Some(_) => return None,
+        None => array_size - 1,
+    };
+
+    Some((start, end))
+}

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)]
@@ -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,

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<UnsugaredRange> {
         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,
     }
 }
 

tests/compile-fail/array_indexing.rs

@@ -1,13 +1,27 @@
-#![feature(plugin)]
+#![feature(inclusive_range_syntax, plugin)]
 #![plugin(clippy)]
 
+#![deny(indexing_slicing)]
 #![deny(out_of_bounds_indexing)]
 #![allow(no_effect)]
 
 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: 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: range is out of bounds
+    &x[..];
+    &x[1..];
+    &x[..4];
+    &x[..5]; //~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
 }