Move #![feature(const_precise_live_drops)] checks earlier in the pipeline
#91410
Conversation
Otherwise dataflow state will propagate along false edges and cause
things to be marked as maybe init unnecessarily. These should be
separate, since `SimplifyBranches` also makes `if true {} else {}` into
a `goto`, which means we wouldn't lint anything in the `else` block.
rust-highfive
added
the
S-waiting-on-review
...since its name is very close to `RemoveUninitDrops`.
It runs before the real drop elaboration pass.
Instead we run `RemoveFalseEdges` and `RemoveUninitDrops` at the appropriate time. The extra `SimplifyCfg` avoids visiting unreachable blocks during `RemoveUninitDrops`.
ecstatic-morse
force-pushed
the
const-precise-live-drops-take-2
branch
from
704a84c
to
58c996c
Compare
This comment has been minimized.
This comment has been minimized.
|
Does this affect the two newly opened issues from the MCP? If so, please add tests. |
| /// | ||
| /// This is redundant with drop elaboration, but we need to do it prior to const-checking, and | ||
| /// running const-checking after drop elaboration makes it opimization dependent, causing issues | ||
| /// like [#90770]. |
There was a problem hiding this comment.
Is it possible to move drop elaboration earlier, so we can still do const-checking after drop elaboration?
Also, isn't this additionally redundant with things done inside borrowck, which also knows to ignore those Drop/DropAndReplace terminators? It doesn't actually remove them, presumably it has an analysis to figure out that they are not needed. Is that the same analysis that is also used here?
There was a problem hiding this comment.
Is it possible to move drop elaboration earlier, so we can still do const-checking after drop elaboration?
How is that better than the status quo? To be clear, I think the status quo is perfectly valid, it just needed a stronger warning about what is allowed as part of post_borrowck_cleanup.
Also, isn't this additionally redundant with things done inside borrowck, which also knows to ignore those Drop/DropAndReplace terminators? It doesn't actually remove them, presumably it has an analysis to figure out that they are not needed. Is that the same analysis that is also used here?
My long term goal (as I stated in this comment) is to move remove_uninit_drops prior to borrow checking and remove the duplicate logic in the borrow checker. I'm a little nervous about this, since I don't fully understand the purpose of the FalseEdge/FalseUnwind terminators, which the borrow checker requires . FalseEdges seem to be used to connect distinct match arms to one another, thus losing information about what the active variant is in a match arm body. That last bit is what makes it possible for Option::unwrap to be const.
I requested some input about this idea from the MIR borrowck implementors as part of that comment. Perhaps you have some thoughts as well? It's trivial to (optionally) ignore FalseEdges while computing MaybeInitializedPlaces, but I don't feel comfortable doing so unilaterally.
There was a problem hiding this comment.
How is that better than the status quo?
My thinking was: if we can do drop elaboration early enough, maybe we don't have to redundantly also run remove_uninit_drops.
I'm a little nervous about this, since I don't fully understand the purpose of the FalseEdge/FalseUnwind terminators, which the borrow checker requires . FalseEdges seem to be used to connect distinct match arms to one another, thus losing information about what the active variant is in a match arm body.
My rough idea for their purpose is that they make the borrow checker accept code that we do not want to accept -- the extra edges make variables 'more live', so that we do not accept code which depends on details such as the exact order in which disjoint match arms are being generated in the MIR. However, I do not have a good understanding of the borrow checker, so I hope you can get better info the the borrowck people -- @nikomatsakis might be able to help or at least say who should know these details. :) @lqd and @matthewjasper might also know more.
|
@oli-obk I added a regression test for #90770. #91009 is not fixed by this PR. Really fixing #91009 will require changes to promotion. |
|
Thanks! Yea, I think I need to have a closer look at promotion here before we stabilize precise drops But that shouldn't block this PR. I already reviewed the logic and only the test was missing, so: @bors r+ |
|
📌 Commit 37fa925 has been approved by |
bors
added
S-waiting-on-bors
apiraino
added
the
T-compiler
…rops-take-2, r=oli-obk Move `#![feature(const_precise_live_drops)]` checks earlier in the pipeline Should mitigate the issues found during MCP on rust-lang#73255. Once this is done, we should clean up the queries a bit, since I think `mir_drops_elaborated_and_const_checked` can be merged back into `mir_promoted`. Fixes rust-lang#90770. cc `@rust-lang/wg-const-eval` r? `@nikomatsakis` (since they reviewed rust-lang#71824)
…askrgr Rollup of 12 pull requests Successful merges: - rust-lang#89954 (Fix legacy_const_generic doc arguments display) - rust-lang#91321 (Handle placeholder regions in NLL type outlive constraints) - rust-lang#91329 (Fix incorrect usage of `EvaluatedToOk` when evaluating `TypeOutlives`) - rust-lang#91364 (Improve error message for incorrect field accesses through raw pointers) - rust-lang#91387 (Clarify and tidy up explanation of E0038) - rust-lang#91410 (Move `#![feature(const_precise_live_drops)]` checks earlier in the pipeline) - rust-lang#91435 (Improve diagnostic for missing half of binary operator in `if` condition) - rust-lang#91444 (disable tests in Miri that take too long) - rust-lang#91457 (Add additional test from rust issue number 91068) - rust-lang#91460 (Document how `last_os_error` should be used) - rust-lang#91464 (Document file path case sensitivity) - rust-lang#91466 (Improve the comments in `Symbol::interner`.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Should mitigate the issues found during MCP on #73255.
Once this is done, we should clean up the queries a bit, since I think
mir_drops_elaborated_and_const_checkedcan be merged back intomir_promoted.Fixes #90770.
cc @rust-lang/wg-const-eval
r? @nikomatsakis (since they reviewed #71824)