From aa7ffa5257667edb284de16b529df3d4111d70ab Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 1 Sep 2020 22:39:09 +0900 Subject: [PATCH 1/2] Fix FP in `same_item_push` Don't emit a lint when the pushed item doesn't have Clone trait --- clippy_lints/src/loops.rs | 78 +++++++++++++++++++++----------------- tests/ui/same_item_push.rs | 6 +++ 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index c95e43a94304..25345f8fa316 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1140,43 +1140,51 @@ fn detect_same_item_push<'tcx>( 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 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 { - 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 - ), - ) + let ty = cx.typeck_results().expr_ty(pushed_item); + 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 + 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 { + 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 + ), + ) + } } + } else if mutated_variables(pushed_item, cx).map_or(false, |mutvars| mutvars.is_empty()) { + 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()) { - 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 - ), - ) } } } diff --git a/tests/ui/same_item_push.rs b/tests/ui/same_item_push.rs index bfe27e020445..0903a8738265 100644 --- a/tests/ui/same_item_push.rs +++ b/tests/ui/same_item_push.rs @@ -94,4 +94,10 @@ fn main() { vec13.push(item); item += 10; } + + // Fix #5979 + let mut vec14: Vec = Vec::new(); + for _ in 0..10 { + vec14.push(std::fs::File::open("foobar").unwrap()); + } } From 96b31a5b36ed06b6781804d3384a3a84a52b8ce6 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Sun, 6 Sep 2020 00:02:35 +0900 Subject: [PATCH 2/2] Fix FP when coercion kicks in --- clippy_lints/src/loops.rs | 3 ++- tests/ui/same_item_push.rs | 11 +++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 25345f8fa316..b65ae15edcd9 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1140,7 +1140,8 @@ fn detect_same_item_push<'tcx>( 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 ty = cx.typeck_results().expr_ty(pushed_item); + let vec_ty = cx.typeck_results().expr_ty(vec); + let ty = vec_ty.walk().nth(1).unwrap().expect_ty(); if cx .tcx .lang_items() diff --git a/tests/ui/same_item_push.rs b/tests/ui/same_item_push.rs index 0903a8738265..0928820892b1 100644 --- a/tests/ui/same_item_push.rs +++ b/tests/ui/same_item_push.rs @@ -100,4 +100,15 @@ fn main() { for _ in 0..10 { vec14.push(std::fs::File::open("foobar").unwrap()); } + // Fix #5979 + #[derive(Clone)] + struct S {} + + trait T {} + impl T for S {} + + let mut vec15: Vec> = Vec::new(); + for _ in 0..10 { + vec15.push(Box::new(S {})); + } }