Skip to content

Commit

Permalink
Auto merge of #6016 - giraffate:restrict_same_item_push, r=ebroto
Browse files Browse the repository at this point in the history 
Restrict `same_item_push` to suppress false positives

It only emits a lint when the pushed item is a literal, a constant and an immutable binding that are initialized with those, as discussed in #5997 (review).

Fix #5985

changelog: Restrict `same_item_push` to literals, constants and immutable bindings that are initialized with those.

r? `@ebroto`
  • Loading branch information
@bors
bors committed Sep 8, 2020
2 parents 8d6cf3a + 1d8ae3a commit 8b54f1e
Show file tree
Hide file tree
Showing 3 changed files with 150 additions and 86 deletions.
91 changes: 55 additions & 36 deletions clippy_lints/src/loops.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1131,6 +1131,27 @@ 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;
}

// Determine whether it is safe to lint the body
let mut same_item_push_visitor = SameItemPushVisitor {
should_lint: true,
Expand All @@ -1149,43 +1170,41 @@ 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 {
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 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) => {
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, ..) = qpath_res(cx, path, init.hir_id) {
emit_lint(cx, vec, pushed_item);
}
}
_ => {},
}
}
}
},
// constant
Res::Def(DefKind::Const, ..) => emit_lint(cx, vec, pushed_item),
_ => {},
}
} 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
),
)
}
},
ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item),
_ => {},
}
}
}
Expand Down
103 changes: 70 additions & 33 deletions tests/ui/same_item_push.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#![warn(clippy::same_item_push)]

const VALUE: u8 = 7;

fn mutate_increment(x: &mut u8) -> u8 {
*x += 1;
*x
Expand All @@ -9,65 +11,81 @@ 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' ']);
}
fn fun() -> usize {
42
}

let mut vec2: Vec<u8> = Vec::new();
fn main() {
// ** linted cases **
let mut vec: Vec<u8> = Vec::new();
let item = 2;
for _ in 5..=20 {
vec2.push(item);
vec.push(item);
}

let mut vec3: Vec<u8> = Vec::new();
let mut vec: Vec<u8> = Vec::new();
for _ in 0..15 {
let item = 2;
vec3.push(item);
vec.push(item);
}

let mut vec4: Vec<u8> = Vec::new();
let mut vec: Vec<u8> = 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<u8> = Vec::new();
let mut vec: Vec<u8> = 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<u8> = Vec::new();
let mut vec: Vec<u8> = 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<usize> = Vec::new();
let mut vec: Vec<usize> = Vec::new();
for (a, b) in [0, 1, 4, 9, 16].iter().enumerate() {
vec7.push(a);
vec.push(a);
}

let mut vec8: Vec<u8> = Vec::new();
let mut vec: Vec<u8> = Vec::new();
for i in 0..30 {
vec8.push(increment(i));
vec.push(increment(i));
}

let mut vec9: Vec<u8> = Vec::new();
let mut vec: Vec<u8> = 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<u8> = Vec::new();
let mut vec: Vec<u8> = 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
Expand All @@ -82,23 +100,23 @@ fn main() {
for i in 0..30 {
vec_a.push(A { kind: i });
}
let mut vec12: Vec<u8> = Vec::new();
let mut vec: Vec<u8> = 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<u8> = Vec::new();
let mut vec: Vec<u8> = Vec::new();
let mut item = 0;
for _ in 0..10 {
vec13.push(item);
vec.push(item);
item += 10;
}

// Fix #5979
let mut vec14: Vec<std::fs::File> = Vec::new();
let mut vec: Vec<std::fs::File> = 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)]
Expand All @@ -107,8 +125,27 @@ fn main() {
trait T {}
impl T for S {}

let mut vec15: Vec<Box<dyn T>> = Vec::new();
let mut vec: Vec<Box<dyn T>> = Vec::new();
for _ in 0..10 {
vec15.push(Box::new(S {}));
vec.push(Box::new(S {}));
}

// Fix #5985
let mut vec = Vec::new();
let item = 42;
let item = fun();
for _ in 0..20 {
vec.push(item);
}

// Fix #5985
let mut vec = Vec::new();
let key = 1;
for _ in 0..20 {
let item = match key {
1 => 10,
_ => 0,
};
vec.push(item);
}
}
42 changes: 25 additions & 17 deletions tests/ui/same_item_push.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,35 +1,43 @@
error: it looks like the same item is being pushed into this Vec
--> $DIR/same_item_push.rs:16:9
--> $DIR/same_item_push.rs:23:9
|
LL | spaces.push(vec![b' ']);
| ^^^^^^
LL | vec.push(item);
| ^^^
|
= note: `-D clippy::same-item-push` implied by `-D warnings`
= help: try using vec![vec![b' '];SIZE] or spaces.resize(NEW_SIZE, vec![b' '])
= 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:22:9
--> $DIR/same_item_push.rs:29:9
|
LL | vec2.push(item);
| ^^^^
LL | vec.push(item);
| ^^^
|
= 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:28:9
--> $DIR/same_item_push.rs:34:9
|
LL | vec3.push(item);
| ^^^^
LL | vec.push(13);
| ^^^
|
= help: try using vec![item;SIZE] or vec3.resize(NEW_SIZE, item)
= 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:33:9
--> $DIR/same_item_push.rs:39:9
|
LL | vec4.push(13);
| ^^^^
LL | vec.push(VALUE);
| ^^^
|
= help: try using vec![13;SIZE] or vec4.resize(NEW_SIZE, 13)
= help: try using vec![VALUE;SIZE] or vec.resize(NEW_SIZE, VALUE)

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:45:9
|
LL | vec.push(item);
| ^^^
|
= help: try using vec![item;SIZE] or vec.resize(NEW_SIZE, item)

error: aborting due to 5 previous errors

0 comments on commit 8b54f1e

Please sign in to comment.