Make dataflow-based const qualification the canonical one #66385
Conversation
This comment has been minimized.
This comment has been minimized.
rust-highfive added the S-waiting-on-review 
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ecstatic-morse force-pushed the check-only-pass2 branch from 8e40d60 to ebebc65 Compare ecstatic-morse force-pushed the check-only-pass2 branch from ebebc65 to 236b96d Compare 
There was a problem hiding this comment.
Can we stop using this hack, btw?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've removed UNPROMOTABLE. Let's see what happens with CI.
There was a problem hiding this comment.
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; } src/librustc_mir/transform/mod.rs Outdated
There was a problem hiding this comment.
Does anything break if you use QualifSet::default() here?
There was a problem hiding this comment.
Apparently not 😃
ecstatic-morse force-pushed the check-only-pass2 branch from 236b96d to 059856e Compare 
src/librustc_mir/transform/mod.rs Outdated
There was a problem hiding this comment.
A doc comment would be good especially for the return type.
There was a problem hiding this comment.
I think we can replace it now with:
struct ConstQualif { has_mut_interior: bool, needs_drop: bool, }There was a problem hiding this comment.
This change was made in the latest version.
There was a problem hiding this comment.
"If you have to ask..."? ;)
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I would prefer not to do this so that we can see changes in diffs and whatnot.
There was a problem hiding this comment.
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?
| ☔ The latest upstream changes (presumably #66233) made this pull request unmergeable. Please resolve the merge conflicts. |
ecstatic-morse force-pushed the check-only-pass2 branch from 059856e to 910ae81 Compare | Rebased to fix merge conflicts. The latest version moves |
src/librustc/mir/mod.rs Outdated
There was a problem hiding this comment.
I don't think "set" is relevant anymore, the bitset was really just an optimization.
ConstQualifs might be better. Or ValueQualifs?
There was a problem hiding this comment.
I would lean towards ConstQualifs since IsNotPromotable is no longer, but either is fine.
src/librustc/mir/mod.rs Outdated
There was a problem hiding this comment.
I wonder if these two fields should have similar doc comments to the HasMutInterior and NeedsDrop types in rustc_mir::transform::check_consts.
There was a problem hiding this comment.
Would a link to the Qualif implementers suffice? I don't wanna copy-paste lest they get out of sync.
There was a problem hiding this comment.
Oh I wasn't thinking copy-pasting, maybe a few words and a link (or even just the rustc_mir::...::HasMutInterior etc. path).
There was a problem hiding this comment.
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.
src/librustc/query/mod.rs Outdated
There was a problem hiding this comment.
This doc comment should be updated.
src/librustc_metadata/rmeta/mod.rs Outdated
There was a problem hiding this comment.
I don't think there's a point in having this newtype.
| ☔ 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 force-pushed the check-only-pass2 branch from 93eac78 to a1135cc Compare | @eddyb I believe I've resolved all your posted concerns. Let me know what else needs to be done. |
| @bors r+ |
| 📌 Commit a1135cc has been approved by |
bors added S-waiting-on-bors 
| @bors rollup=never |
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
| ☀️ Test successful - checks-azure |
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).
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
6 participants
For over a month, dataflow-based const qualification has been running in parallel with
qualify_conststo check the bodies ofconstandstatics. This PR removes the old qualification pass completely in favor of the dataflow-based one.edit:
This PR also stops checking
QUALIF_ERROR_BITduring 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