Should unions use type based const qualification?

[tmiasko]

For example, after laundering interior mutability through a union, a resulting constant undergoes promotion:

#![feature(untagged_unions)]
use std::cell::Cell;

pub const CELL: Option<Cell<u32>> = {
    union U { i: u32, c: Cell<u32> }
    Some(unsafe { U { i: 0 }.c })
};

fn main() {
    let _ = &CELL;
}

error[E0080]: it is undefined behavior to use this value
 --> u.rs:9:1
  |
9 | fn main() {
  | ^^^^^^^^^ type validation failed at .<deref>.<enum-variant(Some)>.0.value: encountered `UnsafeCell` in a `const`
...

@rustbot label +A-const-eval +F-untagged_unions

[oli-obk]

Yea, unions should not be "more powerful" than a plain transmute imo.

cc @rust-lang/wg-const-eval

[RalfJung]

I assume with transmute one can also produce a similar example as the OP? So in which sense are unions more powerful than transmute, @oli-obk?

Why does this example behave any different than the following?

use std::cell::Cell;

pub const CELL: Option<Cell<u32>> = Some(Cell::new(0));

fn main() {
    let _ = &CELL;
}

[tmiasko]

The union local initially starts without interior mutability qualif. This persist after the first assignment, since u32 does not have interior mutability The move from the cell variant is qualified based on the qualification of the local, so the overall constant is consider not to have interior mutability.

The use of the constant in the main function is promoted and fails dynamic checks.

[RalfJung]

I totally forgot promotion of &CONST looks into the definition of CONST... that'll continue to haunt us for a while I think. :/

Yeah, the analysis clearly is wrong for reading from union fields.

But even for values like U { i: 0 }, I have no idea what the wider consequences are (given that the type U is considered interior mutable).

[tmiasko]

The example can be also modified by moving union access to the runtime code, in
which case it will compile successfully, with promoted being placed in .rodata
section:

#![feature(untagged_unions)]
use std::cell::Cell;

pub union U { i: u32, c: Cell<u32> }

pub const CELL: U = {
    U { i: 0 }
};

fn main() {
    let cell: &'static U = &CELL;
    unsafe { (cell.c).set(1) };
}

$ rustc a.rs
$ ./a
Segmentation fault

In the context of static analysis, I think it would make sense to use strictly
type based analysis for unions, in which case the first assignment to a union
local would already qualify the local as having interior mutability.

[RalfJung]

But also, there's supposed to be a 2nd line of defense in our interning code that is somehow failing.

[RalfJung]

Turns out interning relies in validity checking for this:

rust/compiler/rustc_const_eval/src/interpret/intern.rs

Line 133 in c5fc260

// Validation will ensure that there is no `UnsafeCell` on an immutable allocation.

The example in the OP was caught by validity, so that is good. But the 2nd example was not caught, because we never actually hit an UnsafeCell -- we stop at the union boundary.

So maybe the union part of the visitor should also have the kind of check UnsafeCell does?

rust/compiler/rustc_const_eval/src/interpret/validity.rs

Line 740 in 45b989a

fn visit_union(

[tmiasko]

@RalfJung do we have an established approach to testing that this second line of defense works? Carefully crafted constant with undefined behaviour might do its job in this case (not sure yet), but maybe we have better alternatives?

[RalfJung]

We have -Zunleash-the-miri-inside-of-you but that only helps for const checks, not for promoted restrictions... I don't think we have anything for promoteds (and it seems non-trivial, we would need a way to force something to be promoted despite it violating the usual conditions).