From 1e8ada3cab5470a4f2070c0aa9d1a94922476621 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Sat, 18 Jul 2020 23:28:31 +0900 Subject: [PATCH 1/9] Add lint `same_item_push` --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/loops.rs | 265 +++++++++++++++++++++++++++++++++ src/lintlist/mod.rs | 7 + tests/ui/same_item_push.rs | 77 ++++++++++ tests/ui/same_item_push.stderr | 35 +++++ 6 files changed, 388 insertions(+) create mode 100644 tests/ui/same_item_push.rs create mode 100644 tests/ui/same_item_push.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 43d83d978b8..fbc783f1c2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1687,6 +1687,7 @@ Released 2018-09-13 [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn [`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges [`same_functions_in_if_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_functions_in_if_condition +[`same_item_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push [`search_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#search_is_some [`serde_api_misuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#serde_api_misuse [`shadow_reuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_reuse diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 26aff6af8cd..2bd5ae0ed98 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -610,6 +610,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &loops::WHILE_IMMUTABLE_CONDITION, &loops::WHILE_LET_LOOP, &loops::WHILE_LET_ON_ITERATOR, + &loops::SAME_ITEM_PUSH, ¯o_use::MACRO_USE_IMPORTS, &main_recursion::MAIN_RECURSION, &manual_async_fn::MANUAL_ASYNC_FN, @@ -1405,6 +1406,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&repeat_once::REPEAT_ONCE), LintId::of(&returns::NEEDLESS_RETURN), LintId::of(&returns::UNUSED_UNIT), + LintId::of(&loops::SAME_ITEM_PUSH), LintId::of(&serde_api::SERDE_API_MISUSE), LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), @@ -1543,6 +1545,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(®ex::TRIVIAL_REGEX), LintId::of(&returns::NEEDLESS_RETURN), LintId::of(&returns::UNUSED_UNIT), + LintId::of(&loops::SAME_ITEM_PUSH), LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(&strings::STRING_LIT_AS_BYTES), LintId::of(&tabs_in_doc_comments::TABS_IN_DOC_COMMENTS), diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 6359c20040c..48891a59c00 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -419,6 +419,39 @@ "variables used within while expression are not mutated in the body" } +declare_clippy_lint! { + /// **What it does:** Checks whether a for loop is being used to push a constant + /// value into a Vec. + /// + /// **Why is this bad?** This kind of operation can be expressed more succinctly with + /// `vec![item;SIZE]` or `vec.resize(NEW_SIZE, item)` and using these alternatives may also + /// have better performance. + /// **Known problems:** None + /// + /// **Example:** + /// ```rust + /// let item1 = 2; + /// let item2 = 3; + /// let mut vec: Vec = Vec::new(); + /// for _ in 0..20 { + /// vec.push(item1); + /// } + /// for _ in 0..30 { + /// vec.push(item2); + /// } + /// ``` + /// could be written as + /// ```rust + /// let item1 = 2; + /// let item2 = 3; + /// let mut vec: Vec = vec![item1; 20]; + /// vec.resize(20 + 30, item2); + /// ``` + pub SAME_ITEM_PUSH, + style, + "the same item is pushed inside of a for loop" +} + declare_lint_pass!(Loops => [ MANUAL_MEMCPY, NEEDLESS_RANGE_LOOP, @@ -435,6 +468,7 @@ NEVER_LOOP, MUT_RANGE_BOUND, WHILE_IMMUTABLE_CONDITION, + SAME_ITEM_PUSH, ]); impl<'tcx> LateLintPass<'tcx> for Loops { @@ -740,6 +774,7 @@ fn check_for_loop<'tcx>( 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); + detect_same_item_push(cx, pat, arg, body, expr); } fn same_var<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, var: HirId) -> bool { @@ -1016,6 +1051,236 @@ fn detect_manual_memcpy<'tcx>( } } +// Delegate that traverses expression and detects mutable variables being used +struct UsesMutableDelegate { + found_mutable: bool, +} + +impl<'tcx> Delegate<'tcx> for UsesMutableDelegate { + fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: ConsumeMode) {} + + fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, bk: ty::BorrowKind) { + // Mutable variable is found + if let ty::BorrowKind::MutBorrow = bk { + self.found_mutable = true; + } + } + + fn mutate(&mut self, _: &PlaceWithHirId<'tcx>) {} +} + +// Uses UsesMutableDelegate to find mutable variables in an expression expr +fn has_mutable_variables<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { + let mut delegate = UsesMutableDelegate { found_mutable: false }; + let def_id = expr.hir_id.owner.to_def_id(); + cx.tcx.infer_ctxt().enter(|infcx| { + ExprUseVisitor::new( + &mut delegate, + &infcx, + def_id.expect_local(), + cx.param_env, + cx.tables(), + ).walk_expr(expr); + }); + + delegate.found_mutable +} + +// Scans for the usage of the for loop pattern +struct ForPatternVisitor<'a, 'tcx> { + found_pattern: bool, + // Pattern that we are searching for + for_pattern: &'a Pat<'tcx>, + cx: &'a LateContext<'tcx>, +} + +impl<'a, 'tcx> Visitor<'tcx> for ForPatternVisitor<'a, 'tcx> { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + // Recursively explore an expression until a ExprKind::Path is found + match &expr.kind { + ExprKind::Array(expr_list) | ExprKind::MethodCall(_, _, expr_list, _) | ExprKind::Tup(expr_list) => { + for expr in *expr_list { + self.visit_expr(expr) + } + }, + ExprKind::Binary(_, lhs_expr, rhs_expr) => { + self.visit_expr(lhs_expr); + self.visit_expr(rhs_expr); + }, + ExprKind::Box(expr) + | ExprKind::Unary(_, expr) + | ExprKind::Cast(expr, _) + | ExprKind::Type(expr, _) + | ExprKind::AddrOf(_, _, expr) + | ExprKind::Struct(_, _, Some(expr)) => self.visit_expr(expr), + _ => { + // Exploration cannot continue ... calculate the hir_id of the current + // expr assuming it is a Path + if let Some(hir_id) = var_def_id(self.cx, &expr) { + // Pattern is found + if hir_id == self.for_pattern.hir_id { + self.found_pattern = true; + } + // If the for loop pattern is a tuple, determine whether the current + // expr is inside that tuple pattern + if let PatKind::Tuple(pat_list, _) = &self.for_pattern.kind { + let hir_id_list: Vec = pat_list.iter().map(|p| p.hir_id).collect(); + if hir_id_list.contains(&hir_id) { + self.found_pattern = true; + } + } + } + }, + } + } + + // This is triggered by walk_expr() for the case of vec.push(pat) + fn visit_qpath(&mut self, qpath: &'tcx QPath<'_>, _: HirId, _: Span) { + if_chain! { + if let QPath::Resolved(_, path) = qpath; + if let Res::Local(hir_id) = &path.res; + then { + if *hir_id == self.for_pattern.hir_id{ + self.found_pattern = true; + } + + if let PatKind::Tuple(pat_list, _) = &self.for_pattern.kind { + let hir_id_list: Vec = pat_list.iter().map(|p| p.hir_id).collect(); + if hir_id_list.contains(&hir_id) { + self.found_pattern = true; + } + } + } + } + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } +} + +// 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 match_type(cx, cx.tables().expr_ty(self_expr), &paths::VEC); + 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<'_>, +) { + // 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 { + // Make sure that the push does not involve possibly mutating values + if !has_mutable_variables(cx, pushed_item) { + // Walk through the expression being pushed and make sure that it + // does not contain the for loop pattern + let mut for_pat_visitor = ForPatternVisitor { + found_pattern: false, + for_pattern: pat, + cx, + }; + walk_expr(&mut for_pat_visitor, pushed_item); + + if !for_pat_visitor.found_pattern { + let vec_str = snippet(cx, vec.span, ""); + let item_str = snippet(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 + ), + ) + } + } + } + } +} + /// Checks for looping over a range and then indexing a sequence with it. /// The iteratee must be a range literal. #[allow(clippy::too_many_lines)] diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index a08d7da6dcb..1b10226c930 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1935,6 +1935,13 @@ deprecation: None, module: "copies", }, + Lint { + name: "same_item_push", + group: "style", + desc: "default lint description", + deprecation: None, + module: "same_item_push", + }, Lint { name: "search_is_some", group: "complexity", diff --git a/tests/ui/same_item_push.rs b/tests/ui/same_item_push.rs new file mode 100644 index 00000000000..e3a5a647f76 --- /dev/null +++ b/tests/ui/same_item_push.rs @@ -0,0 +1,77 @@ +#![warn(clippy::same_item_push)] + +fn mutate_increment(x: &mut u8) -> u8 { + *x += 1; + *x +} + +fn increment(x: u8) -> u8 { + x + 1 +} + +fn main() { + // Test for basic case + let mut spaces = Vec::with_capacity(10); + for _ in 0..10 { + spaces.push(vec![b' ']); + } + + let mut vec2: Vec = Vec::new(); + let item = 2; + for _ in 5..=20 { + vec2.push(item); + } + + let mut vec3: Vec = Vec::new(); + for _ in 0..15 { + let item = 2; + vec3.push(item); + } + + let mut vec4: Vec = Vec::new(); + for _ in 0..15 { + vec4.push(13); + } + + // Suggestion should not be given as pushed variable can mutate + let mut vec5: Vec = Vec::new(); + let mut item: u8 = 2; + for _ in 0..30 { + vec5.push(mutate_increment(&mut item)); + } + + let mut vec6: Vec = Vec::new(); + let mut item: u8 = 2; + let mut item2 = &mut mutate_increment(&mut item); + for _ in 0..30 { + vec6.push(mutate_increment(item2)); + } + + let mut vec7: Vec = Vec::new(); + for (a, b) in [0, 1, 4, 9, 16].iter().enumerate() { + vec7.push(a); + } + + let mut vec8: Vec = Vec::new(); + for i in 0..30 { + vec8.push(increment(i)); + } + + let mut vec9: Vec = Vec::new(); + for i in 0..30 { + vec9.push(i + i * i); + } + + // Suggestion should not be given as there are multiple pushes that are not the same + let mut vec10: Vec = Vec::new(); + let item: u8 = 2; + for _ in 0..30 { + vec10.push(item); + vec10.push(item * 2); + } + + // Suggestion should not be given as Vec is not involved + for _ in 0..5 { + println!("Same Item Push"); + } +} diff --git a/tests/ui/same_item_push.stderr b/tests/ui/same_item_push.stderr new file mode 100644 index 00000000000..559cc450b9d --- /dev/null +++ b/tests/ui/same_item_push.stderr @@ -0,0 +1,35 @@ +error: it looks like the same item is being pushed into this Vec + --> $DIR/same_item_push.rs:16:9 + | +LL | spaces.push(vec![b' ']); + | ^^^^^^ + | + = note: `-D clippy::same-item-push` implied by `-D warnings` + = help: try using vec![<[_]>::into_vec(box [$($x),+]);SIZE] or spaces.resize(NEW_SIZE, <[_]>::into_vec(box [$($x),+])) + +error: it looks like the same item is being pushed into this Vec + --> $DIR/same_item_push.rs:22:9 + | +LL | vec2.push(item); + | ^^^^ + | + = help: try using vec![item;SIZE] or vec2.resize(NEW_SIZE, item) + +error: it looks like the same item is being pushed into this Vec + --> $DIR/same_item_push.rs:28:9 + | +LL | vec3.push(item); + | ^^^^ + | + = help: try using vec![item;SIZE] or vec3.resize(NEW_SIZE, item) + +error: it looks like the same item is being pushed into this Vec + --> $DIR/same_item_push.rs:33:9 + | +LL | vec4.push(13); + | ^^^^ + | + = help: try using vec![13;SIZE] or vec4.resize(NEW_SIZE, 13) + +error: aborting due to 4 previous errors + From 161f47510076d36722546c3541a546f9b724fadd Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Sun, 19 Jul 2020 23:43:35 +0900 Subject: [PATCH 2/9] Add test case for `same_item_push` --- clippy_lints/src/loops.rs | 1 + tests/ui/same_item_push.rs | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 48891a59c00..1c2f1225497 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1114,6 +1114,7 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { | ExprKind::Cast(expr, _) | ExprKind::Type(expr, _) | ExprKind::AddrOf(_, _, expr) + | ExprKind::Field(expr, _) | ExprKind::Struct(_, _, Some(expr)) => self.visit_expr(expr), _ => { // Exploration cannot continue ... calculate the hir_id of the current diff --git a/tests/ui/same_item_push.rs b/tests/ui/same_item_push.rs index e3a5a647f76..4bb5e73ff0d 100644 --- a/tests/ui/same_item_push.rs +++ b/tests/ui/same_item_push.rs @@ -74,4 +74,16 @@ fn main() { for _ in 0..5 { println!("Same Item Push"); } + + struct A { + kind: u32, + } + let mut vec_a: Vec = Vec::new(); + for i in 0..30 { + vec_a.push(A{kind: i}); + } + let mut vec12: Vec = Vec::new(); + for a in vec_a { + vec12.push(2u8.pow(a.kind)); + } } From 2beb9090d1b9adb2b0930da511bf1750e570905b Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Mon, 20 Jul 2020 22:40:31 +0900 Subject: [PATCH 3/9] Rename TypeckTables to TypeckResults --- clippy_lints/src/loops.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 1c2f1225497..766c68df623 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1079,7 +1079,7 @@ fn has_mutable_variables<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> &infcx, def_id.expect_local(), cx.param_env, - cx.tables(), + cx.typeck_results(), ).walk_expr(expr); }); @@ -1224,7 +1224,7 @@ fn get_vec_push<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) -> Option<(& 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 match_type(cx, cx.tables().expr_ty(self_expr), &paths::VEC); + if match_type(cx, cx.typeck_results().expr_ty(self_expr), &paths::VEC); if path.ident.name.as_str() == "push"; then { return Some((self_expr, pushed_item)) From 1543e117cc7459bef2b57389503f0f526a903f45 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Mon, 20 Jul 2020 22:52:30 +0900 Subject: [PATCH 4/9] cargo dev update_lints --- clippy_lints/src/lib.rs | 6 +++--- src/lintlist/mod.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2bd5ae0ed98..308868fc90a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -607,10 +607,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &loops::NEEDLESS_COLLECT, &loops::NEEDLESS_RANGE_LOOP, &loops::NEVER_LOOP, + &loops::SAME_ITEM_PUSH, &loops::WHILE_IMMUTABLE_CONDITION, &loops::WHILE_LET_LOOP, &loops::WHILE_LET_ON_ITERATOR, - &loops::SAME_ITEM_PUSH, ¯o_use::MACRO_USE_IMPORTS, &main_recursion::MAIN_RECURSION, &manual_async_fn::MANUAL_ASYNC_FN, @@ -1293,6 +1293,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::NEEDLESS_COLLECT), LintId::of(&loops::NEEDLESS_RANGE_LOOP), LintId::of(&loops::NEVER_LOOP), + LintId::of(&loops::SAME_ITEM_PUSH), LintId::of(&loops::WHILE_IMMUTABLE_CONDITION), LintId::of(&loops::WHILE_LET_LOOP), LintId::of(&loops::WHILE_LET_ON_ITERATOR), @@ -1406,7 +1407,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&repeat_once::REPEAT_ONCE), LintId::of(&returns::NEEDLESS_RETURN), LintId::of(&returns::UNUSED_UNIT), - LintId::of(&loops::SAME_ITEM_PUSH), LintId::of(&serde_api::SERDE_API_MISUSE), LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), @@ -1495,6 +1495,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::EMPTY_LOOP), LintId::of(&loops::FOR_KV_MAP), LintId::of(&loops::NEEDLESS_RANGE_LOOP), + LintId::of(&loops::SAME_ITEM_PUSH), LintId::of(&loops::WHILE_LET_ON_ITERATOR), LintId::of(&main_recursion::MAIN_RECURSION), LintId::of(&manual_async_fn::MANUAL_ASYNC_FN), @@ -1545,7 +1546,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(®ex::TRIVIAL_REGEX), LintId::of(&returns::NEEDLESS_RETURN), LintId::of(&returns::UNUSED_UNIT), - LintId::of(&loops::SAME_ITEM_PUSH), LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(&strings::STRING_LIT_AS_BYTES), LintId::of(&tabs_in_doc_comments::TABS_IN_DOC_COMMENTS), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 1b10226c930..1f79e44049f 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1938,9 +1938,9 @@ Lint { name: "same_item_push", group: "style", - desc: "default lint description", + desc: "the same item is pushed inside of a for loop", deprecation: None, - module: "same_item_push", + module: "loops", }, Lint { name: "search_is_some", From 14a4e3bcc8082b0323886ae15365ea2424b512cf Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 21 Jul 2020 08:15:13 +0900 Subject: [PATCH 5/9] Fix a lint message --- clippy_lints/src/loops.rs | 11 ++++++----- tests/ui/same_item_push.stderr | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 766c68df623..8ca67cae0e9 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -3,9 +3,10 @@ use crate::utils::sugg::Sugg; use crate::utils::usage::{is_unused, mutated_variables}; use crate::utils::{ - get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, - is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_trait_method, - match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_opt, snippet_with_applicability, span_lint, + get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, + implements_trait, is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, + last_path_segment, match_trait_method, match_type, match_var, multispan_sugg, qpath_res, + snippet, snippet_opt, snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq, }; use if_chain::if_chain; @@ -1262,8 +1263,8 @@ fn detect_same_item_push<'tcx>( walk_expr(&mut for_pat_visitor, pushed_item); if !for_pat_visitor.found_pattern { - let vec_str = snippet(cx, vec.span, ""); - let item_str = snippet(cx, pushed_item.span, ""); + 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, diff --git a/tests/ui/same_item_push.stderr b/tests/ui/same_item_push.stderr index 559cc450b9d..ddc5d48cd41 100644 --- a/tests/ui/same_item_push.stderr +++ b/tests/ui/same_item_push.stderr @@ -5,7 +5,7 @@ LL | spaces.push(vec![b' ']); | ^^^^^^ | = note: `-D clippy::same-item-push` implied by `-D warnings` - = help: try using vec![<[_]>::into_vec(box [$($x),+]);SIZE] or spaces.resize(NEW_SIZE, <[_]>::into_vec(box [$($x),+])) + = help: try using vec![vec![b' '];SIZE] or spaces.resize(NEW_SIZE, vec![b' ']) error: it looks like the same item is being pushed into this Vec --> $DIR/same_item_push.rs:22:9 From b7ceb4d3d7ed3ea7039caf803073e86ad3643e21 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 21 Jul 2020 08:25:11 +0900 Subject: [PATCH 6/9] rustfmt --- clippy_lints/src/loops.rs | 5 +++-- tests/ui/same_item_push.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 8ca67cae0e9..3a1fbc4bfed 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1081,7 +1081,8 @@ fn has_mutable_variables<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> def_id.expect_local(), cx.param_env, cx.typeck_results(), - ).walk_expr(expr); + ) + .walk_expr(expr); }); delegate.found_mutable @@ -1271,7 +1272,7 @@ fn detect_same_item_push<'tcx>( SAME_ITEM_PUSH, vec.span, "it looks like the same item is being pushed into this Vec", - None, + None, &format!( "try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})", item_str, vec_str, item_str diff --git a/tests/ui/same_item_push.rs b/tests/ui/same_item_push.rs index 4bb5e73ff0d..ff1088f86f6 100644 --- a/tests/ui/same_item_push.rs +++ b/tests/ui/same_item_push.rs @@ -80,7 +80,7 @@ struct A { } let mut vec_a: Vec = Vec::new(); for i in 0..30 { - vec_a.push(A{kind: i}); + vec_a.push(A { kind: i }); } let mut vec12: Vec = Vec::new(); for a in vec_a { From 228f668282daab05ec20adbbdeb227e923d10864 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Wed, 22 Jul 2020 22:11:31 +0900 Subject: [PATCH 7/9] Use `mutated_variables` --- clippy_lints/src/loops.rs | 38 +------------------------------------- 1 file changed, 1 insertion(+), 37 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 3a1fbc4bfed..86952c10dfc 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1052,42 +1052,6 @@ fn detect_manual_memcpy<'tcx>( } } -// Delegate that traverses expression and detects mutable variables being used -struct UsesMutableDelegate { - found_mutable: bool, -} - -impl<'tcx> Delegate<'tcx> for UsesMutableDelegate { - fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: ConsumeMode) {} - - fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, bk: ty::BorrowKind) { - // Mutable variable is found - if let ty::BorrowKind::MutBorrow = bk { - self.found_mutable = true; - } - } - - fn mutate(&mut self, _: &PlaceWithHirId<'tcx>) {} -} - -// Uses UsesMutableDelegate to find mutable variables in an expression expr -fn has_mutable_variables<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { - let mut delegate = UsesMutableDelegate { found_mutable: false }; - let def_id = expr.hir_id.owner.to_def_id(); - cx.tcx.infer_ctxt().enter(|infcx| { - ExprUseVisitor::new( - &mut delegate, - &infcx, - def_id.expect_local(), - cx.param_env, - cx.typeck_results(), - ) - .walk_expr(expr); - }); - - delegate.found_mutable -} - // Scans for the usage of the for loop pattern struct ForPatternVisitor<'a, 'tcx> { found_pattern: bool, @@ -1253,7 +1217,7 @@ fn detect_same_item_push<'tcx>( if same_item_push_visitor.should_lint { if let Some((vec, pushed_item)) = same_item_push_visitor.vec_push { // Make sure that the push does not involve possibly mutating values - if !has_mutable_variables(cx, pushed_item) { + if mutated_variables(pushed_item, cx).map_or(false, |mutvars| mutvars.is_empty()) { // Walk through the expression being pushed and make sure that it // does not contain the for loop pattern let mut for_pat_visitor = ForPatternVisitor { From e48685edef9889d7c0ae391cf050f878d228ae25 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Wed, 22 Jul 2020 23:22:17 +0900 Subject: [PATCH 8/9] Just check if it contains `_` in `for pat` --- clippy_lints/src/loops.rs | 87 +-------------------------------------- 1 file changed, 1 insertion(+), 86 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 86952c10dfc..3104f0c137e 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1052,82 +1052,6 @@ fn detect_manual_memcpy<'tcx>( } } -// Scans for the usage of the for loop pattern -struct ForPatternVisitor<'a, 'tcx> { - found_pattern: bool, - // Pattern that we are searching for - for_pattern: &'a Pat<'tcx>, - cx: &'a LateContext<'tcx>, -} - -impl<'a, 'tcx> Visitor<'tcx> for ForPatternVisitor<'a, 'tcx> { - type Map = Map<'tcx>; - - fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - // Recursively explore an expression until a ExprKind::Path is found - match &expr.kind { - ExprKind::Array(expr_list) | ExprKind::MethodCall(_, _, expr_list, _) | ExprKind::Tup(expr_list) => { - for expr in *expr_list { - self.visit_expr(expr) - } - }, - ExprKind::Binary(_, lhs_expr, rhs_expr) => { - self.visit_expr(lhs_expr); - self.visit_expr(rhs_expr); - }, - ExprKind::Box(expr) - | ExprKind::Unary(_, expr) - | ExprKind::Cast(expr, _) - | ExprKind::Type(expr, _) - | ExprKind::AddrOf(_, _, expr) - | ExprKind::Field(expr, _) - | ExprKind::Struct(_, _, Some(expr)) => self.visit_expr(expr), - _ => { - // Exploration cannot continue ... calculate the hir_id of the current - // expr assuming it is a Path - if let Some(hir_id) = var_def_id(self.cx, &expr) { - // Pattern is found - if hir_id == self.for_pattern.hir_id { - self.found_pattern = true; - } - // If the for loop pattern is a tuple, determine whether the current - // expr is inside that tuple pattern - if let PatKind::Tuple(pat_list, _) = &self.for_pattern.kind { - let hir_id_list: Vec = pat_list.iter().map(|p| p.hir_id).collect(); - if hir_id_list.contains(&hir_id) { - self.found_pattern = true; - } - } - } - }, - } - } - - // This is triggered by walk_expr() for the case of vec.push(pat) - fn visit_qpath(&mut self, qpath: &'tcx QPath<'_>, _: HirId, _: Span) { - if_chain! { - if let QPath::Resolved(_, path) = qpath; - if let Res::Local(hir_id) = &path.res; - then { - if *hir_id == self.for_pattern.hir_id{ - self.found_pattern = true; - } - - if let PatKind::Tuple(pat_list, _) = &self.for_pattern.kind { - let hir_id_list: Vec = pat_list.iter().map(|p| p.hir_id).collect(); - if hir_id_list.contains(&hir_id) { - self.found_pattern = true; - } - } - } - } - } - - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::None - } -} - // Scans the body of the for loop and determines whether lint should be given struct SameItemPushVisitor<'a, 'tcx> { should_lint: bool, @@ -1218,16 +1142,7 @@ fn detect_same_item_push<'tcx>( if let Some((vec, pushed_item)) = same_item_push_visitor.vec_push { // Make sure that the push does not involve possibly mutating values if mutated_variables(pushed_item, cx).map_or(false, |mutvars| mutvars.is_empty()) { - // Walk through the expression being pushed and make sure that it - // does not contain the for loop pattern - let mut for_pat_visitor = ForPatternVisitor { - found_pattern: false, - for_pattern: pat, - cx, - }; - walk_expr(&mut for_pat_visitor, pushed_item); - - if !for_pat_visitor.found_pattern { + if let PatKind::Wild = pat.kind { let vec_str = snippet_with_macro_callsite(cx, vec.span, ""); let item_str = snippet_with_macro_callsite(cx, pushed_item.span, ""); From 610d4e3c8b1bfa27e059043554f4156fe1254142 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Wed, 5 Aug 2020 23:10:30 +0900 Subject: [PATCH 9/9] rustfmt --- clippy_lints/src/loops.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 3104f0c137e..5c918ff648f 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -3,11 +3,11 @@ use crate::utils::sugg::Sugg; use crate::utils::usage::{is_unused, mutated_variables}; use crate::utils::{ - get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, - implements_trait, is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, - last_path_segment, match_trait_method, match_type, match_var, multispan_sugg, qpath_res, - snippet, snippet_opt, snippet_with_applicability, snippet_with_macro_callsite, span_lint, - span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq, + get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, + is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_trait_method, + match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_opt, snippet_with_applicability, + snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, + SpanlessEq, }; use if_chain::if_chain; use rustc_ast::ast;