-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Suggest removal of borrow in index when appropriate #117913
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
Conversation
self.lookup_indexing(expr, base, base_t, idx, idx_t) | ||
{ | ||
let (_ty, err) = | ||
self.demand_coerce_diag(idx, idx_t, index_ty, None, AllowTwoPhase::No); |
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.
I think you should manually recreate the logic in lookup_indexing
, and if you need to process obligations, you should throw them into your own ObligationCtxt
.
As for the demand_coerce
call, I think it's probably best for us to approximate it with a ObligationCtxt::eq
call instead, to again avoid poisoning our own fulfillment context.
Essentially what I want to avoid is that fulfillment_ctxt.select_where_possible
. It's not great, and unless you can convince me that there's a good reason that we can't approximate it with less invasive machinery, then I think we should try our best to avoid it.
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 of creating a new fulfillment context for this check instead?
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.
either works, though I think ObligationCtxt
has an easier to use API
When encountering ```rust let a = std::collections::HashMap::<String,String>::new(); let s = "hello"; let _b = &a[&s]; ``` suggest `let _b = &a[s];`. Fix rust-lang#66023.
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.
Sorry for the churn, but this is still not totally side-effect-less
let mut autoderef = self.autoderef(base.span, base_t); | ||
let mut result = None; | ||
while result.is_none() && autoderef.next().is_some() { | ||
result = self.try_index_step(expr, base, &autoderef, idx_t, idx); |
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.
try_index_step
still registers things into the fnctxt's fulfullment context :(
I think it's simpler to be creating your own Index
trait obligation here -- the only thing that you'd be losing is the [u8; N]
-> [u8]
deref step, which I guess you could replicate here.
☔ The latest upstream changes (presumably #124952) made this pull request unmergeable. Please resolve the merge conflicts. |
@estebank any updates on this? thanks |
When encountering
suggest
let _b = &a[s];
.Fix #66023.
r? @compiler-errors