diff --git a/CHANGELOG.md b/CHANGELOG.md index d3c8071f369..c95d96c2c3f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2d600277e83..b840c3ddd02 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 42b31f8eaaa..a0300530a59 100644 --- a/clippy_lints/src/loops.rs +++ b/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, + node_id_high: Option, + span_low: Option, + span_high: Option, +} + +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, Option) { + (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) { + 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 { + 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]) -> (Option, Option) { + 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 { diff --git a/mut_range_bound b/mut_range_bound new file mode 100755 index 00000000000..fdf917d5158 Binary files /dev/null and b/mut_range_bound differ diff --git a/tests/ui/mut_range_bound.rs b/tests/ui/mut_range_bound.rs new file mode 100644 index 00000000000..835ceeedc94 --- /dev/null +++ b/tests/ui/mut_range_bound.rs @@ -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 +} diff --git a/tests/ui/mut_range_bound.stderr b/tests/ui/mut_range_bound.stderr new file mode 100644 index 00000000000..d7be7ae1e6f --- /dev/null +++ b/tests/ui/mut_range_bound.stderr @@ -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 +