From 3d30ef7818b9ed562f2c48f3d6d15d90253ae732 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Mon, 7 Sep 2020 22:17:31 +0900 Subject: [PATCH] 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 f13e369907b..f417e3a0caf 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 0928820892b..bd4792c4a76 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 ddc5d48cd41..4896479791a 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