Add help message for missing `IndexMut` impl #52788

LukasKalbertodt · 2018-07-27T19:53:02Z

Code:

let mut map = HashMap::new();
map.insert("peter", 23);
map["peter"] = 27;

Before:

error[E0594]: cannot assign to immutable indexed content
 --> src/main.rs:7:5
  |
7 |     map["peter"] = 27;
  |     ^^^^^^^^^^^^^^^^^ cannot borrow as mutable

With this change (just the help was added):

error[E0594]: cannot assign to immutable indexed content
 --> index-error.rs:7:5
  |
7 |     map["peter"] = 27;
  |     ^^^^^^^^^^^^^^^^^ cannot borrow as mutable
  |
  = help: trait `IndexMut` is required to modify indexed content, but it is not implemented for std::collections::HashMap<&str, i32>

Yesterday I did some pair programming with a Rust-beginner. We created a type and implemented Index for it. Trying to modify the value returned by the index operation returns in a rather vague error that was not very clear for the Rust beginner. So I tried to improve the situation.

Notes/questions for reviewers:

Is the formulation OK like that? I'm fine with changing it.
Can we be absolutely sure that IndexMut is actually not implemented in the case my help message is added? I'm fairly sure myself, but there could be some cases I didn't think of. Also, I don't know the compiler very well, so I don't know what exactly certain enum variants are used for.
- It would be nice to test if IndexMut is in fact not implemented for the type, but I couldn't figure out how to check that. If you think that additional check would be beneficial, could you tell me how to check if a trait is implemented?
Do you think I should change the error message instead of only adding an additional help message?

rust-highfive · 2018-07-27T19:53:05Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

ljedrz · 2018-07-27T21:10:33Z

src/librustc_borrowck/borrowck/mod.rs

+
+                                db.help(&format!(
+                                    "to modify indexed content, the trait `IndexMut` needs to be \
+                                     implemented, but it's not implemented for {}",


Suggestion: "trait IndexMutis required to modify indexed content, but it is not implemented for {}".

Much better :) Will change this soon unless an even better idea gets suggested here.

Before this change it simply says "cannot assign to immutable indexed content". This might be confusing for Rust beginners, as implementing two traits for "one operator" is not intuitive.

TimNN · 2018-08-07T17:34:17Z

Ping from triage, @eddyb / @rust-lang/compiler. This PR requires your review.

oli-obk · 2018-08-07T17:57:41Z

Can you add a bunch of tests checking whether the message is emitted in the desired case? Also I think &mut map["peter"] or implicit deref variants of that (map["peter"].mutating_method()) should gain the same diagnostic improvement. Do they lack the same way or is assignment special?

Previously it was only emitted for assigments, which was an unnecessary restriction. Now it doesn't care where the mutability comes from. This commit also adds `` quotes around the printed type.

LukasKalbertodt · 2018-08-07T21:11:29Z

@oli-obk I made the help message appear in more situations now (including the two you mentioned). I also added one ui test (and updated another one).

The NLL version of the error doesn't have my addition yet. Should I add it? Or do we want to merge this without adding it to NLL errors?

estebank · 2018-08-08T00:15:59Z

src/test/ui/borrowck/index-mut-help.nll.stderr

+  --> $DIR/index-mut-help.rs:21:5
+   |
+LL |     map["peter"].clear();           //~ ERROR
+   |     ^^^^^^^^^^^^ cannot borrow as mutable


Can you also add the message to the NLL errors? Eventually we'll switch over and lose the new note otherwise.

I tried to do that in the past couple of hours and now I really don't know how to proceed.

So I'm pretty sure I have to add my code to report_mutability_error() in librustc_mir/borrow_check/mutability_errors.rs. But I don't know how I can get the information whether this error was caused by an index operation. I have access to the following things:

access_place and the_place_err: these were my biggest hope of getting information about the expression causing the error. Sadly, in all three cases mentioned in this thread, they are ProjectionElem::Deref { base: Place::Local } which doesn't help me. I would have expected one of them to be a ProjectionElem::Index.

span and error_access: not helpful AFAICT

location: it's only a reference to the enclosing basic block, right? So not really helpful either

self.mir_def_id: this is a DefId and thus references a definition of an item, right? So I guess it references the enclosing function. So not useful either.

self.{other_fields}: AFAICT most of them are global structures and nothing pointing to the expression in question. I don't think anything is helpful here.

So I'm stuck and can't invest a lot more time into this. Could someone help me to get this done fairly quickly? If you people in this thread don't know about this stuff, could you ping someone who does know the code? Or tell me where I should go and ask about this?

ProjectionElem::Index is only for simple arrays and slices. Everything else goes through the IndexMut/Index trait methods. I'm assuming rustc decides on Index due to the lack of IndexMut and then tries to take a mutable reference to the result of the method call (which is where your Deref comes from: &mut Index::index(&map, "peter"))

cc @rust-lang/wg-compiler-nll any ideas how to do this in the MIR borrowchecker? Right now rustc (with the nll feature gate active) produces

error[E0596]: cannot borrow data in a `&` reference as mutable --> src/main.rs:7:5 | 7 | map["peter"].clear(); //~ ERROR | ^^^^^^^^^^^^ cannot borrow as mutable

which actually is less informative than the message of old borrowck even before this PR: cannot borrow immutable indexed content as mutable

I think the thing to do would be to look at the & reference and try to figure out where it came from. It'll probably be a temporary here, which is a good indicator that we should try to get more info about it. We'd see it as the result of an Index call -- that is probably a good signal for us to insert this advice, since unless the call to Index was manually written by the user, it must imply that IndexMut does not exist.

maybe worth filing a bug for it?

Thanks for your advice! Would it be possible to create a bug about this and then merge this PR as is? I don't think I will have time to work on this anytime soon. Then someone else can work on adding the same advice to the NLL version.

Filed #53228 to follow up.

estebank · 2018-08-09T18:44:58Z

@bors r+

bors · 2018-08-09T18:44:59Z

📌 Commit 24abef3 has been approved by estebank

bors · 2018-08-09T22:05:26Z

⌛ Testing commit 24abef3 with merge fb65d75...

…ebank Add help message for missing `IndexMut` impl Code: ```rust let mut map = HashMap::new(); map.insert("peter", 23); map["peter"] = 27; ``` Before: ``` error[E0594]: cannot assign to immutable indexed content --> src/main.rs:7:5 | 7 | map["peter"] = 27; | ^^^^^^^^^^^^^^^^^ cannot borrow as mutable ``` With this change (just the `help` was added): ``` error[E0594]: cannot assign to immutable indexed content --> index-error.rs:7:5 | 7 | map["peter"] = 27; | ^^^^^^^^^^^^^^^^^ cannot borrow as mutable | = help: trait `IndexMut` is required to modify indexed content, but it is not implemented for std::collections::HashMap<&str, i32> ``` --- Yesterday I did some pair programming with a Rust-beginner. We created a type and implemented `Index` for it. Trying to modify the value returned by the index operation returns in a rather vague error that was not very clear for the Rust beginner. So I tried to improve the situation. ## Notes/questions for reviewers: - Is the formulation OK like that? I'm fine with changing it. - Can we be absolutely sure that `IndexMut` is actually not implemented in the case my `help` message is added? I'm fairly sure myself, but there could be some cases I didn't think of. Also, I don't know the compiler very well, so I don't know what exactly certain enum variants are used for. - It would be nice to test if `IndexMut` is in fact not implemented for the type, but I couldn't figure out how to check that. If you think that additional check would be beneficial, could you tell me how to check if a trait is implemented? - Do you think I should change the error message instead of only adding an additional help message?

bors · 2018-08-10T00:14:46Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing fb65d75 to master...

rust-highfive assigned eddyb Jul 27, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2018

ljedrz reviewed Jul 27, 2018

View reviewed changes

Add help message for missing IndexMut impl

9d59c6b

Before this change it simply says "cannot assign to immutable indexed content". This might be confusing for Rust beginners, as implementing two traits for "one operator" is not intuitive.

LukasKalbertodt force-pushed the improve-index-mut-error branch from 344579b to 9d59c6b Compare July 28, 2018 12:01

Make IndexMut note apply to more cases

764d472

Previously it was only emitted for assigments, which was an unnecessary restriction. Now it doesn't care where the mutability comes from. This commit also adds `` quotes around the printed type.

This comment has been minimized.

Sign in to view

Add and update tests for IndexMut help message

24abef3

LukasKalbertodt force-pushed the improve-index-mut-error branch from 47dfc4c to 24abef3 Compare August 7, 2018 22:30

estebank reviewed Aug 8, 2018

View reviewed changes

estebank mentioned this pull request Aug 9, 2018

Improve borrow of immutable indexed content with NLL enabled #53228

Closed

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 9, 2018

bors merged commit 24abef3 into rust-lang:master Aug 10, 2018

estebank mentioned this pull request Aug 14, 2018

Unhelpful error message when trying to add elements to hash map #45491

Closed

pnkfelix mentioned this pull request Oct 8, 2018

[1.30 beta] Test suite of the jemalloc-ctl crate is failing #54478

Closed

LukasKalbertodt deleted the improve-index-mut-error branch May 3, 2020 16:42

estebank mentioned this pull request Aug 24, 2022

Improve diagnostic for missing IndexMut: HashMap #100873

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add help message for missing `IndexMut` impl #52788

Add help message for missing `IndexMut` impl #52788

LukasKalbertodt commented Jul 27, 2018 •

edited

Loading

rust-highfive commented Jul 27, 2018

ljedrz Jul 27, 2018

LukasKalbertodt Jul 27, 2018

TimNN commented Aug 7, 2018

oli-obk commented Aug 7, 2018

LukasKalbertodt commented Aug 7, 2018

This comment has been minimized.

estebank Aug 8, 2018

LukasKalbertodt Aug 8, 2018

oli-obk Aug 8, 2018

nikomatsakis Aug 9, 2018

nikomatsakis Aug 9, 2018

LukasKalbertodt Aug 9, 2018

estebank Aug 9, 2018

LukasKalbertodt Aug 9, 2018

estebank commented Aug 9, 2018

bors commented Aug 9, 2018

bors commented Aug 9, 2018

bors commented Aug 10, 2018

Add help message for missing IndexMut impl #52788

Add help message for missing IndexMut impl #52788

Conversation

LukasKalbertodt commented Jul 27, 2018 • edited Loading

Notes/questions for reviewers:

rust-highfive commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimNN commented Aug 7, 2018

oli-obk commented Aug 7, 2018

LukasKalbertodt commented Aug 7, 2018

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Aug 9, 2018

bors commented Aug 9, 2018

bors commented Aug 9, 2018

bors commented Aug 10, 2018

Add help message for missing `IndexMut` impl #52788

Add help message for missing `IndexMut` impl #52788

LukasKalbertodt commented Jul 27, 2018 •

edited

Loading