False positive on redundant clone: borrow of moved value

[garritfra]

Summary

Clippy falsely reports a "redundant clone" warning. Removing the clone leads to a borrow of moved value error.

This bug occurs on 1.70.0 as well as on nightly.

Reproducer

I tried this code:

fn generate_for_loop(ident: Variable, expr: Expression, body: Statement) -> String {
    // Assign expression to variable to access it from within the loop
    let mut expr_ident = ident.clone();
    expr_ident.name = format!("loop_orig_{}", ident.name);
    let mut out_str = format!("{};\n", generate_declare(&expr_ident, Some(expr)));

    // Loop signature
    out_str += &format!(
        "for (let iter_{I} = 0; iter_{I} < {E}.length; iter_{I}++)",
        I = ident.name,
        E = expr_ident.name
    );

    // Block with prepended declaration of the actual variable
    out_str += &generate_block(
        body,
        Some(format!(
            "let {I} = {E}[iter_{I}];\n",
            I = ident.name,
            E = expr_ident.name,
        )),
    );
    out_str
}

I expected to see this happen: The code to run successfully and apply fixes suggested by cargo clippy --fix.

Instead, this happened: I encountered the following error:

warning: failed to automatically apply fixes suggested by rustc to crate `sb`

after fixes were automatically applied the compiler reported errors within these files:

  * src/generator/js.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see 
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust-clippy/issues
quoting the full output of this command we'd be very appreciative!
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

The following errors were reported:
error[E0382]: borrow of moved value: `ident`
   --> src/generator/js.rs:188:47
    |
185 | fn generate_for_loop(ident: Variable, expr: Expression, body: Statement) -> String {
    |                      ----- move occurs because `ident` has type `ast::Variable`, which does not implement the `Copy` trait
186 |     // Assign expression to variable to access it from within the loop
187 |     let mut expr_ident = ident;
    |                          ----- value moved here
188 |     expr_ident.name = format!("loop_orig_{}", ident.name);
    |                                               ^^^^^^^^^^ value borrowed here after move
    |
    = note: this error originates in the macro `$crate::__export::format_args` which comes from the expansion of the macro `format` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider cloning the value if the performance cost is acceptable
    |
187 |     let mut expr_ident = ident.clone();
    |                               ++++++++

error: aborting due to previous error

For more information about this error, try `rustc --explain E0382`.
Original diagnostics will follow.

warning: redundant clone
   --> src/generator/js.rs:187:31
    |
187 |     let mut expr_ident = ident.clone();
    |                               ^^^^^^^^ help: remove this
    |
note: cloned value is neither consumed nor mutated
   --> src/generator/js.rs:187:

26
    |
187 |     let mut expr_ident = ident.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 default

warning: `antimony-lang` (bin "sb") generated 1 warning (run `cargo clippy --fix --bin "sb"` to apply 1 suggestion)

Version

rustc 1.72.0-nightly (df77afbca 2023-06-12)
binary: rustc
commit-hash: df77afbcaf3365a32066a8ca4a00ae6fc9a69647
commit-date: 2023-06-12
host: aarch64-apple-darwin
release: 1.72.0-nightly
LLVM version: 16.0.5

Additional Labels

@rustbot label +I-suggestion-causes-error

[KisaragiEffective]

@rustbot label I-false-positive

[ndrewxie]

Hmm, appears to have something to do with the format! macro.

The following snippet does NOT generate a redundant clone warning:

#[derive(Clone)]
struct Variable {
    name: String,
}
fn generate_for_loop(ident: Variable) -> String {
    let mut expr_ident = ident.clone();
    expr_ident.name = "testing".to_owned();
    ident.name
}
fn main() {}

However, changing the return ident.name to format!("{}", ident.name) yields the desired lint

My (admittedly not-very-well-informed) theory is that the use of format! prevents clippy from noticing that both expr_ident and ident are being used - perhaps because the format_args! (which format! calls) is a compiler built-in, so it likely doesn't expand to anything that clippy would notice as "using the value".

I'd like to take a shot at this: @rustbot claim

[Alexendoo]

Thanks for the small snippet. I think this is likely more complicated though, some of the other reports contain no macro calls - #10873

[ndrewxie]

Aah I see, it is indeed more complicated. I made a code snippet that lets me "toggle" the error on and off by changing whether Asdf derives Copy or not:

#![deny(clippy::redundant_clone)]

#[derive(Clone, Copy)]
pub struct Asdf {}

#[derive(Clone)]
pub struct Foo(Asdf);

pub fn foo(a: Foo) -> Asdf {
    let mut b = a.clone();
    b.0 = Asdf {};
    a.0
}

fn main() {}

And what I've found is that visit_local_usage is not detecting properly mutations to the cloned value... However, this is where the clippy codebase ends and the rustc codebase begins, which doesn't seem like something that would be buggy... So perhaps it's time to take a step back and see if I'm missing something 😅

Attached is a snippet of the diff of some debug output for the case where Asdf is Copy (emits the redundant clone warning, on the left) vs the case where Asdf is not Copy (doesn't emit a warning, on the right):

(the debug line was added in clippy_utils/src/mir/mod.rs::visit_local_usage, inside of the try_fold, and just prints out the basic block ID and the basic block data).

[ndrewxie]

Just clicked through the issue you linked in more depth, and it appears y'all have already found the root issue. If there's anything left to do that I can help with, I'd love to join in, otherwise I'll just leave this here and find something else to do :)

[bernardosulzbach]

This is hitting me particularly hard. I'm running

clippy 0.1.72 (f7ca9df 2023-06-24)

that I guess includes the recent move to nursery. Shouldn't that have removed the warning?

I also tested with a new Cargo project, and the warning is still present with my simplified reproduction below.

#[derive(Clone, Eq, PartialEq)]
struct T {}

impl T {
    fn b(&self) -> bool {
        false
    }

    fn f(&self) -> T {
        T {}
    }
}

fn main() {
    let x = T {};
    let mut y = x.clone();
    while y.b() {
        y = y.f();
    }
    println!("{}", y == x);
}

Do I need to manually disable this warning?

[Alexendoo]

Nightly doesn't have that change yet, the rust-lang/rust-clippy -> rust-lang/rust sync is currently blocked (rust-lang/rust#112708), after that is resolved it will be in the nightly after the next sync

In the meanwhile yeah you can manually disable it

[FirelightFlagboy]

I've a similar error with this sample code on rust 1.70.0

fn test() {
    let foo1 = String::from("I'm a simple string");
    let foo2 = foo1.clone();

    assert_eq!(foo1, foo2);
}

Here is the output of cargo clippy -p redundant-clone --fix --allow-dirty

    Checking redundant-clone v0.1.0 (/home/[REDACTED]/Documents/parsec-cloud/redundant-clone)
warning: failed to automatically apply fixes suggested by rustc to crate `redundant_clone`

after fixes were automatically applied the compiler reported errors within these files:

  * /home/[REDACTED]/.rustup/toolchains/1.70.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/macros/mod.rs
  * redundant-clone/src/lib.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see 
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust/issues
quoting the full output of this command we'd be very appreciative!
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

The following errors were reported:
error[E0382]: borrow of moved value: `foo1`
 --> redundant-clone/src/lib.rs:5:5
  |
2 |     let foo1 = String::from("I'm a simple string");
  |         ---- move occurs because `foo1` has type `std::string::String`, which does not implement the `Copy` trait
3 |     let foo2 = foo1;
  |                ---- value moved here
4 |
5 |     assert_eq!(foo1, foo2);
  |     ^^^^^^^^^^^^^^^^^^^^^^ 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 foo2 = foo1.clone();
  |                    ++++++++

error: aborting due to previous error

For more information about this error, try `rustc --explain E0382`.
Original diagnostics will follow.

warning: redundant clone
 --> redundant-clone/src/lib.rs:3:20
  |
3 |     let foo2 = foo1.clone();
  |                    ^^^^^^^^ help: remove this
  |
note: cloned value is neither consumed nor mutated
 --> redundant-clone/src/lib.rs:3:16
  |
3 |     let foo2 = foo1.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 default

warning: `redundant-clone` (lib test) generated 1 warning (run `cargo clippy --fix --lib -p redundant-clone --tests` to apply 1 suggestion)
warning: `redundant-clone` (lib) generated 1 warning (1 duplicate)
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s

[Parth]

I'm no longer seeing this on 1.71

[garritfra]

Can confirm. Fixed in 1.71

[Jarcho]

This is not fixed. redundant_clone is just allowed by default.