Improve live drop precision

[nnethercote]

I encountered a bug in FlowSensitiveAnalysis and it led in an interesting direction.

[nnethercote]

LLM disclosure: an LLM found the initial bug and answered some questions I had about this code. I made all the code and text changes myself.

[nnethercote]

@oli-obk, @RalfJung, @nikomatsakis: this is relevant to the const_precise_live_drops feature (#73255). This stuff is well outside my usual lane and I don't really know what I'm doing. I just tried to fix an obvious operator bug and didn't expect it would intersect with an unstable feature. I'm happy to hear any opinions about whether merging this is feasible or not.

[RalfJung]

This is not changing precise const drop analysis nor FlowSensitiveAnalysis, it seems to only change the non-precise regular const check's drop part? I have no idea how to relate the PR description or the question with the diff.
I also don't know this part of the const-checking code at all and have no experience with our dataflow framework so I can't say much about that part of the diff.

this is relevant to the const_precise_live_drops feature

I don't think so. Note that we currently have two const-drop checks (which ensure that we don't run destructors in const code): a conservative one that we use by default, and a more precise one that is enabled by const_precise_live_drops. Your PR only changes the conservative one I think.

From the test change it seems like this PR makes the non-precise (stable, default) const drop check a bit more precise. That particular example seems fine to accept, looking at #65394 this isn't even really about precise const drops but about "legacy and dataflow-based const validators". I don't know what that is but I suspect it is a long-finished refactor of the const checker. The test only later incorporated const_precise_live_drops, with a comment about the stable behavior being surprising. So past me apparently also thought we should accept this code.

So overall -- I think this is not relevant const_precise_live_drops, it is relevant to the stable const drop checker. I don't know how it changes that checker; the one test diff looks fine but this is a change observable on stable so this PR is a one-way change. Would be nice to get a better understanding of the new code accepted under this PR.

[nnethercote]

This is not changing precise const drop analysis nor FlowSensitiveAnalysis

The bug fix is in the State type which is the domain used by FlowSensitiveAnalysis. And then the borrow field is removed from State. So it is changing FlowSensitiveAnalysis.

And you are right that this PR doesn't change precise const drop analysis, but it does change the conservative const drop analysis to be more precise, so there is some overlap/relevance, and could be viewed as a partial stabilization of const_precise_live_drops even though it's not using the precise drop-checking mechanism (though it sounds like you don't think so).

More generally, I'm trying to get a sense of whether this is interesting. There's a clear bug that is causing the compiler to reject some valid programs, which seems interesting to me, but as I said I'm not an expert on this stuff. If it's not interesting, I could just close this and open a separate PR that puts an explanatory comment on the short-circuiting || -- "yes, this should be | , but it's like this for historical reasons...".

[RalfJung]

The reason const_precise_live_drops is unstable is that it runs after drop elaboration, which means that changes to drop elaboration can affect whether a program passes the const_precise_live_drops check. That was deemed too risky, we want to be able to evolve drop elaboration without causing breaking changes (indeed such a drop elaboration change happened exactly when const_precise_live_drops when up for stabilization once). Ideally const_precise_live_drops would use the same "view" of what does and does not need drop as the borrow checker, but nobody figured out how to do that yet.

The conservative const drop checker is only conservative to avoid depending on things that we might want to change. It should accept all code that we are reasonably confident we can keep accepting even if we refactor compiler internals. So I think what you found was just a genuine bug, not a deliberately conservative approximation.

But let's hear from the rest of @rust-lang/wg-const-eval. (Oli is on vacation currently.)

[nnethercote]

@RalfJung @oli-obk @lcnr @fee1-dead: Any interest in this PR? In summary, fixing a longstanding but obvious bug in FlowSensitiveAnalysis would improve the precision of live drop analysis at basically zero cost. Details in the commit messages.

[RalfJung]

Personally, yes, I am interested. But to actually evaluate the change (and certainly for the t-lang nomination this will need), I'd need a description of the change and which new code this allows -- at the source code level, not in terms of our dataflow framework. I can't reverse engineer that from the diff. Maybe @oli-obk can. The main evaluation criterion would be "does this expose any compiler implementation details that we may want to change" (which the precise const live drop check does and that's why it is unstable).

[nnethercote]

Here is some sample code that tries to answer some of Ralf's questions.

#![allow(unused)]

// Old and new behaviour: accepted, as intended; x is fully moved, so it doesn't need a drop.
const _: Vec<u32> = {
    let mut x = vec![];
    let y = x;
    y
};

// Old behaviour: error, as intended by the conservative check in
// `TransferFunction::visit_operand` that preserves the old `MaybeMutBorrowedLocals`
// behaviour; the borrow means x is still considered as needing a drop.
//
// New behaviour: ok, due to the loosening of the old `MaybeMutBorrowedLocals` behaviour.
const _: Vec<u32> = {
    let mut x = vec![];
    let r = &mut x;
    let y = x;
    y
};

// Old behaviour: accepted. The basic block boundary between the borrow and the move triggers the
// short-circuiting bug which causes us to lose information, effectively preventing the borrow from
// being observed. This is bad; the analysis is disrupted by basic block boundaries.
//
// New behaviour: ok, due to the loosening of the old `MaybeMutBorrowedLocals` behaviour.
const _: Vec<u32> = {
    let mut x = vec![];
    let r = &mut x;
    let n = if 1 + 1 == 2 { 1 } else { 0 }; // makes the difference
    let y = x;
    y
};

fn main() {}

"does this expose any compiler implementation details that we may want to change"

I don't believe so. In fact, the existing bug exposes some implementation details (basic block boundaries) and fixing the bug hides those details.

[nnethercote]

I have also updated the commit message on the first commit to give a better description of what's going on.

[RalfJung]

Old behaviour: error, as intended by the conservative check

FWIW I have no idea whether that was intended. We never explicitly discussed being conservative here. I think it may just be an accident / leftover from an even older implementation.

[RalfJung]

This looks kind of suspicious. The comment is correct that mutable borrows might change the qualif. We use qualif also e.g. for interior mutability, where None is fine but Some(Cell::new(...)) is not, and the code could use a mutable borrow to swap from one to the other.

Why is it correct to entirely stop tracking borrows here?
@oli-obk could be great to get your take as well since I think you know this code better than I do. :)

View changes since the review

[RalfJung]

Looks like we barely used borrow before... it was only ever checked if IS_CLEARED_ON_MOVE is true, which is only the case for the drop qualifs. I guess we must have some other check to adjust qualifs when a borrow is taken.

[oli-obk]

There is a difference between

1. behave differently during some access of a local if there is a borrow, and
2. collect qualifs through borrows.

Qualif collection eagerly walks through a borrow to its local and collects everything and acts upon it. The thing that was changed here is that const qualif stops doing 1.

[RalfJung]

Suggested change

	// If a local with no projections is moved from (e.g. `x` in `y = x`), record that
	// it no longer has the qualif (as requested by `IS_CLEARED_ON_MOVE`).

View changes since the review

[tmiasko]

With proposed semantics from move elimination RFC, move deallocates the local, and any borrows become invalid. In contrast, this is far from obvious why it would be correct now. Consider the following example running under tree borrows:

struct PanicOnDrop;
impl Drop for PanicOnDrop { fn drop(&mut self) { panic!(); } }
pub const fn main() {
    let mut a = None;
    let p = &raw mut a;
    std::mem::forget(a);
    a = None;
    unsafe { std::ptr::write(p, Some(PanicOnDrop)) };
    a = a;
}
const _: () = main();

$ rustc +stage1 a.rs
error[E0080]: calling non-const function `<PanicOnDrop as Drop>::drop`
  --> a.rs:11:15
   |
11 | const _: () = main();
   |               ^^^^^^ evaluation of `_` failed inside this call
   |
note: inside `main`
  --> a.rs:10:1
   |
10 | }
   | ^
note: inside `std::ptr::drop_glue::<Option<PanicOnDrop>> - shim(Some(Option<PanicOnDrop>))`
  --> library/core/src/ptr/mod.rs:848:0
note: inside `std::ptr::drop_glue::<PanicOnDrop> - shim(Some(PanicOnDrop))`
  --> library/core/src/ptr/mod.rs:848:0

error: aborting due to 1 previous error; 1 warning emitted

For more information about this error, try `rustc --explain E0080`.

View changes since the review

[RalfJung]

This code only runs when IS_CLEARED_ON_MOVE is set. So qualifs remain in control of whether this should be done. And for drop it seems fine since the drop flag is indeed cleared on move.

[tmiasko]

The example passes const checks and executes live drop. I can't see how this can be fine, besides arguing that program has undefined behavior.

If the flag was actually unset at drop time, the drop wouldn't run.

[RalfJung]

a = a; does indeed not clear the drop flag on a, so if this analysis thinks that that's a move from a then that does seem incorrect -- but that doesn't invalidate the general point about moves I think?

[tmiasko]

There is definitely a room for some kind of justification, my point is simply that it requires something beyond operational semantics.

a = a is irrelevant, you can remove it.

[RalfJung]

this is what was happening before this PR 😆

Yeah, but not in the right spot -- the move does unconditionally clear the drop flag. It does not make sense to me that we'd check borrow.contains there.
It is the a = None; where we should check borrow.contains: if a has been borrowed before, then we should not analyze the value on the right.

struct PanicOnDrop;
impl Drop for PanicOnDrop { fn drop(&mut self) { panic!(); } }
pub const fn main() {
    let mut a = None;
    let p = &raw mut a;
    std::mem::forget(a);
    a = None; // <-- this one
    unsafe { std::ptr::write(p, Some(PanicOnDrop)) };
    a = a;
}
const _: () = main();

[RalfJung]

Oh interesting. Yeah that does not sound great. This sounds like even before this PR it should be possible to write code that triggers the "calling non-const function" error. We do these days detect the UB from mutating &i32 even in CTFE, but on something like &(i32, Cell<i32>) we don't detect the UB from mutating the first field.

Ah no it just works out -- the UB we detect is just barely enough to avoid "calling non-const function" in the code that we accept based on shared_borrow_allows_mutation.

That is an accident though...

[tmiasko]

The const-checker doesn't care about UB. It cares about making sure that a const only calls const fn. As in, syntactically, even in dead code or code unreachable because of UB, all function calls must be const fn calls. This is important because CTFE does not actually detect all UB and we never want to run non-const-fn during CTFE. This is mostly trivial except for drop calls which are not yet explicit in MIR by the time we do const checking, so we need this non-trivial analysis to predict whether a call will be present after drop lowering.

The analysis, as implemented now, is in some ways both more precise and less precise than drop elaboration. It doesn't predict a syntactical absence of non-const drops after drop elaboration. Rather, it tries to predict whether they might be executed when the code runs.

For example, it concludes that let a: Option<NonConstDrop> = None; is a not live non-const drop, even though drop is present after drop elaboration.

[RalfJung]

Yeah so it's a funny mix. For let a: Option<NonConstDrop> = None; with nothing else in the MIR body we don't have to rely on any UB though. So while I agree it makes the correctness statement more tricky, I am not otherwise concerned.

The shared_borrow_allows_mutation situation is more tricky though. But I guess it's not worth a breaking change, we just have to update the comments to explain that this is fine because const-eval reliably aborts on the UB this is based on.

Which brings us back to this PR and the new example -- there, const-eval does not reliably abort, so even if this is UB, we can't use that UB. I think ideally we'd make it so that we do indeed mark a local as "does not need drop" when it is moved-out-of (what the PR does), and additionally we inhibit value-based reasoning on assignments to locals after the local has been (mutably) borrowed. That should work?

[tmiasko]

With MIR move elimination semantics, an allocation is deallocated after move. Once implemented, I would expect it would take an extra effort to miss such UB.