Handling of packed fields causes semver hazard: layout details from other crates affect type-checking

[RalfJung]

This is very similar to #78586: we have another situation where the compiler calls layout_of to determine whether some code should compile or how it should be compiled, but layout_of completely ignores visibility and crate boundaries, thus creating a semver hazard.

This issue is about packed fields. When S is a packed(N) type, and we write &s.field, then this will be allowed by the compiler iff the type of field requires alignment no more than N. This has 2 semver consequences:

• If you have a packed struct with public fields, it is a breaking change to make it "more packed" by reducing the N -- that part is probably fine, it is similar to how it is obviously a breaking change to add repr(packed) in the first place.
• Increasing the alignment of a type can be a breaking change, if that type is ever used as the type of a public field in a packed struct (including in a nested way, for cases like &s.field.inner_field). This might not be what we want, at least I recall @scottmcm being concerned about this.

Now, of course increasing the alignment of a type can already have visible effects downstream such as increasing the size of other types, changing the result of align_of, etc -- but my understanding is that generally, changing the size and alignment of a type is considered semver-compatible, at least if there is at least one private field. Right now, changing a private field in some type T from u8 to u64 can break downstream code that tries to take references to fields of type T inside a packed struct.

There's also an interaction with closure capturing, where if a field requires alignment more than 1 and is inside a packed struct, then it will not be field-closure-captured, and we capture the field of packed type instead. This means that increasing the alignment of a type from 1 to more can change how closure captures are computed, which can change when drop is run, which... that just sounds pretty bad.

So what shall we do? Even ignoring backwards compatibility issues, what would be a better behavior here? We'd basically need a notion of "this type publicly has an alignment requirement of at most N", and then we could use that. I am not sure how that notion should be defined though. We could also say we always reject references to fields of packed structs, but there's a lot of code out there that takes &packed.field where field: [u8; LEN] or so, and there is no good reason to reject such code. If it was just for references we could start with something like "primitive types have public alignment but ADTs do not", and we could refine this over time to accept more and more code (such as ADTs in the same crate where also all fields have types from this crate, recursively), but (a) it is very possible that even that would break too much existing code, and (b) due to the interaction with closure field capturing, even considering more things publicly aligned is a potential breaking change.

For closures it really seems best to just stop field capturing when there is any packed projection and don't look at field alignment. That would be a breaking change of course, but after we did it then at least it is entirely decoupled from how aligned fields are.

See below for some examples.

Cc @ehuss @scottmcm

[RalfJung]

For closures it really seems best to just stop field capturing when there is any packed projection and don't look at field alignment. That would be a breaking change of course, but after we did it then at least it is entirely decoupled from how aligned fields are.

I've implemented that in #115315. If that works out, then we only have to worry about taking references, where at least "accepting more things as aligned" is a backwards-compatible change and we only decide whether code passes the type-checker, we never alter how code behaves.

[ehuss]

Just FYI, cargo has recently published guidelines on semver compatibility of layout changes here: https://doc.rust-lang.org/nightly/cargo/reference/semver.html#type-layout. The general gist is that any change to layout or alignment is a breaking change for a well-defined type where well-defined has a certain definition (a specific repr, publicly documented, etc.). There are certainly some hazards and gray areas, and I tried to highlight those.

[RalfJung]

Those guidelines say fairly clearly that in a repr(Rust) struct, changing a private field from u8 to u64 is allowed. And yet, this can currently lead both to code that stops compiling, and to changes in how closures capture fields (with implications for drop ordering).

[ehuss]

Can you show an example of what code would stop compiling if a private field was changed from u8 to u64 in a repr(Rust) struct? Or how that would affect closure captures?

[RalfJung]

Here's an example of the build failing after a private field type change due to the check on references to packed fields:

#![allow(dead_code)]

mod m {
    // before patch
    pub struct S1(u8);
    // after patch
    pub struct S2(u64);
}

#[repr(packed)]
struct MyType {
    field: m::S1, // build fails when this becomes S2
}

fn test(r: &MyType) {
    let _r = &r.field;
}

Here's an example where the build starts failing due to closure captures:

#![allow(dead_code)]

mod m {
    // before patch
    pub struct S1(u8);
    // after patch
    pub struct S2(u64);
}

#[repr(packed)]
struct MyType {
    field: m::S1, // build fails when this becomes S2
    other_field: Vec<()>,
}

fn test(r: MyType) {
    let c = || {
        let _val = std::ptr::addr_of!(r.field);
    };
    let _move = r.other_field;
    c();
}

Here is an example where the output of the program changes:

#![allow(dead_code)]

mod m {
    // before patch
    #[derive(Default)]
    pub struct S1(u8);
    // after patch
    #[derive(Default)]
    pub struct S2(u64);
}

struct NoisyDrop;
impl Drop for NoisyDrop {
    fn drop(&mut self) {
        eprintln!("dropped!");
    }
}

#[repr(packed)]
struct MyType {
    field: m::S1, // output changes when this becomes S2
    other_field: NoisyDrop,
    third_field: Vec<()>,
}

fn test(r: MyType) {
    let c = || {
        let _val = std::ptr::addr_of!(r.field);
        let _val = r.third_field;
    };
    drop(c);
    eprintln!("before dropping");
}

fn main() {
    test(MyType {
        field: Default::default(),
        other_field: NoisyDrop,
        third_field: Vec::new(),
    });
}

[ehuss]

I see, thanks! I'll need to think about how to express that. It seems like if you have repr(packed) that there is a certain amount of reliance on the types of the fields to be stable (that is, you can't use a type unless you know its size/alignment and you know that it will never change).

[RalfJung]

Those are the current rules, yes. But I think we should change the compiler to accept less code, similar to how we changed repr(transparent) so that it now warns when relying on the fact that some type with private fields or non_exhaustive is a 1-ZST (#99020). That ensured that changing private field types cannot break code due to the repr(transparent) check; this is an entirely analogous situations.

[joshtriplett]

We talked about this in today's @rust-lang/lang meeting. repr(packed) does indeed seem like a substantial compatibility hazard. We'd be open to seeing lint proposals to warn about potential compatibility issues (e.g. layouts relying on private fields). We would want to see crater runs showing how widespread those issues are.