Fix 'assign to data in an index of' collection suggestions by GTimothy · Pull Request #152216 · rust-lang/rust

GTimothy · 2026-02-06T12:15:41Z

fixes #150001
fixes rust-lang/rust-analyzer#16076
fixes #134917

The issues are threefold and linked:

Assigning data to a ~~collection~~BTreeMap/HashMap suggests 3 solutions all marked as MachineApplicable
The suggestions are slightly wrong with regards to their borrowing needs.
The suggestions are not guaranteed to produce code that is valid, and suggestion number two is equivalent to an update, not an insertion.

This PR:

splits the large triple suggestion into three
sets them to MaybeIncorrect
automatically determines the required borrowing to use.

I think this solution may not be very elegant, expecially the key typechecking / borrowing part, but it works. I am however very open to any improvement/change :)

edit: edited to replace 'collection' with BTreeMap/HashMap'

rustbot · 2026-02-06T12:15:47Z

r? @JonathanBrouwer

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

Why was this reviewer chosen?

The reviewer was selected based on:

People who recently interacted with files modified in this PR: borrowck, compiler
borrowck, compiler expanded to 21 candidates
Random selection from 13 candidates

GTimothy · 2026-02-06T12:21:23Z

oops, meant to title "Fix 'assign to data in an index of' map suggestions"
Should probably rename the commit too.

edit for clarity: for now, these suggestions only apply to BTreeMap and HashMap.

rust/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

Lines 657 to 661 in fef627b

    
               fn suggest_map_index_mut_alternatives(&self, ty: Ty<'tcx>, err: &mut Diag<'infcx>, span: Span) { 
        
                   let Some(adt) = ty.ty_adt_def() else { return }; 
        
                   let did = adt.did(); 
        
                   if self.infcx.tcx.is_diagnostic_item(sym::HashMap, did) 
        
                       || self.infcx.tcx.is_diagnostic_item(sym::BTreeMap, did)

splits the large triple suggestion into three sets them to MaybeIncorrect automatically determines the required borrowing to use.

GTimothy · 2026-02-16T19:09:05Z

@JonathanBrouwer, I think this is ready for review. Do I need to change anything? Thanks in advance.

JonathanBrouwer · 2026-02-16T19:14:42Z

I'll take a look sometime in the next few days :)

JonathanBrouwer

Thanks for your first contribution to rustc!
The idea looks good, just would like the code to be slightly nicer :)

View changes since this review

JonathanBrouwer · 2026-02-20T15:36:10Z

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

+                    /// Simplified to counting only
+                    /// Peels off all references on the type. Returns the number of references
+                    /// removed.
+                    fn count_ty_refs<'tcx>(ty: Ty<'tcx>) -> usize {


Does it makes sense to put this as a method on Ty?

Maybe, if counting references could be useful in other places? I did not want to touch too many places in the code base, but since there is https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Ty.html#method.peel_refs, it could fit fine there.

JonathanBrouwer · 2026-02-20T15:55:29Z

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

+                                ref_depth_difference = index_ref_depth - key_ref_depth; //index should
+                            //be deeper than key
+                            } else {
+                                // no type ?


Is this else branch even reachable?
Since this runs strictly after type checking I'd expect not.
We can use expr_ty instead of expr_ty_opt in this case

JonathanBrouwer · 2026-02-20T15:56:47Z

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

+                            {
+                                let index_ref_depth = count_ty_refs(index_ty);
+                                ref_depth_difference = index_ref_depth - key_ref_depth; //index should
+                            //be deeper than key


Why is this necessarily true?

It is not, you are right to ask, there is a big issue here I think:

impl<K, Q: ?Sized, V, S> Index<&Q> for HashMap<K, V, S> where K: Eq + Hash + Borrow<Q>, Q: Eq + Hash, S: BuildHasher, { type Output = V;

Let look at for example:

#[derive(PartialEq, Eq, Hash)] struct Bk { inner: str, } impl Borrow<str> for &&&Bk { fn borrow(&self) -> &str { &self.inner } }

Then with my PR,

let mut map = HashMap::<&&&Bk, String>::new(); map["test"] = "test".to_string();

crashes the compiler, and does not suggest a fix.

here, we have index=&str and key=&&&Bk, with Q=str and K=&&&Bk:
index is a single ref and key is a triple ref => ref_depth_difference is negative

JonathanBrouwer · 2026-02-20T16:00:55Z

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

+                                0 => String::from("&"),
+                                n => "*".repeat(n - 1),
+                            }
+                        };


This code is duplicated from above, would it be nice to deduplicate it into a function/method?

Counting the references is common, but the suggestions for the method call branch is only get_mut which only requires one one prefix which saves one String allocations. In hindsight, that may not matter. If so, I can deduplicate.

JonathanBrouwer · 2026-02-20T16:02:38Z

tests/ui/borrowck/index-mut-help.stderr

   |
 LL -     map["peter"].clear();
-LL +     if let Some(val) = map.get_mut("peter") { val.clear(); };
+LL +     if let Some(val) = map.get_mut(&"peter") { val.clear(); };


Hmmm this extra reference does not seem to be needed to make the code compile

JonathanBrouwer · 2026-02-20T16:08:22Z

tests/ui/collections/hashmap/hashmap-index-mut.stderr

   |
 LL -     map[&0] = 1;
-LL +     map.insert(&0, 1);
+LL +     map.insert(*&0, 1);


That is also not entirely the desired suggestion, would be nice to simplify this to map.insert(0, 1);

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

rustbot · 2026-02-20T16:11:10Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

- properly propose methods depending of the type relationship between the index used and the map key. - use whether key type is copy/clone to orient propositions.

GTimothy · 2026-02-23T22:15:46Z

thank you @JonathanBrouwer for your remarks. I had made a completely wrong assumptions about the types in questions.
I have completely reworked the PR in a much more elegant and correct way i think.

… and key is ok too for `insert` and `entry`

GTimothy · 2026-02-24T19:04:12Z

@rustbot ready

rustbot assigned JonathanBrouwer Feb 6, 2026

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 Feb 6, 2026

This comment has been minimized.

Sign in to view

GTimothy force-pushed the map-diagnostics-fix branch from 8034656 to 2371906 Compare February 6, 2026 12:18

This comment has been minimized.

Sign in to view

GTimothy marked this pull request as draft February 6, 2026 13:03

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 Feb 6, 2026

This comment has been minimized.

Sign in to view

GTimothy force-pushed the map-diagnostics-fix branch from 0fedd40 to 77c2a6e Compare February 14, 2026 15:36

This comment has been minimized.

Sign in to view

GTimothy added 3 commits February 16, 2026 10:07

Fix 'assign to data in an index of' collection suggestions

efb2ca0

splits the large triple suggestion into three sets them to MaybeIncorrect automatically determines the required borrowing to use.

restore missing case, method call on value of map

9b0864d

rebless the ui tests

5d997ef

GTimothy force-pushed the map-diagnostics-fix branch from 77c2a6e to 5d997ef Compare February 16, 2026 09:48

GTimothy marked this pull request as ready for review February 16, 2026 19:03

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 Feb 16, 2026

JonathanBrouwer requested changes Feb 20, 2026

View reviewed changes

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2026

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 20, 2026

second version

193b59c

- properly propose methods depending of the type relationship between the index used and the map key. - use whether key type is copy/clone to orient propositions.

fix: the same borrowed type (ignoring lifetime differences) for index…

2e22102

… and key is ok too for `insert` and `entry`

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 Feb 24, 2026

Uh oh!

Comments

Conversation

GTimothy commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 6, 2026

Uh oh!

This comment has been minimized.

GTimothy commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GTimothy commented Feb 16, 2026

Uh oh!

JonathanBrouwer commented Feb 16, 2026

Uh oh!

JonathanBrouwer left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

rustbot commented Feb 20, 2026

Uh oh!

GTimothy commented Feb 23, 2026

Uh oh!

GTimothy commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GTimothy commented Feb 6, 2026 •

edited

Loading

GTimothy commented Feb 6, 2026 •

edited

Loading

JonathanBrouwer left a comment •

edited by rustbot

Loading