Skip to content

cannot borrow as mutable suggestion confusing #113886

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

Open
jdonszelmann opened this issue Jul 20, 2023 · 2 comments
Open

cannot borrow as mutable suggestion confusing #113886

jdonszelmann opened this issue Jul 20, 2023 · 2 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jdonszelmann
Copy link
Contributor

jdonszelmann commented Jul 20, 2023

Code

fn do_thing(collection: Vec<i32>) {
    collection.push(1);
}

fn main() {
    let collection = Vec::new();
    for i in 0..20 {
        do_thing(collection)
    }
}

Current output

error[E0596]: cannot borrow `collection` as mutable, as it is not declared as mutable
 --> src/main.rs:4:5
  |
4 |     collection.push(1);
  |     ^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
  |
help: consider changing this to be mutable
  |
3 | fn do_thing(mut collection: Vec<i32>) {
  |             +++

error[E0382]: use of moved value: `collection`
  --> src/main.rs:10:18
   |
8  |     let collection = Vec::new();
   |         ---------- move occurs because `collection` has type `Vec<i32>`, which does not implement the `Copy` trait
9  |     for i in 0..20 {
   |     -------------- inside of this loop
10 |         do_thing(collection)
   |                  ^^^^^^^^^^ value moved here, in previous iteration of loop
   |
note: consider changing this parameter type in function `do_thing` to borrow instead if owning the value isn't necessary
  --> src/main.rs:3:25
   |
3  | fn do_thing(collection: Vec<i32>) {
   |    --------             ^^^^^^^^ this parameter takes ownership of the value
   |    |
   |    in this function
help: consider cloning the value if the performance cost is acceptable
   |
10 |         do_thing(collection.clone())
   |                            ++++++++

Some errors have detailed explanations: E0382, E0596.
For more information about an error, try `rustc --explain E0382`.
error: could not compile `demo2` (bin "demo2") due to 2 previous errors

Desired output

error[E0596]: cannot borrow `collection` as mutable, as it is not declared as mutable
 --> src/main.rs:4:5
  |
4 |     collection.push(1);
  |     ^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
  |
help: consider changing this to be mutable
  |
3 | fn do_thing(mut collection: Vec<i32>) {
  |             +++
help: consider passing a mutable reference
  |
3 | fn do_thing(collection: &mut Vec<i32>) {
  |                         ++++


error[E0382]: use of moved value: `collection`
  --> src/main.rs:10:18
   |
8  |     let collection = Vec::new();
   |         ---------- move occurs because `collection` has type `Vec<i32>`, which does not implement the `Copy` trait
9  |     for i in 0..20 {
   |     -------------- inside of this loop
10 |         do_thing(collection)
   |                  ^^^^^^^^^^ value moved here, in previous iteration of loop
   |
note: consider changing this parameter type in function `do_thing` to borrow instead if owning the value isn't necessary
  --> src/main.rs:3:25
   |
3  | fn do_thing(collection: Vec<i32>) {
   |    --------             ^^^^^^^^ this parameter takes ownership of the value
   |    |
   |    in this function
help: consider cloning the value if the performance cost is acceptable
   |
10 |         do_thing(collection.clone())
   |                            ++++++++

Some errors have detailed explanations: E0382, E0596.
For more information about an error, try `rustc --explain E0382`.
error: could not compile `demo2` (bin "demo2") due to 2 previous errors

Rationale and extra context

I encountered this while discussing a larger code example (also involving async move and spawning tokio tasks). When you read the diagnostics top to bottom, something that's common in an IDE (rust-analyzer also shows a similar suggestion) you might be tempted to apply what the compiler says: make the variable mut.

More often than not, that's not the desired behaviour I find. Making the parameter mut often locally fixes the problem (the do_thing function compiles) but globally the program gets worse, and might cause other errors.

Even if the suggestion is right, it might be nice to show both options since they would both fix the error.

Other cases

No response

Anything else?

@estebank @compiler-errors

@jdonszelmann jdonszelmann added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 20, 2023
@jdonszelmann
Copy link
Contributor Author

Note that if this is indeed something you agree with that it should change,or a similar change, I would be interested in implementing it (maybe with some guidance). I've been interested in contributing to the compiler for a while now and this seems like a manageable task

@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 8, 2023
@Noratrieb
Copy link
Member

@jdonszelmann
diagnostics improvements are always welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. 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

4 participants