From 3b7f3dc8e7124869c9d2399c37f5d59ddfe4e0ef Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Tue, 1 Aug 2017 00:58:26 +0200 Subject: [PATCH] WIP: Find binding or assignment within outer loop --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/loops.rs | 147 +++++++++++++++++++++++++++++++------ tests/ui/while_loop.rs | 8 +- tests/ui/while_loop.stderr | 9 ++- 4 files changed, 140 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0c1c14f5b8c..18759fb0a91 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -253,7 +253,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box shadow::Pass); reg.register_late_lint_pass(box types::LetPass); reg.register_late_lint_pass(box types::UnitCmp); - reg.register_late_lint_pass(box loops::Pass::default()); + reg.register_late_lint_pass(box loops::Pass); reg.register_late_lint_pass(box lifetimes::LifetimePass); reg.register_late_lint_pass(box entry::HashMapLint); reg.register_late_lint_pass(box ranges::StepByZero); diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 8b31969d254..b4501315cd3 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -304,10 +304,8 @@ declare_lint! { "any loop that will always `break` or `return`" } -#[derive(Copy, Clone, Default)] -pub struct Pass { - loop_count: usize, -} +#[derive(Copy, Clone)] +pub struct Pass; impl LintPass for Pass { fn get_lints(&self) -> LintArray { @@ -329,15 +327,6 @@ impl LintPass for Pass { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { - fn check_expr_post(&mut self, _: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { - match expr.node { - ExprWhile(..) | ExprLoop(..) => { - self.loop_count -= 1; - }, - _ => (), - } - } - fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { if let Some((pat, arg, body)) = higher::for_loop(expr) { check_for_loop(cx, pat, arg, body, expr); @@ -347,7 +336,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { match expr.node { ExprWhile(_, ref block, _) | ExprLoop(ref block, _, _) => { - self.loop_count += 1; if never_loop(block, &expr.id) { span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops"); } @@ -410,10 +398,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { &ExprMethodCall(ref method_path, _, ref method_args)) = (pat, &match_expr.node) { let iter_expr = &method_args[0]; let lhs_constructor = last_path_segment(qpath); - if self.loop_count < 2 && method_path.name == "next" && - match_trait_method(cx, match_expr, &paths::ITERATOR) && + if method_path.name == "next" && match_trait_method(cx, match_expr, &paths::ITERATOR) && lhs_constructor.name == "Some" && !is_refutable(cx, &pat_args[0]) && - !is_iterator_used_after_while_let(cx, iter_expr) { + !is_iterator_used_after_while_let(cx, iter_expr) && !is_nested(cx, expr, &method_args[0]) { let iterator = snippet(cx, method_args[0].span, "_"); let loop_var = snippet(cx, pat_args[0].span, "_"); span_lint_and_sugg(cx, @@ -939,6 +926,15 @@ fn pat_is_wild<'tcx>(pat: &'tcx PatKind, body: &'tcx Expr) -> bool { } } +fn match_var(expr: &Expr, var: Name) -> bool { + if let ExprPath(QPath::Resolved(None, ref path)) = expr.node { + if path.segments.len() == 1 && path.segments[0].name == var { + return true + } + } + false +} + struct UsedVisitor { var: ast::Name, // var to look for used: bool, // has the var been used otherwise? @@ -946,15 +942,13 @@ struct UsedVisitor { impl<'tcx> Visitor<'tcx> for UsedVisitor { fn visit_expr(&mut self, expr: &'tcx Expr) { - if let ExprPath(QPath::Resolved(None, ref path)) = expr.node { - if path.segments.len() == 1 && path.segments[0].name == self.var { - self.used = true; - return; - } + if match_var(expr, self.var) { + self.used = true; + return; } - walk_expr(self, expr); } + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { NestedVisitorMap::None } @@ -1328,3 +1322,110 @@ fn is_conditional(expr: &Expr) -> bool { _ => false, } } + +fn is_nested(cx: &LateContext, match_expr: &Expr, iter_expr: &Expr) -> bool { + if_let_chain! {[ + let Some(loop_block) = get_enclosing_block(cx, match_expr.id), + let Some(map::Node::NodeExpr(loop_expr)) = cx.tcx.hir.find(cx.tcx.hir.get_parent_node(loop_block.id)), + let Some(scope) = get_enclosing_block(cx, loop_expr.id) + ], { + return is_loop_nested(cx, scope, loop_expr.id, iter_expr) + }} + false +} + +fn is_loop_nested(cx: &LateContext, scope: &Block, expr_id: NodeId, iter_expr: &Expr) -> bool { + let mut b = scope; + let mut e = expr_id; + if let Some(name) = path_name(iter_expr) { + loop { + if b.stmts.iter().take_while(|stmt| !is_expr_stmt(stmt, e)).any(|stmt| + is_binding_or_assignment(stmt, name)) { + return false; + } + if let Some(map::Node::NodeExpr(outer)) = cx.tcx.hir.find(cx.tcx.hir.get_parent_node(scope.id)) { + if let ExprLoop(..) = outer.node { + return true; + } + e = outer.id; + if let Some(eb) = get_enclosing_block(cx, e) { + b = eb; + } else { + return false; + } + } else { + return false; + } + } + } + true +} + +fn path_name(e: &Expr) -> Option { + if let ExprPath(QPath::Resolved(_, ref path)) = e.node { + let segments = &path.segments; + if segments.len() == 1 { + return Some(segments[0].name); + } + }; + None +} + +fn is_binding_or_assignment(stmt: &Stmt, name: Name) -> bool { + match stmt.node { + StmtExpr(ref e, _) | StmtSemi(ref e, _) => contains_assignment(e, name), + StmtDecl(ref decl, _) => is_binding(decl, name) + } +} + +struct AssignmentVisitor { + var: ast::Name, // var to look for + assigned: bool, // has the var been assigned? +} + +impl<'tcx> Visitor<'tcx> for AssignmentVisitor { + fn visit_expr(&mut self, expr: &'tcx Expr) { + match expr.node { + ExprAssign(ref path, _) | + ExprAssignOp(_, ref path, _) => if match_var(path, self.var) { + self.assigned = true; + } + ExprLoop(..) | + ExprIf(..) | + ExprWhile(..) => (), + _ => walk_expr(self, expr) + } + } + + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } +} + +fn contains_assignment(e: &Expr, name: Name) -> bool { + let mut av = AssignmentVisitor { var: name, assigned: false }; + walk_expr(&mut av, e); + av.assigned +} + +fn is_binding(decl: &Decl, name: Name) -> bool { + match decl.node { + DeclLocal(ref local) => { + !local.pat.walk(&mut |p: &Pat| { + if let PatKind::Binding(_, _, span_name, _) = p.node { + name == span_name.node + } else { + false + } + }) + }, + _ => false + } +} + +fn is_expr_stmt(stmt: &Stmt, expr_id: NodeId) -> bool { + match stmt.node { + StmtExpr(ref e, _) | StmtSemi(ref e, _) => e.id == expr_id, + _ => false + } +} diff --git a/tests/ui/while_loop.rs b/tests/ui/while_loop.rs index 58df3ba9dcb..84873582609 100644 --- a/tests/ui/while_loop.rs +++ b/tests/ui/while_loop.rs @@ -174,7 +174,13 @@ fn refutable() { let mut y = a.iter(); for _ in 0..2 { - while let Some(v) = y.next() { + while let Some(v) = y.next() { // y is reused, don't lint + } + } + + loop { + let mut y = a.iter(); + while let Some(v) = y.next() { // use a for loop here } } } diff --git a/tests/ui/while_loop.stderr b/tests/ui/while_loop.stderr index 7abdefe881b..c31fff3b19e 100644 --- a/tests/ui/while_loop.stderr +++ b/tests/ui/while_loop.stderr @@ -103,5 +103,12 @@ error: empty `loop {}` detected. You may want to either use `panic!()` or add `s | = note: `-D empty-loop` implied by `-D warnings` -error: aborting due to 10 previous errors +error: this loop could be written as a `for` loop + --> while_loop.rs:177:9 + | +177 | / while let Some(v) = y.next() { +178 | | } + | |_________^ help: try: `for v in y { .. }` + +error: aborting due to 11 previous errors