overhaul `&mut` suggestions in borrowck errors #145013

fee1-dead · 2025-08-06T17:45:33Z

This refactors the logic so that it does not use fuzzy string matching for suggestions; it instead uses information directly from MIR.
If something comes from a custom Index impl for which the IndexMut trait does not apply, do not suggest adding mut after &.
Suggest get_mut with unwrap if error is fired on BTreeMap or HashMap.

Supersedes #144018 cc @xizheyin
Closes #143732

rustbot · 2025-08-06T17:45:38Z

rustbot has assigned @nnethercote.
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

nnethercote

I have only looked at the tests and their outputs so far, some of the suggestions seem suboptimal. Is this expected?

@rustbot author

nnethercote · 2025-08-09T09:44:28Z

tests/ui/array-slice-vec/slice-mut-2.stderr

   |
-LL |     let x: &[isize] = &mut [1, 2, 3, 4, 5];
-   |                        +++
+LL |     let x: &mut [isize] = &[1, 2, 3, 4, 5];


This is still invalid, it needs to be &mut [1, 2, 3, 4, 5].

nnethercote · 2025-08-09T09:47:06Z

tests/ui/borrowck/borrow-raw-address-of-deref-mutability.stderr

   |
-LL |     let x = &mut 0 as *const i32;
-   |              +++
+LL |     let x: *mut i32 = &0 as *const i32;


Also still invalid, let x = &mut 0 as *mut i32; would work, without needing the type annotation.

nnethercote · 2025-08-09T09:48:28Z

tests/ui/borrowck/borrowck-access-permissions.stderr

+help: consider changing this binding's type
+   |
+LL -         let ptr_x: *const _ = &x;
+LL +         let ptr_x: *mut i32 = &x;


Should be &x instead of &mut x.

tests/ui/borrowck/suggestions/overloaded-index-not-mut-but-should-be-mut.stderr

nnethercote · 2025-08-10T11:03:08Z

This is marked as waiting on review, but my comments about the invalid suggestions haven't been addressed. What's the status there?

fee1-dead · 2025-08-10T12:17:23Z

Rustbot didn't change it when you included as the "review comment", it has to be a normal comment

@rustbot author

rustbot · 2025-08-16T17:26:19Z

This PR was rebased onto a different master commit! Check out the changes with our range-diff.

fee1-dead · 2025-08-16T17:33:14Z

w.r.t. invalid suggestions: every time I look into this code I find things to refactor, so this is currently turning into an endless time sink.

The logic is an entangled mess and I tried to untangle it a bit. It is still entangled, but better from the previous state of using stringy logic for everything, having the same code path for everything from ref x to let x to &x to &T.

Some of them have complicated mir, which is probably why the original author chose to do string operations instead of matching on mir. (if you want to take a look, run with RUSTC_LOG=rustc_borrowck::diagnostics rustc +stage1 PATH_TO_TEST -Zdump-mir=nll)

I put down some infrastructure to allow both an expr suggestion and a type change suggestion to exist. For some reason this doesn't fire at the moment. But this is a pre-existing issue and my PR doesn't regress them too much so I propose we could land this and fix later.

fee1-dead · 2025-08-16T17:34:22Z

if you want me to look into this even more, feel free to turn the labels back

@rustbot ready

nnethercote · 2025-08-18T05:54:40Z

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

+            return;
+        }
+
+        // no suggestion for expression; find a binding's type to make mutable.


My most boomer-coded opinion is that I like sentences to begin with an upper-case letter.

To clarify:

Punctuationless phrase fragments are bearable, e.g. "this is a comment"

A trailing period makes it worse, e..g "this is a comment."

Multiple sentences are beyond the pale e.g. "this is a comment. this is another comment." If it's heading into "proper paragraph" territory there are rules for a reason, they make the text easier to read.

I almost always write comments as full sentences with upper-case first letter and closing period. Occasionally I'll do a "this is a comment"-style punctuation-free sentence fragment, perhaps as a trailing comment on a line with code.

nnethercote · 2025-08-18T06:06:27Z

Is it accurate to mark this as fully fixing #143732?

The PR is hard to review. It's hard to see what is new code and what is just moved around. I'm not sure if it could have been broken into small pieces. I'm not inclined to go over it with a fine-tooth comb but I also don't want to block it, based on the fact that it does somewhat improve the error messages and you say there is follow-up work to be done. So you can have an r=me if you want it and think it's progress.

fee1-dead · 2025-08-18T15:38:21Z

Is it accurate to mark this as fully fixing #143732?

Yes, because I believe either the "specify this binding's type" will fire or it will go down the path of either suggesting .get_mut if possible or not suggesting anything at all.

I like sentences to begin with an upper-case letter.

Went through the file and applied the stylistic changes you suggested.

It's hard to see what is new code and what is just moved around. I'm not sure if it could have been broken into small pieces.

Yeah, I don't know either. Perhaps it would be easier if the refactor was done first, and the new logic was added later. But I still think it would be hard to review. The followups would probably be easier.

@bors r=nnethercote

bors · 2025-08-18T15:38:24Z

📌 Commit c6db6f2 has been approved by nnethercote

It is now in the queue for this repository.

…m, r=nnethercote overhaul `&mut` suggestions in borrowck errors * This refactors the logic so that it does not use fuzzy string matching for suggestions; it instead uses information directly from MIR. * If something comes from a custom `Index` impl for which the `IndexMut` trait does not apply, do not suggest adding `mut` after `&`. * Suggest `get_mut` with `unwrap` if error is fired on `BTreeMap` or `HashMap`. Supersedes rust-lang#144018 cc `@xizheyin` Closes rust-lang#143732

…m, r=nnethercote overhaul `&mut` suggestions in borrowck errors * This refactors the logic so that it does not use fuzzy string matching for suggestions; it instead uses information directly from MIR. * If something comes from a custom `Index` impl for which the `IndexMut` trait does not apply, do not suggest adding `mut` after `&`. * Suggest `get_mut` with `unwrap` if error is fired on `BTreeMap` or `HashMap`. Supersedes rust-lang#144018 cc ``@xizheyin`` Closes rust-lang#143732

Rollup of 19 pull requests Successful merges: - #140956 (`impl PartialEq<{str,String}> for {Path,PathBuf}`) - #141744 (Stabilize `ip_from`) - #142681 (Remove the `#[no_sanitize]` attribute in favor of `#[sanitize(xyz = "on|off")]`) - #142871 (Trivial improve doc for transpose ) - #144252 (Do not copy .rmeta files into the sysroot of the build compiler during check of rustc/std) - #144476 (rustdoc-search: search backend with partitioned suffix tree) - #144567 (Fix RISC-V Test Failures in ./x test for Multiple Codegen Cases) - #144804 (Don't warn on never to any `as` casts as unreachable) - #144960 ([RTE-513] Ignore sleep_until test on SGX) - #145013 (overhaul `&mut` suggestions in borrowck errors) - #145041 (rework GAT borrowck limitation error) - #145243 (take attr style into account in diagnostics) - #145405 (cleanup: use run_in_tmpdir in run-make/rustdoc-scrape-examples-paths) - #145432 (cg_llvm: Small cleanups to `owned_target_machine`) - #145484 (Remove `LlvmArchiveBuilder` and supporting code/bindings) - #145557 (Fix uplifting in `Assemble` step) - #145563 (Remove the `From` derive macro from prelude) - #145565 (Improve context of bootstrap errors in CI) - #145584 (interpret: avoid forcing all integer newtypes into memory during clear_provenance) Failed merges: - #145359 (Fix bug where `rustdoc-js` tester would not pick the right `search.js` file if there is more than one) - #145573 (Add an experimental unsafe(force_target_feature) attribute.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #145013 - fee1-dead-contrib:push-vwvsqsqnrxqm, r=nnethercote overhaul `&mut` suggestions in borrowck errors * This refactors the logic so that it does not use fuzzy string matching for suggestions; it instead uses information directly from MIR. * If something comes from a custom `Index` impl for which the `IndexMut` trait does not apply, do not suggest adding `mut` after `&`. * Suggest `get_mut` with `unwrap` if error is fired on `BTreeMap` or `HashMap`. Supersedes #144018 cc ```@xizheyin``` Closes #143732

Zalathar · 2025-08-19T10:10:05Z

Bors hasn't noticed that this was merged.

@bors r- retry

Rollup of 19 pull requests Successful merges: - rust-lang/rust#140956 (`impl PartialEq<{str,String}> for {Path,PathBuf}`) - rust-lang/rust#141744 (Stabilize `ip_from`) - rust-lang/rust#142681 (Remove the `#[no_sanitize]` attribute in favor of `#[sanitize(xyz = "on|off")]`) - rust-lang/rust#142871 (Trivial improve doc for transpose ) - rust-lang/rust#144252 (Do not copy .rmeta files into the sysroot of the build compiler during check of rustc/std) - rust-lang/rust#144476 (rustdoc-search: search backend with partitioned suffix tree) - rust-lang/rust#144567 (Fix RISC-V Test Failures in ./x test for Multiple Codegen Cases) - rust-lang/rust#144804 (Don't warn on never to any `as` casts as unreachable) - rust-lang/rust#144960 ([RTE-513] Ignore sleep_until test on SGX) - rust-lang/rust#145013 (overhaul `&mut` suggestions in borrowck errors) - rust-lang/rust#145041 (rework GAT borrowck limitation error) - rust-lang/rust#145243 (take attr style into account in diagnostics) - rust-lang/rust#145405 (cleanup: use run_in_tmpdir in run-make/rustdoc-scrape-examples-paths) - rust-lang/rust#145432 (cg_llvm: Small cleanups to `owned_target_machine`) - rust-lang/rust#145484 (Remove `LlvmArchiveBuilder` and supporting code/bindings) - rust-lang/rust#145557 (Fix uplifting in `Assemble` step) - rust-lang/rust#145563 (Remove the `From` derive macro from prelude) - rust-lang/rust#145565 (Improve context of bootstrap errors in CI) - rust-lang/rust#145584 (interpret: avoid forcing all integer newtypes into memory during clear_provenance) Failed merges: - rust-lang/rust#145359 (Fix bug where `rustdoc-js` tester would not pick the right `search.js` file if there is more than one) - rust-lang/rust#145573 (Add an experimental unsafe(force_target_feature) attribute.) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned nnethercote Aug 6, 2025

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 Aug 6, 2025

nnethercote reviewed Aug 9, 2025

View reviewed changes

rustbot 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 Aug 10, 2025

overhaul &mut suggestions in borrowck errors

4335405

fee1-dead force-pushed the push-vwvsqsqnrxqm branch from 2d1f731 to b4b62f1 Compare August 16, 2025 17:26

fee1-dead force-pushed the push-vwvsqsqnrxqm branch from b4b62f1 to fe98063 Compare August 16, 2025 17:28

This comment has been minimized.

Sign in to view

refactor return type of suggest_ampmut into an enum

e78b417

fee1-dead force-pushed the push-vwvsqsqnrxqm branch from fe98063 to e78b417 Compare August 16, 2025 17:33

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

nnethercote reviewed Aug 18, 2025

View reviewed changes

comment style changes

c6db6f2

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2025

Zalathar mentioned this pull request Aug 19, 2025

Rollup of 19 pull requests #145589

Merged

bors merged commit 181480d into rust-lang:master Aug 19, 2025
10 checks passed

rustbot added this to the 1.91.0 milestone Aug 19, 2025

fee1-dead deleted the push-vwvsqsqnrxqm branch August 20, 2025 05:05

overhaul &mut suggestions in borrowck errors #145013

overhaul &mut suggestions in borrowck errors #145013

Conversation

fee1-dead commented Aug 6, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 6, 2025

Uh oh!

nnethercote left a comment

Choose a reason for hiding this comment

Uh oh!

nnethercote Aug 9, 2025

Choose a reason for hiding this comment

Uh oh!

nnethercote Aug 9, 2025

Choose a reason for hiding this comment

Uh oh!

nnethercote Aug 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nnethercote commented Aug 10, 2025

Uh oh!

fee1-dead commented Aug 10, 2025

Uh oh!

rustbot commented Aug 16, 2025

Uh oh!

fee1-dead commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

fee1-dead commented Aug 16, 2025

Uh oh!

nnethercote Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

nnethercote Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Aug 18, 2025

Uh oh!

fee1-dead commented Aug 18, 2025

Uh oh!

bors commented Aug 18, 2025

Uh oh!

Uh oh!

Zalathar commented Aug 19, 2025

Uh oh!

Uh oh!

overhaul `&mut` suggestions in borrowck errors #145013

overhaul `&mut` suggestions in borrowck errors #145013

fee1-dead commented Aug 6, 2025 •

edited by rustbot

Loading

fee1-dead commented Aug 16, 2025 •

edited

Loading