Make dataflow-based const qualification the canonical one #66385

ecstatic-morse · 2019-11-13T21:01:01Z

For over a month, dataflow-based const qualification has been running in parallel with qualify_consts to check the bodies of const and statics. This PR removes the old qualification pass completely in favor of the dataflow-based one.

edit:
This PR also stops checking QUALIF_ERROR_BIT during promotion. This check appears to no longer serve a purpose now that the CTFE engine is more robust.

As a side-effect, this resolves #66167.

r? @eddyb

eddyb · 2019-11-13T23:57:01Z

src/librustc_mir/transform/check_consts/qualifs.rs


 impl QualifSet {
-    fn contains<Q: ?Sized + Qualif>(self) -> bool {
+    pub const UNPROMOTABLE: Self = QualifSet(1 << Unpromotable::IDX);


Can we stop using this hack, btw?

There's only two places it can appear now. One is where you left a comment below, which doesn't seem to break anything. The other is when a const panics unconditionally, which is just a hack to approximate the existing behavior. I'll try removing it completely.

I've removed UNPROMOTABLE. Let's see what happens with CI.

We pass all the tests, and I've tried the following example locally and no ICE occurs, just an error pointing to X saying that const evaluation failed.

#![feature(const_panic)] #![allow(const_err)] const X: u32 = panic!(); fn main() { let x: &'static _ = &X; }

eddyb · 2019-11-14T00:05:42Z

src/librustc_mir/transform/mod.rs

+
+    if body.return_ty().references_error() {
+        tcx.sess.delay_span_bug(body.span, "mir_const_qualif: MIR had errors");
+        return check_consts::QualifSet::UNPROMOTABLE.into();


Does anything break if you use QualifSet::default() here?

Apparently not 😃

Centril · 2019-11-14T06:05:15Z

src/librustc_mir/transform/mod.rs

@@ -185,6 +185,41 @@ pub fn run_passes(
    body.phase = mir_phase;
 }

+fn mir_const_qualif(tcx: TyCtxt<'_>, def_id: DefId) -> u8 {


A doc comment would be good especially for the return type.

I think we can replace it now with:

struct ConstQualif { has_mut_interior: bool, needs_drop: bool, }

This change was made in the latest version.

Centril · 2019-11-14T06:13:26Z

src/librustc_mir/transform/check_consts/validation.rs

+        // These blocks often drop locals that would otherwise be returned from the function.
+        //
+        // FIXME: This shouldn't be unsound since a panic at compile time will cause a compiler
+        // error anyway, but maybe we should do more here?


"If you have to ask..."? ;)

@eddyb I was hoping someone could authoritatively tell me we don't need to do anything for cleanup blocks. I'm pretty sure we don't.

Centril · 2019-11-14T06:17:42Z

src/test/ui/consts/miri_unleashed/enum_discriminants.rs

@@ -1,6 +1,9 @@
-// compile-flags: -Zunleash-the-miri-inside-of-you
+// compile-flags: -Zunleash-the-miri-inside-of-you -Awarnings


I would prefer not to do this so that we can see changes in diffs and whatnot.

The "skipping const checks" annotations were only added last week since they are now mandatory in all UI tests (see #66213). They are unrelated to the actual purpose of the test; they just add noise (to both the test itself and PRs that work on const qualification). Ignoring warnings was explicitly mentioned as an opt-out path for tests like this.

Interestingly, #![allow(warnings)] did not work for this purpose. Not sure why?

bors · 2019-11-14T08:05:45Z

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

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

ecstatic-morse · 2019-11-14T17:29:09Z

Rebased to fix merge conflicts. The latest version moves QualifSet into rustc::mir and uses it everywhere.

eddyb · 2019-11-14T18:47:06Z

src/librustc/mir/mod.rs

@@ -2769,6 +2769,13 @@ pub struct BorrowCheckResult<'tcx> {
    pub used_mut_upvars: SmallVec<[Field; 8]>,
 }

+/// The result of the `mir_const_qualif` query.
+#[derive(Clone, Copy, Debug, Default, RustcEncodable, RustcDecodable, HashStable)]
+pub struct QualifSet {


I don't think "set" is relevant anymore, the bitset was really just an optimization.
ConstQualifs might be better. Or ValueQualifs?

I would lean towards ConstQualifs since IsNotPromotable is no longer, but either is fine.

eddyb · 2019-11-14T18:48:06Z

src/librustc/mir/mod.rs

+#[derive(Clone, Copy, Debug, Default, RustcEncodable, RustcDecodable, HashStable)]
+pub struct QualifSet {
+    pub has_mut_interior: bool,
+    pub needs_drop: bool,


I wonder if these two fields should have similar doc comments to the HasMutInterior and NeedsDrop types in rustc_mir::transform::check_consts.

Would a link to the Qualif implementers suffice? I don't wanna copy-paste lest they get out of sync.

Oh I wasn't thinking copy-pasting, maybe a few words and a link (or even just the rustc_mir::...::HasMutInterior etc. path).

I added a note on ConstQualifs that points the user to librustc_mir/transform/check_consts/qualifs.rs. Lemme know if you want something more here.

eddyb · 2019-11-14T18:50:52Z

src/librustc/query/mod.rs

@@ -93,7 +93,7 @@ rustc_queries! {
        /// Maps DefId's that have an associated `mir::Body` to the result
        /// of the MIR qualify_consts pass. The actual meaning of
        /// the value isn't known except to the pass itself.


This doc comment should be updated.

eddyb · 2019-11-14T19:51:50Z

src/librustc_metadata/rmeta/mod.rs

@@ -295,7 +295,7 @@ enum EntryKind<'tcx> {
 /// Additional data for EntryKind::Const and EntryKind::AssocConst
 #[derive(Clone, Copy, RustcEncodable, RustcDecodable)]
 struct ConstQualif {


I don't think there's a point in having this newtype.

bors · 2019-11-15T13:00:50Z

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

Unlike the original pass, we check *every* non-cleanup basic block instead of stopping at `SwitchInt`. We use the `is_cfg_cyclic` function to check for loops unlike the original checker which could not differentiate between true cycles and basic blocks with more than two predecessors. The last three functions are all copied verbatim from `qualify_consts`.

Now `mir_const_qualif` must be called for `static`s and `const fn`s as well as `const`s since it is responsible for const-checking. We return the qualifs in the return place for everything, even though they will only be used for `const`s.

I believe this occurs because the old checker stopped processing basic blocks after a `SwitchInt`.

We have a proper type for these now, so the wrapper is no longer necessary.

ecstatic-morse · 2019-11-15T18:38:08Z

@eddyb I believe I've resolved all your posted concerns. Let me know what else needs to be done.

eddyb · 2019-11-15T20:06:47Z

@bors r+

bors · 2019-11-15T20:06:48Z

📌 Commit a1135cc has been approved by eddyb

Centril · 2019-11-16T05:47:50Z

@bors rollup=never

bors · 2019-11-17T18:38:19Z

⌛ Testing commit a1135cc with merge 0f0c640...

@eddyb

Make dataflow-based const qualification the canonical one For over a month, dataflow-based const qualification has been running in parallel with `qualify_consts` to check the bodies of `const` and `static`s. This PR removes the old qualification pass completely in favor of the dataflow-based one. **edit:** This PR also stops checking `QUALIF_ERROR_BIT` during promotion. This check appears to no longer serve a purpose now that the CTFE engine is more robust. As a side-effect, this resolves #66167. r? @eddyb

bors · 2019-11-17T21:47:31Z

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 0f0c640 to master...

rust-highfive · 2019-11-17T21:48:44Z

📣 Toolstate changed by #66385!

Tested on commit 0f0c640.
Direct link to PR: #66385

💔 rustc-guide on linux: test-pass → test-fail (cc @amanjeev @spastorino @mark-i-m, @rust-lang/infra).

@amanjeev

Tested on commit rust-lang/rust@0f0c640. Direct link to PR: <rust-lang/rust#66385> 💔 rustc-guide on linux: test-pass → test-fail (cc @amanjeev @spastorino @mark-i-m, @rust-lang/infra).

@oli-obk

Enable `if` and `match` in constants behind a feature flag This PR is an initial implementation of #49146. It introduces a `const_if_match` feature flag and does the following if it is enabled: - Allows `Downcast` projections, `SwitchInt` terminators and `FakeRead`s for matched places through the MIR const-checker. - Allows `if` and `match` expressions through the HIR const-checker. - Stops converting `&&` to `&` and `||` to `|` in `const` and `static` items. As a result, the following operations are now allowed in a const context behind the feature flag: - `if` and `match` - short circuiting logic operators (`&&` and `||`) - the `assert` and `debug_assert` macros (if the `const_panic` feature flag is also enabled) However, the following operations remain forbidden: - `while`, `loop` and `for` (see #52000) - the `?` operator (calls `From::from` on its error variant) - the `assert_eq` and `assert_ne` macros, along with their `debug` variants (calls `fmt::Debug`) This PR is possible now that we use dataflow for const qualification (see #64470 and #66385). r? @oli-obk cc @rust-lang/wg-const-eval @eddyb

rust-highfive assigned eddyb Nov 13, 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 13, 2019

This comment has been minimized.

Sign in to view

ecstatic-morse mentioned this pull request Nov 13, 2019

Tracking issue for RFC 2342, "Allow if and match in constants" #49146

Closed

4 tasks

This comment has been minimized.

Sign in to view

ecstatic-morse force-pushed the check-only-pass2 branch from 8e40d60 to ebebc65 Compare November 13, 2019 23:25

ecstatic-morse marked this pull request as ready for review November 13, 2019 23:25

ecstatic-morse force-pushed the check-only-pass2 branch from ebebc65 to 236b96d Compare November 13, 2019 23:26

eddyb reviewed Nov 13, 2019

View reviewed changes

eddyb reviewed Nov 14, 2019

View reviewed changes

ecstatic-morse force-pushed the check-only-pass2 branch from 236b96d to 059856e Compare November 14, 2019 01:12

Centril reviewed Nov 14, 2019

View reviewed changes

matthewjasper reviewed Nov 14, 2019

View reviewed changes

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

ecstatic-morse force-pushed the check-only-pass2 branch from 059856e to 910ae81 Compare November 14, 2019 17:27

eddyb reviewed Nov 14, 2019

View reviewed changes

ecstatic-morse added 8 commits November 15, 2019 10:33

Remove QUALIF_ERROR_BIT checking

1924cc8

Remove requires_sync

dbae15a

Allow Validator to get qualifs for the return place

4881104

Use new const-checker for mir_const_qualif

55da0c0

Now `mir_const_qualif` must be called for `static`s and `const fn`s as well as `const`s since it is responsible for const-checking. We return the qualifs in the return place for everything, even though they will only be used for `const`s.

Remove qualify_consts completely

6d4e204

Move delay_span_bug into emit_error for if/loop

7527766

Remove remaining validator mismatch code

3d2bc50

ecstatic-morse added 8 commits November 15, 2019 10:33

Silence miri unleashed warnings in test

8f5494b

Add explanation of test for validation mismatch

d1f0ef5

Remove -Z flag for suppressing validation mismatch ICE

9a2e53a

A Downcast is now reached when const-checking a for loop

a4ce201

I believe this occurs because the old checker stopped processing basic blocks after a `SwitchInt`.

Use a bespoke type for the result of mir_const_qualif

056edc0

Use mir::QualifSet to store qualifs

1e840f8

Fix nits from review

ea95e94

Remove newtype for qualifs in rustc_metadata

a1135cc

We have a proper type for these now, so the wrapper is no longer necessary.

ecstatic-morse force-pushed the check-only-pass2 branch from 93eac78 to a1135cc Compare November 15, 2019 18:34

eddyb approved these changes Nov 15, 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 15, 2019

ecstatic-morse mentioned this pull request Nov 16, 2019

promotion tweaks rust-lang/const-eval#31

Merged

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

bors merged commit a1135cc into rust-lang:master Nov 17, 2019

bors mentioned this pull request Nov 17, 2019

Organize intrinsics promotion checks #66275

Merged

ecstatic-morse mentioned this pull request Nov 18, 2019

Enable if and match in constants behind a feature flag #66507

Merged

ecstatic-morse added the F-const_if_match label Nov 20, 2019

ecstatic-morse mentioned this pull request Jun 1, 2020

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

Closed

3 tasks

ecstatic-morse deleted the check-only-pass2 branch October 6, 2020 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make dataflow-based const qualification the canonical one #66385

Make dataflow-based const qualification the canonical one #66385

ecstatic-morse commented Nov 13, 2019 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb Nov 13, 2019

ecstatic-morse Nov 14, 2019

ecstatic-morse Nov 14, 2019

ecstatic-morse Nov 14, 2019

eddyb Nov 14, 2019

ecstatic-morse Nov 14, 2019

Centril Nov 14, 2019

eddyb Nov 14, 2019

ecstatic-morse Nov 14, 2019

Centril Nov 14, 2019

ecstatic-morse Nov 15, 2019

Centril Nov 14, 2019

ecstatic-morse Nov 14, 2019 •

edited

Loading

bors commented Nov 14, 2019

ecstatic-morse commented Nov 14, 2019

eddyb Nov 14, 2019

ecstatic-morse Nov 14, 2019

eddyb Nov 14, 2019

ecstatic-morse Nov 14, 2019

eddyb Nov 14, 2019

ecstatic-morse Nov 14, 2019

eddyb Nov 14, 2019

eddyb Nov 14, 2019

bors commented Nov 15, 2019

ecstatic-morse commented Nov 15, 2019

eddyb commented Nov 15, 2019

bors commented Nov 15, 2019

Centril commented Nov 16, 2019

bors commented Nov 17, 2019

bors commented Nov 17, 2019

rust-highfive commented Nov 17, 2019

		@@ -1,6 +1,9 @@
		// compile-flags: -Zunleash-the-miri-inside-of-you
		// compile-flags: -Zunleash-the-miri-inside-of-you -Awarnings

Make dataflow-based const qualification the canonical one #66385

Make dataflow-based const qualification the canonical one #66385

Conversation

ecstatic-morse commented Nov 13, 2019 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

bors commented Nov 14, 2019

ecstatic-morse commented Nov 14, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 15, 2019

ecstatic-morse commented Nov 15, 2019

eddyb commented Nov 15, 2019

bors commented Nov 15, 2019

Centril commented Nov 16, 2019

bors commented Nov 17, 2019

bors commented Nov 17, 2019

rust-highfive commented Nov 17, 2019

ecstatic-morse commented Nov 13, 2019 •

edited

Loading

ecstatic-morse Nov 14, 2019 •

edited

Loading