Do not materialise X in [X; 0] when X is unsizing a const

[dingxiangfei2009]

Fix #143671

It turns out that MIR builder materialise X in [X; 0] into a temporary local when X is unsizing a const. This led to a confusing call to destructor of X when such a destructor is declared. Playground

This patch may miss out other cases that we should avoid materialisation in case of [X; 0]. Suggestions to include is most welcome!

[rustbot]

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

[dingxiangfei2009]

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.

[traviscross]

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

[rfcbot]

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.

[theemathas]

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];

[nnethercote]

s/ek/kind/

I've never seen ek used for an ExprKind, kind is the normal name.

[dingxiangfei2009]

Thanks! Renamed.

[nnethercote]

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.

[dingxiangfei2009]

@bors try

[dingxiangfei2009]

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

[dingxiangfei2009]

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

[rust-bors]

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

[dingxiangfei2009]

Hello team. I would like to request

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

for a quick triage. 🙇

[nnethercote]

Let's do the top 1000, just for more coverage :)

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

[craterbot]

👌 Experiment pr-145277 created and queued.
🤖 Automatically detected try build 981ca44
⚠️ Try build based on commit c1ab3ed, but latest commit is 8444dde. Did you forget to make a new try build?
🔍 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

[theemathas]

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.

I found some similar-ish behavior with transmute instead of arrays. #146917

[dingxiangfei2009]

@theemathas @traviscross @RalfJung

It seems that an explicit cast in the language _ as _ has significance, in the sense that it gets compiled into a value expression. Therefore, X as WithCoerce<dyn Trait> get compiled into THIR Use expression.

That has the following definition.

rust/compiler/rustc_middle/src/thir.rs

Lines 358 to 364 in 4056082

	/// Forces its contents to be treated as a value expression, not a place
	/// expression. This is inserted in some places where an operation would
	/// otherwise be erased completely (e.g. some no-op casts), but we still
	/// need to ensure that its operand is treated as a value and not a place.
	Use {
	source: ExprId,
	},

How shall we interpret this? I read this as "disregard whether this could be a constant, build it as a value anyway."

Specifically for our example, it is lowered into

Scope {
    PointerCoercion {
        ValueTypeAscription {
            Use {
                NamedConst(X) } } } }

The consequence is that a drop will happen.

[RalfJung]

Constants are values, so I can't make sense of "treat as value not constant".

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[dingxiangfei2009]

I will open an issue for discussion on a future fix plan. The categorisation of the repeated operand could use some better, thorough analysis from the compiler, so that we can have a consistent evaluation strategy.

[dingxiangfei2009]

@rustbot ready

I have added a test and additional predicates on the repeated expression.

[traviscross]

We talked about this in the lang/RfL call with most of us who had checked a box present and agreed that this caveat about as casts doesn't disturb our FCP as long as landing this doesn't make it harder to fix that case as well.

[nnethercote]

I will open an issue for discussion on a future fix plan. The categorisation of the repeated operand could use some better, thorough analysis from the compiler, so that we can have a consistent evaluation strategy.

Once you have made the PR, please link to it from here. Otherwise, I think this has been discussed sufficiently.

@bors r+

[bors]

📌 Commit b77de83 has been approved by nnethercote

It is now in the queue for this repository.

[dingxiangfei2009]

Let's move discussion on a proper and comprehensive fix to the next issue #147021