r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

A question to @traviscross ...

Should we also need to run this through t-lang as well? It is hard to imagine that anything would rely on destructor called in this way, but I am not sure.

A question to @traviscross ...

Should we also need to run this through t-lang as well? It is hard to imagine that anything would rely on destructor called in this way, but I am not sure.

Well, let's see. It looks like this issue does affect the user-visible stable behavior of the language:

use core::{mem, ptr, sync::atomic::{AtomicU64, Ordering}};
static COUNT: AtomicU64 = AtomicU64::new(0);
struct S;
impl Drop for S {
 fn drop(&mut self) {
 COUNT.fetch_add(1, Ordering::Relaxed);
 }
}
trait Tr {}
impl Tr for S {}
const X: Box<dyn Tr> = unsafe {
 mem::transmute::<_, Box<S>>(ptr::dangling_mut::<S>())
};
fn main() {
 assert_eq!(0, COUNT.load(Ordering::Relaxed));
 let _a: &'static [Box<dyn Tr>; 0] = &[X; 0];
 assert_eq!(1, COUNT.load(Ordering::Relaxed));
 //~^ Drop was run.
 let _b = [X; 0];
 assert_eq!(1, COUNT.load(Ordering::Relaxed));
 //~^ Drop wasn't run.
}

So, yes, it's ours to fix via FCP.

Anyone think we need a crater run? For my part, I could go either way on that, and I do want us to fix this, so let's:

@rfcbot fcp merge

@rustbot labels +I-lang-nominated +needs-fcp

cc @rust-lang/lang @rust-lang/lang-advisors

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

Another fun edge case where this change is visible in user code: This code compiles with this PR, but fails on nightly due to dropping in const

use std::rc::Weak;
const X: Weak<i32> = Weak::new();
const Y: [Weak<dyn Send>; 0] = [X; 0];

The code changes seem fine, speaking as a reviewer who is not at all familiar with the MIR building and relevant lang details. The example from @theemathas might be worth adding to the test case.

@bors try

Okay, I will request a crater run ahead of the next meeting.

I will push a commit to include the second test cases.

☀️ Try build successful (CI)
Build commit: 981ca44 (981ca440e18f6cc4bc9026d4be8ddb1ca8378af7, parent: a1531335fe2807715fff569904d99602022643a7)

Hello team. I would like to request

@ craterbot run mode=build-and-test crates=top-100

for a quick triage. 🙇

I.e., all the regressions were spurious.

Does this relate to #79580 somehow?

r=me once the lang team has approved.

@RalfJung Yes. I'm not sure if I'm misunderstanding that issue (which seems to have an outdated description). But this PR is fixing the behavior to be correct, right?

I'm not sure, I didn't page this back in -- I just remembered seeing something about empty arrays before.

In the lang call, we noted (h/t @joshtriplett) that another observable manifestation of this is when a post-mono panic occurs or does not occur, e.g.:

use std::rc::Weak;
fn main() {
 const X: Weak<dyn Send> = Weak::<u8>::new();
 let _a: &'static [Weak<dyn Send>; 0] = &[const { panic!(); X }; 0];
 //~^ Panics.
 let _b = &[const { panic!(); X }; 0];
 //~^ Does not panic.
}

@rfcbot reviewed

My understanding of what is happening here is that, before, we had some special-case code to avoid dropping X in the case of [X; 0] if X is a constant. But we are not recognizing this as a constant because of the unsizing that is occurring -- from what I can tell, this unsizing is a sorta silly case where the compiler inserts an "unsize" to convert from dyn Unsize to dyn Unsize (we do that to allow for upcasts; not needed here).

I don't 100% understand why why [X; 0] would drop X ever -- I kind of expect that X would be unevaluated much like an if false { X } -- but... that's orthogonal.

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot reviewed

Here's the way I understand the problem:

When the type is annotated, the compiler inserts a no-op unsize coercion that turns [X; 0] into [X as Weak<dyn Send>; 0].

The X as Weak<dyn Send> is technically not a const. Instead, it's a value expression that actually creates a temporary. (As if you wrote [identity(X); 0].) This temporary is then immediately dropped due to the array length being zero.

I think of the fix in this PR as effectively changing the expression to [const { X as Weak<dyn Send> }; 0]. That is, we "assign" the result of the coercion into a new const, and then use that new const in the array expression.

Applying a similar construction to arrays with nonzero lengths would fix #140123. (Not done in this PR.) In that issue, a similar coercion causes the compiler to try to copy a non-Copy type when creating an array with length 2 or more. (See #140123 (comment) for a simplified repro.)

Hm, this doesn't seem very satisfying... this will also treat explicit coercions introduces by an as cast as still "being a constant", right? We're basically engaging in a game of whack-a-mole with the MIR builder about the exact shapes that the MIR for [X; 0] can have.

Is it really worth having a special case for [X; 0]? What's so bad with having it behave like let x = X; [x; 0], which I presume is identical to let x = X; drop(x); []?

I guess this could become tricky in the general case where the array length is generic...

I think it is fine that

• we build MIR [X; N] as let x = X; drop(x); [], if N is generic or we cannot evaluate the const into zero; our MIR should work irrespective of valuation of N, including the case N not being zero of course
• we build MIR [X; N] as [], if N is zero literal or can be evaluated into zero in the environment

Do one and two behave differently after this PR and/or with RalfJung's proposal?

const X: HasDrop = .....
fn generic<const N: usize>() {
 let _ = [X; N];
}
fn one() {
 generic::<0>();
}
fn two() {
 let _ = [X; 0];
}

I think it is fine that

That does not sound fine; the behavior should be the same in generic and monomorphic code.

I don't 100% understand why why [X; 0] would drop X ever -- I kind of expect that X would be unevaluated much like an if false { X } -- but... that's orthogonal.

if false is an interesting idea for the desugaring... it's not currently what happens in the non-const case either, though:

struct HasDrop;
impl Drop for HasDrop {
 fn drop(&mut self) {
 println!("drop!");
 }
}
fn main() {
 let x = HasDrop;
 let y = [x; 0];
 println!("after array");
}

This prints

drop!
after array

so the move+drop of x actually occurs when the [x; 0] is evaluated. If this was more like if false { drop(x); loop {} } else { [] }, the drop! would happen later, when x goes out of scope.

That said, in general if [expr; 0] has side-effects, we probably want to run those. Not sure how you'd imagine the desugaring to look like to ensure that?

@theemathas

Do one and two behave differently after this PR?

I am testing the following snippet. Playground

use std::sync::atomic::{AtomicUsize, Ordering};
struct X;
static DROP_COUNT: AtomicUsize = AtomicUsize::new(0);
impl Drop for X {
 fn drop(&mut self) {
 DROP_COUNT.fetch_add(1, Ordering::Relaxed);
 }
}
fn main() {
 one();
 assert_eq!(DROP_COUNT.load(Ordering::Relaxed), 0);
}
fn generic<const N: usize>() {
 const A: X = X;
 let _ = [A; N];
}
fn one() {
 generic::<0>();
}

This runs without assertion failure on stable. Inspecting the MIR after the runtime optimisation phase, fn generic does not seem to care about the valuation of N: the statement is simply _1 = const [A; N]; and it is handed off to monomorphism to decide whether A should be instantiated.

the behavior should be the same in generic and monomorphic code

So, in some sense our generic code is already working differently from monomorphic code on stable today.

I have reflected on my earlier comment and I want to retract it. Instead I now agree that we should take the monomorphised code as the golden benchmark.

This is what I would think, a better way to solve this. I would like to propose that we build the MIR slightly differently,

[ $expr; $repeat ]

... shall be built into a MIR that is roughly equivalent to

if $($repeat equals the const zero) {
// ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// This will most likely be optimised away;
// if not, we can investigate further how we could
// optimise harder after monomorphism.
 try_const_promotion_orelse_instantiate_and_drop!($expr);
 []
} else {
 [ build_and_move($expr); $repeat ]
}

Here what I mean by cannot get const-promoted is that evaluation of $expr depends on runtime values, including owned use of a place or calls, and is exempted from const promotion in the PromoteTemps pass.

What this PR concerns about is the case when a const literal is behind a pointer coercion, which is promoted into a const today.

So, in some sense our generic code is already working differently from monomorphic code on stable today.

Is it? If I make the code monomorphic, the assertion still passes: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0c47f27743a2e5b1f8431ea679d8fd20

I really want to say, yes it is and , but apparently I cannot write a code that demonstrate this in the runtime because when constant propagation is concerned we can only work with types without Copy and, therefore, without any destructor. #143671 is the best example. Admittedly the code is rejected due to lack of Copy bound when anyone tries to write [$expr; N] where N is a const generic, but inspecting the initial MIR built one can find that [$expr; 0] is built differently from [$expr; N]. The core mechanism here is that the MIR builder disagrees with const promotion whether $expr should be viewed as const.

More concretely speaking, given [$expr; 0], the MIR builder may interpret it as [ { $expr }; 0 ] so that this $expr needs to be instantiate at least once, but the next instant when const promotion is performed, the MIR is transformed into effectively like let _ = $expr; [const { $expr }; 0]. This is definitely different from [const { $expr }; N] where N is generic: the MIR is built always into _1 = const [ {some-const}; N ] regardless of N's value.

My question is, is my proposal in the last comment reasonable? I would like to take this opportunity to make consensus. If the idea is interesting, I can further explain what I mean bytry_const_promotion_orelse_instantiate_and_drop.

I am a bit confused by your mention of const promotion... there's no & here so why would that even kick in?

I have the example in which as & triggers const promotion we get into a situation where MIR builder contradicts with const promotion on the view of the nature of the repeat THIR expression. It was first spotted here. Playground

The &[WithCoerce { value: &1 }; N] part is const-promoted, but before running that pass the MIR builder thinks that WithCoerce { value: &1 } is a temporary.

Stable version (no nightly features) of the above inconsistency:

use std::mem::transmute;
use std::ptr::dangling_mut;
struct Thing;
impl Drop for Thing {
 fn drop(&mut self) {
 println!("drop");
 }
}
const X: Box<dyn Send> = unsafe { transmute::<*mut Thing, Box<Thing>>(dangling_mut()) };
fn main() {
 let _a: &'static [Box<dyn Send>; 0] = &[X; 0];
}

I have the example in which as & triggers const promotion we get into a situation where MIR builder contradicts with const promotion on the view of the nature of the repeat THIR expression. It was first spotted here. Playground

Oh yeah that's an interesting case. Promotion seems to say "this array is empty, so there's no destructor, so we can promote", but MIR building already inserted a direct call to the destructor... but that call has no data dependency with the actual array so promotion doesn't even see it as related to the array.

I think a fundamental part of the problem is that we have various different "modes of use" of array expressions that need to be considered to desugar in different ways. We allow [expr; N] if any of these conditions is met:

1. The elem type is Copy
2. The initializer expression is a const
3. N is 0 or 1

These conditions can overlap, and specifically when 2 and 3 overlap there's an observable difference since 3, in general, should run the destructor for N == 0.

I feel like we need to sort out the entire desugaring story here, considering not only #143671 but also #140123. It seems what happens is that some early analysis concludes that the array is legal based on case 2, but then later due to the coercion it doesn't look like a case 2 any more, and then it either ICEs (since no case fits) or starts doing weird drop things (since case 3 was used).

What does this PR do with #140123 (specifically examples like this)? And how sure are we that check_constness will match the exact same cases as whatever pass does the initial array expression checking? (There should at the very least be comments linking them.) Ideally we'd avoid recomputing which case we are in based on a different representation, that seems rather fragile in the long term.