Merge #3339

3339: Check for known array length in `needless_range_loop` r=phansch a=HMPerson1

In `VarVisitor`, we now keep track of the type of the thing that was directly indexed and, if it's an array, check if the range's end is (or is past) the array's length.

Fixes  #3033

Co-authored-by: HMPerson1 <hmperson1@gmail.com>

clippy_lints/src/loops.rs

@@ -1068,7 +1068,7 @@ fn check_for_loop_range<'a, 'tcx>(
 
             // linting condition: we only indexed one variable, and indexed it directly
             if visitor.indexed_indirectly.is_empty() && visitor.indexed_directly.len() == 1 {
-                let (indexed, indexed_extent) = visitor
+                let (indexed, (indexed_extent, indexed_ty)) = visitor
                     .indexed_directly
                     .into_iter()
                     .next()
@@ -1119,7 +1119,7 @@ fn check_for_loop_range<'a, 'tcx>(
                         }
                     }
 
-                    if is_len_call(end, indexed) {
+                    if is_len_call(end, indexed) || is_end_eq_array_len(cx, end, limits, indexed_ty) {
                         String::new()
                     } else {
                         match limits {
@@ -1207,6 +1207,28 @@ fn is_len_call(expr: &Expr, var: Name) -> bool {
     false
 }
 
+fn is_end_eq_array_len(
+    cx: &LateContext<'_, '_>,
+    end: &Expr,
+    limits: ast::RangeLimits,
+    indexed_ty: Ty<'_>,
+) -> bool {
+    if_chain! {
+        if let ExprKind::Lit(ref lit) = end.node;
+        if let ast::LitKind::Int(end_int, _) = lit.node;
+        if let ty::TyKind::Array(_, arr_len_const) = indexed_ty.sty;
+        if let Some(arr_len) = arr_len_const.assert_usize(cx.tcx);
+        then {
+            return match limits {
+                ast::RangeLimits::Closed => end_int + 1 >= arr_len.into(),
+                ast::RangeLimits::HalfOpen => end_int >= arr_len.into(),
+            };
+        }
+    }
+
+    false
+}
+
 fn check_for_loop_reverse_range<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arg: &'tcx Expr, expr: &'tcx Expr) {
     // if this for loop is iterating over a two-sided range...
     if let Some(higher::Range {
@@ -1678,7 +1700,7 @@ struct VarVisitor<'a, 'tcx: 'a> {
     indexed_indirectly: FxHashMap<Name, Option<region::Scope>>,
     /// subset of `indexed` of vars that are indexed directly: `v[i]`
     /// this will not contain cases like `v[calc_index(i)]` or `v[(i + 4) % N]`
-    indexed_directly: FxHashMap<Name, Option<region::Scope>>,
+    indexed_directly: FxHashMap<Name, (Option<region::Scope>, Ty<'tcx>)>,
     /// Any names that are used outside an index operation.
     /// Used to detect things like `&mut vec` used together with `vec[i]`
     referenced: FxHashSet<Name>,
@@ -1725,7 +1747,10 @@ impl<'a, 'tcx> VarVisitor<'a, 'tcx> {
                                 self.indexed_indirectly.insert(seqvar.segments[0].ident.name, Some(extent));
                             }
                             if index_used_directly {
-                                self.indexed_directly.insert(seqvar.segments[0].ident.name, Some(extent));
+                                self.indexed_directly.insert(
+                                    seqvar.segments[0].ident.name,
+                                    (Some(extent), self.cx.tables.node_id_to_type(seqexpr.hir_id)),
+                                );
                             }
                             return false;  // no need to walk further *on the variable*
                         }
@@ -1734,7 +1759,10 @@ impl<'a, 'tcx> VarVisitor<'a, 'tcx> {
                                 self.indexed_indirectly.insert(seqvar.segments[0].ident.name, None);
                             }
                             if index_used_directly {
-                                self.indexed_directly.insert(seqvar.segments[0].ident.name, None);
+                                self.indexed_directly.insert(
+                                    seqvar.segments[0].ident.name,
+                                    (None, self.cx.tables.node_id_to_type(seqexpr.hir_id)),
+                                );
                             }
                             return false;  // no need to walk further *on the variable*
                         }

tests/ui/for_loop.stderr

@@ -97,8 +97,8 @@ error: the loop variable `j` is only used to index `STATIC`.
     |              ^^^^
 help: consider using an iterator
     |
-110 |     for <item> in STATIC.iter().take(4) {
-    |         ^^^^^^    ^^^^^^^^^^^^^^^^^^^^^
+110 |     for <item> in &STATIC {
+    |         ^^^^^^    ^^^^^^^
 
 error: the loop variable `j` is only used to index `CONST`.
    --> $DIR/for_loop.rs:114:14
@@ -107,8 +107,8 @@ error: the loop variable `j` is only used to index `CONST`.
     |              ^^^^
 help: consider using an iterator
     |
-114 |     for <item> in CONST.iter().take(4) {
-    |         ^^^^^^    ^^^^^^^^^^^^^^^^^^^^
+114 |     for <item> in &CONST {
+    |         ^^^^^^    ^^^^^^
 
 error: the loop variable `i` is used to index `vec`
    --> $DIR/for_loop.rs:118:14

tests/ui/needless_range_loop.rs

@@ -13,7 +13,7 @@ fn calc_idx(i: usize) -> usize {
 }
 
 fn main() {
-    let ns = [2, 3, 5, 7];
+    let ns = vec![2, 3, 5, 7];
 
     for i in 3..10 {
         println!("{}", ns[i]);
@@ -76,4 +76,18 @@ fn main() {
     for i in x..=x + 4 {
         vec[i] += 1;
     }
+
+    let arr = [1,2,3];
+
+    for i in 0..3 {
+        println!("{}", arr[i]);
+    }
+
+    for i in 0..2 {
+        println!("{}", arr[i]);
+    }
+
+    for i in 1..3 {
+        println!("{}", arr[i]);
+    }
 }

tests/ui/needless_range_loop.stderr

@@ -50,5 +50,35 @@ help: consider using an iterator
 76 |     for <item> in vec.iter_mut().skip(x).take(4 + 1) {
    |         ^^^^^^    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 5 previous errors
+error: the loop variable `i` is only used to index `arr`.
+  --> $DIR/needless_range_loop.rs:82:14
+   |
+82 |     for i in 0..3 {
+   |              ^^^^
+help: consider using an iterator
+   |
+82 |     for <item> in &arr {
+   |         ^^^^^^    ^^^^
+
+error: the loop variable `i` is only used to index `arr`.
+  --> $DIR/needless_range_loop.rs:86:14
+   |
+86 |     for i in 0..2 {
+   |              ^^^^
+help: consider using an iterator
+   |
+86 |     for <item> in arr.iter().take(2) {
+   |         ^^^^^^    ^^^^^^^^^^^^^^^^^^
+
+error: the loop variable `i` is only used to index `arr`.
+  --> $DIR/needless_range_loop.rs:90:14
+   |
+90 |     for i in 1..3 {
+   |              ^^^^
+help: consider using an iterator
+   |
+90 |     for <item> in arr.iter().skip(1) {
+   |         ^^^^^^    ^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 8 previous errors