-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add help message for missing IndexMut impl with NLL #53830
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Huh, strange. I'll rebase locally and see if I can reproduce. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think? maybe make a test for a manual call using Index::index
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether I'm suggesting you do this or not, have to think about it.
In any case, I like the error.
if self.tcx.parent(id) == self.tcx.lang_items().index_trait() { | ||
|
||
let mut found = false; | ||
self.tcx.for_each_relevant_impl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this check is pretty crude. I guess it'll work, but (e.g.) if IndexMut
is implemented, but for a different index type, it will get the wrong result (though that is likely to get a different error, earlier in the compiler).
Another option might be to use the predicate_may_hold
method (or perhaps predicate_must_hold
) to test if the trait holds.
It's a bit more work though — we'd have to construct the proper predicate and everything. Seems like something where some helper functions would be useful, I feel like this comes up from time to time in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've discovered that due to a bug in the canonicalization code, it's not possible to switch to predicate_may_hold
at the moment - the code that demonstrated this is available on this branch.
@bors r+ |
📌 Commit 08a4a37 has been approved by |
@bors p=1 RC milestone fix |
⌛ Testing commit 08a4a37 with merge aacf17bb775f952ee54ab3322df68553fe929826... |
💔 Test failed - status-travis |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors retry
|
Add help message for missing IndexMut impl with NLL Fixes #53228. r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
Fixes #53228.
r? @nikomatsakis