Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Restrict same_item_push to suppress false positives #6016

Merged
merged 6 commits into from
Sep 8, 2020
Merged

Restrict same_item_push to suppress false positives #6016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3d30ef7
Restrict `same_item_push` to suppress false positives
giraffate Sep 7, 2020
619ca76
Refactoring: use inner function
giraffate Sep 7, 2020
5d085ad
Some refactoring
giraffate Sep 7, 2020
2c9f82e
Add some tests to `same_item_push`
giraffate Sep 8, 2020
952c046
Refactoring: tests in `same_item_push`
giraffate Sep 8, 2020
1d8ae3a
Address `items_after_statement`
giraffate Sep 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 55 additions & 36 deletions clippy_lints/src/loops.rs
View file
Open in desktop
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! {
ebroto marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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 {}));
}

ebroto marked this conversation as resolved.
Show resolved Hide resolved
// 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);
}
ebroto marked this conversation as resolved.
Show resolved Hide resolved
}
42 changes: 25 additions & 17 deletions tests/ui/same_item_push.stderr
View file
Open in desktop
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