diff options
| -rw-r--r-- | clippy_lints/src/loops/infinite_loop.rs | 138 | ||||
| -rw-r--r-- | clippy_lints/src/loops/mod.rs | 137 |
2 files changed, 143 insertions, 132 deletions
diff --git a/clippy_lints/src/loops/infinite_loop.rs b/clippy_lints/src/loops/infinite_loop.rs new file mode 100644 index 00000000000..b89942fb647 --- /dev/null +++ b/clippy_lints/src/loops/infinite_loop.rs @@ -0,0 +1,138 @@ +use crate::consts::constant; +use crate::utils::span_lint_and_then; +use crate::utils::usage::mutated_variables; +use if_chain::if_chain; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_hir::def::{DefKind, Res}; +use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use rustc_hir::{def_id, Expr, ExprKind, HirId, QPath}; +use rustc_lint::LateContext; +use rustc_middle::hir::map::Map; +use std::iter::Iterator; + +pub(super) fn check_infinite_loop<'tcx>(cx: &LateContext<'tcx>, cond: &'tcx Expr<'_>, expr: &'tcx Expr<'_>) { + if constant(cx, cx.typeck_results(), cond).is_some() { + // A pure constant condition (e.g., `while false`) is not linted. + return; + } + + let mut var_visitor = VarCollectorVisitor { + cx, + ids: FxHashSet::default(), + def_ids: FxHashMap::default(), + skip: false, + }; + var_visitor.visit_expr(cond); + if var_visitor.skip { + return; + } + let used_in_condition = &var_visitor.ids; + let no_cond_variable_mutated = if let Some(used_mutably) = mutated_variables(expr, cx) { + used_in_condition.is_disjoint(&used_mutably) + } else { + return; + }; + let mutable_static_in_cond = var_visitor.def_ids.iter().any(|(_, v)| *v); + + let mut has_break_or_return_visitor = HasBreakOrReturnVisitor { + has_break_or_return: false, + }; + has_break_or_return_visitor.visit_expr(expr); + let has_break_or_return = has_break_or_return_visitor.has_break_or_return; + + if no_cond_variable_mutated && !mutable_static_in_cond { + span_lint_and_then( + cx, + super::WHILE_IMMUTABLE_CONDITION, + cond.span, + "variables in the condition are not mutated in the loop body", + |diag| { + diag.note("this may lead to an infinite or to a never running loop"); + + if has_break_or_return { + diag.note("this loop contains `return`s or `break`s"); + diag.help("rewrite it as `if cond { loop { } }`"); + } + }, + ); + } +} + +struct HasBreakOrReturnVisitor { + has_break_or_return: bool, +} + +impl<'tcx> Visitor<'tcx> for HasBreakOrReturnVisitor { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if self.has_break_or_return { + return; + } + + match expr.kind { + ExprKind::Ret(_) | ExprKind::Break(_, _) => { + self.has_break_or_return = true; + return; + }, + _ => {}, + } + + walk_expr(self, expr); + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { + NestedVisitorMap::None + } +} + +/// Collects the set of variables in an expression +/// Stops analysis if a function call is found +/// Note: In some cases such as `self`, there are no mutable annotation, +/// All variables definition IDs are collected +struct VarCollectorVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + ids: FxHashSet<HirId>, + def_ids: FxHashMap<def_id::DefId, bool>, + skip: bool, +} + +impl<'a, 'tcx> VarCollectorVisitor<'a, 'tcx> { + fn insert_def_id(&mut self, ex: &'tcx Expr<'_>) { + if_chain! { + if let ExprKind::Path(ref qpath) = ex.kind; + if let QPath::Resolved(None, _) = *qpath; + let res = self.cx.qpath_res(qpath, ex.hir_id); + then { + match res { + Res::Local(hir_id) => { + self.ids.insert(hir_id); + }, + Res::Def(DefKind::Static, def_id) => { + let mutable = self.cx.tcx.is_mutable_static(def_id); + self.def_ids.insert(def_id, mutable); + }, + _ => {}, + } + } + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, ex: &'tcx Expr<'_>) { + match ex.kind { + ExprKind::Path(_) => self.insert_def_id(ex), + // If there is any function/method call… we just stop analysis + ExprKind::Call(..) | ExprKind::MethodCall(..) => self.skip = true, + + _ => walk_expr(self, ex), + } + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { + NestedVisitorMap::None + } +} diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index d434762bc22..6adfbb6b955 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -4,10 +4,10 @@ mod for_loop_over_map_kv; mod for_loop_range; mod for_mut_range_bound; mod for_single_element_loop; +mod infinite_loop; mod manual_flatten; mod utils; -use crate::consts::constant; use crate::utils::sugg::Sugg; use crate::utils::usage::mutated_variables; use crate::utils::{ @@ -18,13 +18,13 @@ use crate::utils::{ }; use if_chain::if_chain; use rustc_ast::ast; -use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::fx::FxHashMap; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; use rustc_hir::intravisit::{walk_block, walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor}; use rustc_hir::{ - def_id, BinOpKind, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, GenericArg, HirId, InlineAsmOperand, - Local, LoopSource, MatchSource, Mutability, Node, Pat, PatKind, QPath, Stmt, StmtKind, + BinOpKind, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, GenericArg, HirId, InlineAsmOperand, Local, + LoopSource, MatchSource, Mutability, Node, Pat, PatKind, QPath, Stmt, StmtKind, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; @@ -683,7 +683,7 @@ impl<'tcx> LateLintPass<'tcx> for Loops { } if let Some((cond, body)) = higher::while_loop(&expr) { - check_infinite_loop(cx, cond, body); + infinite_loop::check_infinite_loop(cx, cond, body); } check_needless_collect(expr, cx); @@ -1895,133 +1895,6 @@ impl<'tcx> Visitor<'tcx> for LoopNestVisitor { } } -fn check_infinite_loop<'tcx>(cx: &LateContext<'tcx>, cond: &'tcx Expr<'_>, expr: &'tcx Expr<'_>) { - if constant(cx, cx.typeck_results(), cond).is_some() { - // A pure constant condition (e.g., `while false`) is not linted. - return; - } - - let mut var_visitor = VarCollectorVisitor { - cx, - ids: FxHashSet::default(), - def_ids: FxHashMap::default(), - skip: false, - }; - var_visitor.visit_expr(cond); - if var_visitor.skip { - return; - } - let used_in_condition = &var_visitor.ids; - let no_cond_variable_mutated = if let Some(used_mutably) = mutated_variables(expr, cx) { - used_in_condition.is_disjoint(&used_mutably) - } else { - return; - }; - let mutable_static_in_cond = var_visitor.def_ids.iter().any(|(_, v)| *v); - - let mut has_break_or_return_visitor = HasBreakOrReturnVisitor { - has_break_or_return: false, - }; - has_break_or_return_visitor.visit_expr(expr); - let has_break_or_return = has_break_or_return_visitor.has_break_or_return; - - if no_cond_variable_mutated && !mutable_static_in_cond { - span_lint_and_then( - cx, - WHILE_IMMUTABLE_CONDITION, - cond.span, - "variables in the condition are not mutated in the loop body", - |diag| { - diag.note("this may lead to an infinite or to a never running loop"); - - if has_break_or_return { - diag.note("this loop contains `return`s or `break`s"); - diag.help("rewrite it as `if cond { loop { } }`"); - } - }, - ); - } -} - -struct HasBreakOrReturnVisitor { - has_break_or_return: bool, -} - -impl<'tcx> Visitor<'tcx> for HasBreakOrReturnVisitor { - type Map = Map<'tcx>; - - fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - if self.has_break_or_return { - return; - } - - match expr.kind { - ExprKind::Ret(_) | ExprKind::Break(_, _) => { - self.has_break_or_return = true; - return; - }, - _ => {}, - } - - walk_expr(self, expr); - } - - fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { - NestedVisitorMap::None - } -} - -/// Collects the set of variables in an expression -/// Stops analysis if a function call is found -/// Note: In some cases such as `self`, there are no mutable annotation, -/// All variables definition IDs are collected -struct VarCollectorVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - ids: FxHashSet<HirId>, - def_ids: FxHashMap<def_id::DefId, bool>, - skip: bool, -} - -impl<'a, 'tcx> VarCollectorVisitor<'a, 'tcx> { - fn insert_def_id(&mut self, ex: &'tcx Expr<'_>) { - if_chain! { - if let ExprKind::Path(ref qpath) = ex.kind; - if let QPath::Resolved(None, _) = *qpath; - let res = self.cx.qpath_res(qpath, ex.hir_id); - then { - match res { - Res::Local(hir_id) => { - self.ids.insert(hir_id); - }, - Res::Def(DefKind::Static, def_id) => { - let mutable = self.cx.tcx.is_mutable_static(def_id); - self.def_ids.insert(def_id, mutable); - }, - _ => {}, - } - } - } - } -} - -impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> { - type Map = Map<'tcx>; - - fn visit_expr(&mut self, ex: &'tcx Expr<'_>) { - match ex.kind { - ExprKind::Path(_) => self.insert_def_id(ex), - // If there is any function/method call… we just stop analysis - ExprKind::Call(..) | ExprKind::MethodCall(..) => self.skip = true, - - _ => walk_expr(self, ex), - } - } - - fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { - NestedVisitorMap::None - } -} - const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed"; fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { |