Detect `mut arg: &Ty` meant to be `arg: &mut Ty` and provide structured suggestion#134977

estebank · 2024-12-31T17:37:04Z

When a newcomer attempts to use an "out parameter" using borrows, they sometimes get confused and instead of mutating the borrow they try to mutate the function-local binding instead. This leads to either type errors (due to assigning an owned value to a mutable binding of reference type) or a multitude of lifetime errors and unused binding warnings.

This change adds a suggestion to the type error

error[E0308]: mismatched types
  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:6:14
   |
LL | fn change_object(mut object: &Object) {
   |                              ------- expected due to this parameter type
LL |     let object2 = Object;
LL |     object = object2;
   |              ^^^^^^^ expected `&Object`, found `Object`
   |
help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding
   |
LL ~ fn change_object(object: &mut Object) {
LL |     let object2 = Object;
LL ~     *object = object2;
   |

and to the unused assignment lint

error: value assigned to `object` is never read
  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:11:5
   |
LL |     object = &object2;
   |     ^^^^^^
   |
note: the lint level is defined here
  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:1:9
   |
LL | #![deny(unused_assignments, unused_variables)]
   |         ^^^^^^^^^^^^^^^^^^
help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding
   |
LL ~ fn change_object2(object: &mut Object) {
LL |     let object2 = Object;
LL ~     *object = object2;
   |

Fix #112357.

rustbot · 2024-12-31T17:37:13Z

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

estebank · 2024-12-31T18:07:53Z

If this is considered too esoteric, I'm ok with closing the PR and ticket, but it feels directionally correct to me.

BoxyUwU · 2025-01-09T14:25:44Z

tests/ui/fn/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.stderr

+help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding
+   |
+LL ~ fn change_object2(object: &mut Object) {
+LL |     let object2 = Object;
+LL ~     *object = object2;
+   |


I think this help: only being on the warning instead of the actual error makes it far less useful 🤔 Personally I just skip over all the warnings when i have a compiler error so would likely not even see this in my own workflow.

Is it particularly difficult to give the help on the borrowck error, I could definitely understand if it seems like a lot more work and you just don't want to deal with that.

Agreed on all accounts. You got it right. I'd want us to come back to this and handle it in borrock, but landing a partial solution earlier feels better than waiting until I come up with a full solution.

BoxyUwU · 2025-01-09T14:26:08Z

tests/ui/fn/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.stderr

+LL ~ fn change_object(object: &mut Object) {
+LL |     let object2 = Object;
+LL ~     *object = object2;


The simple case works though so 🤷‍♀️ still valuable

BoxyUwU · 2025-01-09T14:26:54Z

tests/ui/fn/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs

+
+fn change_object(mut object: &Object) {
+    let object2 = Object;
+    object = object2; //~ ERROR mismatched types


Would you want to add a //~| HELP: annotation so that we continue testing that the help note you added, stays

compiler/rustc_passes/src/liveness.rs

BoxyUwU · 2025-01-09T15:04:12Z

compiler/rustc_passes/src/liveness.rs

+                .iter()
+                .filter_map(|ty| {
+                    if ty.span == *ty_span
+                        && let hir::TyKind::Ref(lt, mut_ty) = ty.kind


why mut_ty? this isnt a &mut ref its some &ty

mut_ty because the second positional field of Ref is MutTy, as in "mutability and type". Granted, the name is barely a step above x, and arguably would be better if it was x.

compiler/rustc_hir_typeck/src/demand.rs

BoxyUwU · 2025-01-09T18:01:27Z

compiler/rustc_hir_typeck/src/demand.rs

+            })
+            .next()
+        else {
+            return false;


can you not use like 10 different let ... else ... in this function and use a series of let chains instead like we do in annotate_mut_binding_to_immutable_binding. There's so much noise reading this with all the else { return false; } taking up multiple lines after practically every check in this function

Not convinced that these long chains of conditions look better as let chains or let elses. I flip flop on my opinion by the hour 😅

Yeah both always feel kind of gruelling to read lol...

compiler/rustc_hir_typeck/src/demand.rs

This is a mistake I've seen newcomers make where they want to express an "out" argument.

``` error[E0308]: mismatched types --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:6:14 | LL | fn change_object(mut object: &Object) { | ------- expected due to this parameter type LL | let object2 = Object; LL | object = object2; | ^^^^^^^ expected `&Object`, found `Object` | help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding | LL ~ fn change_object(object: &mut Object) { LL | let object2 = Object; LL ~ *object = object2; | ``` This might be the first thing someone tries to write to mutate the value *behind* an argument. We avoid suggesting `object = &object2;`, as that is less likely to be what was intended.

``` error: value assigned to `object` is never read --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:11:5 | LL | object = &object2; | ^^^^^^ | note: the lint level is defined here --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:1:9 | LL | #![deny(unused_assignments, unused_variables)] | ^^^^^^^^^^^^^^^^^^ help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding | LL ~ fn change_object2(object: &mut Object) { LL | let object2 = Object; LL ~ *object = object2; | ``` This might be the first thing someone tries to write to mutate the value *behind* an argument, trying to avoid an E0308.

BoxyUwU · 2025-01-13T22:08:08Z

@bors r+ rollup

bors · 2025-01-13T22:08:11Z

📌 Commit 4438b32 has been approved by BoxyUwU

It is now in the queue for this repository.

Rollup of 10 pull requests Successful merges: - rust-lang#134498 (Fix cycle error only occurring with -Zdump-mir) - rust-lang#134977 (Detect `mut arg: &Ty` meant to be `arg: &mut Ty` and provide structured suggestion) - rust-lang#135390 (Re-added regression test for rust-lang#122638) - rust-lang#135393 (uefi: helpers: Introduce OwnedDevicePath) - rust-lang#135440 (rm unnecessary `OpaqueTypeDecl` wrapper) - rust-lang#135441 (Make sure to mark `IMPL_TRAIT_REDUNDANT_CAPTURES` as `Allow` in edition 2024) - rust-lang#135444 (Update books) - rust-lang#135450 (Fix emscripten-wasm-eh with unwind=abort) - rust-lang#135452 (bootstrap: fix outdated feature name in comment) - rust-lang#135454 (llvm: Allow sized-word rather than ymmword in tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134977 - estebank:issue-112357, r=BoxyUwU Detect `mut arg: &Ty` meant to be `arg: &mut Ty` and provide structured suggestion When a newcomer attempts to use an "out parameter" using borrows, they sometimes get confused and instead of mutating the borrow they try to mutate the function-local binding instead. This leads to either type errors (due to assigning an owned value to a mutable binding of reference type) or a multitude of lifetime errors and unused binding warnings. This change adds a suggestion to the type error ``` error[E0308]: mismatched types --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:6:14 | LL | fn change_object(mut object: &Object) { | ------- expected due to this parameter type LL | let object2 = Object; LL | object = object2; | ^^^^^^^ expected `&Object`, found `Object` | help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding | LL ~ fn change_object(object: &mut Object) { LL | let object2 = Object; LL ~ *object = object2; | ``` and to the unused assignment lint ``` error: value assigned to `object` is never read --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:11:5 | LL | object = &object2; | ^^^^^^ | note: the lint level is defined here --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:1:9 | LL | #![deny(unused_assignments, unused_variables)] | ^^^^^^^^^^^^^^^^^^ help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding | LL ~ fn change_object2(object: &mut Object) { LL | let object2 = Object; LL ~ *object = object2; | ``` Fix rust-lang#112357.

Rollup of 10 pull requests Successful merges: - rust-lang#134498 (Fix cycle error only occurring with -Zdump-mir) - rust-lang#134977 (Detect `mut arg: &Ty` meant to be `arg: &mut Ty` and provide structured suggestion) - rust-lang#135390 (Re-added regression test for rust-lang#122638) - rust-lang#135393 (uefi: helpers: Introduce OwnedDevicePath) - rust-lang#135440 (rm unnecessary `OpaqueTypeDecl` wrapper) - rust-lang#135441 (Make sure to mark `IMPL_TRAIT_REDUNDANT_CAPTURES` as `Allow` in edition 2024) - rust-lang#135444 (Update books) - rust-lang#135450 (Fix emscripten-wasm-eh with unwind=abort) - rust-lang#135452 (bootstrap: fix outdated feature name in comment) - rust-lang#135454 (llvm: Allow sized-word rather than ymmword in tests) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned BoxyUwU Dec 31, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 31, 2024

This comment has been minimized.

Sign in to view

estebank force-pushed the issue-112357 branch from be0cfb3 to f4df967 Compare December 31, 2024 18:10

This comment has been minimized.

Sign in to view

estebank force-pushed the issue-112357 branch from f4df967 to b5ecbe0 Compare December 31, 2024 18:26

BoxyUwU reviewed Jan 9, 2025

View reviewed changes

BoxyUwU added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2025

estebank added 4 commits January 11, 2025 01:34

Add test for mut arg: &Ty meant to be arg: &mut Ty

57f9f8f

This is a mistake I've seen newcomers make where they want to express an "out" argument.

review comments and make test run-rustfix

4438b32

estebank force-pushed the issue-112357 branch from b5ecbe0 to 4438b32 Compare January 11, 2025 01:58

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 13, 2025

jhpratt mentioned this pull request Jan 14, 2025

Rollup of 10 pull requests #135465

Merged

bors merged commit 54c324f into rust-lang:master Jan 14, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Jan 14, 2025

This was referenced Jan 24, 2025

New suggestion for overwriting a reference is wrong for mutable references #136028

Closed

Compiler-Hint to assign to a mutably borrowed value #136026

Closed

moxian mentioned this pull request May 8, 2025

Attempting to store function callback of a particular incorrect type produces ICE #140823

Closed

Uh oh!

Conversation

estebank commented Dec 31, 2024

Uh oh!

rustbot commented Dec 31, 2024

Uh oh!

This comment has been minimized.

estebank commented Dec 31, 2024

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BoxyUwU commented Jan 13, 2025

Uh oh!

bors commented Jan 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants