-use super::utils::{LoopNestVisitor, Nesting};
use super::WHILE_LET_ON_ITERATOR;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::ty::implements_trait;
-use clippy_utils::usage::mutated_variables;
use clippy_utils::{
- get_enclosing_block, is_refutable, is_trait_method, last_path_segment, path_to_local, path_to_local_id,
+ get_enclosing_loop_or_closure, is_refutable, is_trait_method, match_def_path, paths, visitors::is_res_used,
};
use if_chain::if_chain;
use rustc_errors::Applicability;
-use rustc_hir::intravisit::{walk_block, walk_expr, NestedVisitorMap, Visitor};
-use rustc_hir::{Expr, ExprKind, HirId, MatchSource, Node, PatKind};
+use rustc_hir::intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor};
+use rustc_hir::{def::Res, Expr, ExprKind, HirId, Local, MatchSource, Mutability, Node, PatKind, QPath, UnOp};
use rustc_lint::LateContext;
-use rustc_middle::hir::map::Map;
-use rustc_span::symbol::sym;
+use rustc_span::{symbol::sym, Span, Symbol};
pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
- if let ExprKind::Match(ref match_expr, ref arms, MatchSource::WhileLetDesugar) = expr.kind {
- let pat = &arms[0].pat.kind;
- if let (
- &PatKind::TupleStruct(ref qpath, ref pat_args, _),
- &ExprKind::MethodCall(ref method_path, _, ref method_args, _),
- ) = (pat, &match_expr.kind)
- {
- let iter_expr = &method_args[0];
-
- // Don't lint when the iterator is recreated on every iteration
- if_chain! {
- if let ExprKind::MethodCall(..) | ExprKind::Call(..) = iter_expr.kind;
- if let Some(iter_def_id) = cx.tcx.get_diagnostic_item(sym::Iterator);
- if implements_trait(cx, cx.typeck_results().expr_ty(iter_expr), iter_def_id, &[]);
- then {
- return;
- }
- }
+ let (scrutinee_expr, iter_expr, some_pat, loop_expr) = if_chain! {
+ if let ExprKind::Match(scrutinee_expr, [arm, _], MatchSource::WhileLetDesugar) = expr.kind;
+ // check for `Some(..)` pattern
+ if let PatKind::TupleStruct(QPath::Resolved(None, pat_path), some_pat, _) = arm.pat.kind;
+ if let Res::Def(_, pat_did) = pat_path.res;
+ if match_def_path(cx, pat_did, &paths::OPTION_SOME);
+ // check for call to `Iterator::next`
+ if let ExprKind::MethodCall(method_name, _, [iter_expr], _) = scrutinee_expr.kind;
+ if method_name.ident.name == sym::next;
+ if is_trait_method(cx, scrutinee_expr, sym::Iterator);
+ if let Some(iter_expr) = try_parse_iter_expr(cx, iter_expr);
+ // get the loop containing the match expression
+ if let Some((_, Node::Expr(loop_expr))) = cx.tcx.hir().parent_iter(expr.hir_id).nth(1);
+ if !uses_iter(cx, &iter_expr, arm.body);
+ then {
+ (scrutinee_expr, iter_expr, some_pat, loop_expr)
+ } else {
+ return;
+ }
+ };
- let lhs_constructor = last_path_segment(qpath);
- if method_path.ident.name == sym::next
- && is_trait_method(cx, match_expr, sym::Iterator)
- && lhs_constructor.ident.name == sym::Some
- && (pat_args.is_empty()
- || !is_refutable(cx, &pat_args[0])
- && !is_used_inside(cx, iter_expr, &arms[0].body)
- && !is_iterator_used_after_while_let(cx, iter_expr)
- && !is_nested(cx, expr, &method_args[0]))
- {
- let mut applicability = Applicability::MachineApplicable;
- let iterator = snippet_with_applicability(cx, method_args[0].span, "_", &mut applicability);
- let loop_var = if pat_args.is_empty() {
- "_".to_string()
- } else {
- snippet_with_applicability(cx, pat_args[0].span, "_", &mut applicability).into_owned()
- };
- span_lint_and_sugg(
- cx,
- WHILE_LET_ON_ITERATOR,
- expr.span.with_hi(match_expr.span.hi()),
- "this loop could be written as a `for` loop",
- "try",
- format!("for {} in {}", loop_var, iterator),
- applicability,
- );
- }
+ let mut applicability = Applicability::MachineApplicable;
+ let loop_var = if let Some(some_pat) = some_pat.first() {
+ if is_refutable(cx, some_pat) {
+ // Refutable patterns don't work with for loops.
+ return;
}
- }
-}
+ snippet_with_applicability(cx, some_pat.span, "..", &mut applicability)
+ } else {
+ "_".into()
+ };
-fn is_used_inside<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, container: &'tcx Expr<'_>) -> bool {
- let def_id = match path_to_local(expr) {
- Some(id) => id,
- None => return false,
+ // If the iterator is a field or the iterator is accessed after the loop is complete it needs to be
+ // borrowed mutably. TODO: If the struct can be partially moved from and the struct isn't used
+ // afterwards a mutable borrow of a field isn't necessary.
+ let ref_mut = if !iter_expr.fields.is_empty() || needs_mutable_borrow(cx, &iter_expr, loop_expr) {
+ if cx.typeck_results().node_type(iter_expr.hir_id).ref_mutability() == Some(Mutability::Mut) {
+ // Reborrow for mutable references. It may not be possible to get a mutable reference here.
+ "&mut *"
+ } else {
+ "&mut "
+ }
+ } else {
+ ""
};
- if let Some(used_mutably) = mutated_variables(container, cx) {
- if used_mutably.contains(&def_id) {
- return true;
+
+ let iterator = snippet_with_applicability(cx, iter_expr.span, "_", &mut applicability);
+ span_lint_and_sugg(
+ cx,
+ WHILE_LET_ON_ITERATOR,
+ expr.span.with_hi(scrutinee_expr.span.hi()),
+ "this loop could be written as a `for` loop",
+ "try",
+ format!("for {} in {}{}", loop_var, ref_mut, iterator),
+ applicability,
+ );
+}
+
+#[derive(Debug)]
+struct IterExpr {
+ /// The span of the whole expression, not just the path and fields stored here.
+ span: Span,
+ /// The HIR id of the whole expression, not just the path and fields stored here.
+ hir_id: HirId,
+ /// The fields used, in order of child to parent.
+ fields: Vec<Symbol>,
+ /// The path being used.
+ path: Res,
+}
+/// Parses any expression to find out which field of which variable is used. Will return `None` if
+/// the expression might have side effects.
+fn try_parse_iter_expr(cx: &LateContext<'_>, mut e: &Expr<'_>) -> Option<IterExpr> {
+ let span = e.span;
+ let hir_id = e.hir_id;
+ let mut fields = Vec::new();
+ loop {
+ match e.kind {
+ ExprKind::Path(ref path) => {
+ break Some(IterExpr {
+ span,
+ hir_id,
+ fields,
+ path: cx.qpath_res(path, e.hir_id),
+ });
+ },
+ ExprKind::Field(base, name) => {
+ fields.push(name.name);
+ e = base;
+ },
+ // Dereferencing a pointer has no side effects and doesn't affect which field is being used.
+ ExprKind::Unary(UnOp::Deref, base) if cx.typeck_results().expr_ty(base).is_ref() => e = base,
+
+ // Shouldn't have side effects, but there's no way to trace which field is used. So forget which fields have
+ // already been seen.
+ ExprKind::Index(base, idx) if !idx.can_have_side_effects() => {
+ fields.clear();
+ e = base;
+ },
+ ExprKind::Unary(UnOp::Deref, base) => {
+ fields.clear();
+ e = base;
+ },
+
+ // No effect and doesn't affect which field is being used.
+ ExprKind::DropTemps(base) | ExprKind::AddrOf(_, _, base) | ExprKind::Type(base, _) => e = base,
+ _ => break None,
}
}
- false
}
-fn is_iterator_used_after_while_let<'tcx>(cx: &LateContext<'tcx>, iter_expr: &'tcx Expr<'_>) -> bool {
- let def_id = match path_to_local(iter_expr) {
- Some(id) => id,
- None => return false,
- };
- let mut visitor = VarUsedAfterLoopVisitor {
- def_id,
- iter_expr_id: iter_expr.hir_id,
- past_while_let: false,
- var_used_after_while_let: false,
- };
- if let Some(enclosing_block) = get_enclosing_block(cx, def_id) {
- walk_block(&mut visitor, enclosing_block);
+fn is_expr_same_field(cx: &LateContext<'_>, mut e: &Expr<'_>, mut fields: &[Symbol], path_res: Res) -> bool {
+ loop {
+ match (&e.kind, fields) {
+ (&ExprKind::Field(base, name), [head_field, tail_fields @ ..]) if name.name == *head_field => {
+ e = base;
+ fields = tail_fields;
+ },
+ (ExprKind::Path(path), []) => {
+ break cx.qpath_res(path, e.hir_id) == path_res;
+ },
+ (&(ExprKind::DropTemps(base) | ExprKind::AddrOf(_, _, base) | ExprKind::Type(base, _)), _) => e = base,
+ _ => break false,
+ }
}
- visitor.var_used_after_while_let
}
-fn is_nested(cx: &LateContext<'_>, match_expr: &Expr<'_>, iter_expr: &Expr<'_>) -> bool {
- if_chain! {
- if let Some(loop_block) = get_enclosing_block(cx, match_expr.hir_id);
- let parent_node = cx.tcx.hir().get_parent_node(loop_block.hir_id);
- if let Some(Node::Expr(loop_expr)) = cx.tcx.hir().find(parent_node);
- then {
- return is_loop_nested(cx, loop_expr, iter_expr)
- }
+/// Checks if the given expression is the same field as, is a child of, or is the parent of the
+/// given field. Used to check if the expression can be used while the given field is borrowed
+/// mutably. e.g. if checking for `x.y`, then `x.y`, `x.y.z`, and `x` will all return true, but
+/// `x.z`, and `y` will return false.
+fn is_expr_same_child_or_parent_field(cx: &LateContext<'_>, expr: &Expr<'_>, fields: &[Symbol], path_res: Res) -> bool {
+ match expr.kind {
+ ExprKind::Field(base, name) => {
+ if let Some((head_field, tail_fields)) = fields.split_first() {
+ if name.name == *head_field && is_expr_same_field(cx, base, fields, path_res) {
+ return true;
+ }
+ // Check if the expression is a parent field
+ let mut fields_iter = tail_fields.iter();
+ while let Some(field) = fields_iter.next() {
+ if *field == name.name && is_expr_same_field(cx, base, fields_iter.as_slice(), path_res) {
+ return true;
+ }
+ }
+ }
+
+ // Check if the expression is a child field.
+ let mut e = base;
+ loop {
+ match e.kind {
+ ExprKind::Field(..) if is_expr_same_field(cx, e, fields, path_res) => break true,
+ ExprKind::Field(base, _) | ExprKind::DropTemps(base) | ExprKind::Type(base, _) => e = base,
+ ExprKind::Path(ref path) if fields.is_empty() => {
+ break cx.qpath_res(path, e.hir_id) == path_res;
+ },
+ _ => break false,
+ }
+ }
+ },
+ // If the path matches, this is either an exact match, or the expression is a parent of the field.
+ ExprKind::Path(ref path) => cx.qpath_res(path, expr.hir_id) == path_res,
+ ExprKind::DropTemps(base) | ExprKind::Type(base, _) | ExprKind::AddrOf(_, _, base) => {
+ is_expr_same_child_or_parent_field(cx, base, fields, path_res)
+ },
+ _ => false,
}
- false
}
-fn is_loop_nested(cx: &LateContext<'_>, loop_expr: &Expr<'_>, iter_expr: &Expr<'_>) -> bool {
- let mut id = loop_expr.hir_id;
- let iter_id = if let Some(id) = path_to_local(iter_expr) {
- id
- } else {
- return true;
+/// Strips off all field and path expressions. This will return true if a field or path has been
+/// skipped. Used to skip them after failing to check for equality.
+fn skip_fields_and_path(expr: &'tcx Expr<'_>) -> (Option<&'tcx Expr<'tcx>>, bool) {
+ let mut e = expr;
+ let e = loop {
+ match e.kind {
+ ExprKind::Field(base, _) | ExprKind::DropTemps(base) | ExprKind::Type(base, _) => e = base,
+ ExprKind::Path(_) => return (None, true),
+ _ => break e,
+ }
};
- loop {
- let parent = cx.tcx.hir().get_parent_node(id);
- if parent == id {
- return false;
+ (Some(e), e.hir_id != expr.hir_id)
+}
+
+/// Checks if the given expression uses the iterator.
+fn uses_iter(cx: &LateContext<'tcx>, iter_expr: &IterExpr, container: &'tcx Expr<'_>) -> bool {
+ struct V<'a, 'b, 'tcx> {
+ cx: &'a LateContext<'tcx>,
+ iter_expr: &'b IterExpr,
+ uses_iter: bool,
+ }
+ impl Visitor<'tcx> for V<'_, '_, 'tcx> {
+ type Map = ErasedMap<'tcx>;
+ fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+ NestedVisitorMap::None
}
- match cx.tcx.hir().find(parent) {
- Some(Node::Expr(expr)) => {
- if let ExprKind::Loop(..) = expr.kind {
- return true;
- };
- },
- Some(Node::Block(block)) => {
- let mut block_visitor = LoopNestVisitor {
- hir_id: id,
- iterator: iter_id,
- nesting: Nesting::Unknown,
- };
- walk_block(&mut block_visitor, block);
- if block_visitor.nesting == Nesting::RuledOut {
- return false;
+
+ fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+ if self.uses_iter {
+ // return
+ } else if is_expr_same_child_or_parent_field(self.cx, e, &self.iter_expr.fields, self.iter_expr.path) {
+ self.uses_iter = true;
+ } else if let (e, true) = skip_fields_and_path(e) {
+ if let Some(e) = e {
+ self.visit_expr(e);
}
- },
- Some(Node::Stmt(_)) => (),
- _ => {
- return false;
- },
+ } else if let ExprKind::Closure(_, _, id, _, _) = e.kind {
+ if is_res_used(self.cx, self.iter_expr.path, id) {
+ self.uses_iter = true;
+ }
+ } else {
+ walk_expr(self, e);
+ }
}
- id = parent;
}
-}
-struct VarUsedAfterLoopVisitor {
- def_id: HirId,
- iter_expr_id: HirId,
- past_while_let: bool,
- var_used_after_while_let: bool,
+ let mut v = V {
+ cx,
+ iter_expr,
+ uses_iter: false,
+ };
+ v.visit_expr(container);
+ v.uses_iter
}
-impl<'tcx> Visitor<'tcx> for VarUsedAfterLoopVisitor {
- type Map = Map<'tcx>;
+#[allow(clippy::too_many_lines)]
+fn needs_mutable_borrow(cx: &LateContext<'tcx>, iter_expr: &IterExpr, loop_expr: &'tcx Expr<'_>) -> bool {
+ struct AfterLoopVisitor<'a, 'b, 'tcx> {
+ cx: &'a LateContext<'tcx>,
+ iter_expr: &'b IterExpr,
+ loop_id: HirId,
+ after_loop: bool,
+ used_iter: bool,
+ }
+ impl Visitor<'tcx> for AfterLoopVisitor<'_, '_, 'tcx> {
+ type Map = ErasedMap<'tcx>;
+ fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+ NestedVisitorMap::None
+ }
- fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
- if self.past_while_let {
- if path_to_local_id(expr, self.def_id) {
- self.var_used_after_while_let = true;
+ fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+ if self.used_iter {
+ return;
+ }
+ if self.after_loop {
+ if is_expr_same_child_or_parent_field(self.cx, e, &self.iter_expr.fields, self.iter_expr.path) {
+ self.used_iter = true;
+ } else if let (e, true) = skip_fields_and_path(e) {
+ if let Some(e) = e {
+ self.visit_expr(e);
+ }
+ } else if let ExprKind::Closure(_, _, id, _, _) = e.kind {
+ self.used_iter = is_res_used(self.cx, self.iter_expr.path, id);
+ } else {
+ walk_expr(self, e);
+ }
+ } else if self.loop_id == e.hir_id {
+ self.after_loop = true;
+ } else {
+ walk_expr(self, e);
+ }
+ }
+ }
+
+ struct NestedLoopVisitor<'a, 'b, 'tcx> {
+ cx: &'a LateContext<'tcx>,
+ iter_expr: &'b IterExpr,
+ local_id: HirId,
+ loop_id: HirId,
+ after_loop: bool,
+ found_local: bool,
+ used_after: bool,
+ }
+ impl Visitor<'tcx> for NestedLoopVisitor<'a, 'b, 'tcx> {
+ type Map = ErasedMap<'tcx>;
+ fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+ NestedVisitorMap::None
+ }
+
+ fn visit_local(&mut self, l: &'tcx Local<'_>) {
+ if !self.after_loop {
+ l.pat.each_binding_or_first(&mut |_, id, _, _| {
+ if id == self.local_id {
+ self.found_local = true;
+ }
+ });
+ }
+ if let Some(e) = l.init {
+ self.visit_expr(e);
+ }
+ }
+
+ fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+ if self.used_after {
+ return;
+ }
+ if self.after_loop {
+ if is_expr_same_child_or_parent_field(self.cx, e, &self.iter_expr.fields, self.iter_expr.path) {
+ self.used_after = true;
+ } else if let (e, true) = skip_fields_and_path(e) {
+ if let Some(e) = e {
+ self.visit_expr(e);
+ }
+ } else if let ExprKind::Closure(_, _, id, _, _) = e.kind {
+ self.used_after = is_res_used(self.cx, self.iter_expr.path, id);
+ } else {
+ walk_expr(self, e);
+ }
+ } else if e.hir_id == self.loop_id {
+ self.after_loop = true;
+ } else {
+ walk_expr(self, e);
}
- } else if self.iter_expr_id == expr.hir_id {
- self.past_while_let = true;
}
- walk_expr(self, expr);
}
- fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
- NestedVisitorMap::None
+
+ if let Some(e) = get_enclosing_loop_or_closure(cx.tcx, loop_expr) {
+ // The iterator expression will be used on the next iteration (for loops), or on the next call (for
+ // closures) unless it is declared within the enclosing expression. TODO: Check for closures
+ // used where an `FnOnce` type is expected.
+ let local_id = match iter_expr.path {
+ Res::Local(id) => id,
+ _ => return true,
+ };
+ let mut v = NestedLoopVisitor {
+ cx,
+ iter_expr,
+ local_id,
+ loop_id: loop_expr.hir_id,
+ after_loop: false,
+ found_local: false,
+ used_after: false,
+ };
+ v.visit_expr(e);
+ v.used_after || !v.found_local
+ } else {
+ let mut v = AfterLoopVisitor {
+ cx,
+ iter_expr,
+ loop_id: loop_expr.hir_id,
+ after_loop: false,
+ used_iter: false,
+ };
+ v.visit_expr(&cx.tcx.hir().body(cx.enclosing_body.unwrap()).value);
+ v.used_iter
}
}
projects / rust.git / blobdiff