Reorder mir_const_qualif to make promotion in consts more consistent
#65883
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Computing `mir_const_qualif` requires running `QualifyAndPromoteConstants`, but no promotion was actually done until `mir_validated`. This required persisting a `BitSet` of promoted locals across queries, making it difficult to properly handle promotion in `const`s. This commit swaps the order of the two queries, and stores the result of `mir_const_qualif` (a single `u8`) in the result of `mir_validated`. `mir_const_qualif` now simply invokes `mir_validated` and extracts this result.
The old promotion logic ignores anything after a branch or loop in a `const`, so the new logic will often find new things to promote. Note that we can't actually do promotion in a `const` within a loop body using the current approach of removing `Drop` and `StorageDead`.
rust-highfive
added
the
S-waiting-on-review
Centril
reviewed
Oct 28, 2019
| --> $DIR/issue-36163.rs:1:18 | ||
| | | ||
| LL | const A: isize = Foo::B as isize; | ||
| | ^^^^^^^^^^^^^^^ | ||
| = note: ...which again requires const checking `Foo::B::{{constant}}#0`, completing the cycle | ||
| note: ...which requires computing qualifs for `Foo::B::{{constant}}#0`... |
There was a problem hiding this comment.
Leaking "qualifs" into diagnostics is ungreat. :)
There was a problem hiding this comment.
It would be nice if there was a way to silence unimportant queries–ones that are always invoked by another query–in the diagnostic output for cycles.
|
I think this complication is entirely unnecessary - we can simply move promotion to its own pass, using the new promotion validator, and disconnect the two systems. |
|
Sound good. I'll close this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Computing
mir_const_qualifrequires running most ofQualifyAndPromoteConstants, but no promotion is actually done untilmir_validated. This required persisting aBitSetcontaining the promotable locals across queries and makes it difficult to properly handle promotion inconsts, since the promotion context (Candidate) for each local is gone. This PR swaps the order of the two queries, and stores the result ofmir_const_qualif(a singleu8) in the result ofmir_validated.mir_const_qualifnow simply invokesmir_validatedand extracts this result.I don't think this should do any more work, but I'm new to the query system. At the very least, this saves us from having to keep a
BitSetin the arena for eachconst. It does, however, cause a small diagnostics regression for cyclical dependencies betweenconsts, since there is an extra query in the cycle.After this PR is merged, promotion for every item (non-const
fn,const, etc.) will be done using a list ofCandidates. This makes it easier to fix places where promotion in aconstacts differently than in regularfnsuch as #65732, and will allow us to create promoted MIR fragments for aconstwhich (I think) will be needed to implement #52000.I initially intended for this to resolve issue #65732, but I wasn't able to fix the
nll::type_checkerrors that were caused by convertingMoves of promotable array initializers toCopys within aconst. The code that did this is commented out and aFIXMEwas added to this effect. I think the best solution may be to start creating promoted MIR fragments, but others should weigh in.r? @eddyb