-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 missing normalization when checking const generics #114702
Conversation
I'm not actually totally convinced this is something we want to support. We don't normalize other aliases --
|
Even if we were to normalize things here, we should not be using |
Are projections the same as aliases? Also, sorry, just realized that some context is missing (only present in the issue). Without the feature enabled, the code compiles just fine, meaning that aliases are supposed to work for const generics. But maybe I should only normalize if this is a weak ty alias? As you prefer. As for using projections in const generics, it was something that was kinda agreed upon in #104443 but it wasn't merged because it was introducing new bugs (hence why I'm suggesting to only normalize if this is a weak ty alias).
Sounds good to me! So what do you want me to do here? Should I update the PR to use the correct normalization, should I only normalize if it's a weak ty alias or this is not something that the compiler wants at all and then we can close this PR? |
Both "lazy type aliases" ("weak") and projections are forms of alias, so I expect them to act consistently.
The point of the
I think we should then probably bring this question back up formally rather than just fix it ad-hoc like this. The more that these features diverge, the more we're gonna have to clean up and reconcile later, imo. Also, doesn't this lead to the same exact bugs as #104443? Either that, or this PR is incomplete? |
Thanks for the clarification!
Makes sense! Just a bit weird. 😸
It's not doing the same change (maybe because the code changed a lot since then?) as it doesn't fail the test, so I suppose it's unrelated to supporting projections in const generics. Just an early check I guess. Anyway, I'll update the code and if the compiler team doesn't want it, I'll close this PR and instead open a new one with the failing code so it's not forgotten. |
When changing: - let param_env = tcx.param_env(param.def_id);
- let ty = tcx.normalize_erasing_regions(param_env, ty);
+ enter_wf_checking_ctxt(tcx, hir_ty.span, param.def_id, |wfcx| {
+ let ty = wfcx.normalize(hir_ty.span, Some(WellFormedLoc::Ty(param.def_id)), ty);
So I suppose either this fix is incomplete or it's simply wrong to change it. I updated the code, but I guess it'll just be closed for now. |
dc00714
to
9195cf1
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
Fixes #114217.
Part of #112792.
r? @compiler-errors