Fix FP in same_item_push

[giraffate]

Don't emit a lint when the pushed item doesn't have Clone trait

Fix #5979

changelog: Fix FP in same_item_push not to emit a lint when the pushed item doesn't have Clone trait

[rust-highfive]

r? @flip1995

(rust_highfive has picked a reviewer for you, use r? to override)

[ebroto]

r? @ebroto

(Will dig into this later)

[ebroto]

Besides the requested change, I think we should restrict same_item_push to just literals and constants (and immutable bindings that are initialized with those). Allowing the pushed item to be an arbitrary expression is very difficult to get right as there are many possibilities for false positives. For example, the very same function used in the test can return different values when called multiple times.

Would you be up to do that change, either in this PR or in a follow-up?

[giraffate]

@ebroto Thanks for your comment! I agree with restricting same_item_push to suppress many false positives.

I think we should restrict same_item_push to just literals and constants (and immutable bindings that are initialized with those).

I have a question. In the above restriction, does this test emit a lint? This pattern is mentioned in the original issue #4078 of same_item_push.

rust-clippy/tests/ui/same_item_push.rs

Lines 13 to 17 in f98ffa2

	// Test for basic case
	let mut spaces = Vec::with_capacity(10);
	for _ in 0..10 {
	spaces.push(vec![b' ']);
	}

Would you be up to do that change, either in this PR or in a follow-up?

I will prepare a follow-up PR.

[ebroto]

I have a question. In the above restriction, does this test emit a lint? This pattern is mentioned in the original issue #4078 of same_item_push.

I think we should keep it simple for now and not special case this. We can always extend the lint later, but IMO the simpler approach will still be valuable to those that are not familiar with resize() and vec![x; N].

These are the cases I had in mind:

const VALUE: usize = 42; // doesn't matter how it's initialized

fn main() {
    let mut vec = Vec::new();
    for _ in 0..20 {
        vec.push(42); // literal
    }

    let mut vec = Vec::new();
    for _ in 0..20 {
        vec.push(VALUE); // constant
    }

    // For bindings, we should check if shadowing is involved

    let mut vec = Vec::new();
    let x = 42; // immutable, initialized with literal, could be inside the loop
    for _ in 0..20 {
        vec.push(x);
    }

    let mut vec = Vec::new();
    let x = VALUE; // immutable, initialized with constant, could be inside the loop
    for _ in 0..20 {
        vec.push(x);
    }
}

(playground)

I will prepare a follow-up PR.

Great, thanks in advance!

[ebroto]

@bors r+

Thanks!

[bors]

📌 Commit 96b31a5 has been approved by ebroto

[bors]

⌛ Testing commit 96b31a5 with merge e9440cb...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing e9440cb to master...

[giraffate]

I think we should keep it simple for now and not special case this. We can always extend the lint later, but IMO the simpler approach will still be valuable to those that are not familiar with resize() and vec![x; N].

Ok, I agree with you. Thanks!