Detect uninhabited types early in const eval

[oli-obk]

r? @RalfJung

implements #108442 (comment)

this is a breaking change, as some UB during const eval is now detected instead of silently being ignored. Users can see this and other UB that may cause future breakage with -Zextra-const-ub-checks or just by running miri on their code, which sets that flag by default.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @RalfJung (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

[oli-obk]

@bors try @rust-timer queue

I don't expect perf to be affected, but let's check.

We need a try build for a crater run anyway

[bors]

⌛ Trying commit 2965d7c7e4e6d6e30eae2623bfe5c5971f6fff02 with merge 8f22a9063e227203491552646fe23bfaeddd99ba...

[RalfJung]

r=me, except -- should we be worried about the breaking aspect of this? Do we need a transition period?

[oli-obk]

Do we need a transition period?

no, last time we had a situation like this we concluded that having the -Z flag (and miri already setting it) is enough. If you are using unsafe code, best practices (like using miri and not transmuting to uninhabited types) should be upheld. This does not cause new silent miscompilations, just stops compiling code that was already unsound. I'll still run crater, but that doesn't take more than two days anymore nowadays.

[bors]

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

[bors]

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

[rust-timer]

Finished benchmarking commit (8f22a9063e227203491552646fe23bfaeddd99ba): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[1.3%, 1.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[-0.5%, 1.9%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.4%, -0.4%]	1

[scottmcm]

Oh, nice! That also means future PRs can easily try tuning this to catch more things where validation is cheap enough that perf says it's ok. (Like maybe checking just bool+char, or .abi.is_scalar().)

[oli-obk]

Sadly perf being clean mostly means we don't have stress tests going through that path

[oli-obk]

@bors r=RalfJung

heads up @rust-lang/lang we're catching more UB at const eval time. Notably we're now rejecting the following piece of code:

#[derive(Copy, Clone)]
pub struct ChildStdin {
    inner: AnonPipe,
}
#[derive(Copy, Clone)]
enum AnonPipe {}
const FOO: () = {
    union Foo {
        a: ChildStdin,
        b: (),
    }
    let x = unsafe { Foo { b: () }.a };
};

which we accepted before, as the value x of uninhabited type was never used. We only errored on use cases.

This is not going through crater or an FCP as we already report this if the constant is used in a program checked by miri or if the user passes -Zextra-const-ub-checks. There is no warning period, as this is a very niche issue, obviously UB in other cases (using transmute instead of a union would've already errored) and would already have errored if x were used in any interesting way (so anything but taking a reference to it).

Last time we accidentally broke such a case, the verdict was that the existance of a -Z flag or miri to check for that UB is sufficient for us to break code using that UB.

[bors]

📌 Commit f066d67 has been approved by RalfJung

It is now in the queue for this repository.

[Noratrieb]

@bors rollup=maybe
perf neutral