Add a HIR pass to check consts for `if`, `loop`, etc. #66170

ecstatic-morse · 2019-11-06T20:56:17Z

Resolves #66125.

This PR adds a HIR pass to check for high-level control flow constructs that are forbidden in a const-context. The MIR const-checker is unable to provide good spans for these since they are lowered to control flow primitives (e.g., Goto and SwitchInt), and these often don't map back to the underlying statement as a whole. This PR is intended only to improve diagnostics once if and match become commonplace in constants (behind a feature flag). The MIR const-checker will continue to operate unchanged, and will catch anything this check might miss.

In this implementation, the HIR const-checking pass is run much earlier than the MIR one, so it will supersede any errors from the latter. I will need some mentoring if we wish to change this, since I'm not familiar with the diagnostics system. Moving this pass into the same phase as the MIR const-checker could also help keep backwards compatibility for items like const _: () = loop { break; };, which are currently (erroneously?) accepted by the MIR const-checker (see #62272).

r? @Centril
cc @eddyb (since they filed #62272)

eddyb · 2019-11-06T21:32:00Z

cc @rust-lang/compiler I was hoping we would implement const-checking for arbitrary CFGs and never have to deal with a HIR check on constants again, but I guess I was wrong.

Centril

The checker code looks good aside from some nits and some thoughts for the future (cleanup tasks mostly unrelated to the PR but which I noticed along the way).

src/librustc_passes/error_codes.rs

src/librustc/hir/map/mod.rs

Centril · 2019-11-07T00:06:14Z

src/librustc/query/mod.rs

@@ -329,6 +329,11 @@ rustc_queries! {
            desc { |tcx| "checking for unstable API usage in {}", key.describe_as_module(tcx) }
        }

+        /// Checks the const bodies in the module for illegal operations (e.g. `if` or `loop`).
+        query check_mod_const_bodies(key: DefId) -> () {


Aside: I noticed that the categorization in this module wrt. Other, TypeChecking, and friends is rather messy and there doesn't seem to be a lot of logic to it? -- Maybe another C-cleanup issue...? (cc @Zoxc & @nikomatsakis)

Oh, this categorization was added as a very coarse grouping for profiling purposes IIRC, we can certainly clean things up here. cc @wesleywiser

Cleanups wrt categorization are definitely appreciated. I did the initial pass ~1yr ago and I don't think they've changed substantially since that time. Feel free to assign me to any such cleanup PRs.

src/librustc_passes/check_const.rs

src/test/ui/consts/min_const_fn/min_const_fn.rs

Centril · 2019-11-07T00:39:24Z

In this implementation, the HIR const-checking pass is run much earlier than the MIR one, so it will supersede any errors from the latter. I will need some mentoring if we wish to change this, since I'm not familiar with the diagnostics system. Moving this pass into the same phase as the MIR const-checker could also help keep backwards compatibility for items like const _: () = loop { break; };, which are currently (erroneously?) accepted by the MIR const-checker (see #62272).

I wouldn't expend any energy on making #62272 work; it's not very useful in any case and was never intended to be allowed on stable other than as a side-effect of how the compiler is structured. The check as written in this PR is good. :)

src/test/ui/consts/const-loop.rs

src/test/ui/consts/const-eval/issue-62272.rs

ecstatic-morse · 2019-11-07T02:00:03Z

Okay, I fixed most of @Centril's suggestions, blessed the back-compat breakages, and added an explanation to them. Let me know if you're okay with the new error messages.

This should pass CI now. I'd like people to weigh in on where to put this in the compilation pipeline; I think it should go alongside the MIR const-checker if possible. ~~I'll also look into checking array length expressions.~~ (done)

Centril · 2019-11-07T02:04:24Z

On my end, the code & diagnostics look good, so r=me on that aspect when you feel comfortable.

src/test/ui/rfc-2497-if-let-chains/disallowed-positions.rs

ecstatic-morse · 2019-11-07T18:13:06Z

src/test/ui/borrowck/issue-64453.rs

@@ -13,7 +11,6 @@ fn set_editor(_: Value) {}

 fn main() {
    let settings_data = from_string(settings_dir);
-    //~^ ERROR cannot move out of static item `settings_dir` [E0507]


It's unfortunate that this error no longer occurs. I believe it's simply because the compiler stops working before the move checker runs?

ecstatic-morse · 2019-11-07T18:17:29Z

src/test/ui/consts/const-eval/issue-62272.rs


 fn main() {
-    [FOO; { let x; loop { x = 5; break; } x }];
+    [FOO; { let x; loop { x = 5; break; } x }]; //~ ERROR `loop` is not allowed in a `const`


How do we feel about the term const for an array initializer? Is there a preexisting convention here? It's pretty easy to change.

Maybe @estebank has thoughts?

I would prefer us to be correct mentioning the array length is a const context, but this should be ok for now, we have plenty of other places where we are inconsistent about calling this out.

Oh, also, I think const generic arguments are also anon_const, so this isn't that easy. Probably the right way is to add an AnonConstKind to AnonConst in hir/mod.rs. With that in mind, I'm inclined to fix this later :).

ecstatic-morse · 2019-11-07T18:24:30Z

I've rebased to clean up the history a bit and (hopefully) blessed the remaining tests. I've left some comments where the new error messages aren't a clear improvement.

This is still waiting for @rust-lang/compiler to weigh in on whether breaking #62272 is okay. It might need an FCP/crater run?

Also, I would still like for someone to review the relative order of the check_mod_const_bodies query. It would be nice run this at the same time as the MIR const-checker instead of superseding it, so we could leave the min_const_fn tests unchanged. We could then change the if/match errors in the MIR const-checker to delay_span_bugs (I think).

Centril · 2019-11-08T07:46:31Z

src/librustc_passes/error_codes.rs

+Control-flow expressions are not allowed inside a const context.
+
+At the moment, `if` and `match`, as well as the looping constructs `for`,
+`while`, and `loop`, are forbidden inside a `const`, `static`, or `const fn`.


Upon re-reading this it occurred to me that we could link to the tracking issues... should we perhaps do that?
cc @estebank @oli-obk

I'll do this in a follow-up PR since the bulk of this one is ready.

Centril · 2019-11-08T07:49:08Z

Also, I would still like for someone to review the relative order of the check_mod_const_bodies query. It would be nice run this at the same time as the MIR const-checker instead of superseding it, so we could leave the min_const_fn tests unchanged. We could then change the if/match errors in the MIR const-checker to delay_span_bugs (I think).

This part is unfortunately out of my expertise to mentor; I'll defer to folks on T-Compiler.

This PR BREAKS CODE THAT WAS ACCEPTED ON STABLE. It's arguably a bug that this was accepted in the first place, but here we are. See rust-lang#62272 for more info.

The MIR const-checker errors for if/match/loop are now delay span bugs, so nothing will be emitted unless the HIR checker misses something.

ecstatic-morse · 2019-11-13T18:49:28Z

Once again broken by a refactoring. @Centril can I make this p=1? I guess it won't matter since this is old enough to be at the top of the queue now.

@bors r=Centril,oli-obk

bors · 2019-11-13T18:49:29Z

📌 Commit 7552bd6 has been approved by Centril,oli-obk

bors · 2019-11-13T20:11:00Z

⌛ Testing commit 7552bd6 with merge ded5ee0...

@Centril

Add a HIR pass to check consts for `if`, `loop`, etc. Resolves #66125. This PR adds a HIR pass to check for high-level control flow constructs that are forbidden in a const-context. The MIR const-checker is unable to provide good spans for these since they are lowered to control flow primitives (e.g., `Goto` and `SwitchInt`), and these often don't map back to the underlying statement as a whole. This PR is intended only to improve diagnostics once `if` and `match` become commonplace in constants (behind a feature flag). The MIR const-checker will continue to operate unchanged, and will catch anything this check might miss. In this implementation, the HIR const-checking pass is run much earlier than the MIR one, so it will supersede any errors from the latter. I will need some mentoring if we wish to change this, since I'm not familiar with the diagnostics system. Moving this pass into the same phase as the MIR const-checker could also help keep backwards compatibility for items like `const _: () = loop { break; };`, which are currently (erroneously?) accepted by the MIR const-checker (see #62272). r? @Centril cc @eddyb (since they filed #62272)

bors · 2019-11-13T23:21:49Z

☀️ Test successful - checks-azure
Approved by: Centril,oli-obk
Pushing ded5ee0 to master...

@GuillaumeGomez

…msg, r=estebank Link to tracking issue in HIR const-check error code description This is a follow up to rust-lang#66170 that addresses [this comment](rust-lang#66170 (comment)). As an aside, the only other error code whose description uses the phrase "tracking issue" is `E0202`. We may want to come up with standards around this, especially since error codes are now centralized and easier to keep track of (thanks @GuillaumeGomez). r? @estebank (since they +1'ed the comment)

@GuillaumeGomez

…msg, r=estebank Link to tracking issue in HIR const-check error code description This is a follow up to rust-lang#66170 that addresses [this comment](rust-lang#66170 (comment)). As an aside, the only other error code whose description uses the phrase "tracking issue" is `E0202`. We may want to come up with standards around this, especially since error codes are now centralized and easier to keep track of (thanks @GuillaumeGomez). r? @estebank (since they +1'ed the comment)

@GuillaumeGomez

…msg, r=estebank Link to tracking issue in HIR const-check error code description This is a follow up to rust-lang#66170 that addresses [this comment](rust-lang#66170 (comment)). As an aside, the only other error code whose description uses the phrase "tracking issue" is `E0202`. We may want to come up with standards around this, especially since error codes are now centralized and easier to keep track of (thanks @GuillaumeGomez). r? @estebank (since they +1'ed the comment)

@GuillaumeGomez

…msg, r=estebank Link to tracking issue in HIR const-check error code description This is a follow up to rust-lang#66170 that addresses [this comment](rust-lang#66170 (comment)). As an aside, the only other error code whose description uses the phrase "tracking issue" is `E0202`. We may want to come up with standards around this, especially since error codes are now centralized and easier to keep track of (thanks @GuillaumeGomez). r? @estebank (since they +1'ed the comment)

rust-highfive assigned Centril Nov 6, 2019

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

Centril reviewed Nov 7, 2019

View reviewed changes

Centril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2019

ecstatic-morse commented Nov 7, 2019

View reviewed changes

src/test/ui/consts/const-loop.rs Show resolved Hide resolved

Centril reviewed Nov 7, 2019

View reviewed changes

src/test/ui/consts/const-eval/issue-62272.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ecstatic-morse force-pushed the hir-const-check branch from ddc87f8 to f539d6e Compare November 7, 2019 03:43

This comment has been minimized.

Sign in to view

ecstatic-morse force-pushed the hir-const-check branch from eede3d6 to 1fdd6bd Compare November 7, 2019 18:03

ecstatic-morse commented Nov 7, 2019

View reviewed changes

src/test/ui/rfc-2497-if-let-chains/disallowed-positions.rs Show resolved Hide resolved

ecstatic-morse commented Nov 7, 2019

View reviewed changes

ecstatic-morse force-pushed the hir-const-check branch from 1fdd6bd to 469040b Compare November 7, 2019 18:50

This comment has been minimized.

Sign in to view

ecstatic-morse force-pushed the hir-const-check branch from 469040b to 05c4808 Compare November 7, 2019 20:49

This comment has been minimized.

Sign in to view

Centril reviewed Nov 8, 2019

View reviewed changes

Centril approved these changes Nov 8, 2019

View reviewed changes

ecstatic-morse added 7 commits November 13, 2019 10:44

Bless back-compat breakages

281e898

This PR BREAKS CODE THAT WAS ACCEPTED ON STABLE. It's arguably a bug that this was accepted in the first place, but here we are. See rust-lang#62272 for more info.

Small fixes to comments

eff83e5

Fix broken doc-test

0123cbd

Change control flow error to delay span bug

70aa781

Bless less verbose error messages

86734b1

The MIR const-checker errors for if/match/loop are now delay span bugs, so nothing will be emitted unless the HIR checker misses something.

Bless miri unleashed test now that errors are mandatory

5e048da

Use ast::Mutability

7552bd6

ecstatic-morse force-pushed the hir-const-check branch from 16c674f to 7552bd6 Compare November 13, 2019 18:47

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 13, 2019

ecstatic-morse mentioned this pull request Nov 13, 2019

Make dataflow-based const qualification the canonical one #66385

Merged

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

bors merged commit 7552bd6 into rust-lang:master Nov 13, 2019

This was referenced Nov 13, 2019

Move error codes #66314

Merged

Adjust cannot move out of static error in prep for removing Statics from Place #66328

Closed

Cleanup rmeta::MacroDef #66364

Merged

rust-lang deleted a comment from bors Nov 13, 2019

ecstatic-morse mentioned this pull request Nov 14, 2019

Link to tracking issue in HIR const-check error code description #66418

Merged

ecstatic-morse deleted the hir-const-check branch October 6, 2020 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a HIR pass to check consts for `if`, `loop`, etc. #66170

Add a HIR pass to check consts for `if`, `loop`, etc. #66170

ecstatic-morse commented Nov 6, 2019 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

eddyb commented Nov 6, 2019

This comment has been minimized.

Centril left a comment

Centril Nov 7, 2019

eddyb Nov 11, 2019

wesleywiser Nov 11, 2019

Centril commented Nov 7, 2019

ecstatic-morse commented Nov 7, 2019 •

edited

Loading

Centril commented Nov 7, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ecstatic-morse Nov 7, 2019

ecstatic-morse Nov 7, 2019

Centril Nov 7, 2019

estebank Nov 7, 2019

ecstatic-morse Nov 7, 2019

ecstatic-morse commented Nov 7, 2019

This comment has been minimized.

This comment has been minimized.

Centril Nov 8, 2019

ecstatic-morse Nov 10, 2019

Centril commented Nov 8, 2019

ecstatic-morse commented Nov 13, 2019 •

edited

Loading

bors commented Nov 13, 2019

bors commented Nov 13, 2019

bors commented Nov 13, 2019

Add a HIR pass to check consts for if, loop, etc. #66170

Add a HIR pass to check consts for if, loop, etc. #66170

Conversation

ecstatic-morse commented Nov 6, 2019 • edited Loading

This comment has been minimized.

This comment has been minimized.

eddyb commented Nov 6, 2019

This comment has been minimized.

Centril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril commented Nov 7, 2019

ecstatic-morse commented Nov 7, 2019 • edited Loading

Centril commented Nov 7, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse commented Nov 7, 2019

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril commented Nov 8, 2019

ecstatic-morse commented Nov 13, 2019 • edited Loading

bors commented Nov 13, 2019

bors commented Nov 13, 2019

bors commented Nov 13, 2019

Add a HIR pass to check consts for `if`, `loop`, etc. #66170

Add a HIR pass to check consts for `if`, `loop`, etc. #66170

ecstatic-morse commented Nov 6, 2019 •

edited

Loading

ecstatic-morse commented Nov 7, 2019 •

edited

Loading

ecstatic-morse commented Nov 13, 2019 •

edited

Loading