Add new alias kind Ambiguous and renormalize instantiated binders#156453
Add new alias kind Ambiguous and renormalize instantiated binders#156453adwinwhite wants to merge 38 commits into
Ambiguous and renormalize instantiated binders#156453Conversation
rustbot
added
S-waiting-on-author
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
Add new alias kind `Ambiguous` and renormalize instantiated binders
This comment has been minimized.
This comment has been minimized.
|
@bors try cancel |
|
Try build cancelled. Cancelled workflows: |
|
Ignore tools failures for now. @bors try @rust-timer queue |
|
This pull request is already queued and waiting for a try build to finish. |
This comment has been minimized.
This comment has been minimized.
Add new alias kind `Ambiguous` and renormalize instantiated binders
|
💔 Test for 1c015e6 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
|
|
||
| // Placeholders (all printed as `_` to uniformize them). | ||
| ty::Param(_) | ty::Bound(..) | ty::Placeholder(_) | ty::Infer(_) | ty::Error(_) => { | ||
| ty::Alias(ty::AliasTy { kind: ty::Ambiguous { .. }, .. }) |
There was a problem hiding this comment.
unreachable
| Unnormalized::new_wip(unnormalized_external_impl_sig), | ||
| ); | ||
| let external_impl_sig = | ||
| ocx.normalize(&norm_cause, param_env, Unnormalized::new(unnormalized_external_impl_sig)); |
There was a problem hiding this comment.
| ocx.normalize(&norm_cause, param_env, Unnormalized::new(unnormalized_external_impl_sig)); | |
| ocx.normalize(&norm_cause, param_env, Unnormalized::new_wip(unnormalized_external_impl_sig)); |
ideally this should be a map_unnormalized call in which we instantiate the binder
There was a problem hiding this comment.
Doesn't Unnormalized::map keep the wrapper? Or you mean something like Unnormalized::normalize_with?
There was a problem hiding this comment.
my idea would be to rewrite these to the following longterm
let unnormalized_external_impl_sig = tcx.fn_sig(external_impl)
.instantiate(
tcx,
infcx.fresh_args_for_item(external_impl_span, external_impl.to_def_id()),
)
.map(|sig| infcx.instantiate_binder_with_fresh_vars(
external_impl_span,
infer::BoundRegionConversionTime::HigherRankedType,
sig
));
let external_impl_sig = ocx.normalize(
&norm_cause,
param_env,
unnormalized_external_impl_sig,
);
|
|
||
| // Placeholders (all printed as `_` to uniformize them). | ||
| ty::Param(_) | ty::Bound(..) | ty::Placeholder(_) | ty::Infer(_) | ty::Error(_) => { | ||
| ty::Alias(ty::AliasTy { kind: ty::Ambiguous { .. }, .. }) |
There was a problem hiding this comment.
unreachable
There was a problem hiding this comment.
can you also make ty::Infer unreachable?
| // Now that we perform eager normalization inside the solver in some places, | ||
| // The aliases might be normalized away. |
There was a problem hiding this comment.
when do we do eager norm for the inputs of alias relate goals? 🤔
There was a problem hiding this comment.
I added normalize_ambiguous_aliases in EvalCtxt::enter_forall which is used in EvalCtxt::compute_goal.
There was a problem hiding this comment.
alias relate should only ever relate non ambiguous aliases :3 so i think if entering binders only ever renormalizes these, alias relate stays as is
There was a problem hiding this comment.
Yeah, currently we don't renormalize in binder relating. That should the cause of this. Unsure how to do that 🤔
There was a problem hiding this comment.
the easiest is to normalize by looking at all ambiguous aliases which no longer has bound vars, replace it with an infer var, emit Projection(contained_Alias, ?infer) as nested goal
There was a problem hiding this comment.
not the ideal end-state, but definitely fine as the temporary solution until this has all settled down a bit more and we can think about how to clean it up
There was a problem hiding this comment.
Got it. Something like ReplaceAliasWithInfer but for ambiguous aliases only.
| /// next solver. See `NormalizationFolder`. | ||
| /// | ||
| /// The def_id and args are the same as the original alias. | ||
| AmbiguousTy { def_id: I::DefId }, |
There was a problem hiding this comment.
why does this contain the def_id
| } | ||
| } | ||
|
|
||
| pub fn instantiate_binder_with_fresh_vars_and_normalize_with<T, F>( |
There was a problem hiding this comment.
hmm, this is different from what I'd imagine 🤔
I would expect us to have
fn instantiate_binder_no_ambig_aliases(x: Binder<T>) -> T {
instantiate_binder_renormalize_ambig_aliases(x, |_| unreachable!())
}
fn instantiate_binder_renormalize_ambig_aliases(x: Binder<T>, normalize: impl FnMut(AliasTerm<'tcx>) -> Term<'tcx>) -> T {
instantiate binder
walk with renormalize ambig aliases folder
}I'd then keep the borrowck code etc the same as rn while using instantiate_binder_no_ambig_aliases and use instantiate_binder_renormalize_ambig_aliases only in the places that can encounter ambig aliases
There was a problem hiding this comment.
where all the old solver exclusive code can just use instantiate_binder_no_ambig_aliases
There was a problem hiding this comment.
and the new solver places that currently can't easily use a normalization routine can just recreate the ty::Alias or ConstKind::Alias with a FIXME that they should instead normalize
| self.print_def_path(def_id, args)?; | ||
| ty::Alias(ref data @ ty::AliasTy { kind, args, .. }) => match kind | ||
| .reveal_ambiguous(self.tcx()) | ||
| { |
There was a problem hiding this comment.
hmm, instead of reeal_ambiguous Ambiguous aliases can just forward to their contained alias?
| // Convert `AmbiguousTy` into its original kind. | ||
| pub fn reveal_ambiguous(self, interner: I) -> Self { | ||
| if let AliasTermKind::AmbiguousTy { def_id } = self { | ||
| interner.alias_term_kind_from_def_id(def_id) | ||
| } else { | ||
| self | ||
| } | ||
| } |
There was a problem hiding this comment.
ah, so what you do is ty::Alias(Projection { def_id }, [Self, T]) to ty::Alias(Ambiguous { def_id }, [Self, T]) instead of
ty::Alias(Projection { def_id }, [Self, T]) to ty::Alias(Ambiguous, ty::Alias(Projection { def_id }, [Self, T])]) which is what I would have expected 🤔
There was a problem hiding this comment.
I considered storing the original alias as the first generic arg at first.
The problem is that AliasTyKind expects an DefId now.
And we can't synthesize def_id on the fly in typeck, I think?
There was a problem hiding this comment.
Oh I see, calls to def_id are on the way to be removed with the AliasTy refactor. I'll remove the def_id and wrap the original alias in generic arg then.
There was a problem hiding this comment.
yeah, ICE when calling def_id for ambiguous aliases
| ) -> InferOk<'tcx, T> { | ||
| if self.infcx.next_trait_solver() { | ||
| let Normalized { value, obligations } = | ||
| crate::solve::normalize(*self, value, NormalizationScope::AmbiguousAlias); |
There was a problem hiding this comment.
i would not add support for this to the old trait solver at all 🤔 i would only ever use ambiguous aliases in the new solver, so here ambiguous aliases should be unreachable
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // We shouldn't register predicates to fcx in `probe()` as they're not rollbacked. | ||
| // Although it probably doesn't matter much for diagnostics. | ||
| let ocx = ObligationCtxt::new(&self.infcx); | ||
| let fty = ocx.normalize(&ObligationCause::dummy(), self.param_env, fty); |
There was a problem hiding this comment.
impacts behavior, but diagnostics only code afaict
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
5 participants
View all comments
Still WIP.
- fix rustdoc
- also normalize for diagnostics and assert the remaining instantiation doesn't need renormalization.
- remove the accidental staged
cargosubmoduleBut maybe you wanna have a quick look first.
r? lcnr