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

[GTimothy]

View all comments

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

The issues are threefold and linked:

1. Assigning data to a collectionBTreeMap/HashMap suggests 3 solutions all marked as MachineApplicable
2. The suggestions are slightly wrong with regards to their borrowing needs.
3. 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]

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]

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)

[GTimothy]

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

[JonathanBrouwer]

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

[rustbot]

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

[GTimothy]

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.

[GTimothy]

@rustbot ready

[GTimothy]

@JonathanBrouwer , I have added a test and removed the Copy check.

Questions:

• should I remove the big comment block?
• should I remove the fixme comment about ref_entry?

[JonathanBrouwer]

• If you could clean up the comment block a bit, maybe narrow it down to a few sentences, but keep the essence of it, that would be awesome :)
• I don't think there are currently concrete plans to add ref_entry to std (correct me if I'm wrong), so I think removing it makes sense

[GTimothy]

I have encountered a couple issues that need to be fixed before this is ready.
Notably, index = Ref(K), K, or &Q are not the only cases it seems because of autoderef. Working on it.

[JonathanBrouwer]

The error does not need to be perfect btw, just making it better is already nice :3

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[GTimothy]

@rustbot ready
I have finally got the time to finish this.
The suggestions now use the minimal mount of & possible and the exact amount of * necessary.
I have added a test (tests/ui/borrowck/index-mut-help2.rs) describing peculiar auto-dereferencing behaviours encountered with slightly different cases.