Tracking issue for #![feature(const_precise_live_drops)]

[ecstatic-morse]

Feature gate for the more precise version of const-checking in #71824.

(Potential) blockers:

• Values that need to be dropped are promoted in const / static initializers #91009

[RalfJung]

Cc @rust-lang/wg-const-eval (which I think never happened for this feature or the PR)

[DoumanAsh]

Is there particular reason why this was moved to be a feature?
It is more or less bug fix to inaccurate drop detection.
So shouldn't it be already stable?
This would make it easier to create builders with generic parameters as we currently cannot mutate self

[oli-obk]

cc @rust-lang/lang I am nominating this feature for stabilization

citing @ecstatic-morse from the impl PR:

This isn't really a "feature" but a refinement to const-checking, and it's kind of non-obvious what the user opts in to with the feature gate, since drop elaboration is an implementation detail. A relevant precedent is #![feature(nll)], but that was much larger and more fundamental than this change would be.

This is also somewhat relevant to the stabilization of #![feature(const_if_match)]. Nothing in this PR is specifically related to branching in constants. For instance, it makes the example below compile as well. However, I expect users will commonly want to move out of Option::<T>::Some within a match, which won't work without this PR if T: !Copy

const _: Vec<i32> = {
    let vec_tuple = (Vec::new(),);
    vec_tuple.0
};

To clarify, this PR makes that code compile because previously we were dropping the vec_tuple which had type Option<(Vec,)>?

This code is functionally no different from the following, which currently works on stable because x is moved into the return place unconditionally.

const X: Option<Vec<i32>> = { let x = Vec::new(); x };

Const-checking only considers whole locals for const qualification, but drop elaboration is more precise: It works on projections as well. Because drop elaboration sees that all fields of the tuple have been moved from by the end of the initializer, no Drop terminator is left in the MIR, despite the fact that the tuple itself is never moved from.

[RalfJung]

@oli-obk how do things look like in implementation complexity and the risk of locking us into a scheme that might be hard to maintain or make compatible with other future extensions?

[RalfJung]

@DoumanAsh due to the halting problem, rustc will always report "incorrect errors". It is impossible to predict at compiletime if e.g. drop will be executed when a piece of code is being run. Similarly, the borrow checker will always reject some correct programs. That's just a fact of programming languages. Neither of these are a bug.

So there's always a trade-off between analysis complexity and "incorrect errors", but even the best analysis will sometimes show "incorrect errors". My impression after looking at the PR here is that the analysis complexity is totally reasonable (it mostly implicitly reuses the analysis performed by drop elaboration), but this is still a decision we should make explicitly, not implicitly.

More precise drop analysis in const fn is a new feature, not a bugfix. (Just like, for example, NLL was a new feature, not a bugfix.)

@oli-obk Thanks. :) As you probably saw, I also left some questions in the PR that implemented the analysis.

[oli-obk]

I did see these comments, just didn't have time to look more deeply yet.

Cross posting from the comments on that PR:

I am guessing the difference between the needs_drop analysis, and drop elaboration's use of MaybeInitializedPlaces and MaybeUninitializedPlaces is the reason that this feature gate exists at all. 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

[RalfJung]

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.

Even worse, it would change drop elaboration, which is a rather tricky part of the compiler.^^ That should be done with utmost care.

[nikomatsakis]

r? @nikomatsakis

[Mark-Simulacrum]

Dropping the nomination here as it's waiting on Niko; that's tracked in the language team action item list.

[inquisitivecrystal]

What remains to be done before this can be stabilized?

[ecstatic-morse]

Hey y'all. I'll try to summarize exactly what this feature gate does to help inform a decision on stabilization. Oli and Ralf probably already know this information, and can skip the next section.

Background

Currently, it is forbidden to run custom Drop implementations during const-eval. This is because Drop::drop is a trait method, and there's no (stable) way to declare that trait methods are const-safe. MIR uses a special terminator (Drop) to indicate that something is dropped. When building the MIR initially, a Drop terminator is inserted for every variable that has drop glue (ty.needs_drop()). Typically, const-checks are run on the newly built MIR. This is to prevent MIR optimizations from eliminating or transforming code that would fail const-checking into code that passes it, which would make compilation success dependent on MIR optimizations. In general, we try very hard to avoid this.

However, some Drop terminators that we generate during MIR building will never actually execute. For example,

• A struct has a field with a custom Drop impl (e.g. Foo(Box<i32>)), but that field is always moved out of before the variable goes out of scope.
• An enum has a variant with a custom Drop impl, but we are in a match arm for a different variant without a custom Drop impl.

We rely on a separate MIR pass, elaborate_drops, to remove these terminators prior to codegen (it also does some extra stuff, like adding dynamic drop flags). This happens pretty early in the optimization pipeline, but it is still an optimization, so it runs after const-checking. As a result, the stable version of const-checking sees some Drop terminators that will never actually run, and raises an error.

Current Implementation

#![feature(const_precise_live_drops)] defers the Drop terminator const-checks until after drop elaboration is run. As I mention above, this is unusual, since it makes compilation success dependent on the result of an optimization. On the other hand,

• Drop elaboration doesn't change very often. Unlike some other optimizations, it is conceptually pretty simple to determine whether a Drop terminator is frivolous along a given code path. It depends solely on whether some set of move paths are initialized at that point in the CFG.
• It is always profitable to remove drop terminators from the MIR when we can. This makes it unlikely that we will ever want to "dial back" drop-elaboration.
• Despite being conceptually straightforward, drop elaboration is not free to compute and having two separate implementations of it (one in const-checking and one in the optimization pipeline) is both inefficient and bad for maintainability.
• If there is bug in drop elaboration that causes us to wrongly eliminate Drops, wrongly accepting some const fns will be the least of our worries, relatively speaking.

For these reasons, I chose the current implementation strategy. I realize that none of these arguments are dispositive, and I don't think it's unreasonable to gate stabilization of this feature on reimplementing the relevant bits of drop elaboration inside const-checking, although I still think it's overly conservative.

Besides that big question, I think there were also some concerns from const-eval members around the use of the NeedsDrop qualif and test coverage (e.g. #71824 (comment)). I'll try to answer those so they can provide a recommendation.

[inquisitivecrystal]

I'm not educated on the details, but it would be super nice to see this stabilized in some form. There are a comparatively large number of new APIs that rely on this. For examples, see issues #76654, #82814, and PR #84087, the last of which is an approved stabilization PR that can't be merged until this is stabilized.

That's why I was checking in on the progress towards stabilization a few days ago. I'm sorry about that, by the way. I know that that sort of message can be annoying, but I wanted to know there if there was anything I could do to help move this along.

[ecstatic-morse]

The fact that the main blocker (and why this was feature gated in the first place) is the implementation makes it somewhat unusual (see #71824 (comment)). That makes it more the domain of the compiler-team rather than the lang-team. Niko reviewed #71824 and is assigned to this issue, but I'm hesitant to ping them specifically unless their expertise is required.

So, if you want to see this stabilized I would figure out some process for getting consent from the compiler team. I think they use the MCP process for this exclusively? Oli is a member, so there's at least one potential sponsor. The team might require documenting the drop-elaboration/const-checking dependency in the dev-guide and maybe the module itself, which I'm happy to do if asked. After that, I can write a stabilization report with some examples and we can do lang-team FCP (assuming any lingering concerns from @rust-lang/wg-const-eval have been addressed).

I'm, uh, not great at navigating bureaucratic systems with many veto points, so if you want to drive this forward your help would be greatly appreciated. However, unless we end up reimplementing drop-elaboration in const-checking like I mention above, I don't think much of the remaining work is technical.

[oli-obk]

I think your summary post contains most of what we need for a stabilization report (full instructions here). We can just mark this issue as requiring sign-off from both T-compiler and T-lang and do all of this at once.

[scottmcm]

I noticed in a PR today that I accidentally changed what this was doing by making what I thought was just an optimization -- same as SimplifyBranches -- but changed the precise drop errors. So I'm glad to see the cancel here, since it made me nervous about where this was happening in the pipe.

[Mark-Simulacrum]

Visiting this for T-lang backlog bonanza. It looks like there's a recent-ish comment #91410 (comment) indicating that #91009 remains a blocker, but we'd like an updated summary to confirm that and ask if there are other issues that are blocking moving ahead here (reference material, perhaps?).

[RalfJung]

I wonder if it is possible to make our existing const drop checks a bit smarter so that at least Option::unwrap can be accepted, without the future-compat hazard of letting dropck do the checking? It would be great to unblock (parts of) #67441.

[clarfonthey]

Is this still blocked? It appears that #91009 has been closed.

The ability to make unwrap work in const fn would help a lot with simplifying a lot of existing code, since it can help replace a lot of manual match statements with unwraps.

[RalfJung]

We still have this problem: #73255 (comment). Basically exactly the thing we were worried about (depending on subtle dropck details) actually came up just after the attempt to stabilize.

[clarfonthey]

Ah! Is there a dedicated issue open for that that can be linked in the description, or is that just a known issue at the moment?

[RalfJung]

It's not a very concrete issue, and I don't think is tracked anywhere explicitly outside of this tracking issue.

[clarfonthey]

That's fair. As is expected with all these const features, something subtle and complicated lurks in the depths that makes it hard to finish up.

I was kinda hopeful that this was mostly done, but alas.

[RalfJung]

It might be, I am honestly not familiar enough with drop elaboration to really evaluate the trade-offs here.

I hope someone else reading along has some good ideas for what can be done before we ask the lang team to discuss this again.

[RalfJung]

One thing I did miss when writing the comments above is #91410: this feature no longer relies on full drop elaboration, just a lightweight version of dead drop removal.

Still, doing this somewhere in the middle of our MIR pipeline does not feel great. Instead maybe it is possible to run the analysis that remove_uninit_drops performs during const-checking so that we can just ask "can this drop ever happen" rather than manifesting the result as explicit MIR? Then this could just happen during regular const checking.

(Borrowck already does something like that, doesn't it? If someone knows a borrowck expert, please point them to this thread, maybe they can help :)

Also see Zulip

[nikomatsakis]

These days I've been thinking that we should reformulate borrow check (and drop check) to take place on THIR. I plan to prototype this in a-mir-formality, which I suppose may have to be renamed to just formality as a result... ;'(. Seems relevant.

…

On Mon, Mar 18, 2024, at 4:44 AM, Ralf Jung wrote: One thing I did miss when writing the comments above is #91410 <#91410>: this feature no longer relies on full drop elaboration, just a lightweight version of dead drop removal. Still, doing this somewhere in the middle of our MIR pipeline does not feel great. Instead maybe it is possible to run the analysis that remove_uninit_drops performs during const-checking so that we can just ask "can this drop ever happen" rather than manifesting the result as explicit MIR? Then this could just happen during regular const checking. (Borrowck already does something like that, doesn't it? If someone knows a borrowck expert, please point them to this thread, maybe they can help :) — Reply to this email directly, view it on GitHub <#73255 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZUKPPUCNG2OQHVJBPDYY2SPXAVCNFSM4N3YYLLKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBQGMZDCMZYGEYQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>