Remove Binder::bind() and use Binder::dummy()
#83825
Conversation
camelid
added
I-compiletime
rust-highfive
added
the
S-waiting-on-review
|
At least one test ( I think you said that there was one case where we needed to keep the EDIT: It looks like that is the only test that is failing. |
This comment has been minimized.
This comment has been minimized.
|
Yep that's the one test that I had troubles with. I've tried to dig into it a couple times, but didn't get very far. |
|
So do you think there is a solution to that one without using |
Well, we should first check if we expect bound vars there. Then, either need to figure out if we would know the binders elsewhere, or if we can pass through binders. For what it's worth, it wasn't obvious to me. |
|
Should still be able to do a perf run, I think. And I'm curious. @bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 61efe217f48b22ab8984eb9891955d87e029d65c with merge 24305f7b8bdf7853fd1537332a15bc69c0f7d9c4... |
|
☀️ Try build successful - checks-actions |
|
Queued 24305f7b8bdf7853fd1537332a15bc69c0f7d9c4 with parent 39eee17, future comparison URL. |
|
Finished benchmarking try commit (24305f7b8bdf7853fd1537332a15bc69c0f7d9c4): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
Slight perf win, but only a small portion of the regression from the binder refactor |
camelid
added
the
S-waiting-on-author
|
Here's a snippet of the debug logging of rustc running on the test. (I added |
|
The ICE disappears if I remove either of the conflicting impls, so I'm guessing this codepath is caused by coherence checking? |
|
Possibly. That sounds potentially reasonable. You could probably find that in the log? |
|
Hmm, it seems like the code that is panicking is run after MIR basic block simplification: The failed assertion occurs a bit later, I think as part of a chain of function calls? The logs seem to be noisy and it's hard to look through them. I think I'll try looking through the code a bit. |
|
This line looks potentially interesting: ( |
|
So, I think this is the crucial part of the logs: (There's a bunch of MIR dataflow stuff before the "simplifying bb0" line. That's I noticed that the I then modified the test so that each I'm still very new to the trait solving and const generics code, so I don't know |
camelid
removed
the
S-waiting-on-author
|
Yeah, maybe try asking someone more familiar with the const eval code and if we expect bound vars there (and if so, should we be keeping those around?) |
jackh726
added
the
WG-traits
jackh726
removed
the
S-waiting-on-review
jackh726
added
the
S-waiting-on-author
|
@camelid if you want to split out all but the one difficult change for this, that would be nice. Then we could remove |
|
Sure, that sounds like a good plan! |
This comment has been minimized.
This comment has been minimized.
camelid
force-pushed
the
binder-bind
branch
2 times, most recently
from
20934da to
7d09df2
Compare
camelid
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
See [1] for motivation. In one place, it manually uses the `BoundVarsCollector` for now because we don't know what the bound variables are, and adding support for tracking the bound variables will probably be a tricky change. [1]: rust-lang#76814 (comment)
| @@ -115,7 +116,12 @@ fn resolve_associated_item<'tcx>( | |||
| ); | |||
|
|
|||
| let trait_ref = ty::TraitRef::from_method(tcx, trait_id, rcvr_substs); | |||
| let vtbl = tcx.codegen_fulfill_obligation((param_env, ty::Binder::bind(trait_ref, tcx)))?; | |||
| // FIXME: we should instead track bound variables from their origin, | |||
There was a problem hiding this comment.
I really don't want someone to start using BoundVarsCollector for something else. As much as it isn't the right place for it, I think it's better to just move that here until this is fixed. (With a very clear comment on why)
| let (impl_m_own_bounds, _) = infcx.replace_bound_vars_with_fresh_vars( | ||
| impl_m_span, | ||
| infer::HigherRankedType, | ||
| ty::Binder::bind(impl_m_own_bounds.predicates, tcx), | ||
| ty::Binder::dummy(impl_m_own_bounds.predicates), | ||
| ); |
There was a problem hiding this comment.
This is weird. If we don't ever have bound vars, why do we have to replace_bound_vars_with_fresh_vars
There was a problem hiding this comment.
Okay, I think we can remove replace_bound_vars_with_fresh_vars here because impl_m_predicates is simply the Predicates of the impl, which obviously won't have escaping bound vars. And impl_to_placeholder_substs will only have substs for the impl itself, none of which are late-bound.
| let self_ty = | ||
| fcx.tcx.liberate_late_bound_regions(method.def_id, ty::Binder::bind(self_ty, fcx.tcx)); | ||
| let self_ty = fcx.tcx.liberate_late_bound_regions(method.def_id, ty::Binder::dummy(self_ty)); | ||
|
|
||
| let receiver_ty = sig.inputs()[0]; | ||
|
|
||
| let receiver_ty = fcx.normalize_associated_types_in(span, receiver_ty); | ||
| let receiver_ty = | ||
| fcx.tcx.liberate_late_bound_regions(method.def_id, ty::Binder::bind(receiver_ty, fcx.tcx)); | ||
| fcx.tcx.liberate_late_bound_regions(method.def_id, ty::Binder::dummy(receiver_ty)); |
There was a problem hiding this comment.
These again are a bit sus: There aren't late-bound vars, but we're calling liberate_late_bound_regions? (mental note to future me: this bit of code is touched in #85499, did we remove these calls?)
There was a problem hiding this comment.
Okay yeah, we liberate_late_bound_regions above, we shouldn't need to do that again here.
| @@ -1767,7 +1767,7 @@ fn fn_sig(tcx: TyCtxt<'_>, def_id: DefId) -> ty::PolyFnSig<'_> { | |||
| } | |||
| diag.emit(); | |||
|
|
|||
| ty::Binder::bind(fn_sig, tcx) | |||
| ty::Binder::dummy(fn_sig) | |||
There was a problem hiding this comment.
Can you just move this up to right under fn_sig is created? Then when used, just skip_binder()
| @@ -119,7 +119,7 @@ impl<'a, 'tcx> ConfirmContext<'a, 'tcx> { | |||
| // We won't add these if we encountered an illegal sized bound, so that we can use | |||
| // a custom error in that case. | |||
| if illegal_sized_bound.is_none() { | |||
| let method_ty = self.tcx.mk_fn_ptr(ty::Binder::bind(method_sig, self.tcx)); | |||
| let method_ty = self.tcx.mk_fn_ptr(ty::Binder::dummy(method_sig)); | |||
| tcx, | ||
| ), | ||
| Binder::dummy(TraitPredicate { | ||
| trait_ref: TraitRef::from_method(tcx, trait_id, substs), |
There was a problem hiding this comment.
Preexisting, but why not jsut use the trait_ref create directly above
| @@ -404,7 +404,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
| obligations.extend(traits::predicates_for_generics(cause.clone(), self.param_env, bounds)); | |||
|
|
|||
| // Also add an obligation for the method type being well-formed. | |||
| let method_ty = tcx.mk_fn_ptr(ty::Binder::bind(fn_sig, tcx)); | |||
| let method_ty = tcx.mk_fn_ptr(ty::Binder::dummy(fn_sig)); | |||
| let self_ty = | ||
| fcx.tcx.liberate_late_bound_regions(method.def_id, ty::Binder::bind(self_ty, fcx.tcx)); | ||
| let self_ty = fcx.tcx.liberate_late_bound_regions(method.def_id, ty::Binder::dummy(self_ty)); | ||
|
|
||
| let receiver_ty = sig.inputs()[0]; | ||
|
|
||
| let receiver_ty = fcx.normalize_associated_types_in(span, receiver_ty); | ||
| let receiver_ty = | ||
| fcx.tcx.liberate_late_bound_regions(method.def_id, ty::Binder::bind(receiver_ty, fcx.tcx)); | ||
| fcx.tcx.liberate_late_bound_regions(method.def_id, ty::Binder::dummy(receiver_ty)); |
There was a problem hiding this comment.
Okay yeah, we liberate_late_bound_regions above, we shouldn't need to do that again here.
jackh726
added
S-waiting-on-author
See here for motivation.
In one place, it manually uses the
BoundVarsCollectorfor now becausewe don't know what the bound variables are, and adding support for
tracking the bound variables will probably be a tricky change.
r? @jackh726