Skip to content

False positive on redundant clone: borrow of moved value #10940

New issue
New issue
Open
Open
False positive on redundant clone: borrow of moved value#10940
Labels
C-bugCategory: Clippy is not doing the correct thingI-false-positiveIssue: The lint was triggered on code it shouldn't haveI-suggestion-causes-errorIssue: The suggestions provided by this Lint cause an ICE/error when applied
@garritfra

Description

@garritfra
garritfra
opened on Jun 13, 2023

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

Activity

garritfra
added
C-bugCategory: Clippy is not doing the correct thing
on Jun 13, 2023
rustbot
added
I-suggestion-causes-errorIssue: The suggestions provided by this Lint cause an ICE/error when applied
on Jun 13, 2023
garritfra
mentioned this on Jun 13, 2023
KisaragiEffective

KisaragiEffective commented on Jun 14, 2023

@KisaragiEffective
KisaragiEffective
on Jun 14, 2023 · edited by KisaragiEffective
Contributor

@rustbot label I-false-positive

rustbot
added
I-false-positiveIssue: The lint was triggered on code it shouldn't have
on Jun 14, 2023
ndrewxie

ndrewxie commented on Jun 14, 2023

@ndrewxie
ndrewxie
on Jun 14, 2023 · edited by ndrewxie
Contributor

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

rustbot
assigned
ndrewxie
on Jun 14, 2023
Alexendoo

Alexendoo commented on Jun 14, 2023

@Alexendoo
Alexendoo
on Jun 14, 2023
Member

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

ndrewxie

ndrewxie commented on Jun 15, 2023

@ndrewxie
ndrewxie
on Jun 15, 2023 · edited by ndrewxie
Contributor

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):
image

(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

ndrewxie commented on Jun 15, 2023

@ndrewxie
ndrewxie
on Jun 15, 2023 · edited by ndrewxie
Contributor

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 :)

ndrewxie
removed their assignment
on Jun 18, 2023
bernardosulzbach

bernardosulzbach commented on Jun 25, 2023

@bernardosulzbach
bernardosulzbach
on Jun 25, 2023

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

Alexendoo commented on Jun 25, 2023

@Alexendoo
Alexendoo
on Jun 25, 2023
Member

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

Parth
mentioned this on Jun 28, 2023
FirelightFlagboy

FirelightFlagboy commented on Jul 11, 2023

@FirelightFlagboy
FirelightFlagboy
on Jul 11, 2023

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

9 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-bugCategory: Clippy is not doing the correct thingI-false-positiveIssue: The lint was triggered on code it shouldn't haveI-suggestion-causes-errorIssue: The suggestions provided by this Lint cause an ICE/error when applied

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions
      False positive on redundant clone: borrow of moved value · Issue #10940 · rust-lang/rust-clippy