generic_const_args: allow paths to non type consts#155341
generic_const_args: allow paths to non type consts#155341khyperia wants to merge 2 commits intorust-lang:mainfrom
Conversation
|
changes to the core type system cc @lcnr Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred in engine.rs, potentially modifying the public API of cc @lcnr Some changes occurred in cc @BoxyUwU HIR ty lowering was modified cc @fmease changes to the core type system cc @lcnr |
rustbot
added
S-waiting-on-review
There was a problem hiding this comment.
cool ✨ two big picture things:
- you've changed
evaluate_constto take an unevaluated const instead of aty::Const. iirc we talked about doing this to make IACs work but since we've dropped support for them can those changes be reverted? - if it would be kinda chill for you to do can you make the change of adding fields to
AliasTermKind's variants be in a separate commit so it's easier to review just the "meaningful" stuff? If it's non-trivial/difficult it's fine to keep it as is 🤔
|
|
||
| // Skip type consts as mGCA doesn't support evaluatable clauses. | ||
| if self.tcx.is_type_const(uv.def) { | ||
| if self.tcx.is_type_const(uv.def) || self.tcx.features().generic_const_args() { |
There was a problem hiding this comment.
do you remember why this was needed?
There was a problem hiding this comment.
Not really. Removing it!
- the similar line of code in wf.rs is needed because
(
cannot satisfy `the constant `<T as Trait>::PROJECTED` can be evaluated`Tis a generic parameter to the function,fn f<T: Trait>(), which blocks const eval, because, free var) - this is the other place that
ty::ClauseKind::ConstEvaluatableis constructed, and I think I slapped on the same guard to this place in predicates_of.rs without fully understanding how/why this line of code is hit
| && !(matches!(tcx.def_kind(def_id), DefKind::AssocConst { is_type_const: false }) | ||
| && matches!( | ||
| tcx.def_kind(tcx.parent(def_id)), | ||
| DefKind::Impl { of_trait: false } |
There was a problem hiding this comment.
can you move this out to a let is_iac so it's easier to understand what we're actually conditional on
| cx.const_of_item(free_alias.def_id).instantiate(cx, free_alias.args).into() | ||
| } | ||
| ty::AliasTermKind::FreeConst { is_type_const: false } => { | ||
| self.try_evaluate_added_goals()?; |
There was a problem hiding this comment.
do you remember why this was necessary? 🤔 (and same for the call in the other normalization routines in the new solver)
There was a problem hiding this comment.
It was originally needed for IACs, because the "gen fresh vars, eq self with impl type, see what vars resolved to" song-and-dance needed to be computed to be able to see the concrete args for const eval.
I left it in since I was scared that some parent caller of this function might pass in some vars that have had uncomputed goals added about them that would resolve said vars, but I suppose that might not happen, I'm not quite sure how the new trait solver works and if we're guaranteed to be in a context without Stuff(tm) happening above it.
I'll remove it!
| cx.const_of_item(inherent.def_id).instantiate(cx, inherent_args).into() | ||
| } | ||
| ty::AliasTermKind::InherentConst { is_type_const: false } => { | ||
| // FIXME(gca): This is dead code at the moment. |
There was a problem hiding this comment.
I think it's ok to leave the logic here as unreachable!() with a FIXME, same for the equivalent logic in the old solver
| ProjectionConst { is_type_const: bool }, | ||
| /// A top level const item not part of a trait or impl. | ||
| FreeConst, | ||
| FreeConst { is_type_const: bool }, | ||
| /// An associated const in an inherent `impl` | ||
| InherentConst, | ||
| InherentConst { is_type_const: bool }, |
There was a problem hiding this comment.
cc @WaffleLapkin when AliasTermKind has been updated to store DefIds like you've done for AliasTyKind this can be removed and we can just call tcx.is_type_const(def) everywhere instead
khyperia
force-pushed
the
non-type-const
branch
from
cd8a2ae to
53e96d1
Compare
yeah I guess, it just makes me sad to revert, the change seems nice :c :P - I've stashed the work so can maybe done sometime in the future
done! edit: ack, the split wasn't fully clean, there's a swap from edit: ok, pushed, moving the change to the other commit
I think the |
khyperia
force-pushed
the
non-type-const
branch
from
53e96d1 to
bc46b88
Compare
3 participants
Non type consts should be usable in the type system in
feature(generic_const_args). These are directly plugged into the constant evaluator, unlike type consts, which are attempted to be reasoned about by the type system.Inherent associated constants are not supported at this time, due to complications around how generic arguments are represented for them (it's currently a mess). The mess is being cleaned up (e.g. #154758), so instead of trying to hack support in before the refactoring is done, let's just wait to be able to implement it more cleanly.
r? @BoxyUwU