Enable `loop` and `while` in constants behind a feature flag #67216

ecstatic-morse · 2019-12-11T05:50:46Z

This PR is an initial implementation of #52000. It adds a const_loop feature gate, which allows while and loop expressions through both HIR and MIR const-checkers if enabled. for expressions remain forbidden by the HIR const-checker, since they desugar to a call to IntoIterator::into_iter, which will be rejected anyways.

while loops also require #![feature(const_if_match)], since they have a conditional built into them. The diagnostics from the HIR const checker will suggest this to the user.

r? @oli-obk
cc @rust-lang/wg-const-eval

ecstatic-morse · 2019-12-11T05:57:22Z

cc #66946. This needn't be blocked on my changes to the infinite loop detector or vice versa, but having loop will make invoking the detector a lot easier. It's possible we want to add a #![rustc_const_eval_iteration_limit] attribute before this PR lands that could, e.g., be set to -1 to disable the loop detector entirely (see #67217).

src/librustc_passes/check_const.rs

src/librustc_feature/active.rs

src/librustc_passes/check_const.rs

src/test/ui/consts/control-flow/loop.rs

src/test/ui/consts/control-flow/drop-failure.rs

Centril · 2019-12-11T07:55:45Z

cc #66946. This needn't be blocked on my changes to the infinite loop detector or vice versa, but having loop will make invoking the detector a lot easier. It's possible we want to add a #![rustc_const_eval_iteration_limit] attribute before this PR lands that could, e.g., be set to -1 to disable the loop detector entirely (see #67217).

These seem independent to me and I would prefer landing this PR before beta branches so that we can get this into the new bootstrap compiler.

src/test/ui/consts/control-flow/interior-mutability.rs

src/librustc_mir/transform/qualify_min_const_fn.rs

oli-obk · 2019-12-11T20:54:02Z

All lgtm, I'd just like @Centril to sign off on the fact that both control flow and loops can be used on stable const fn (only in libstd) without any additional annotation

src/librustc_feature/active.rs

src/librustc_mir/transform/qualify_min_const_fn.rs

Conditionals and loops now have unstable features, and `feature_err` has its own error code. I think that `feature_err` should take an error code as a parameter, but don't have the energy to make this change throughout the codebase. Also, the error code system may be torn out entirely.

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

oli-obk · 2019-12-13T22:43:19Z

@bors r+

bors · 2019-12-13T22:43:20Z

📌 Commit faa52d1 has been approved by oli-obk

@oli-obk

Enable `loop` and `while` in constants behind a feature flag This PR is an initial implementation of rust-lang#52000. It adds a `const_loop` feature gate, which allows `while` and `loop` expressions through both HIR and MIR const-checkers if enabled. `for` expressions remain forbidden by the HIR const-checker, since they desugar to a call to `IntoIterator::into_iter`, which will be rejected anyways. `while` loops also require [`#![feature(const_if_match)]`](rust-lang#66507), since they have a conditional built into them. The diagnostics from the HIR const checker will suggest this to the user. r? @oli-obk cc @rust-lang/wg-const-eval

@ghost

Rollup of 5 pull requests Successful merges: - #67151 (doc comments: Less attribute mimicking) - #67216 (Enable `loop` and `while` in constants behind a feature flag) - #67255 (Remove i686-unknown-dragonfly target) - #67267 (Fix signature of `__wasilibc_find_relpath`) - #67282 (Fix example code of OpenOptions::open) Failed merges: r? @ghost

bors · 2019-12-15T01:28:34Z

⌛ Testing commit faa52d1 with merge fc6b5d6...

@oli-obk

Enable `loop` and `while` in constants behind a feature flag This PR is an initial implementation of #52000. It adds a `const_loop` feature gate, which allows `while` and `loop` expressions through both HIR and MIR const-checkers if enabled. `for` expressions remain forbidden by the HIR const-checker, since they desugar to a call to `IntoIterator::into_iter`, which will be rejected anyways. `while` loops also require [`#![feature(const_if_match)]`](#66507), since they have a conditional built into them. The diagnostics from the HIR const checker will suggest this to the user. r? @oli-obk cc @rust-lang/wg-const-eval

bors · 2019-12-15T04:46:03Z

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

eddyb · 2020-01-16T23:07:24Z

src/librustc_mir/transform/qualify_min_const_fn.rs

@@ -390,8 +390,12 @@ fn check_terminator(
            cleanup: _,
        } => check_operand(tcx, cond, span, def_id, body),

+        | TerminatorKind::FalseUnwind { .. }
+        if feature_allowed(tcx, def_id, sym::const_loop)
+        => Ok(()),


This seems very fragile. FalseUnwind is supposed to be ignored (unless you care about unwind paths).

What this code should be doing is a CFG cyclicity check.

There is one in check_consts that will run even if #![feature(const_fn)] is specified. I hope we can lose that feature gate in favor of fine-grained ones in the future, as well as merge check_consts and qualify_min_const_fn.

Do both run? That makes me happier, I guess, but then the FalseUnwind check here is not needed at all, is it?

check_consts validation is always run. qualify_min_const_fn is run when #![feature(const_fn)] is not specified. My opinion is that this pass should be removed along with #![feature(const_fn)] and any check that isn't already in check_consts given its own feature gate along with an entry in check_consts/ops.rs. I don't wanna spend political capital to spearhead this, however.

qualify_min_const_fn was added as a hack back when the normal const-checking pass was such a mess that nobody was confident in its ability to rule out all bad code. It was always meant to be temporary.

So, assuming const-check these days is well-structured enough that everyone is confident in its ability to do what it is supposed to do, I don't think you'll need to spend any political capital to get rid of it. Quite the contrary, at least I personally would be delighted to see it go. :)

I would prefer if we could wait with removing qualify_min_const_fn. Not necessarily because the new const checker is messy, but because I don't understand the new code (which is based on data-flow) nearly as well as the old code. And as the lang team point-person for the const things, I would like to understand the thing that checks for stability wrt. const. So I'd appreciate if we don't do it right now and allow me some time to study the new stuff. (Also, is there an urgency to this?)

There's no urgency, there are some preliminary things that can be done like eliminating the const_fn feature gate

You could just make the handling of FalseUnwind consistent, for now.
It's not, and should not be mistaken for, a loop.

As the name says, it's a "false" (or "phantom") unwind edge. It's for situations where analyses which care about unwinding / cleanup blocks need to be conservative and assume unwinding can happen "out of the blue".

Everything else in const-checking ignores unwind edges (e.g. from calls) and associated cleanup blocks, because we completely bypass unwinding at compile-time.

See #62274 for precedent.

Also looks like FalseEdges is also mishandled?

rust-highfive assigned oli-obk Dec 11, 2019

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

Centril reviewed Dec 11, 2019

View reviewed changes

Centril self-assigned this Dec 11, 2019

Centril added F-const_loop labels Dec 11, 2019

This comment has been minimized.

Sign in to view

ecstatic-morse mentioned this pull request Dec 11, 2019

Tracking issue for const_eval_limit: configurable, per-item instruction limit for CTFE #67217

Closed

This comment has been minimized.

Sign in to view

ecstatic-morse force-pushed the const-loop branch from 4e0ea6e to 93fd93c Compare December 11, 2019 19:25

oli-obk reviewed Dec 11, 2019

View reviewed changes

src/test/ui/consts/control-flow/interior-mutability.rs Show resolved Hide resolved

oli-obk reviewed Dec 11, 2019

View reviewed changes

src/librustc_mir/transform/qualify_min_const_fn.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ecstatic-morse force-pushed the const-loop branch from 7b37d44 to 93bdd54 Compare December 11, 2019 22:05

Centril reviewed Dec 12, 2019

View reviewed changes

src/librustc_feature/active.rs Show resolved Hide resolved

src/librustc_mir/transform/qualify_min_const_fn.rs Outdated Show resolved Hide resolved

ecstatic-morse mentioned this pull request Dec 12, 2019

const limit for CTFE #67260

Merged

This comment has been minimized.

Sign in to view

ecstatic-morse added 10 commits December 13, 2019 10:38

Get active features dynamically by their Symbol

8f3021b

Add feature gate for const_loop

57959b2

Put MIR checks for loops behind the feature flag

8f59902

Restructue HIR const-checker to handle features with multiple gates

ee233c0

Extend control flow basics tests with loops

99e132d

Add qualif smoke tests for const loops

1122404

Bless unrelated tests with new help message

caa7c99

Bless modified tests

3325671

Improve message when active feature indexing panics

2add77d

Improve comment

a8e997c

ecstatic-morse and others added 9 commits December 13, 2019 10:39

Fix grammar in test description

b3aecd0

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

Apply suggestions from review

2b5ae1c

Replace Index impl with enabled method

80581be

Use better name for local containing required feature gates

34ce0ba

Ensure test actually uses dataflow, not simulation

598bed6

Use correct nightly version for feature

029725f

Document Features::enabled

0f0bfc9

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

Correctly mark things as min_const_fn

faa52d1

ecstatic-morse force-pushed the const-loop branch from aeb4da4 to faa52d1 Compare December 13, 2019 18:49

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

Centril mentioned this pull request Dec 14, 2019

Rollup of 5 pull requests #67291

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 15, 2019

bors merged commit faa52d1 into rust-lang:master Dec 15, 2019

This was referenced Dec 15, 2019

Initial implementation of #![feature(bindings_after_at)] #66296

Merged

Use a DFS to compare CTFE snapshots #66946

Closed

ecstatic-morse mentioned this pull request Dec 15, 2019

Tracking issue for RFC 2344, "Allow loop in constant evaluation" #52000

Closed

3 tasks

eddyb reviewed Jan 16, 2020

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `loop` and `while` in constants behind a feature flag #67216

Enable `loop` and `while` in constants behind a feature flag #67216

ecstatic-morse commented Dec 11, 2019 •

edited

Loading

ecstatic-morse commented Dec 11, 2019 •

edited

Loading

Centril commented Dec 11, 2019

This comment has been minimized.

This comment has been minimized.

oli-obk commented Dec 11, 2019 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

oli-obk commented Dec 13, 2019

bors commented Dec 13, 2019

bors commented Dec 15, 2019

bors commented Dec 15, 2019

eddyb Jan 16, 2020

ecstatic-morse Jan 17, 2020 •

edited

Loading

eddyb Jan 17, 2020 •

edited

Loading

ecstatic-morse Jan 17, 2020 •

edited

Loading

RalfJung Jan 17, 2020

Centril Jan 17, 2020

oli-obk Jan 17, 2020

eddyb Jan 17, 2020 •

edited

Loading

eddyb Jan 17, 2020

eddyb Jan 17, 2020

Enable loop and while in constants behind a feature flag #67216

Enable loop and while in constants behind a feature flag #67216

Conversation

ecstatic-morse commented Dec 11, 2019 • edited Loading

ecstatic-morse commented Dec 11, 2019 • edited Loading

Centril commented Dec 11, 2019

This comment has been minimized.

This comment has been minimized.

oli-obk commented Dec 11, 2019 • edited Loading

This comment has been minimized.

This comment has been minimized.

oli-obk commented Dec 13, 2019

bors commented Dec 13, 2019

bors commented Dec 15, 2019

bors commented Dec 15, 2019

eddyb Jan 16, 2020

Choose a reason for hiding this comment

ecstatic-morse Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

eddyb Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung Jan 17, 2020

Choose a reason for hiding this comment

Centril Jan 17, 2020

Choose a reason for hiding this comment

oli-obk Jan 17, 2020

Choose a reason for hiding this comment

eddyb Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

eddyb Jan 17, 2020

Choose a reason for hiding this comment

eddyb Jan 17, 2020

Choose a reason for hiding this comment

Enable `loop` and `while` in constants behind a feature flag #67216

Enable `loop` and `while` in constants behind a feature flag #67216

ecstatic-morse commented Dec 11, 2019 •

edited

Loading

ecstatic-morse commented Dec 11, 2019 •

edited

Loading

oli-obk commented Dec 11, 2019 •

edited

Loading

ecstatic-morse Jan 17, 2020 •

edited

Loading

eddyb Jan 17, 2020 •

edited

Loading

ecstatic-morse Jan 17, 2020 •

edited

Loading

eddyb Jan 17, 2020 •

edited

Loading