Warning on `redundant_clone` when required with scopes

[JonathanWoollett-Light]

Summary

The following code requires that let mut state = orig_state.clone(); is cloned.

        {
            // The frequency difference is within tolerance.
            let mut state = orig_state.clone();
            state.tsc_khz = Some(state.tsc_khz.unwrap() + (TSC_KHZ_TOL / 2.0).round() as u32);
            assert!(!vcpu
                .is_tsc_scaling_required(state.tsc_khz.unwrap())
                .unwrap());
        }

        {
            // The frequency difference is over the tolerance.
            let mut state = orig_state;
            state.tsc_khz = Some(state.tsc_khz.unwrap() + (TSC_KHZ_TOL * 2.0).round() as u32);
            assert!(!vcpu
                .is_tsc_scaling_required(state.tsc_khz.unwrap())
                .unwrap());
        }

Without it an error is emitted:

error[E0382]: use of moved value: `orig_state`
   --> src/vmm/src/vstate/vcpu/x86_64.rs:919:29
    |
906 |         let orig_state = vcpu.save_state().unwrap();
    |             ---------- move occurs because `orig_state` has type `vstate::vcpu::x86_64::VcpuState`, which does not implement the `Copy` trait
...
910 |             let mut state = orig_state;
    |                             ---------- value moved here
...
919 |             let mut state = orig_state;
    |                             ^^^^^^^^^^ value used here after move
    |
help: consider cloning the value if the performance cost is acceptable
    |
910 |             let mut state = orig_state.clone();
    |                                       ++++++++

For more information about this error, try `rustc --explain E0382`.

With it, clippy emits a warning:

warning: redundant clone
   --> src/vmm/src/vstate/vcpu/x86_64.rs:910:39
    |
910 |             let mut state = orig_state.clone();
    |                                       ^^^^^^^^ help: remove this
    |
note: cloned value is neither consumed nor mutated
   --> src/vmm/src/vstate/vcpu/x86_64.rs:910:29
    |
910 |             let mut state = orig_state.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: `vmm` (lib test) generated 1 warning (run `cargo clippy --fix --lib -p vmm --tests` to apply 1 suggestion)

See: https://github.com/firecracker-microvm/firecracker/blob/main/src/vmm/src/vstate/vcpu/x86_64.rs#L908:L924

Lint Name

clippy::redundant_clone

Version

rustc 1.70.0 (90c541806 2023-05-31)
binary: rustc
commit-hash: 90c541806f23a127002de5b4038be731ba1458ca
commit-date: 2023-05-31
host: x86_64-unknown-linux-gnu
release: 1.70.0
LLVM version: 16.0.2

Additional Labels

No response

[whfuyn]

I encountered a similar problem and have created a minimal reproducible example for my case:

#[derive(Clone)]
struct A {
    a: u32,
    b: u32,
}

fn main(){
    let a = A { a: 42, b: 0 };
    let mut aa = a.clone();
    aa.b = 1024;
    dbg!(&a.a);
}

Playground

Tools -> Clippy

    Checking playground v0.0.1 (/playground)
warning: redundant clone
  --> src/main.rs:10:19
   |
10 |     let mut aa = a.clone();
   |                   ^^^^^^^^ help: remove this
   |
note: cloned value is neither consumed nor mutated
  --> src/main.rs:10:18
   |
10 |     let mut aa = a.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: `playground` (bin "playground") generated 1 warning (run `cargo clippy --fix --bin "playground"` to apply 1 suggestion)
    Finished dev [unoptimized + debuginfo] target(s) in 0.98s

[y21]

Thanks for the MRE @whfuyn ! At least in this example, it looks like the way clippy checks if a clone is mutated does not understand all mutating uses:

rust-clippy/clippy_utils/src/mir/mod.rs

Lines 70 to 74 in 7c34ec8

	if matches!(
	ctx,
	PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)
	| PlaceContext::MutatingUse(MutatingUseContext::Borrow)
	) {

This only considers either moving or borrowing mutably, but no direct stores (like in the example aa.b = 1024). So this lints, but this does not. Seems like we could fix this one by changing it to this:

if matches!( 
     ctx, 
     PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) 
-         | PlaceContext::MutatingUse(MutatingUseContext::Borrow) 
+         | PlaceContext::MutatingUse(MutatingUseContext::Borrow | MutatingUseContext::Store)
 ) { 

I don't think this is the same issue as in the OP though. That one seems to trigger even without any mutation at all:

#![warn(clippy::redundant_clone)]

fn main() {
    let a = String::new();
    {
        let _a2 = a.clone();
    }
    {
        let _a3 = a;
    }
}