Merge branch 'pr-1945'

Author: Manishearth

CHANGELOG.md

@@ -1,6 +1,9 @@
 # Change Log
 All notable changes to this project will be documented in this file.
 
+## Trunk
+* New lint: [`mut_range_bound`]
+
 ## 0.0.164
 * Update to *rustc 1.22.0-nightly (6c476ce46 2017-09-25)*
 * New lint: [`int_plus_one`]
@@ -561,6 +564,7 @@ All notable changes to this project will be documented in this file.
 [`modulo_one`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#modulo_one
 [`mut_from_ref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mut_from_ref
 [`mut_mut`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mut_mut
+[`mut_range_bound`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mut_range_bound
 [`mutex_atomic`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mutex_atomic
 [`mutex_integer`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mutex_integer
 [`naive_bytecount`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#naive_bytecount

clippy_lints/src/lib.rs

@@ -449,6 +449,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         loops::FOR_LOOP_OVER_RESULT,
         loops::ITER_NEXT_LOOP,
         loops::MANUAL_MEMCPY,
+        loops::MUT_RANGE_BOUND,
         loops::NEEDLESS_RANGE_LOOP,
         loops::NEVER_LOOP,
         loops::REVERSE_RANGE_LOOP,

clippy_lints/src/loops.rs

@@ -2,16 +2,22 @@ use itertools::Itertools;
 use reexport::*;
 use rustc::hir::*;
 use rustc::hir::def::Def;
+use rustc::hir::def_id; 
 use rustc::hir::intravisit::{walk_block, walk_decl, walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor};
 use rustc::hir::map::Node::{NodeBlock, NodeExpr, NodeStmt};
 use rustc::lint::*;
 use rustc::middle::const_val::ConstVal;
 use rustc::middle::region;
+// use rustc::middle::region::CodeExtent;
+use rustc::middle::expr_use_visitor::*;
+use rustc::middle::mem_categorization::Categorization;
+use rustc::middle::mem_categorization::cmt;
 use rustc::ty::{self, Ty};
 use rustc::ty::subst::{Subst, Substs};
 use rustc_const_eval::ConstContext;
 use std::collections::{HashMap, HashSet};
 use syntax::ast;
+use syntax::codemap::Span;
 use utils::sugg;
 use utils::const_to_u64;
 
@@ -328,6 +334,14 @@ declare_lint! {
     "any loop that will always `break` or `return`"
 }
 
+/// TODO: add documentation
+
+declare_lint! {
+    pub MUT_RANGE_BOUND,
+    Warn,
+    "for loop over a range where one of the bounds is a mutable variable"
+}
+
 #[derive(Copy, Clone)]
 pub struct Pass;
 
@@ -348,7 +362,8 @@ impl LintPass for Pass {
             EMPTY_LOOP,
             WHILE_LET_ON_ITERATOR,
             FOR_KV_MAP,
-            NEVER_LOOP
+            NEVER_LOOP, 
+            MUT_RANGE_BOUND
         )
     }
 }
@@ -605,6 +620,7 @@ fn check_for_loop<'a, 'tcx>(
     check_for_loop_arg(cx, pat, arg, expr);
     check_for_loop_explicit_counter(cx, arg, body, expr);
     check_for_loop_over_map_kv(cx, pat, arg, body, expr);
+    check_for_mut_range_bound(cx, arg, body);
     detect_manual_memcpy(cx, pat, arg, body, expr);
 }
 
@@ -1294,6 +1310,102 @@ fn check_for_loop_over_map_kv<'a, 'tcx>(
     }
 }
 
+struct MutateDelegate {
+    node_id_low: Option<NodeId>,
+    node_id_high: Option<NodeId>,
+    span_low: Option<Span>,
+    span_high: Option<Span>,
+}
+
+impl<'tcx> Delegate<'tcx> for MutateDelegate {
+    fn consume(&mut self, _: NodeId, _: Span, _: cmt<'tcx>, _: ConsumeMode) {
+    }
+  
+    fn matched_pat(&mut self, _: &Pat, _: cmt<'tcx>, _: MatchMode) {
+    }
+
+    fn consume_pat(&mut self, _: &Pat, _: cmt<'tcx>, _: ConsumeMode) {
+    }
+
+    fn borrow(&mut self, _: NodeId, sp: Span, cmt: cmt<'tcx>, _: ty::Region, bk: ty::BorrowKind, _: LoanCause) {
+        if let ty::BorrowKind::MutBorrow = bk {
+            if let Categorization::Local(id) = cmt.cat {
+                if Some(id) == self.node_id_low {
+                    self.span_low = Some(sp)
+                }
+                if Some(id) == self.node_id_high {
+                    self.span_high = Some(sp)
+                }
+            }
+        }
+    }
+
+    fn mutate(&mut self, _: NodeId, sp: Span, cmt: cmt<'tcx>, _: MutateMode) {
+        if let Categorization::Local(id) = cmt.cat {
+            if Some(id) == self.node_id_low {
+                self.span_low = Some(sp)
+            }
+            if Some(id) == self.node_id_high {
+                self.span_high = Some(sp)
+            }
+        }
+    }
+
+    fn decl_without_init(&mut self, _: NodeId, _: Span) {
+    }
+}
+
+impl<'tcx> MutateDelegate {
+    fn mutation_span(&self) -> (Option<Span>, Option<Span>) {
+        (self.span_low, self.span_high)
+    }
+}
+
+fn check_for_mut_range_bound(cx: &LateContext, arg: &Expr, body: &Expr) {
+    if let Some(higher::Range { start: Some(start), end: Some(end), .. }) = higher::range(arg) {
+        let mut_ids = vec![check_for_mutability(cx, start), check_for_mutability(cx, end)];
+        if mut_ids[0].is_some() || mut_ids[1].is_some() {
+            let (span_low, span_high) = check_for_mutation(cx, body, &mut_ids);
+            mut_warn_with_span(cx, span_low);
+            mut_warn_with_span(cx, span_high);
+        }
+    }
+}
+
+fn mut_warn_with_span(cx: &LateContext, span: Option<Span>) {
+    if let Some(sp) = span {
+        span_lint(cx, MUT_RANGE_BOUND, sp, "attempt to mutate range bound within loop; note that the range of the loop is unchanged");
+    }
+}
+
+fn check_for_mutability(cx: &LateContext, bound: &Expr) -> Option<NodeId> {
+    if_let_chain! {[
+        let ExprPath(ref qpath) = bound.node,
+        let QPath::Resolved(None, _) = *qpath,
+    ], {
+        let def = cx.tables.qpath_def(qpath, bound.hir_id);
+        if let Def::Local(node_id) = def {
+            let node_str = cx.tcx.hir.get(node_id);
+            if_let_chain! {[
+                let map::Node::NodeBinding(pat) = node_str,
+                let PatKind::Binding(bind_ann, _, _, _) = pat.node,
+                let BindingAnnotation::Mutable = bind_ann,
+            ], {
+                return Some(node_id);
+            }}
+        }
+    }}
+    None
+}
+
+fn check_for_mutation(cx: &LateContext, body: &Expr, bound_ids: &[Option<NodeId>]) -> (Option<Span>, Option<Span>) {
+    let mut delegate = MutateDelegate { node_id_low: bound_ids[0], node_id_high: bound_ids[1], span_low: None, span_high: None };
+    let def_id = def_id::DefId::local(body.hir_id.owner);
+    let region_scope_tree = &cx.tcx.region_scope_tree(def_id);
+    ExprUseVisitor::new(&mut delegate, cx.tcx, cx.param_env, region_scope_tree, cx.tables).walk_expr(body);
+    delegate.mutation_span()
+}
+
 /// Return true if the pattern is a `PatWild` or an ident prefixed with `'_'`.
 fn pat_is_wild<'tcx>(pat: &'tcx PatKind, body: &'tcx Expr) -> bool {
     match *pat {

mut_range_bound

@@ -0,0 +1,56 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![allow(unused)]
+
+fn main() {
+    mut_range_bound_upper();
+    mut_range_bound_lower();
+    mut_range_bound_both();
+    mut_range_bound_no_mutation();
+    immut_range_bound();
+    mut_borrow_range_bound();
+    immut_borrow_range_bound();
+}
+
+fn mut_range_bound_upper() {
+    let mut m = 4;
+    for i in 0..m { m = 5; } // warning
+}
+
+fn mut_range_bound_lower() {
+    let mut m = 4;
+    for i in m..10 { m *= 2; } // warning
+}
+
+fn mut_range_bound_both() {
+    let mut m = 4;
+    let mut n = 6;
+    for i in m..n { m = 5; n = 7; } // warning (1 for each mutated bound)
+}
+
+fn mut_range_bound_no_mutation() {
+    let mut m = 4;
+    for i in 0..m { continue; } // no warning
+}
+
+fn mut_borrow_range_bound() {
+    let mut m = 4;
+    for i in 0..m {
+        let n = &mut m;  // warning
+        *n += 1;
+    }
+}
+
+fn immut_borrow_range_bound() {
+    let mut m = 4;
+    for i in 0..m {
+        let n = &m;   // should be no warning?
+    }
+}
+
+
+fn immut_range_bound() {
+    let m = 4;
+    for i in 0..m { continue; } // no warning
+}

tests/ui/mut_range_bound.rs

@@ -0,0 +1,34 @@
+error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
+  --> $DIR/mut_range_bound.rs:18:21
+   |
+18 |     for i in 0..m { m = 5; } // warning
+   |                     ^^^^^
+   |
+   = note: `-D mut-range-bound` implied by `-D warnings`
+
+error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
+  --> $DIR/mut_range_bound.rs:23:22
+   |
+23 |     for i in m..10 { m *= 2; } // warning
+   |                      ^^^^^^
+
+error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
+  --> $DIR/mut_range_bound.rs:29:21
+   |
+29 |     for i in m..n { m = 5; n = 7; } // warning (1 for each mutated bound)
+   |                     ^^^^^
+
+error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
+  --> $DIR/mut_range_bound.rs:29:28
+   |
+29 |     for i in m..n { m = 5; n = 7; } // warning (1 for each mutated bound)
+   |                            ^^^^^
+
+error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
+  --> $DIR/mut_range_bound.rs:40:22
+   |
+40 |         let n = &mut m;  // warning
+   |                      ^
+
+error: aborting due to 5 previous errors
+