From 2c1f676bfe831f488cbd8e07d46b7b9be727ca24 Mon Sep 17 00:00:00 2001 From: nahuakang Date: Tue, 9 Feb 2021 21:58:10 +0100 Subject: [PATCH 1/1] Add detect_same_item_push to its own module --- clippy_lints/src/loops/mod.rs | 171 +---------------------- clippy_lints/src/loops/same_item_push.rs | 168 ++++++++++++++++++++++ 2 files changed, 174 insertions(+), 165 deletions(-) create mode 100644 clippy_lints/src/loops/same_item_push.rs diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index 043bbc9d1c5..66b71a7202e 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -7,6 +7,7 @@ mod infinite_loop; mod manual_flatten; mod needless_collect; +mod same_item_push; mod utils; use crate::utils::sugg::Sugg; @@ -14,18 +15,17 @@ use crate::utils::{ get_enclosing_block, get_parent_expr, get_trait_def_id, higher, implements_trait, is_in_panic_handler, is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_trait_method, - path_to_local, path_to_local_id, paths, snippet, snippet_with_applicability, snippet_with_macro_callsite, - span_lint, span_lint_and_help, span_lint_and_sugg, sugg, + path_to_local, path_to_local_id, paths, snippet, snippet_with_applicability, span_lint, span_lint_and_help, + span_lint_and_sugg, sugg, }; use if_chain::if_chain; use rustc_ast::ast; 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::{ - BinOpKind, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, HirId, InlineAsmOperand, LoopSource, MatchSource, - Mutability, Node, Pat, PatKind, Stmt, StmtKind, + BinOpKind, Block, BorrowKind, Expr, ExprKind, HirId, InlineAsmOperand, LoopSource, MatchSource, Mutability, Node, + Pat, PatKind, Stmt, StmtKind, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; @@ -866,7 +866,7 @@ fn check_for_loop<'tcx>( for_loop_over_map_kv::check_for_loop_over_map_kv(cx, pat, arg, body, expr); for_mut_range_bound::check_for_mut_range_bound(cx, arg, body); for_single_element_loop::check_for_single_element_loop(cx, pat, arg, body, expr); - detect_same_item_push(cx, pat, arg, body, expr); + same_item_push::detect_same_item_push(cx, pat, arg, body, expr); manual_flatten::check_manual_flatten(cx, pat, arg, body, span); } @@ -1307,165 +1307,6 @@ fn detect_manual_memcpy<'tcx>( false } -// Scans the body of the for loop and determines whether lint should be given -struct SameItemPushVisitor<'a, 'tcx> { - should_lint: bool, - // this field holds the last vec push operation visited, which should be the only push seen - vec_push: Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)>, - cx: &'a LateContext<'tcx>, -} - -impl<'a, 'tcx> Visitor<'tcx> for SameItemPushVisitor<'a, 'tcx> { - type Map = Map<'tcx>; - - fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - match &expr.kind { - // Non-determinism may occur ... don't give a lint - ExprKind::Loop(..) | ExprKind::Match(..) => self.should_lint = false, - ExprKind::Block(block, _) => self.visit_block(block), - _ => {}, - } - } - - fn visit_block(&mut self, b: &'tcx Block<'_>) { - for stmt in b.stmts.iter() { - self.visit_stmt(stmt); - } - } - - fn visit_stmt(&mut self, s: &'tcx Stmt<'_>) { - let vec_push_option = get_vec_push(self.cx, s); - if vec_push_option.is_none() { - // Current statement is not a push so visit inside - match &s.kind { - StmtKind::Expr(expr) | StmtKind::Semi(expr) => self.visit_expr(&expr), - _ => {}, - } - } else { - // Current statement is a push ...check whether another - // push had been previously done - if self.vec_push.is_none() { - self.vec_push = vec_push_option; - } else { - // There are multiple pushes ... don't lint - self.should_lint = false; - } - } - } - - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::None - } -} - -// Given some statement, determine if that statement is a push on a Vec. If it is, return -// the Vec being pushed into and the item being pushed -fn get_vec_push<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) -> Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)> { - if_chain! { - // Extract method being called - if let StmtKind::Semi(semi_stmt) = &stmt.kind; - if let ExprKind::MethodCall(path, _, args, _) = &semi_stmt.kind; - // Figure out the parameters for the method call - if let Some(self_expr) = args.get(0); - if let Some(pushed_item) = args.get(1); - // Check that the method being called is push() on a Vec - if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_expr), sym::vec_type); - if path.ident.name.as_str() == "push"; - then { - return Some((self_expr, pushed_item)) - } - } - None -} - -/// Detects for loop pushing the same item into a Vec -fn detect_same_item_push<'tcx>( - cx: &LateContext<'tcx>, - pat: &'tcx Pat<'_>, - _: &'tcx Expr<'_>, - body: &'tcx Expr<'_>, - _: &'tcx Expr<'_>, -) { - fn emit_lint(cx: &LateContext<'_>, vec: &Expr<'_>, pushed_item: &Expr<'_>) { - let vec_str = snippet_with_macro_callsite(cx, vec.span, ""); - let item_str = snippet_with_macro_callsite(cx, pushed_item.span, ""); - - span_lint_and_help( - cx, - SAME_ITEM_PUSH, - vec.span, - "it looks like the same item is being pushed into this Vec", - None, - &format!( - "try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})", - item_str, vec_str, item_str - ), - ) - } - - if !matches!(pat.kind, PatKind::Wild) { - return; - } - - // Determine whether it is safe to lint the body - let mut same_item_push_visitor = SameItemPushVisitor { - should_lint: true, - vec_push: None, - cx, - }; - walk_expr(&mut same_item_push_visitor, body); - if same_item_push_visitor.should_lint { - if let Some((vec, pushed_item)) = same_item_push_visitor.vec_push { - let vec_ty = cx.typeck_results().expr_ty(vec); - let ty = vec_ty.walk().nth(1).unwrap().expect_ty(); - if cx - .tcx - .lang_items() - .clone_trait() - .map_or(false, |id| implements_trait(cx, ty, id, &[])) - { - // Make sure that the push does not involve possibly mutating values - match pushed_item.kind { - ExprKind::Path(ref qpath) => { - match cx.qpath_res(qpath, pushed_item.hir_id) { - // immutable bindings that are initialized with literal or constant - Res::Local(hir_id) => { - if_chain! { - let node = cx.tcx.hir().get(hir_id); - if let Node::Binding(pat) = node; - if let PatKind::Binding(bind_ann, ..) = pat.kind; - if !matches!(bind_ann, BindingAnnotation::RefMut | BindingAnnotation::Mutable); - let parent_node = cx.tcx.hir().get_parent_node(hir_id); - if let Some(Node::Local(parent_let_expr)) = cx.tcx.hir().find(parent_node); - if let Some(init) = parent_let_expr.init; - then { - match init.kind { - // immutable bindings that are initialized with literal - ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item), - // immutable bindings that are initialized with constant - ExprKind::Path(ref path) => { - if let Res::Def(DefKind::Const, ..) = cx.qpath_res(path, init.hir_id) { - emit_lint(cx, vec, pushed_item); - } - } - _ => {}, - } - } - } - }, - // constant - Res::Def(DefKind::Const, ..) => emit_lint(cx, vec, pushed_item), - _ => {}, - } - }, - ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item), - _ => {}, - } - } - } - } -} - 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, diff --git a/clippy_lints/src/loops/same_item_push.rs b/clippy_lints/src/loops/same_item_push.rs new file mode 100644 index 00000000000..62efb58a38f --- /dev/null +++ b/clippy_lints/src/loops/same_item_push.rs @@ -0,0 +1,168 @@ +use crate::utils::{implements_trait, is_type_diagnostic_item, snippet_with_macro_callsite, span_lint_and_help}; +use if_chain::if_chain; +use rustc_hir::def::{DefKind, Res}; +use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, Node, Pat, PatKind, Stmt, StmtKind}; +use rustc_lint::LateContext; +use rustc_middle::hir::map::Map; +use rustc_span::symbol::sym; +use std::iter::Iterator; + +/// Detects for loop pushing the same item into a Vec +pub(super) fn detect_same_item_push<'tcx>( + cx: &LateContext<'tcx>, + pat: &'tcx Pat<'_>, + _: &'tcx Expr<'_>, + body: &'tcx Expr<'_>, + _: &'tcx Expr<'_>, +) { + fn emit_lint(cx: &LateContext<'_>, vec: &Expr<'_>, pushed_item: &Expr<'_>) { + let vec_str = snippet_with_macro_callsite(cx, vec.span, ""); + let item_str = snippet_with_macro_callsite(cx, pushed_item.span, ""); + + span_lint_and_help( + cx, + super::SAME_ITEM_PUSH, + vec.span, + "it looks like the same item is being pushed into this Vec", + None, + &format!( + "try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})", + item_str, vec_str, item_str + ), + ) + } + + if !matches!(pat.kind, PatKind::Wild) { + return; + } + + // Determine whether it is safe to lint the body + let mut same_item_push_visitor = SameItemPushVisitor { + should_lint: true, + vec_push: None, + cx, + }; + walk_expr(&mut same_item_push_visitor, body); + if same_item_push_visitor.should_lint { + if let Some((vec, pushed_item)) = same_item_push_visitor.vec_push { + let vec_ty = cx.typeck_results().expr_ty(vec); + let ty = vec_ty.walk().nth(1).unwrap().expect_ty(); + if cx + .tcx + .lang_items() + .clone_trait() + .map_or(false, |id| implements_trait(cx, ty, id, &[])) + { + // Make sure that the push does not involve possibly mutating values + match pushed_item.kind { + ExprKind::Path(ref qpath) => { + match cx.qpath_res(qpath, pushed_item.hir_id) { + // immutable bindings that are initialized with literal or constant + Res::Local(hir_id) => { + if_chain! { + let node = cx.tcx.hir().get(hir_id); + if let Node::Binding(pat) = node; + if let PatKind::Binding(bind_ann, ..) = pat.kind; + if !matches!(bind_ann, BindingAnnotation::RefMut | BindingAnnotation::Mutable); + let parent_node = cx.tcx.hir().get_parent_node(hir_id); + if let Some(Node::Local(parent_let_expr)) = cx.tcx.hir().find(parent_node); + if let Some(init) = parent_let_expr.init; + then { + match init.kind { + // immutable bindings that are initialized with literal + ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item), + // immutable bindings that are initialized with constant + ExprKind::Path(ref path) => { + if let Res::Def(DefKind::Const, ..) = cx.qpath_res(path, init.hir_id) { + emit_lint(cx, vec, pushed_item); + } + } + _ => {}, + } + } + } + }, + // constant + Res::Def(DefKind::Const, ..) => emit_lint(cx, vec, pushed_item), + _ => {}, + } + }, + ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item), + _ => {}, + } + } + } + } +} + +// Scans the body of the for loop and determines whether lint should be given +struct SameItemPushVisitor<'a, 'tcx> { + should_lint: bool, + // this field holds the last vec push operation visited, which should be the only push seen + vec_push: Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)>, + cx: &'a LateContext<'tcx>, +} + +impl<'a, 'tcx> Visitor<'tcx> for SameItemPushVisitor<'a, 'tcx> { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + match &expr.kind { + // Non-determinism may occur ... don't give a lint + ExprKind::Loop(..) | ExprKind::Match(..) => self.should_lint = false, + ExprKind::Block(block, _) => self.visit_block(block), + _ => {}, + } + } + + fn visit_block(&mut self, b: &'tcx Block<'_>) { + for stmt in b.stmts.iter() { + self.visit_stmt(stmt); + } + } + + fn visit_stmt(&mut self, s: &'tcx Stmt<'_>) { + let vec_push_option = get_vec_push(self.cx, s); + if vec_push_option.is_none() { + // Current statement is not a push so visit inside + match &s.kind { + StmtKind::Expr(expr) | StmtKind::Semi(expr) => self.visit_expr(&expr), + _ => {}, + } + } else { + // Current statement is a push ...check whether another + // push had been previously done + if self.vec_push.is_none() { + self.vec_push = vec_push_option; + } else { + // There are multiple pushes ... don't lint + self.should_lint = false; + } + } + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } +} + +// Given some statement, determine if that statement is a push on a Vec. If it is, return +// the Vec being pushed into and the item being pushed +fn get_vec_push<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) -> Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)> { + if_chain! { + // Extract method being called + if let StmtKind::Semi(semi_stmt) = &stmt.kind; + if let ExprKind::MethodCall(path, _, args, _) = &semi_stmt.kind; + // Figure out the parameters for the method call + if let Some(self_expr) = args.get(0); + if let Some(pushed_item) = args.get(1); + // Check that the method being called is push() on a Vec + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_expr), sym::vec_type); + if path.ident.name.as_str() == "push"; + then { + return Some((self_expr, pushed_item)) + } + } + None +} -- 2.44.0