Stream: t-compiler/const-eval

Topic: Next steps for promotion and validation


ecstatic-morse (Sep 29 2019 at 22:01, on Zulip):

#64470 has been merged, which means it's time to discuss the next steps on the road to implementing #49146.

ecstatic-morse (Sep 29 2019 at 22:20, on Zulip):

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.
- HasMutInterior is true for &mut T as well as Cell<T>.
- Computing qualifs for borrows of !Frozen or 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?

ecstatic-morse (Sep 29 2019 at 22:22, on Zulip):

@centril? (last paragraph) I'm not sure how questions around naming things are usually resolved.

centril (Sep 29 2019 at 22:24, on Zulip):

@ecstatic-morse I don't think it's a big deal

centril (Sep 29 2019 at 22:24, on Zulip):

we can always rename later if need be

centril (Sep 29 2019 at 22:25, on Zulip):

The one thing that would be nice is to always cc the tracking issue in the PRs

centril (Sep 29 2019 at 22:25, on Zulip):

(which you are doing, so keep doing it... ^^)

ecstatic-morse (Sep 29 2019 at 22:25, on Zulip):

By tracking issue you mean #49146?

ecstatic-morse (Sep 29 2019 at 22:26, on Zulip):

(and whatever the one for loop is)

centril (Sep 29 2019 at 22:27, on Zulip):

yep

ecstatic-morse (Sep 29 2019 at 22:30, on Zulip):

There's also a few small things I need to do, like finishing #64828 and working with the authors of #61835 and #64588 to port their changes to the new validator.

ecstatic-morse (Sep 29 2019 at 22:33, on Zulip):

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.

ecstatic-morse (Sep 29 2019 at 22:38, on Zulip):

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 (check_match).

ecstatic-morse (Sep 29 2019 at 22:41, on Zulip):

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.

ecstatic-morse (Sep 29 2019 at 22:42, on Zulip):

Thanks everyone!

oli (Sep 30 2019 at 07:25, on Zulip):

Question: why are you fixing up the old logic at all?

ecstatic-morse (Sep 30 2019 at 17:29, on Zulip):

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.

oli (Sep 30 2019 at 17:32, on Zulip):

hmm... if you put any new things behind feature gates, you don't need to implement them in the old qualifier, too

ecstatic-morse (Sep 30 2019 at 18:09, on Zulip):

@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

oli (Sep 30 2019 at 18:10, on Zulip):

oh

oli (Sep 30 2019 at 18:10, on Zulip):

I misunderstood

ecstatic-morse (Sep 30 2019 at 18:14, on Zulip):

Things are especially brittle with the way qualifications in statics are currently handled.

ecstatic-morse (Sep 30 2019 at 18:15, on Zulip):

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.

oli (Sep 30 2019 at 18:17, on Zulip):

sweet

oli (Sep 30 2019 at 18:17, on Zulip):

I'll try to be faster this time around

oli (Sep 30 2019 at 18:17, on Zulip):

I'm still online for another hour

ecstatic-morse (Sep 30 2019 at 18:28, on Zulip):

@oli Rebased and pushed! Your suggestions won't quite work since self.analysis() will conflict with &mut self.state below.

oli (Sep 30 2019 at 18:29, on Zulip):

ah, that's too bad

oli (Sep 30 2019 at 18:29, on Zulip):

ok

ecstatic-morse (Sep 30 2019 at 18:35, on Zulip):

Thank you! btw, if there's anything const-eval or mir-opt related that needs more manpower I would be happy to help.

RalfJ (Oct 09 2019 at 13:33, on Zulip):

@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!

ecstatic-morse (Oct 10 2019 at 21:08, on Zulip):

@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?

RalfJ (Oct 11 2019 at 12:13, on Zulip):

Sounds good. I'll edit my PR later today to hopefully fix your confusion.

ecstatic-morse (Oct 11 2019 at 20:02, on Zulip):

@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?

ecstatic-morse (Oct 11 2019 at 22:09, on Zulip):

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

ecstatic-morse (Oct 20 2019 at 18:14, on Zulip):

@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.

eddyb (Oct 20 2019 at 18:14, on Zulip):

okay I guess it's on top of my priorities

Last update: Nov 15 2019 at 20:00UTC