Promoted values are not validated

[oli-obk]

The program below creates a promoted in a constant via

&unsafe { BoolTransmute { val: 3 }.bl }

but immediately casts this to a raw pointer. Now having weird values behind raw pointers is fine, but we have an intermediate reference to an invalid value. I don't understand how this compiles considering that the promoted is essentially

static PROMOTED: bool = transmute(3u8);

Apparently we don't validate promoteds. I think we are allowed to fix this and validate promoteds, even though it will be a breaking change, because the user had to create an intermediate reference that pointed to an invalid bool.

cc @RalfJung

union BoolTransmute {
    val: u8,
    bl: bool,
}

const RAW_TRAIT_OBJ_CONTENT_INVALID: *const dyn Trait = &unsafe { BoolTransmute { val: 3 }.bl } as *const _;

trait Trait {}
impl Trait for bool {}

(Playground)

[RalfJung]

So with the new promotion-as-consts scheme, what does that code "desugar" to?

[oli-obk]

before promotion as consts:

static PROMOTED: bool = unsafe { BoolTransmute { val: 3 }.bl };
const RAW_TRAIT_OBJ_CONTENT_INVALID: *const dyn Trait = &PROMOTED as *const _;

after promotion as consts

const PROMOTED: &bool = &unsafe { BoolTransmute { val: 3 }.bl };
const RAW_TRAIT_OBJ_CONTENT_INVALID: *const dyn Trait = PROMOTED as *const _;

[RalfJung]

Thanks. I agree in both of these cases we should do validation and consider this an error.

[RalfJung]

Hm, actually, thinking about it again... if we didn't do promotion in that const, we'd not show any error. So I am not entirely sure if we really should show an error just because promotion happened? That would mean promoting more things can make compilation fail, which promotion explicitly does not want to happen.

Also, right now validity check rejects some things that are not necessarily UB, such as integers in a reference (there's an open issue for this somewhere).

[oli-obk]

We'll do this in a PR that does nothing else but this change. @spastorino already made the choice to not validate explicit in https://github.com/rust-lang/rust/pull/67000/files#diff-e0b58bb6712edaa8595ad7237542c958R622, so a PR fixing this issue will just remove the bail out and the comment

[RalfJung]

I have no idea what you just said but whatever.^^ That link just goes to the top of the diff, not any specific part of it.

[oli-obk]

So I am not entirely sure if we really should show an error just because promotion happened? That would mean promoting more things can make compilation fail, which promotion explicitly does not want to happen.

Promotion in runtime code may not fail compilation. Promotion in constants should fail compilation imo because it's explicit promotion because you are in a const context

[RalfJung]

My argument was that it's seems to become kind of arbitrary when we do validation (even inside CTFE context) and when not -- suddenly that depends on details like what exactly promotion is doing. Not sure if that's good. It's certainly entirely random from a user's perspective: some CTFE code that causes UB through invalid values will work fine, other code will cause validation errrors.

[oli-obk]

Hmm... that's true. ok, so the current strategy of not validating promoteds makes sense and we can revisit the question later (or make validating promoteds just a lint)

[RalfJung]

I think this is actually closely related to #67534. I am not sure if it is worth keeping both of them open.

Basically this issue is "we should validate promoteds maybe", and the other issue is "we cannot validate promoteds as we promote some things that are not valid".

[RalfJung]

Okay it's different kinds of validation -- one is the interior mutability part, done during interning, and the other is type-based validation of the final value.

[RalfJung]

We no longer promote union field accesses... but we still promote arbitrary const fn calls in const/static initializers so the issue can be reproduced that way.

Also I feel like my argument above only really works for implicit promoteds; explicit promoteds likely should be validated -- but I am not sure if there is an easy way we can make that distinction? It might just not be worth it.

[oli-obk]

The way it's currently set up we'd need to add that information to mir::Body. Considering that we want to rework our error/lint stuff around const eval anyway, we should probably do that first, so the callers to the const eval queries can decide such things. Though now that I think of it, validation is orthogonal to reporting errors on it, so idk.

[RalfJung]

With rust-lang/lang-team#58, it seems like we are moving towards making promotion infallible. I think that means we should also validate promoteds (all of them).

So I take back what I said earlier, where I argued against validating promoteds.