Remove promotion candidate gathering and checking from `qualify_consts.rs` #66066

ecstatic-morse · 2019-11-03T20:56:43Z

This makes promotion candidate gathering and checking the exclusive domain of promote_consts, but the QualifyAndPromoteConsts pass is still responsible for both const-checking and creating promoted MIR fragments.

This should not be merged until the beta branches on Nov. 5.

r? @eddyb

ecstatic-morse · 2019-11-03T22:02:35Z

This is more easily reviewed commit-by-commit, and hiding whitespace changes will make the first commit more intelligible.

ecstatic-morse · 2019-11-03T22:16:45Z

It looks like removing IsNotPromotable will require adding some additional logic to set the qualifs in the return place when an expression that is forbidden in a const context is encountered. Do you want me to do this @eddyb? I'm still trying to avoid making non-trivial modifications to this code since it will go away soon, and I'm not 100% sure how this change will manifest. Alternatively I could revert the last commit.

ecstatic-morse · 2019-11-04T21:12:17Z

Interestingly, just removing it does not cause any test errors. I think this code-path will only be important if we try to promote a reference to a const for which const-checking has failed? I still think we should leave IsNotPromotable in until qualify_consts.rs is removed completely.

ecstatic-morse · 2019-11-05T18:38:34Z

Specifically, this is the code I'm worried about. It looks for IsNotPromotable in the return place and sets NeedsDrop and HasMutInterior if they are possible for the type (as a side effect, this clears IsNotPromotable and IsNotImplicitlyPromotable, which was maybe not intended?). This prevents promotion of complex or errorful consts. I was never sure about the purpose or correctness of this code: all named variables should have IsNotPromotable, so this branch will also trigger if a named variable is returned from a const. Is this intended?

I plan to approximate this behavior in the new const-checker by setting qualifs conservatively (as well as setting an error bit) if there are errors during const-checking. I think this is roughly what was intended in this code? We'll need a crater run anyways when switching to the new const-checker, which should give us more confidence.

I still think we should just leave this in for now (and erase the last commit in this PR). I would rather work on improving the final version, which will replace this entirely, than spend time on this.

bors · 2019-11-06T09:37:47Z

☔ The latest upstream changes (presumably #66143) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2019-11-06T14:32:25Z

I don't think there's any reason to care about const-checking errors, compilation will fail anyway, I was likely trying to avoid weird ICEs but nowadays everything is so much more robust that it's pointless.

eddyb · 2019-11-06T14:38:10Z

src/librustc_mir/transform/qualify_consts.rs

+        // promote anything, since that can cause errors in a `const` if e.g. rvalue static
+        // promotion is attempted within a loop body.
+        let unleash_miri = self.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you;
+        let promotion_candidates = if has_controlflow_error && !unleash_miri {


I was going to say that this isn't needed (since IIRC the reason I added it was because there would be mismatches since qualify_consts doesn't see as many candidates as there are in the body, because it stops looking at BBs) but you did replace my HACK comment so I'm guessing you hit some other issue?

Actually, I'm still not sure this check is needed, promotion within fn bodies works just fine without it, so what gives?

We can't do rvalue-static promotion in a loop with the current "storage dead removal" system for consts; it results in broken MIR and ultimately an ICE as the stack slot for the temp is never cleaned up:

const X: () = loop { let x = &4; };

Actually, there's no ICE on the test that hits this (sorry, it's been a few days since I ran them). There's just a regular error. I vaguely remember hitting something worse, but maybe we just always try promotion?

+ error[E0384]: cannot assign twice to immutable variable `_` + --> $DIR/issue-52475.rs:10:18 + | + LL | x = &0; // Materialize a new AllocId + | ^ cannot assign twice to immutable variable +

eddyb

r=me after a rebase and figuring out that one comment

We no longer compare the results of `promote_consts::validate_candidates` with `checker.promotion_candidates`, and `promote_consts` becomes the canonical source for determining promotability.

Also removes any code used only to populate `promotion_candidates` during const checking.

The former was cleared from `qualifs_in_return_place`, and the latter was never checked. `QUALIF_ERROR_BIT` no longer corresponds to a real `Qualif`, however.

The old const-checker conservatively reset qualifs when `IsNotPromotable` was in the return place. Unfortunately, named variables have `IsNotPromotable`, so this could cause promotion to fail. This should work now.

eddyb · 2019-11-06T16:46:45Z

@bors r+

bors · 2019-11-06T16:46:47Z

📌 Commit ec5ba54 has been approved by eddyb

bors · 2019-11-08T07:55:57Z

⌛ Testing commit ec5ba54 with merge 76ade3e...

@eddyb

…onsts, r=eddyb Remove promotion candidate gathering and checking from `qualify_consts.rs` This makes promotion candidate gathering and checking the exclusive domain of `promote_consts`, but the `QualifyAndPromoteConsts` pass is still responsible for both const-checking and creating promoted MIR fragments. This should not be merged until the beta branches on Nov. 5. r? @eddyb

bors · 2019-11-08T11:08:57Z

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 76ade3e to master...

rust-highfive assigned eddyb Nov 3, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2019

This comment has been minimized.

Sign in to view

ecstatic-morse mentioned this pull request Nov 3, 2019

Move promotion into its own pass #65949

Merged

ecstatic-morse force-pushed the remove-promotion-from-qualify-consts branch 2 times, most recently from 5b09205 to 5e23e46 Compare November 3, 2019 22:43

eddyb reviewed Nov 6, 2019

View reviewed changes

eddyb approved these changes Nov 6, 2019

View reviewed changes

ecstatic-morse added 4 commits November 6, 2019 07:03

Remove valid_promotion_candidates

328a898

We no longer compare the results of `promote_consts::validate_candidates` with `checker.promotion_candidates`, and `promote_consts` becomes the canonical source for determining promotability.

Remove promotion_candidates from Checker

383eb01

Also removes any code used only to populate `promotion_candidates` during const checking.

Remove IsNotPromotable and IsNotImplicitlyPromotable

a9b1abe

The former was cleared from `qualifs_in_return_place`, and the latter was never checked. `QUALIF_ERROR_BIT` no longer corresponds to a real `Qualif`, however.

Add test for promotability in let

ec5ba54

The old const-checker conservatively reset qualifs when `IsNotPromotable` was in the return place. Unfortunately, named variables have `IsNotPromotable`, so this could cause promotion to fail. This should work now.

ecstatic-morse force-pushed the remove-promotion-from-qualify-consts branch from 5e23e46 to ec5ba54 Compare November 6, 2019 15:11

eddyb approved these changes Nov 6, 2019

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 8, 2019

bors merged commit ec5ba54 into rust-lang:master Nov 8, 2019

ecstatic-morse deleted the remove-promotion-from-qualify-consts branch October 6, 2020 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove promotion candidate gathering and checking from `qualify_consts.rs` #66066

Remove promotion candidate gathering and checking from `qualify_consts.rs` #66066

ecstatic-morse commented Nov 3, 2019 •

edited

Loading

ecstatic-morse commented Nov 3, 2019

This comment has been minimized.

ecstatic-morse commented Nov 3, 2019 •

edited

Loading

ecstatic-morse commented Nov 4, 2019

ecstatic-morse commented Nov 5, 2019 •

edited

Loading

bors commented Nov 6, 2019

eddyb commented Nov 6, 2019

eddyb Nov 6, 2019

ecstatic-morse Nov 6, 2019 •

edited

Loading

ecstatic-morse Nov 6, 2019 •

edited

Loading

eddyb left a comment

eddyb commented Nov 6, 2019

bors commented Nov 6, 2019

bors commented Nov 8, 2019

bors commented Nov 8, 2019

Remove promotion candidate gathering and checking from qualify_consts.rs #66066

Remove promotion candidate gathering and checking from qualify_consts.rs #66066

Conversation

ecstatic-morse commented Nov 3, 2019 • edited Loading

ecstatic-morse commented Nov 3, 2019

This comment has been minimized.

ecstatic-morse commented Nov 3, 2019 • edited Loading

ecstatic-morse commented Nov 4, 2019

ecstatic-morse commented Nov 5, 2019 • edited Loading

bors commented Nov 6, 2019

eddyb commented Nov 6, 2019

eddyb Nov 6, 2019

Choose a reason for hiding this comment

ecstatic-morse Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

eddyb left a comment

Choose a reason for hiding this comment

eddyb commented Nov 6, 2019

bors commented Nov 6, 2019

bors commented Nov 8, 2019

bors commented Nov 8, 2019

Remove promotion candidate gathering and checking from `qualify_consts.rs` #66066

Remove promotion candidate gathering and checking from `qualify_consts.rs` #66066

ecstatic-morse commented Nov 3, 2019 •

edited

Loading

ecstatic-morse commented Nov 3, 2019 •

edited

Loading

ecstatic-morse commented Nov 5, 2019 •

edited

Loading

ecstatic-morse Nov 6, 2019 •

edited

Loading

ecstatic-morse Nov 6, 2019 •

edited

Loading