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

Compiler warning unused_mut suggests removing mut keyword, but it would break compilation #113451

Open
cdbennett opened this issue Jul 7, 2023 · 6 comments
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. L-unused_mut Lint: unused_mut T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cdbennett
Copy link

cdbennett commented Jul 7, 2023

The Rust compiler gives a warning the mut keyword on a pattern is not needed, and suggests removing it. However, removing mut makes the code invalid and it then fails to compile.

I tried this code:

(Rust Playground Link)

use std::mem::replace;

fn main() {
    let mut state = State::Waiting { start_at: 1 };
    handle_event(&mut state, 0); // waiting
    handle_event(&mut state, 1); // start
    handle_event(&mut state, 2); // running
}

#[derive(Debug)]
enum State {
    Waiting { start_at: u64 },
    Running { started_at: u64 },
}

fn handle_event(state: &mut State, current_time: u64) {
    println!("time: {current_time}: state: {state:?} - event triggered");
    match state {
        // How to handle the matching of "start_at"?
        // Would like to match as a value, not a reference. Is there a way to do this without using mut?
        State::Waiting { mut start_at } => {
            if current_time >= start_at {
                println!("... Starting at {current_time}");
                let _ = replace(
                    state,
                    State::Running {
                        started_at: start_at,
                    },
                );
            } else {
                println!("... Waiting until {start_at}");
            }
        }
        State::Running { started_at } => {
            println!("... Running at {current_time} since {started_at}")
        }
    }
}

I expected to see this happen: if the code compiles successfully, a warning and its suggested fix should not break the code so it then fails to compile.

Instead, this happened:

warning: variable does not need to be mutable
  --> src/main.rs:21:26
   |
21 |         State::Waiting { mut start_at } => {
   |                          ----^^^^^^^^
   |                          |
   |                          help: remove this `mut`
   |
   = note: `#[warn(unused_mut)]` on by default

warning: `rust-mut-warning` (bin "rust-mut-warning") generated 1 warning (run `cargo fix --bin "rust-mut-warning"` to apply 1 suggestion)

And if we remove mut from { mut start_at } in the code, the compilation fails:

   Compiling rust-mut-warning v0.1.0 (/Code/rust-mut-warning)
error[E0308]: mismatched types
  --> src/main.rs:22:32
   |
22 |             if current_time >= start_at {
   |                ------------    ^^^^^^^^ expected `u64`, found `&mut u64`
   |                |
   |                expected because this is `u64`
   |
help: consider dereferencing the borrow
   |
22 |             if current_time >= *start_at {
   |                                +

error[E0308]: mismatched types
  --> src/main.rs:27:37
   |
27 |                         started_at: start_at,
   |                                     ^^^^^^^^ expected `u64`, found `&mut u64`
   |
help: consider dereferencing the borrow
   |
27 |                         started_at: *start_at,
   |                                     +

For more information about this error, try `rustc --explain E0308`.
error: could not compile `rust-mut-warning` (bin "rust-mut-warning") due to 2 previous errors

Or, we could use the cargo fix feature, and it results in the same issue:

cargo fix --bin "rust-mut-warning"
    Checking rust-mut-warning v0.1.0 (/Code/rust-mut-warning)
warning: failed to automatically apply fixes suggested by rustc to crate `rust_mut_warning`

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

  * src/main.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[E0308]: mismatched types
  --> src/main.rs:22:32
   |
22 |             if current_time >= start_at {
   |                ------------    ^^^^^^^^ expected `u64`, found `&mut u64`
   |                |
   |                expected because this is `u64`
   |
help: consider dereferencing the borrow
   |
22 |             if current_time >= *start_at {
   |                                +

error[E0308]: mismatched types
  --> src/main.rs:27:37
   |
27 |                         started_at: start_at,
   |                                     ^^^^^^^^ expected `u64`, found `&mut u64`
   |
help: consider dereferencing the borrow
   |
27 |                         started_at: *start_at,
   |                                     +

error: aborting due to 2 previous errors

Goal of the sample code

The code that was used (and generates an unused_mut warning) was trying to match on a Copy type (in this case u64). We want to use it by value, and adding the mut keyword to the pattern was used in order to bind by value instead of by reference.

There may be a better way to implement the sample code -- that is perhaps a related issue. It is true that the mut is unneeded. The bound variable is only used to read the value. But it must be matched by value and not by reference, and adding mut is the only way I know to do that.

(There is the alternative of matching by shared reference and adding an immediately following statement let start_at = *start_at; to rebind it by value (it is Copy), but that is also an ugly hack.)

Meta

Occurs on both 1.70 stable and 1.72 nightly:

rustc --version --verbose:

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

rustc 1.72.0-nightly (85bf07972 2023-07-06)
binary: rustc
commit-hash: 85bf07972a1041b9e25393b803d0e006bec3eaaf
commit-date: 2023-07-06
host: x86_64-apple-darwin
release: 1.72.0-nightly
LLVM version: 16.0.5
@cdbennett cdbennett added the C-bug Category: This is a bug. label Jul 7, 2023
@cdbennett cdbennett changed the title Compiler warning unused_mut suggests removing mut keyword but this breaks compilation Compiler warning unused_mut suggests removing mut keyword, but it would break compilation Jul 7, 2023
@jieyouxu
Copy link
Member

jieyouxu commented Jul 7, 2023

Slightly smaller https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e1265fe583e7043654de444f760af008:

#![allow(unused_must_use)]

enum State {
    Waiting { start_at: u64 }
}

fn main() {
    let current_time = 0u64;
    match (&mut State::Waiting { start_at: 0u64 }) {
        State::Waiting { mut start_at } => {
            current_time >= start_at;
        }
    }
}

(Note that this snippet contains another problem, incorrectly fired unused_parens around &mut State::Waiting { start_at: 0u64 } and its accompanying suggestion to remove the parens which could cause compilation to fail because you can't have match &mut State::Waiting { ... } {}...)

@lukas-code
Copy link
Member

A better way to do this is proposed in rust-lang/rfcs#3410.

@Noratrieb Noratrieb added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage-legacy labels Jul 17, 2023
@Expurple
Copy link

Expurple commented Feb 20, 2024

I just had a similar issue with false-positive unused_mut, but on a by-value parameter of inherent async method. The codebase is proprietary and currently I don't have the time to make a reproducer, but I may do so in the future.

It happened on the latest stable:

$ rustc --version --verbose
rustc 1.76.0 (07dca489a 2024-02-04)
binary: rustc
commit-hash: 07dca489ac2d933c78d3c5158e3f43beefeb02ce
commit-date: 2024-02-04
host: x86_64-unknown-linux-gnu
release: 1.76.0
LLVM version: 17.0.6

@Expurple
Copy link

My colleague has found a workaround for our issue: making the parameter itself immutable and then rebinding it to a mutable local variable. For that variable, the false warning is no longer generated. Sorry for still not making the reproducer

@Expurple
Copy link

Expurple commented Mar 2, 2024

Ok, sorry, now I'm pretty sure that my issue is caused by a proc macro and it's not a compiler bug. The linked issue above contains a reproducible example

@Alexendoo Alexendoo added A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Mar 27, 2024
@Alexendoo
Copy link
Member

Minimal example from rust-lang/rust-clippy#12561 (comment):

fn main() {
    let (mut a,) = &mut (0,);
    let _: i32 = a;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. L-unused_mut Lint: unused_mut T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants