-
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
Normalize anon consts in new solver #112183
Conversation
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
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.
Can you try get a more explicit test for this that doesnt rely on std's Default
impl for arrays having an anon const for the const argument to the array. I wouldn't expect that impl to stay like that forever.
@@ -672,7 +672,10 @@ impl<'a, 'b, 'tcx> TypeFolder<TyCtxt<'tcx>> for AssocTypeNormalizer<'a, 'b, 'tcx | |||
#[instrument(skip(self), level = "debug")] | |||
fn fold_const(&mut self, constant: ty::Const<'tcx>) -> ty::Const<'tcx> { | |||
let tcx = self.selcx.tcx(); | |||
if tcx.lazy_normalization() || !needs_normalization(&constant, self.param_env.reveal()) { | |||
if tcx.features().generic_const_exprs | |||
|| tcx.trait_solver_next() |
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 don't know if it makes sense to do this- we dont stop fold_ty
from normalizing things if trait_solver_next
is enabled so I wouldn't think it makes sense to avoid normalization of consts.
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.
It doesn't hurt, though? Like, the only reason we have eager normalization of types currently is because it's difficult to disable because of alias-relate ambiguities (like due to lifetimes primarily).
I could revert this, but it does seem to move us closer to actually-lazy-norm at least for consts?
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 guess i'm a bit nervous about having types/consts diverge especially when it comes to implementing lazy norm since if we only do it for consts it seems more likely that bugs would go hidden since usage of const generics is way rarer than usage of type generics.
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 i'd want @lcnr to be okay with doing this if this is going to get merged as is
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.
That's fair, I can revert it and we can discuss "if and how we want to get to actually-lazy-norm-for-consts" in the new solver separately then.
5062967
to
6c1284d
Compare
ec94d60
to
3144a57
Compare
…IATs, don't partially support const projection for impls
3144a57
to
84196f3
Compare
r=me if CI passes |
@bors r=BoxyUwU |
…mpiler-errors Rollup of 6 pull requests Successful merges: - rust-lang#109609 (Separate AnonConst from ConstBlock in HIR.) - rust-lang#112166 (bootstrap: Rename profile = user to profile = dist) - rust-lang#112168 (Lower `unchecked_div`/`_rem` to MIR's `BinOp::Div`/`Rem`) - rust-lang#112183 (Normalize anon consts in new solver) - rust-lang#112211 (pass `--lib` to `x doc`) - rust-lang#112223 (Don't ICE in new solver when auto traits have associated types) r? `@ghost` `@rustbot` modify labels: rollup
We don't do any of that
expand_abstract_consts
stuff so this isn't sufficient to make GCE work, but it does allow, e.g.[(); 1]: Default
, to solve.r? @BoxyUwU