Add semver guidelines for changing the repr of structs/enums to ref…

[Nilstrieb]

The current semver guidelines do not mention repr at all. Changing the repr of a struct/enum will break code, especially unsafe code relying on layout. These breaking changes will rarely manifest themselves as compiler errors, which makes them all the more dangerous.

The new guidelines specify removing a well-defined repr from a struct or enum to be a breaking change.

This is more of a suggestion on how it could be done, and I'm very open for improvements for the wording and other details.

Another thing I'm not sure about is how this interacts with #[non_exhaustive], and therefore don't mention it at all.

There are also some other breaking changes that are related to this, like reordering the fields of a #[repr(C)] struct, and it's open for discussion whether these should be added as well.

discussion on zulip

[rust-highfive]

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

Please see the contribution instructions for more information.

[Nilstrieb]

I'm having a problem with the struct change, since it isn't a compiler error which causes CI to fail.

I could just do a dirty size assertion which would fix it, but that's a bad example imo

const _: () = assert_foo_size(); // Error: evaluation of constant value failed
const fn assert_foo_size() {
    let size = std::mem::size_of::<Foo>();
    if size != 6 {
        panic!("Foo has changed size!");
    }
}

Is there a flag to tell CI that it doesn't matter that this isn't a compiler error? Or do you know a better example that causes a compiler error?

[weihanglo]

I can’t find any other flag in the current implementation.

cargo/src/doc/semver-check/src/main.rs

Lines 183 to 185 in 0655dd2

	let output = cmd.output()?;
	let stderr = std::str::from_utf8(&output.stderr).unwrap();
	match (output.status.success(), expect_success) {

Maybe adapt semver-check to run the output executable, though that requires some efforts.
Or we can wait for feature(const_ptr_offset) stabilized 😆

[ehuss]

I think it would be useful to extend semver-check to be able to have a "run-fail" mode. I think it shouldn't be too difficult to add. I would mark the example's language tag with something like

```rust,ignore,run-fail
# example here
```

And then check for that flag, and just run the code. From your example, I think that should be relatively safe.

Another possibility is to go with your static assert, but use rustdoc's hidden-line feature (using # as a prefix will hide those lines of code). However, I think semver-check would need to be adapted to strip the # prefix. I would lean towards adding the run-fail mode, though, as I think that could be useful in the future.

[Nilstrieb]

That took some git magic, but let's see whether this passes now 😅

[joshtriplett]

In general, I think there are a number of other possible ways that repr can be a breaking change.

I think it'd be good to generalize:

• It is possible for another crate to depend on the size or alignment or memory layout of one of your structs/unions/enums.
• Therefore, it is possible that a change to the size or alignment or memory layout of one of your structs/unions/enums may be a breaking change.
• Whether such a change is a breaking change in practice depends on what crates actually depend on, and to a much lesser extent what they should be able to depend on. You can potentially mitigate a potential breaking change with documentation saying what is and isn't part of your stable interface...assuming people read your documentation and follow it.
• Here are some (but not by any means all) ways that repr can affect the size or alignment or memory layout of your structs/unions/enums...

[Nilstrieb]

While I like there being hard rules, I agree with you that it is a bit more nuanced with repr. Changing layout is more dangerous than other "classic" breaking changes, because it doesn't actually cause compiler errors in most cases.
But people relying on layout is also generally quite rare I'd guess, and is only supposed to happen with #[repr(C)] and #[repr(transparent)]. If people rely on the layout of your #[repr(Rust)] types, something very fishy is going on.

But it is really hard to come up with hard rules, and these general guidelines sound nice, but I'd still like to direct towards telling that only changing from well-defined reprs should be breaking.

[ehuss]

I think this is what was meant?

Suggested change

	// SAFETY: Foo is repr(C), so we are guaranteed that there will be `3` at this offset (u8, 8 pad, u16)
	// SAFETY: Foo is repr(C), so we are guaranteed that there will be `5` at this offset (u8, 8 pad, u16)

[ehuss]

Just to try to reword this for clarity.

Suggested change

	When a struct has a well-defined repr (`transparent`, `C`) and only public fields, this will break unsafe code relying on
	that repr.
	When a struct has a well-defined repr (`transparent`, `C`) and only public fields, changing the repr will break unsafe code relying on the original repr.

[ehuss]

Just to clarify, is this qualified on "only public fields" because changes to private fields can potentially change the alignment, and that's explicitly hidden from the user?

[ehuss]

I'm wondering if this can be generalized to a single "Major" change that is "Changing the layout of a well-defined type" or something like that.

Instead of trying to have separate sections for every repr modification that can break,
the single section can then enumerate the different ways changes to the repr attribute can cause problems.

For example, list out what kinds of types have well-defined layouts:

• A struct with all public fields with #[repr(C)] or #[repr(transparent)].
• A union with all public fields with #[repr(C)].
• An enum with #[repr(C)]or#[repr(transparent)]`.
• An enum with a primitive representation.

And say any change to the repr is a breaking change for those.

And also mention: Any change to align or packed that changes the alignment or layout.

(Note: I haven't thought about the specifics of what I wrote above, as I just quickly wrote them down, they may need adjustment.)

[bors]

☔ The latest upstream changes (presumably #12339) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

I'm going to close as repr and layout guidelines are now documented via #12169. Thank you for getting this started, though!