Modify MIR building to drop repeat expressions with length zero

[JakobDegen]

Closes #74836 .

Previously, when a user wrote [foo; 0] we used to simply leak foo. The goal is to fix that. This PR changes MIR building to make [foo; 0] equivalent to { drop(foo); [] } in all cases. Of course, this is a breaking change (see below). A crater run did not indicate any regressions though, and given that the previous behavior was almost definitely not what any user wanted, it seems unlikely that anyone was relying on this.

Note that const generics are in general unaffected by this. Inserting the extra drop is only meaningful/necessary when foo is of a non-Copy type, and array repeat expressions with const generic repetition count must always be Copy.

Besides the obvious change to behavior associated with the additional drop, there are three categories of examples where this also changes observable behavior. In all of these cases, the new behavior is consistent with what you would get by replacing [foo; 0] with { drop(foo); [] }. As such, none of these give the user new powers to express more things.

No longer allowed in const (breaking):

const _: [String; 0] = [String::new(); 0];

This compiles on stable today. Because we now introduce the drop of String, this no longer compiles as String may not be dropped in a const context.

Reduced dataflow (non-breaking):

let mut x: i32 = 0;
let r = &x;
let a = [r; 0];
x = 5;
let _b = a;

Borrowck rejects this code on stable because it believes there is dataflow between a and r, and so the lifetime of r has to extend to the last statement. This change removes the dataflow and the above code is allowed to compile.

More const promotion (non-breaking):

let _v: &'static [String; 0] = &[String::new(); 0];

This does not compile today because String having drop glue keeps it from being const promoted (despite that drop glue never being executed). After this change, this is allowed to compile.

Alternatives

A previous attempt at this tried to reduce breakage by various tricks. This is still a possibility, but given that crater showed no regressions it seems unclear why we would want to introduce this complexity.

Disallowing [foo; 0] completely is also an option, but obviously this is more of a breaking change. I do not know how often this is actually used though.

r? @oli-obk

[oli-obk]

cc @RalfJung we're preventing to promote [String::new(); 0] while fixing it that the element is actually dropped instead of leaked. We avoid this by not changing the mir (in a meaningful way) until after promotion and before borrowck.

[oli-obk]

Please reference the bug in the main post and re-explain what the problem is and what code is now changed, as we'll want to nominate this for the lang team. Technically this is a breaking change after all.

Running it through crater, too

@bors try

[bors]

⌛ Trying commit 91095c988f099cb0b7d2d7a793e91c1593e9d789 with merge cd1dad2c64d4db00cf381bc072bc07c5230963ce...

[bors]

☀️ Try build successful - checks-actions
Build commit: cd1dad2c64d4db00cf381bc072bc07c5230963ce (cd1dad2c64d4db00cf381bc072bc07c5230963ce)

[oli-obk]

@craterbot check

[craterbot]

👌 Experiment pr-95953 created and queued.
🤖 Automatically detected try build cd1dad2c64d4db00cf381bc072bc07c5230963ce
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[JakobDegen]

I was very tired by the time I put up this PR yesterday, so I got a little bit lazy 😅 . I've updated the PR description with a summary of the situation, which should hopefully be enough information for T-Lang to be able to make an informed decision (if this is discussed at triage today I can also be there)

[RalfJung]

cc @RalfJung we're preventing to promote [String::new(); 0] while fixing it that the element is actually dropped instead of leaked. We avoid this by not changing the mir (in a meaningful way) until after promotion and before borrowck.

I think we should break code like in "snippet A". It should never have worked to begin with.

What is the problem with allowing code like "snippet B"?

and then immediately after borrowck changing it to this if we are in a non-const context:

Honestly, this sounds like a terrible hack that will come to bite us later.

[JakobDegen]

I don't have strong opinions on breaking snippet A, mostly because I don't have enough experience with how these decisions are made.

What is the problem with allowing code like "snippet B"?

Quoting the reference:

Promotion of a value expression to a 'static slot occurs when the expression could be written in a constant and borrowed, and that borrow could be dereferenced where the expression was originally written, without changing the runtime behavior.

I'm interpreting this to mean that &expr should const promote if &*const { &expr } would work. In the case of &[String::new(); 0], const { &[String::new(); 0] } would not be allowed, and so it should not const promote.

Half related sidenote about implementation

I actually find it somewhat surprising that we chose to implement this in terms of a dataflow analysis. As written, the definition seems to be a mostly syntactic one, and so dataflow would not be my first choice. Is there some complexity here that I'm missing, or some historical details that make this necessary?

Honestly, this sounds like a terrible hack that will come to bite us later.

Agreed. That being said, I'm going to claim that given how horrible the requirements that have emerged are, our options are either to change the requirements or have a terribly hacky implementation. I'm honestly somewhat surprised that the requirements under which I was operating could be implemented with this little code

[RalfJung]

I don't have strong opinions on breaking snippet A, mostly because I don't have enough experience with how these decisions are made.

If crater doesn't find a case of this happening in the wild, the teams are usually on-board with doing the breaking change. We had a bunch of those around promotion last year.

I'm interpreting this to mean that &expr should const promote if &*const { &expr } would work. In the case of &[String::new(); 0], const { &[String::new(); 0] } would not be allowed, and so it should not const promote.

I'd be shocked if that was the only such example.

Promotion is a lot more subtle than that, e.g. when you write &(A+B), the addition gets promoted but the assertion in case of overflow remains unpromoted. With &something[idx] we sometimes promote the &something but leave the indexing to runtime (because the index is dynamic). This list probably goes on.

Also see this document, but I don't think it is complete either, unfortunately.

At this point, as far as I am concerned, promotion is mostly something we keep around to keep old code working. Anything more complicated than &42 works "by accident". My biggest concern is keeping the promotion implementation as simple as possible, since it has been a huge source of pain (and bugs, even some soundness bugs) in the past.

If there is a nice way to prevent &[String::new(); 0] from being promoted, sure. But first constructing "wrong" MIR and then ad-hoc patching it later?!? That's not worth it.

Agreed. That being said, I'm going to claim that given how horrible the requirements that have emerged are, our options are either to change the requirements or have a terribly hacky implementation. I'm honestly somewhat surprised that the requirements under which I was operating could be implemented with this little code

So let's change those requirements then. :)

[RalfJung]

Maybe promotion analysis should have remained on HIR or THIR, it seems easier to specify and contain that way...

I actually find it somewhat surprising that we chose to implement this in terms of a dataflow analysis. As written, the definition seems to be a mostly syntactic one, and so dataflow would not be my first choice. Is there some complexity here that I'm missing, or some historical details that make this necessary?

It's just how things developed historically. Promotion analysis used to be in the same pass as "check if this is const-compatible", and it used to work on HIR. Basically nothing around promotion was ever "designed" -- it all just "happened" over time. In 2020 I managed to cut away most of that cruft without breaking a lot of code. I then ran out of energy and ideas on how to further improve the situation.

Also see

• Solving the problem with promotion const-eval#53
• Tracking Issue for infallible promotion #80619

[JakobDegen]

Yeah, this makes sense to me. Possibly we should cancel the crater run, pending a T-Lang response?

[oli-obk]

@craterbot abort

[craterbot]

🗑️ Experiment pr-95953 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[oli-obk]

We can just crater the version that breaks A and allows B (so basically your original one 😆 ) and then present the result to the lang team. It's easier to talk about if we have data

[JakobDegen]

The new version breaks A and allows B. I took some extra time to learn about how drops work in MIR building, and I believe this should do the right thing. The diagnostic on snippet A leaves a little to be desired, but that probably shouldn't block the crater run.

One interesting thing to note is that this change teaches MIR that there is no dataflow dependency between foo and bar in let foo = [bar; 0]; That seems like what we want, since it is after all true, but this is at least a little orthogonal to the drop stuff

[RalfJung]

One interesting thing to note is that this change teaches MIR that there is no dataflow dependency between foo and bar in let foo = [bar; 0]; That seems like what we want, since it is after all true, but this is at least a little orthogonal to the drop stuff

FWIW, if there is a nice way to pretend to dataflow that such a dependency exists, I wouldn't be opposed to that -- as long as it can be added during MIR building, not as a post-doc semantics-changing MIR-patching transformation.

[bors]

📌 Commit 44410ef has been approved by oli-obk

[bors]

⌛ Testing commit 44410ef with merge 1577bcc8461b0acd25cc943a075be2207283baa1...

[JohnTitor]

Failed in rollup: #97347 (comment)
Needs to ignore some targets on the tests

@bors retry r-

[RalfJung]

@bors rollup=iffy

[JakobDegen]

Added a bunch of ignore directives, stolen from src/test/ui/runtime/rt-explody-panic-payloads.rs

[oli-obk]

@bors r+

[bors]

📌 Commit 0f65bcd has been approved by oli-obk

[bors]

⌛ Testing commit 0f65bcd with merge fe9c64d...