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.

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

FP redundant_clone: Box #10517

Open
matthiaskrgr opened this issue Mar 17, 2023 · 9 comments
Open

FP redundant_clone: Box #10517

matthiaskrgr opened this issue Mar 17, 2023 · 9 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@matthiaskrgr
Copy link
Member

matthiaskrgr commented Mar 17, 2023

Summary

.

Lint Name

redundant_clone

Reproducer

I tried this code:

pub fn main() {
    let i: Box<_> = Box::new(1);
    let j = i.clone();
    assert_eq!(*i, 2);
}

I saw this happen:

warning: redundant clone
 --> unique-decl-init-copy.rs:3:14
  |
3 |     let j = i.clone();
  |              ^^^^^^^^ help: remove this
  |
note: cloned value is neither consumed nor mutated
 --> unique-decl-init-copy.rs:3:13
  |
3 |     let j = i.clone();
  |             ^^^^^^^^^
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
  = note: `#[warn(clippy::redundant_clone)]` on by defaul

But

pub fn main() {
    let i: Box<_> = Box::new(1);
    let j = i;
    assert_eq!(*i, 2);
}

does not compile

warning: unused variable: `j`
 --> unique-decl-init-copy.rs:3:9
  |
3 |     let j = i;
  |         ^ help: if this is intentional, prefix it with an underscore: `_j`
  |
  = note: `#[warn(unused_variables)]` on by default

error[E0382]: borrow of moved value: `i`
 --> unique-decl-init-copy.rs:4:5
  |
2 |     let i: Box<_> = Box::new(1);
  |         - move occurs because `i` has type `std::boxed::Box<i32>`, which does not implement the `Copy` trait
3 |     let j = i;
  |             - value moved here
4 |     assert_eq!(*i, 2);
  |     ^^^^^^^^^^^^^^^^^ value borrowed here after move
  |
  = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider cloning the value if the performance cost is acceptable
  |
3 |     let j = i.clone();
  |              ++++++++

error: aborting due to previous error; 1 warning emitted

Version

rustc 1.70.0-nightly (511364e78 2023-03-16)
binary: rustc
commit-hash: 511364e7874dba9649a264100407e4bffe7b5425
commit-date: 2023-03-16
host: x86_64-unknown-linux-gnu
release: 1.70.0-nightly
LLVM version: 15.0.7

Additional Labels

No response

@matthiaskrgr matthiaskrgr added C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied I-false-positive Issue: The lint was triggered on code it shouldn't have labels Mar 17, 2023
@matthiaskrgr
Copy link
Member Author

This seems to be a regression, at least on beta, there is no such warning, hm.

@matthiaskrgr
Copy link
Member Author

matthiaskrgr commented Mar 17, 2023

Hmm, nightly-2023-03-15-x86_64-unknown-linux-gnu doesn't have this FP while rustc 1.70.0-nightly (511364e78 2023-03-16) does

@matthiaskrgr
Copy link
Member Author

Ooh, I bet this broke in rust-lang/rust#108958 cc @clubby789

@clubby789
Copy link
Contributor

@rustbot claim

@clubby789
Copy link
Contributor

clubby789 commented Mar 17, 2023

Hmm. I'm not sure how this specifically could have broken it - that PR pretty much just removes it from match arms, the expression itself hasn't emitted since I removed it in rust-lang/rust#108516. I'll try a bisect


searched nightlies: from nightly-2023-03-15 to nightly-2023-03-17
regressed nightly: nightly-2023-03-17
searched commit range: rust-lang/rust@ab65486...511364e
regressed commit: rust-lang/rust@511364e

bisected with cargo-bisect-rustc v0.6.5

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc 2023-03-15 --end 2023-03-17 --script=./script.sh -vv 

@clubby789
Copy link
Contributor

clubby789 commented Mar 17, 2023

cc @cjgillot

@rustbot release

@clubby789 clubby789 removed their assignment Mar 17, 2023
@matthiaskrgr
Copy link
Member Author

Oh my bad then, sorry 😅 this looked like the obvious contender to me since it removed box stuff and I thought maybe it removed a "saveguard" when linting against boxes...

@Jarcho
Copy link
Contributor

Jarcho commented Mar 18, 2023

I remember seeing this come up before, but can't find the issue now. This isn't a false positive as the new value isn't used, but the suggestion should be to remove the whole binding.

@Jarcho Jarcho removed the I-false-positive Issue: The lint was triggered on code it shouldn't have label Mar 18, 2023
@dswij dswij added the L-suggestion Lint: Improving, adding or fixing lint suggestions label Mar 22, 2023
@ryoqun
Copy link

ryoqun commented May 18, 2023

I think this issue's root cause is same as this (see my cross post: #10577 (comment))

pub fn main() {
    let i: Box<_> = Box::new(1);
    let j = i.clone();
    assert_eq!(*i, 2);
}

this is fine with +nightly-2023-03-16-x86_64-unknown-linux-gnu but starts to emit false positives with +nightly-2023-03-17-x86_64-unknown-linux-gnu

then again, i think this pr is suspicious: rust-lang/rust#108944

bors added a commit that referenced this issue Jun 5, 2023
Move `redundant_clone` to `nursery`

changelog: [`redundant_clone`]: Move to `nursery`

A bunch of FPs in `redundant_clone` have sprung up after upstream MIR changes: rust-lang/rust#108944

- #10870
- #10577
- #10545
- #10517

r? `@flip1995`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants