Repository navigation
Conversation
rustbot
added
S-waiting-on-review
|
|
There was a problem hiding this comment.
Changes look good! Left a few nits. 🚀
Question: Why do we want to handle rhs consts through the CTFE machinery? The typesystem machinery (valtrees) already works for this, though it's not as expressive as CTFE. Are we planning to eventually use the CTFE machinery for everything, including typesystemy stuff, since valtrees can't handle everything and can ICE on expressions that are valid for CTFE but not supported by the typesystem machinery? is the idea to gradually deprecate valtrees?
| }; | ||
| if self.const_of_item(def_id).is_none() { | ||
| return None; | ||
| }; |
| @@ -374,7 +375,11 @@ where | |||
| assert!(promoted.is_none() || Q::ALLOW_PROMOTED); | |||
|
|
|||
| // Don't peak inside trait associated constants. | |||
There was a problem hiding this comment.
nit:
| // Don't peak inside trait associated constants. | |
| // Don't peek inside trait associated constants. |
| if self.const_of_item(def_id).is_none() { | ||
| return None; | ||
| }; |
There was a problem hiding this comment.
| if self.const_of_item(def_id).is_none() { | |
| return None; | |
| }; | |
| self.const_of_item(def_id)? |
|
What is a "direct const"? All that new logic in From the issue:
That sounds very concerning and fragile. I think we may have to have a design session that includes wg-const-eval. It seems you are building a second const-eval system but it uses the same underlying compiler types as the old system so every part of the compiler needs to treat every const like a raw egg to avoid passing it to the wrong system. |
A direct const is a concept from the generic const args family of features. It's unfortunately quite an extensive explanation of what it is and why it is needed, I would love to properly sync at some point and get folks up to speed on what "direct consts" are. (e.g. had a lovely talk with Niko a few days ago about them). The long story short is that they are consts (both const items, and generic const arguments) that are represented "directly" via
Apologies, I mean reference as like, "refer to",
Generic const args is indeed quite a large feature and it took us many years to figure out a workable approach for const generics. I'm not sure what to say here, other than that this PR is reducing the amount of special-casing and jank, and pushing for a more cohesively designed GCA without as much special-casing.
Indeed, I agree it is very concerning and fragile, and this PR is working towards making things less ICEy and fragile. There are many very difficult questions here (e.g. how to handle initialized padding bytes in the type system) that are being worked on, and we have some good design progress on how to handle them.
Yeah, syncing wg-const-eval and project-const-generics is important to do at some point probably soon! This PR was conceptualized from design discussions with me, @BoxyUwU, @oli-obk, and @lcnr which is a handful of wg-const-eval folks but yeah, would be good to get everyone involved! |
We are still using valtrees here! Definitely not deprecating valtrees in any way. It's a very long story, but I suppose a bit of explanation can be gathered by considering one of the motivating cases of this PR: when a projection const is used in a body, we do not know whether it will resolve to a regular const (a const with a MIR body) or a direct const (a const with a "directly represented" |
For the part of the compiler I keep an eye on, it is a significant increase in what looks like ad-hoc special cases and non-trivial global invariants that are not encoded via separate types. I assume it reduces complexity elsewhere, but moving complexity around doesn't make it a clear win in my book. (The total line diff also confirms that impression.) If we really need two kinds of consts, they should be represented entirely separately in the compiler IMO. It should not be possible to get a (As I told Boxy before, I continue to feel like the amount of compiler and language complexity we are piling up here is entirely out of proportion to the benefit, and we should accept post-mono errors instead. I know that's not easy either due to trait solver interaction, but it avoids having to introduce a "kinding" system for consts and re-introducing an AST-based const-eval, as this proposal seems to be doing. But I don't have the capacity to actually help work out such a design so, well, this is neither here nor there. But I hope this helps you understand why I am so skeptical about this. :) |
|
r? oli-obk |
|
I think there's a lot of context missing here which is partially due to it not being in the PR description but also it's just not feasible to put it all in the PR description. I would like it if you and @oli-obk could speak about this given we (const generics folks) already spent time talking with wg-const-eval folks and building context there. I think it would be useful to keep this PR just to "how to make the impl maintainable" moreso than building context from scratch about how gca works which is probably more suited to a zulip convo or something. |
| @@ -74,8 +74,14 @@ impl<'tcx> TyCtxt<'tcx> { | |||
| // FIXME: maybe have a separate version for resolving mir::UnevaluatedConst? | |||
| match ty::Instance::try_resolve(self, typing_env, ct.def, ct.args) { | |||
There was a problem hiding this comment.
might it make sense to have instance resolution return a new variant/thing saying "needs you to do direct shenanigans" so that when you resolve stuff you're forced to handle it?
I guess I'm curious about two things:
- Do we resolve instances elsewhere that can encounter assoc consts and has wrong behaviour under gca
- Do we invoke any things like
const_eval_global_iddirectly elsewhere incorrectly
There was a problem hiding this comment.
I guess more generally it's not clear to me whether or not we want a more involved change for the API of how we enter const eval
As a wg-const-eval member I wasn't involved. It'd probably help to have you around for such a discussion.
Well I can't really discuss that with my current lack of context. I'd be rather unhappy about landing the PR in its current state, it basically turns |
|
We are going with the alternative approach as posted in #165070 and as such this PR is no longer necessary, closing |
rustbot
removed
the
S-waiting-on-review
|
Thanks! |
…oxyUwU create mir for direct-rhs consts Alternative to rust-lang#164982 - see that PR for context. When referring to a direct constant via a `mir::Const::Unevaluated`, synthesize a MIR body, whose contents are simply returning a `mir::Const::Ty` of itself. This effectively provides both a MIR view, as well as a typesystem view, of the same constant, without risking representing the same constant in different ways in the two different representations, because one is just referring to the other. r? @BoxyUwU fyi @oli-obk @lcnr
Rollup merge of #165070 - khyperia:mir_for_direct_const, r=BoxyUwU create mir for direct-rhs consts Alternative to #164982 - see that PR for context. When referring to a direct constant via a `mir::Const::Unevaluated`, synthesize a MIR body, whose contents are simply returning a `mir::Const::Ty` of itself. This effectively provides both a MIR view, as well as a typesystem view, of the same constant, without risking representing the same constant in different ways in the two different representations, because one is just referring to the other. r? @BoxyUwU fyi @oli-obk @lcnr
Lots of theorizing and discussion has happened here. Here's some words: One such writeup of the issue was done by me in this comment, for some background context: #162336 (comment) - one solution is to lower references to direct-rhs const items in MIR to
Const::Unevaluatedrather thanConst::Ty(as they are fundamentally sourced from regular values rather than the type system, which is whatConst::UnevaluatedvsConst::Tymeans). One particular way to do that is to intercept const eval at a very particular point: where we have just resolved the instance, but still have access to the unresolved instance, due to adding support for evaluating resolved projected items being vaguely annoying (certainly possible to do so, just difficult and annoying to maintain from a tech debt point of view). An example of an alternative implementation would be to add support to the trait solver for resolved projections, and instead intercept const eval within e.g.eval_to_const_value_raworeval_to_allocation_rawThis PR explicitly only addresses direct consts in
ExprKind::NamedConst. It does NOT address direct consts in patterns, insimd_swizzle, or#[const_continue], all of which useconst_eval_resolve_for_typeck.r? @BoxyUwU
fyi @lcnr @oli-obk this is the change we discussed in person