From 3d30ef7818b9ed562f2c48f3d6d15d90253ae732 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Mon, 7 Sep 2020 22:17:31 +0900 Subject: [PATCH 1/6] Restrict `same_item_push` to suppress false positives It emits a lint when the pushed item is a literal, a constant and an immutable binding that are initialized with those. --- clippy_lints/src/loops.rs | 83 ++++++++++++++++++++++++++-------- tests/ui/same_item_push.rs | 13 ++++++ tests/ui/same_item_push.stderr | 34 ++++++++------ 3 files changed, 97 insertions(+), 33 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index f13e369907b2..f417e3a0caf9 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1153,27 +1153,70 @@ fn detect_same_item_push<'tcx>( let vec_str = snippet_with_macro_callsite(cx, vec.span, ""); let item_str = snippet_with_macro_callsite(cx, pushed_item.span, ""); if let ExprKind::Path(ref qpath) = pushed_item.kind { - if_chain! { - if let Res::Local(hir_id) = qpath_res(cx, qpath, pushed_item.hir_id); - 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); - then { - 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 - ), - ) - } + match qpath_res(cx, 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 rustc_hir::Local { init: Some(init), .. } = parent_let_expr; + then { + match init.kind { + // immutable bindings that are initialized with literal + ExprKind::Lit(..) => { + 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 + ), + ) + }, + // immutable bindings that are initialized with constant + ExprKind::Path(ref path) => { + if let Res::Def(DefKind::Const, ..) = qpath_res(cx, path, init.hir_id) { + 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 + ), + ) + } + } + _ => {}, + } + } + } + }, + // constant + Res::Def(DefKind::Const, ..) => 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 + ), + ), + _ => {}, } - } else if mutated_variables(pushed_item, cx).map_or(false, |mutvars| mutvars.is_empty()) { + } else if let ExprKind::Lit(..) = pushed_item.kind { + // literal span_lint_and_help( cx, SAME_ITEM_PUSH, diff --git a/tests/ui/same_item_push.rs b/tests/ui/same_item_push.rs index 0928820892b1..bd4792c4a76b 100644 --- a/tests/ui/same_item_push.rs +++ b/tests/ui/same_item_push.rs @@ -1,5 +1,7 @@ #![warn(clippy::same_item_push)] +const VALUE: u8 = 7; + fn mutate_increment(x: &mut u8) -> u8 { *x += 1; *x @@ -111,4 +113,15 @@ fn main() { for _ in 0..10 { vec15.push(Box::new(S {})); } + + let mut vec16 = Vec::new(); + for _ in 0..20 { + vec16.push(VALUE); + } + + let mut vec17 = Vec::new(); + let item = VALUE; + for _ in 0..20 { + vec17.push(item); + } } diff --git a/tests/ui/same_item_push.stderr b/tests/ui/same_item_push.stderr index ddc5d48cd413..4896479791af 100644 --- a/tests/ui/same_item_push.stderr +++ b/tests/ui/same_item_push.stderr @@ -1,22 +1,14 @@ 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![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 + --> $DIR/same_item_push.rs:24:9 | LL | vec2.push(item); | ^^^^ | + = note: `-D clippy::same-item-push` implied by `-D warnings` = 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 + --> $DIR/same_item_push.rs:30:9 | LL | vec3.push(item); | ^^^^ @@ -24,12 +16,28 @@ 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 + --> $DIR/same_item_push.rs:35:9 | LL | vec4.push(13); | ^^^^ | = help: try using vec![13;SIZE] or vec4.resize(NEW_SIZE, 13) -error: aborting due to 4 previous errors +error: it looks like the same item is being pushed into this Vec + --> $DIR/same_item_push.rs:119:9 + | +LL | vec16.push(VALUE); + | ^^^^^ + | + = help: try using vec![VALUE;SIZE] or vec16.resize(NEW_SIZE, VALUE) + +error: it looks like the same item is being pushed into this Vec + --> $DIR/same_item_push.rs:125:9 + | +LL | vec17.push(item); + | ^^^^^ + | + = help: try using vec![item;SIZE] or vec17.resize(NEW_SIZE, item) + +error: aborting due to 5 previous errors From 619ca76731e7209288fd3ebf1ae243c050486c12 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 8 Sep 2020 08:25:31 +0900 Subject: [PATCH 2/6] Refactoring: use inner function --- clippy_lints/src/loops.rs | 69 ++++++++++++--------------------------- 1 file changed, 21 insertions(+), 48 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index f417e3a0caf9..c27acdd22365 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1150,8 +1150,6 @@ fn detect_same_item_push<'tcx>( { // Make sure that the push does not involve possibly mutating values 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, ""); if let ExprKind::Path(ref qpath) = pushed_item.kind { match qpath_res(cx, qpath, pushed_item.hir_id) { // immutable bindings that are initialized with literal or constant @@ -1167,33 +1165,11 @@ fn detect_same_item_push<'tcx>( then { match init.kind { // immutable bindings that are initialized with literal - ExprKind::Lit(..) => { - 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 - ), - ) - }, + 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, ..) = qpath_res(cx, path, init.hir_id) { - 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 - ), - ) + emit_lint(cx, vec, pushed_item); } } _ => {}, @@ -1202,37 +1178,34 @@ fn detect_same_item_push<'tcx>( } }, // constant - Res::Def(DefKind::Const, ..) => 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 - ), - ), + Res::Def(DefKind::Const, ..) => emit_lint(cx, vec, pushed_item), _ => {}, } } else if let ExprKind::Lit(..) = pushed_item.kind { // literal - 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 - ), - ) + emit_lint(cx, vec, pushed_item); } } } } } + + 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 + ), + ) + } } /// Checks for looping over a range and then indexing a sequence with it. From 5d085ad011a602e004e7d33fa0b9074c7dab36fc Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 8 Sep 2020 08:34:51 +0900 Subject: [PATCH 3/6] Some refactoring --- clippy_lints/src/loops.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index c27acdd22365..c59185bd7bd1 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1131,6 +1131,10 @@ fn detect_same_item_push<'tcx>( body: &'tcx Expr<'_>, _: &'tcx Expr<'_>, ) { + 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, @@ -1149,8 +1153,8 @@ fn detect_same_item_push<'tcx>( .map_or(false, |id| implements_trait(cx, ty, id, &[])) { // Make sure that the push does not involve possibly mutating values - if let PatKind::Wild = pat.kind { - if let ExprKind::Path(ref qpath) = pushed_item.kind { + match pushed_item.kind { + ExprKind::Path(ref qpath) => { match qpath_res(cx, qpath, pushed_item.hir_id) { // immutable bindings that are initialized with literal or constant Res::Local(hir_id) => { @@ -1161,7 +1165,7 @@ fn detect_same_item_push<'tcx>( 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 rustc_hir::Local { init: Some(init), .. } = parent_let_expr; + if let Some(init) = parent_let_expr.init; then { match init.kind { // immutable bindings that are initialized with literal @@ -1181,10 +1185,9 @@ fn detect_same_item_push<'tcx>( Res::Def(DefKind::Const, ..) => emit_lint(cx, vec, pushed_item), _ => {}, } - } else if let ExprKind::Lit(..) = pushed_item.kind { - // literal - emit_lint(cx, vec, pushed_item); - } + }, + ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item), + _ => {}, } } } From 2c9f82e7d22a5cd3f704ed0b932a17fa53f1145b Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 8 Sep 2020 22:45:27 +0900 Subject: [PATCH 4/6] Add some tests to `same_item_push` Add tests in which the variable is initialized with a match expression and function call --- tests/ui/same_item_push.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/ui/same_item_push.rs b/tests/ui/same_item_push.rs index bd4792c4a76b..7ca829854db1 100644 --- a/tests/ui/same_item_push.rs +++ b/tests/ui/same_item_push.rs @@ -11,6 +11,10 @@ fn increment(x: u8) -> u8 { x + 1 } +fn fun() -> usize { + 42 +} + fn main() { // Test for basic case let mut spaces = Vec::with_capacity(10); @@ -124,4 +128,21 @@ fn main() { for _ in 0..20 { vec17.push(item); } + + let mut vec18 = Vec::new(); + let item = 42; + let item = fun(); + for _ in 0..20 { + vec18.push(item); + } + + let mut vec19 = Vec::new(); + let key = 1; + for _ in 0..20 { + let item = match key { + 1 => 10, + _ => 0, + }; + vec19.push(item); + } } From 952c04674e4225d231f0ba78dff32abf7c06cb8b Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 8 Sep 2020 23:03:15 +0900 Subject: [PATCH 5/6] Refactoring: tests in `same_item_push` --- tests/ui/same_item_push.rs | 99 +++++++++++++++++----------------- tests/ui/same_item_push.stderr | 40 +++++++------- 2 files changed, 71 insertions(+), 68 deletions(-) diff --git a/tests/ui/same_item_push.rs b/tests/ui/same_item_push.rs index 7ca829854db1..a37c8782ec33 100644 --- a/tests/ui/same_item_push.rs +++ b/tests/ui/same_item_push.rs @@ -16,64 +16,76 @@ fn fun() -> usize { } 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(); + // ** linted cases ** + let mut vec: Vec = Vec::new(); let item = 2; for _ in 5..=20 { - vec2.push(item); + vec.push(item); } - let mut vec3: Vec = Vec::new(); + let mut vec: Vec = Vec::new(); for _ in 0..15 { let item = 2; - vec3.push(item); + vec.push(item); } - let mut vec4: Vec = Vec::new(); + let mut vec: Vec = Vec::new(); for _ in 0..15 { - vec4.push(13); + vec.push(13); + } + + let mut vec = Vec::new(); + for _ in 0..20 { + vec.push(VALUE); + } + + let mut vec = Vec::new(); + let item = VALUE; + for _ in 0..20 { + vec.push(item); + } + + // ** non-linted cases ** + let mut spaces = Vec::with_capacity(10); + for _ in 0..10 { + spaces.push(vec![b' ']); } // Suggestion should not be given as pushed variable can mutate - let mut vec5: Vec = Vec::new(); + let mut vec: Vec = Vec::new(); let mut item: u8 = 2; for _ in 0..30 { - vec5.push(mutate_increment(&mut item)); + vec.push(mutate_increment(&mut item)); } - let mut vec6: Vec = Vec::new(); + let mut vec: 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)); + vec.push(mutate_increment(item2)); } - let mut vec7: Vec = Vec::new(); + let mut vec: Vec = Vec::new(); for (a, b) in [0, 1, 4, 9, 16].iter().enumerate() { - vec7.push(a); + vec.push(a); } - let mut vec8: Vec = Vec::new(); + let mut vec: Vec = Vec::new(); for i in 0..30 { - vec8.push(increment(i)); + vec.push(increment(i)); } - let mut vec9: Vec = Vec::new(); + let mut vec: Vec = Vec::new(); for i in 0..30 { - vec9.push(i + i * i); + vec.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 mut vec: Vec = Vec::new(); let item: u8 = 2; for _ in 0..30 { - vec10.push(item); - vec10.push(item * 2); + vec.push(item); + vec.push(item * 2); } // Suggestion should not be given as Vec is not involved @@ -88,23 +100,23 @@ fn main() { for i in 0..30 { vec_a.push(A { kind: i }); } - let mut vec12: Vec = Vec::new(); + let mut vec: Vec = Vec::new(); for a in vec_a { - vec12.push(2u8.pow(a.kind)); + vec.push(2u8.pow(a.kind)); } // Fix #5902 - let mut vec13: Vec = Vec::new(); + let mut vec: Vec = Vec::new(); let mut item = 0; for _ in 0..10 { - vec13.push(item); + vec.push(item); item += 10; } // Fix #5979 - let mut vec14: Vec = Vec::new(); + let mut vec: Vec = Vec::new(); for _ in 0..10 { - vec14.push(std::fs::File::open("foobar").unwrap()); + vec.push(std::fs::File::open("foobar").unwrap()); } // Fix #5979 #[derive(Clone)] @@ -113,36 +125,27 @@ fn main() { trait T {} impl T for S {} - let mut vec15: Vec> = Vec::new(); + let mut vec: Vec> = Vec::new(); for _ in 0..10 { - vec15.push(Box::new(S {})); - } - - let mut vec16 = Vec::new(); - for _ in 0..20 { - vec16.push(VALUE); - } - - let mut vec17 = Vec::new(); - let item = VALUE; - for _ in 0..20 { - vec17.push(item); + vec.push(Box::new(S {})); } - let mut vec18 = Vec::new(); + // Fix #5985 + let mut vec = Vec::new(); let item = 42; let item = fun(); for _ in 0..20 { - vec18.push(item); + vec.push(item); } - let mut vec19 = Vec::new(); + // Fix #5985 + let mut vec = Vec::new(); let key = 1; for _ in 0..20 { let item = match key { 1 => 10, _ => 0, }; - vec19.push(item); + vec.push(item); } } diff --git a/tests/ui/same_item_push.stderr b/tests/ui/same_item_push.stderr index 4896479791af..d9ffa15780ad 100644 --- a/tests/ui/same_item_push.stderr +++ b/tests/ui/same_item_push.stderr @@ -1,43 +1,43 @@ error: it looks like the same item is being pushed into this Vec - --> $DIR/same_item_push.rs:24:9 + --> $DIR/same_item_push.rs:23:9 | -LL | vec2.push(item); - | ^^^^ +LL | vec.push(item); + | ^^^ | = note: `-D clippy::same-item-push` implied by `-D warnings` - = help: try using vec![item;SIZE] or vec2.resize(NEW_SIZE, item) + = help: try using vec![item;SIZE] or vec.resize(NEW_SIZE, item) error: it looks like the same item is being pushed into this Vec - --> $DIR/same_item_push.rs:30:9 + --> $DIR/same_item_push.rs:29:9 | -LL | vec3.push(item); - | ^^^^ +LL | vec.push(item); + | ^^^ | - = help: try using vec![item;SIZE] or vec3.resize(NEW_SIZE, item) + = help: try using vec![item;SIZE] or vec.resize(NEW_SIZE, item) error: it looks like the same item is being pushed into this Vec - --> $DIR/same_item_push.rs:35:9 + --> $DIR/same_item_push.rs:34:9 | -LL | vec4.push(13); - | ^^^^ +LL | vec.push(13); + | ^^^ | - = help: try using vec![13;SIZE] or vec4.resize(NEW_SIZE, 13) + = help: try using vec![13;SIZE] or vec.resize(NEW_SIZE, 13) error: it looks like the same item is being pushed into this Vec - --> $DIR/same_item_push.rs:119:9 + --> $DIR/same_item_push.rs:39:9 | -LL | vec16.push(VALUE); - | ^^^^^ +LL | vec.push(VALUE); + | ^^^ | - = help: try using vec![VALUE;SIZE] or vec16.resize(NEW_SIZE, VALUE) + = help: try using vec![VALUE;SIZE] or vec.resize(NEW_SIZE, VALUE) error: it looks like the same item is being pushed into this Vec - --> $DIR/same_item_push.rs:125:9 + --> $DIR/same_item_push.rs:45:9 | -LL | vec17.push(item); - | ^^^^^ +LL | vec.push(item); + | ^^^ | - = help: try using vec![item;SIZE] or vec17.resize(NEW_SIZE, item) + = help: try using vec![item;SIZE] or vec.resize(NEW_SIZE, item) error: aborting due to 5 previous errors From 1d8ae3aa12567b8461436ac10ba9fc4da55adb29 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 8 Sep 2020 23:12:04 +0900 Subject: [PATCH 6/6] Address `items_after_statement` --- clippy_lints/src/loops.rs | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index c59185bd7bd1..6c54c07869ad 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1131,6 +1131,23 @@ fn detect_same_item_push<'tcx>( 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; } @@ -1192,23 +1209,6 @@ fn detect_same_item_push<'tcx>( } } } - - 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 - ), - ) - } } /// Checks for looping over a range and then indexing a sequence with it.