In the review for that PR, @eddyb brought up some shortcomings in the old validation logic that got ported over to the dataflow-enabled one. Namely
- Some error messages are phrased unclearly or flat out wrong.
- Accessing a
static ignores its qualifications.
&mut T as well as
- Computing qualifs for borrows of
Drop types is needlessly complicated
I plan on opening PRs for the first two in the near future, with the first PR also swapping out the ad-hoc error reporting and instead using
NonConstOp errors. I believe the third should wait until we implement promotion separately (see #63812) and remove the old pass, since any fixes can't be ported to the old pass. I'm not quite sure what fixing the last concern would require, so I'll let @eddyb explain more before working to resolve it.
There's some unresolved bikeshedding around the name of the context object that holds information about the thing being validated. I think I will also open a PR with a name change so that discussion can continue. I was planning on using
ItemCtxt in the initial version of this, does anyone object to this as a starting point?
@centril? (last paragraph) I'm not sure how questions around naming things are usually resolved.
@ecstatic-morse I don't think it's a big deal
we can always rename later if need be
The one thing that would be nice is to always cc the tracking issue in the PRs
(which you are doing, so keep doing it... ^^)
By tracking issue you mean #49146?
(and whatever the one for
Although I could hack together an implementation of #49146, I think we should focus on getting #63812 merged in one form or another, since this lets us disable the old validator when the new one is deemed sufficiently backwards-compatible.
Someone has raised concerns about the performance of running both validators in #64470, although I'm skeptical that this is actually a "large perf regression" since it appears that only one crate actually regressed in wall-time and the bulk of that regression was caused by an unrelated query (
So yeah, I'll start work on those follow-up PRs and ping the authors of the PRs that conflict with #64470. In the meantime, @eddyb let me know what I can do to help with #63812. I've read through it a couple of times now and I should leave some comments.
Question: why are you fixing up the old logic at all?
We can't remove the old logic until we collect enough data to be confident that switching to dataflow has not affected the qualifications for simple consts and #63812 gets implemented in some form. I don't know how long either of these will take. Until that point, people will want to continue to modify
qualify_consts.rs, and keeping the passes as similar as possible will help me port their changes to the new pass. Also, keeping the passes in sync will make it easier to debug mismatched validation errors, since there won't be as many places to look.
hmm... if you put any new things behind feature gates, you don't need to implement them in the old qualifier, too
@oli I'm not quite sure what you mean, eddyb's fixes simplify the validation logic or improve error messages, there's no new functionality. We could wait and make these fixes once the old qualifier has been removed, but I need something to do in the meantime XD
Things are especially brittle with the way qualifications in
statics are currently handled.
Oh, saw your reviews on the graphviz debugging PR. I'm fixing the merge conflict and then I'll push some more formatting changes. I think after that it will be good to go.
I'll try to be faster this time around
I'm still online for another hour
@oli Rebased and pushed! Your suggestions won't quite work since
self.analysis() will conflict with
&mut self.state below.
ah, that's too bad
Thank you! btw, if there's anything const-eval or mir-opt related that needs more manpower I would be happy to help.
@ecstatic-morse one thing we have historically been lacking here is docs, in particular design-level docs (what are the high-level properties being checked where and why). there's some groundwork for that at https://github.com/rust-rfcs/const-eval/ but it would be great if you could expand that as you learn things during your rewrite :)
also thanks so much for all of this, it is rgeat to finally see movement in this part of the compiler!
@RalfJ I think I'm going to open a PR to add a longer introduction to the promotion document detailing the contexts where we trigger promotion. This will be blocked on deciding on terminology, but maybe it would be a good place to have the discussion?
Sounds good. I'll edit my PR later today to hopefully fix your confusion.
@eddyb What's the status of #63812? Crater now lets you specify a crate list from the github bot. I would trigger the run myself, but I don't have the requisite permissions. Copy-pasting the linked command should be enough though.
I know this PR requires a special resolver for temp promotion. Would you like me to open a separate PR for it maybe?
RalfJ I think I'm going to open a PR to add a longer introduction to the promotion document detailing the contexts where we trigger promotion. This will be blocked on deciding on terminology, but maybe it would be a good place to have the discussion?
For anyone following along, this is rust-rfcs/const-eval#28
@eddyb I'd like to get a new promotion pass merged sometime next week. Are you planning to get back to #63812 anytime soon? It didn't cause any regressions on crater.
I'm happy to rebase it and finish it up, but there's a few
HACK comments that I assume you wanted to resolve before removing the WIP tag. I don't really know what you had in mind for them.
okay I guess it's on top of my priorities