Check for live drops in constants after drop elaboration #71824
Conversation
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Here's an interesting case: rust/src/test/ui/check-static-values-constraints.rs Lines 56 to 61 in e5f35df MIR before const-checking After drop elaboration: After drop elaboration, we drop only a single field of |
ecstatic-morse
force-pushed
the
const-check-post-drop-elab
branch
2 times, most recently
from
45d454f
to
bfc4fb8
Compare
This comment has been minimized.
This comment has been minimized.
bors
added
the
S-waiting-on-author
ecstatic-morse
changed the title
ecstatic-morse
force-pushed
the
const-check-post-drop-elab
branch
from
bfc4fb8
to
101d012
Compare
|
Okay, this is ready for review I suppose. I think we may want to put this behind a feature gate or at least wait until the next full beta cycle to merge. It also could use some new tests of the interaction between the value-based analysis on locals in r? @oli-obk |
src/librustc_mir/transform/check_consts/post_drop_elaboration.rs
Outdated
Show resolved
Hide resolved
src/librustc_mir/transform/check_consts/post_drop_elaboration.rs
Outdated
Show resolved
Hide resolved
src/librustc_mir/transform/check_consts/post_drop_elaboration.rs
Outdated
Show resolved
Hide resolved
| @@ -600,7 +581,8 @@ impl Visitor<'tcx> for Validator<'mir, 'tcx> { | |||
| }; | |||
|
|
|||
| if needs_drop { | |||
| self.check_op_spanned(ops::LiveDrop, err_span); | |||
| // self.check_op_spanned(ops::LiveDrop, err_span); | |||
There was a problem hiding this comment.
so... in order to do the feature gating you suggested, all we need is to change this to be run if the feature gate is not active and the better logic you implemented will run, too, but since it won't cause more errors, it basically works as redundancy?
There was a problem hiding this comment.
I think so yes. I'm pretty sure the spans will be the same. Should we still force mir_drops_elaborated_and_const_checked even if the feature gate is not enabled?
There was a problem hiding this comment.
Running it helps ensure it's equivalent to the old logic, where the old logic doesn't error, but may regress perf to do so. I think it'd be nice to run both, but if you see it's annoying in tests, only force it if the feature gate is enabled.
There was a problem hiding this comment.
So I'm second guessing whether we actually want to put these changes behind a feature flag.
The post_drop_elaboration part is pretty trivial; the only thing I'm worried about implementing wrong is the query forcing part, but that's not too hard. We really just need to decide whether it's okay for a const-checking pass to depend on drop elaboration. A feature gate just allows us to delay that discussion; it doesn't help resolve it.
There was a problem hiding this comment.
The reason we should have some feature gate is that this does allow stable const fn to do more afaict (well, at least if we consider if/match stable 😆 ) and we should have a test for that, so please add one in case it's not there already.
There was a problem hiding this comment.
@nikomatsakis, do you want me to nominate this for lang-team or compiler-team discussion? Specifically the part about depending on drop elaboration to determine whether code compiles?
| } | ||
| } | ||
|
|
||
| mir::TerminatorKind::DropAndReplace { .. } => { | ||
| unreachable!("`DropAndReplace` should be removed by drop elaboration") |
There was a problem hiding this comment.
Please use span_bug instead of unreachable where possible
…r=oli-obk Use a single enum for the kind of a const context This adds a `ConstContext` enum to the `rustc_hir` crate and method that can be called via `tcx.hir()` to get the `ConstContext` for a given body owner. This arose from discussion in rust-lang#71824. r? @oli-obk
|
So, I have some reservations about this approach. In borrowck, at least, we do our own computation for which drops are live, precisely so that drop elaboration remains a kind of "optimization" and doesn't define our overall semantics. I am unsure whether that's an important invariant to maintain, but I think it's noteworthy that we are giving it up. I think maybe what I'm missing is some content -- ideally in the rustc-dev-guide -- documenting the phases of the pipeline and which sort of safety checks we do and when. |
|
As I understand the status quo, borrow-checking, the unconditional recursion lint, and const-checking/promotion are the only semantic checks done to the MIR prior to drop elaboration. We force the generator state transform (which doesn't emit any lints or errors) and const propagation (which does, although I think this is considered a bug, see #70923) to run after drop elaboration even when optimizations are disabled. In other words, I think you are right that this PR represents a significant change. One alternative is to reimplement parts of drop elaboration inside the const-checker, obviously this is bad for code duplication, but would put less pressure on those who refactor the drop elaboration pass to preserve existing optimizations. However, it seems wasteful to do all the analysis required to perform drop elaboration for const-checking, then throw it away and re-run it a few optimization passes later. Here's the list of passes that run after borrow/const-checking and promotion: rust/src/librustc_mir/transform/mod.rs Lines 275 to 338 in 75e1463 I don't feel qualified to document the split between "MIR for analysis" and "MIR for optimization". For example, I only found out in this PR that |
joelpalmer
added
S-waiting-on-author
|
@ecstatic-morse So, currently, the borrow checker includes a certain amount of duplicated reasoning as well -- in particular, it tracks which values "may be" initialized (as well as those that "must be") at each point, and we can use that to determine when a |
|
If I were to implement this without relying on drop elaboration, I would need only the I suppose the "live drop" aspect of const-checking just seems more tied to drop elaboration than the borrow checking does. The first two are doing very similar things, whereas drop elaboration is sort of a small subset of what the borrow checker is doing. This is why it felt natural for one to depend on the other. In your opinion, how likely is the potential backwards compatibility hazard of relying on drop elaboration to materialize? My thinking is that it's highly unlikely that we will make drop elaboration less precise without changing the underlying dataflow analysis. Both const-checking and drop elaboration will rely on |
Yeah, that's a good question. It doesn't seem very likely for things to diverge, and I feel like drop-elaboration is kind of an "obvious" optimization that doesn't involve any particular heuristics. It just kind of works "as well as it obviously can" and that's the end of it. I think I'm ok with landing it in this form, but I also would like us to sort of document better the "phase transition" point where we move from "static analysis" to "optimizable, go wild". I've always had "drop elaboration" in my head as that transition point, but it seems like after this PR lands, the point has moved somewhat forward, to after this check. Is there a clear query transition? I guess I can skim the diff. I think I'd be happiest if we documented it in the rustc-dev-guide ultimately. |
|
MIR passes are currently divided into "post-borrowck cleanup" and "optimization passes". This runs immediately after "post-borrowck cleanup". |
|
☔ The latest upstream changes (presumably #72778) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@rust-lang/lang, do we want to put these changes behind a feature gate? @oli-obk thinks we should. My opinion is that this isn't really a "feature" but a refinement to const-checking, and it's kind of non-obvious what the user would be opting in to with the feature gate, since drop elaboration is an implementation detail. A relevant precedent is This is also somewhat relevant to the stabilization of const _: Vec<i32> = {
let vec_tuple = (Vec::new(),);
vec_tuple.0
}; |
ecstatic-morse
force-pushed
the
const-check-post-drop-elab
branch
from
7fad28d
to
2dcf7db
Compare
|
@bors r=oli-obk rollup |
|
📌 Commit 2dcf7db has been approved by |
bors
added
S-waiting-on-bors
…p-elab, r=oli-obk Check for live drops in constants after drop elaboration Resolves rust-lang#66753. This PR splits the MIR "optimization" pass series in two and introduces a query–`mir_drops_elaborated_and_const_checked`–that holds the result of the `post_borrowck_cleanup` analyses and checks for live drops. This query is invoked in `rustc_interface` for all items requiring const-checking, which means we now do `post_borrowck_cleanup` for items even if they are unused in the crate. As a result, we are now more precise about when drops are live. This is because drop elaboration can e.g. eliminate drops of a local when all its fields are moved from. This does not mean we are doing value-based analysis on move paths, however; Storing a `Some(CustomDropImpl)` into a field of a local will still set the qualifs for that entire local. r? @oli-obk
…p-elab, r=oli-obk Check for live drops in constants after drop elaboration Resolves rust-lang#66753. This PR splits the MIR "optimization" pass series in two and introduces a query–`mir_drops_elaborated_and_const_checked`–that holds the result of the `post_borrowck_cleanup` analyses and checks for live drops. This query is invoked in `rustc_interface` for all items requiring const-checking, which means we now do `post_borrowck_cleanup` for items even if they are unused in the crate. As a result, we are now more precise about when drops are live. This is because drop elaboration can e.g. eliminate drops of a local when all its fields are moved from. This does not mean we are doing value-based analysis on move paths, however; Storing a `Some(CustomDropImpl)` into a field of a local will still set the qualifs for that entire local. r? @oli-obk
…p-elab, r=oli-obk Check for live drops in constants after drop elaboration Resolves rust-lang#66753. This PR splits the MIR "optimization" pass series in two and introduces a query–`mir_drops_elaborated_and_const_checked`–that holds the result of the `post_borrowck_cleanup` analyses and checks for live drops. This query is invoked in `rustc_interface` for all items requiring const-checking, which means we now do `post_borrowck_cleanup` for items even if they are unused in the crate. As a result, we are now more precise about when drops are live. This is because drop elaboration can e.g. eliminate drops of a local when all its fields are moved from. This does not mean we are doing value-based analysis on move paths, however; Storing a `Some(CustomDropImpl)` into a field of a local will still set the qualifs for that entire local. r? @oli-obk
Rollup of 10 pull requests Successful merges: - rust-lang#71824 (Check for live drops in constants after drop elaboration) - rust-lang#72389 (Explain move errors that occur due to method calls involving `self`) - rust-lang#72556 (Fix trait alias inherent impl resolution) - rust-lang#72584 (Stabilize vec::Drain::as_slice) - rust-lang#72598 (Display information about captured variable in `FnMut` error) - rust-lang#73336 (Group `Pattern::strip_*` method together) - rust-lang#73341 (_match.rs: fix module doc comment) - rust-lang#73342 (Fix iterator copied() documentation example code) - rust-lang#73351 (Update E0446.md) - rust-lang#73353 (structural_match: non-structural-match ty closures) Failed merges: r? @ghost
| mir::TerminatorKind::Drop { location: dropped_place, .. } => { | ||
| let dropped_ty = dropped_place.ty(self.body, self.tcx).ty; | ||
| if !NeedsDrop::in_any_value_of_ty(self.ccx, dropped_ty) { | ||
| return; |
There was a problem hiding this comment.
@oli-obk Is this even reachable? I'd think we don't keep Drop terminators around after drop elaboration for types that do not need dropping...
There was a problem hiding this comment.
This is indeed not reachable.
| return; | ||
| } | ||
|
|
||
| if self.qualifs.needs_drop(self.ccx, dropped_place.local, location) { |
There was a problem hiding this comment.
Same here, this looks like it second-guessing drop elaboration which already determined that this drop is indeed necessary.
There was a problem hiding this comment.
This one is reachable. I'm not sure yet why, but I am guessing the reason is that needs_drop is its own analysis, while drop elaboration relies on MaybeInitializedPlaces and MaybeUninitializedPlaces to make a decision. We could probably rewrite drop elaboration in terms of the NeedsDrop qualif, which would (afaict) allow post_drop_elaboration to not do any checks except for looking for Drop terminators.
Of course such a change would insta-stabilize the feature gate from this PR without any reasonable way to avoid said stabilization. So I'm tempted to stabilize the feature but leave a FIXME on this function to merge its qualif checks into elaborate_drops
There was a problem hiding this comment.
The alternative would be to ignore qualifs here, and just follow what drop elaboration does.
There was a problem hiding this comment.
If we do keep following the qualifs to get an edge in precision over drop elaboration (maybe we have to, for backwards compat), we should add a test case for a program where the qualifs are more precise than drop elaboration is (i.e. a program that would get rejected if we just always called check_live_drop), and reference that testcase from the code here.
There was a problem hiding this comment.
rust/src/test/ui/consts/drop_none.rs
Lines 3 to 10 in a985d8e
Is this what you're looking for? We do have to keep this for back-compat, see #57734
There was a problem hiding this comment.
Sorry, I forgot to post that here: the follow-up to this discussion is at #83351.
There was a problem hiding this comment.
Ah, you already found an equivalent one.
|
|
||
| // `x` is *not* always moved into the final value may be dropped inside the initializer. | ||
| // `x` is *not* always moved into the final value and may be dropped inside the initializer. | ||
| const _: Option<Vec<i32>> = { | ||
| let y: Option<Vec<i32>> = None; |
There was a problem hiding this comment.
If we unconditionally error on drops, this line will cause an error (meaning: this is the test case you're looking for)
There was a problem hiding this comment.
If I interpret the error annotations correctly, this errors (in the next line) even with precise analysis? Doesn't seem terribly useful then to not error here.^^
Isn't there an example where we need this to actually accept more code?
There was a problem hiding this comment.
I think I'll split up the feature gate into two, so we can actually play with examples on the playground.
There was a problem hiding this comment.
I don't think we want this exposed to the user; doesn't just commenting out those lines make some tests fail (in more ways than just moving around the location of the error message)?
There was a problem hiding this comment.
I found examples in the test suite: #83351
…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)
…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)
Resolves #66753.
This PR splits the MIR "optimization" pass series in two and introduces a query–
mir_drops_elaborated_and_const_checked–that holds the result of thepost_borrowck_cleanupanalyses and checks for live drops. This query is invoked inrustc_interfacefor all items requiring const-checking, which means we now dopost_borrowck_cleanupfor items even if they are unused in the crate.As a result, we are now more precise about when drops are live. This is because drop elaboration can e.g. eliminate drops of a local when all its fields are moved from. This does not mean we are doing value-based analysis on move paths, however; Storing a
Some(CustomDropImpl)into a field of a local will still set the qualifs for that entire local.r? @oli-obk