Document layout SemVer compatibility. #12169

ehuss · 2023-05-23T01:49:20Z

This adds some documentation about whether or not alignment, layout, or size changes are SemVer-compatible.

src/doc/src/reference/semver.md

joshtriplett · 2023-06-13T20:58:18Z

src/doc/src/reference/semver.md

+
+It is a breaking change to change the alignment, layout, or size of a type that was previously well-defined.
+
+In general, nominal types that use the [the default representation] do not have a well-defined alignment, layout, or size.


What distinction is "nominal types" meant to make, here? Unnamed types, like tuples, that use the default representation (which is the only option we provide for them as far as I know) also don't have a well-defined alignment, layout, or size.

I was mostly trying to emphasize that this discussion was around types that accept the #[repr] attribute. However, it is not important and I dropped the word.

joshtriplett · 2023-06-13T21:04:49Z

src/doc/src/reference/semver.md

+
+Some examples of changes that are not a breaking change are (assuming no other rules in this guide are violated):
+
+* Adding, removing, or changing fields of a default representation struct, union, or enum.


This one is often going to be a breaking change, and while "assuming no other rules" above does cover that, leading with this one still seems potentially misleading. Should this say something along the lines of "assuming the changes do not otherwise break code using the types, such as code initializing an object of the type".

Hm, yea, I was reluctant to try to repeat all the caveats, and defer to the comment above about assuming no other rules are broken. But you are correct that reading this on its own is very misleading. I have added some caveats, with links to the sections that explain them in more detail.

joshtriplett · 2023-06-13T21:06:19Z

src/doc/src/reference/semver.md

+Some examples of changes that are not a breaking change are (assuming no other rules in this guide are violated):
+
+* Adding, removing, or changing fields of a default representation struct, union, or enum.
+* Adding variants to a default representation enum.


Likewise, this one is often going to be breaking, in the absence of non_exhaustive or an existing private variant. I don't want us to redundantly cover potentially breaking changes repeatedly, but I feel like this could use a bit of qualification (e.g. "Adding variants to a default representation enum, if the enum already has private variants or uses non_exhaustive")

Added qualification here, too, with links.

joshtriplett · 2023-06-13T21:28:55Z

src/doc/src/reference/semver.md

+  Note that this may be a breaking change since it may change the size and alignment of the type.
+  Care should be taken in this case.
+  Public fields may be added if there are private fields, or it is `non_exhaustive`, and the addition does not alter the layout of the other fields.
+* Adding variants to a `repr(C)` enum.


This is breaking if not non_exhaustive.

Same, added the caveat.

joshtriplett · 2023-06-13T21:29:19Z

src/doc/src/reference/semver.md

+* Adding, removing, or changing private fields of a `repr(C)` struct, union, or enum.
+  Note that this may be a breaking change since it may change the size and alignment of the type.
+  Care should be taken in this case.
+  Public fields may be added if there are private fields, or it is `non_exhaustive`, and the addition does not alter the layout of the other fields.


Private fields are also breaking if there aren't yet any private fields and the type isn't non_exhaustive.

Same, added some caveats.

ehuss · 2023-06-15T22:15:50Z

Thanks for the review!

I decided to go the route of splitting each point into a separate section. The #type-layout section provides an overview of all the rules, with links to the specific section. Each section can now go into more detail, and also shows examples (which highlights some of the more esoteric behaviors).

I was wondering if you had any comments on the two TODO comments. I feel like this is maybe going too far into the minutia, so I'm fine ignoring them for now. Sometimes people will just need to use their own judgement.

ehuss · 2023-07-05T18:38:11Z

@joshtriplett Just checking in to see if you'll be able to review the updates.

the8472 · 2023-07-12T11:13:49Z

src/doc/src/reference/semver.md

+In these cases, it may be safe to make changes to the types, though care should be exercised.
+For example, types with private fields that do not otherwise document their alignment, layout, or size guarantees cannot be relied upon by external crates since the public API does not fully define the alignment, layout, or size of the type.
+
+A common example where a type with *private* fields is well-defined is a type with a single private field with a generic type, using `repr(transparent)`, and which is documented as being transparent to the generic type.


"documented as being transparent" is ambiguous since rustdoc shows #[repr(transparent)] on some types even when they have private fields.

Thanks for taking a look! My intent wasn't that #[repr(transparent)] alone in the documentation doesn't mean it is "documented". I was pointing towards the actual text of the documentation discussing the guarantees it provides (for example, the linked UnsafeCell has an entire section about memory layout). If there is a private field, rustdoc by itself doesn't provide enough information to know what type the field is, and thus there is no way to know what the layout of the type is. The prose needs to explicitly indicate that the private field is just T, and that it is transparent to it (or some other explicit guarantee). I adjusted the text a little to try to make that clearer.

bors · 2023-07-12T13:08:14Z

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

obi1kenobi

This is great, I'm strongly in support and I learned a lot from reading it. Thanks for putting it together!

I look forward to updating our existing cargo-semver-checks lints to point to the new sections, as well as adding new lints for things we don't already check.

If you have a moment to spare, I'd love your guidance on a few edge cases related to programmatically determining whether alignment changes are breaking or not — I put details inside PR comments in the relevant spots.

obi1kenobi · 2023-08-02T16:59:15Z

src/doc/src/reference/semver.md

+#### Major: Changing the value N of `repr(packed(N))` if that changes the alignment or layout
+
+It is a breaking change to change the value of N of `repr(packed(N))` if that changes the alignment or layout.


Tangent: might be nice to expand the repr(packed) nomicon docs here to mention that repr(packed) can take an argument. This is the first time I learned that's the case, and I've probably done a bit more digging through the reprs docs than average Rustaceans.

The Type Layout page's section on alignment modifiers of the reference also mentions it, but that's not the natural first place to go look up how repr(packed) can be used.

obi1kenobi · 2023-08-02T17:02:56Z

src/doc/src/reference/semver.md

+If the value `N` is lowered below the alignment of a public field, then that would break any code that attempts to take a reference of that field.
+
+Note that some changes to `N` may not change the alignment or layout, for example increasing it when the current value is already equal to the natural alignment of the type.


Tangent: any recommendations on how to write a high-quality, precise lint for this in cargo-semver-checks? Would love to lint this in a way that doesn't produce false-positives, but I don't feel I understand the nuances yet.

For example, if repr(packed(N)) is used on a type containing repr(Rust) values, how can we best (ideally, programmatically) figure out whether we've hit the "N lowered below alignment of public field" criterion if said fields are repr(Rust)? I'm confused because based on these additions I think the repr(Rust) values might not have well-defined layout or alignment.

Computing the alignment and layout is a nontrivial thing to do. It is target-specific, and I think it would be a huge effort to duplicate everything the compiler does. Some things, like ctest2 create a Rust file with the structure and some assertions to check alignment and layout. The memoffset crate and the standard library functions like align_of_val can be used for gaining some information about a structure. -Zprint-type-sizes can be used to print the layout information (but only for human-readable debugging).

Overall, I don't think it is feasible for cargo-semver-checks to compute the actual layout and alignment.

For this specific clause, I probably wouldn't have any automated checks to determine if alignment changes. I would just say "any change to N is breaking".

You are correct that repr(Rust) does not have a well-defined layout or alignment. That means anything that contains those repr(Rust) types also does not have a well-defined layout.

Thanks, that's super useful and makes sense.

You are correct that repr(Rust) does not have a well-defined layout or alignment. That means anything that contains those repr(Rust) types also does not have a well-defined layout.

Love this! In fact, I think having that statement ("anything that contains repr(Rust) types does not have a well-defined layout") in the doc as an additional clarification might be valuable from a pedagogical point of view. Just my 2 cents.

src/doc/src/reference/semver.md

ehuss · 2023-08-06T16:07:49Z

Thanks for taking a look @obi1kenobi!

src/doc/src/reference/semver.md

obi1kenobi · 2023-08-06T19:11:13Z

src/doc/src/reference/semver.md

+* Adding variants to a `repr(C)` enum, if the enum uses `non_exhastive`.
+  See [repr-c-enum-variant-new](#repr-c-enum-variant-new).


The reordering concern I mentioned above seems to pose an interesting question here as well: if the new variants of the #[repr(C)] #[non_exhaustive] enum are added in the middle, they may change the (externally-observable) discriminants of existing variants. Might that be a major change?

Adding new variants after all existing variants seems safe, though.

I think whether or not changing enum discriminant values is specific to the situation. I see that as a similar situation to whether or not changing a const is a breaking change. I think in most cases the intent is to have some abstract representation of some value, but the exact value of the const (or enum discriminant) doesn't matter. https://internals.rust-lang.org/t/should-we-be-more-strict-with-const-and-semver/14302 contains more discussion on this. I'm not sure what the rule should be for that, but I added it to #8736 to track it.

obi1kenobi · 2023-08-16T01:03:28Z

src/doc/src/reference/semver.md

+* Removing `repr(C)` from a struct, union, or enum.
+  See [repr-c-remove](#repr-c-remove).


Is there broad consensus on repr being always public, or can it sometimes be private?

A few folks involved with the Rust project recently mentioned that in their view, it's possible for repr(C) and other repr attributes to be public, and that a removal of repr(C) is not necessarily a semver-major change.

This is the PR in question where that discussion was taking place: rust-lang/rust#90435

I think I misunderstood something, we're good.

src/doc/src/reference/semver.md

joshtriplett · 2023-08-17T20:41:45Z

src/doc/src/reference/semver.md

+* Adding `repr(align)` to a struct, union, or enum.
+  See [repr-align-add](#repr-align-add).


This is true, but given how rare and suboptimal repr(packed) is, I suspect in practice some crates might not treat this as a breaking change.

Also, it seems like in theory we could remove the incompatibility between the two, though I doubt that's a priority.

Yea, there's a TODO comment in that section about how unlikely that is to be a problem. I'm not sure how best to handle cases like that. There are more shades than I expected in the spectrum from "definitely compatible" to "definitely not compatible", and it is hard to know how much of a stickler to be.

I think this is something to revisit and I'm going to leave the TODO in there for now.

joshtriplett · 2023-08-17T20:43:41Z

src/doc/src/reference/semver.md

+* Changing the order of public fields of a `repr(C)` type.
+  See [repr-c-shuffle](#repr-c-shuffle).


It seems like, as described, this is only a breaking change if the type has no private fields? Or should we consider the offsets of public fields to be stable even if there are private fields?

Nevermind, the disclaimer at the end of this list saying "In some cases, types with a repr attribute may not have an alignment, layout, or size that is well-defined." covers this well enough. (Might be worth mentioning that in the specific section, but not blocking on this.)

I rearranged the text to put the caveat higher. It can be challenging to be precise, but keep things concise enough to read without repeating all the qualifications.

joshtriplett · 2023-08-17T20:47:29Z

r=me with the (very minor) wording nits fixed.

Co-authored-by: the8472 <the8472@users.noreply.github.com>

ehuss · 2023-08-18T01:26:08Z

Thanks for the review!
@bors r=joshtriplett

bors · 2023-08-18T01:26:13Z

📌 Commit 5dc86dc has been approved by joshtriplett

It is now in the queue for this repository.

bors · 2023-08-18T01:26:18Z

⌛ Testing commit 5dc86dc with merge 5633e2a...

bors · 2023-08-18T02:31:26Z

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 5633e2a to master...

Update cargo 15 commits in 7c3904d6c3ed54e8a413023519b55a536ad44d5b..80eca0e58fb2ff52c1e94fc191b55b37ed73e0e4 2023-08-14 20:11:43 +0000 to 2023-08-19 00:52:06 +0000 - chore: Downgrade serde below the binary blob (rust-lang/cargo#12528) - Improve error message for when no credential providers are available (rust-lang/cargo#12526) - Fix typo: "use" -> "used" (rust-lang/cargo#12522) - Document layout SemVer compatibility. (rust-lang/cargo#12169) - Make cargo-credential-gnome-secret built-in as cargo:libsecret (rust-lang/cargo#12521) - login: allow passing additional args to provider (rust-lang/cargo#12499) - cargo-credential-gnome-secret: dynamically load libsecret (rust-lang/cargo#12518) - credential-providers: make 1password no longer built-in (rust-lang/cargo#12507) - Print environment variables for `cargo run` in extra verbose mode (rust-lang/cargo#12498) - chore(cargo-util): bump version to 0.2.6 (rust-lang/cargo#12517) - credential: rename cargo:basic to cargo:token-from-stdout (rust-lang/cargo#12512) - fix(xtask-bump-check): query by package name to detect changes (rust-lang/cargo#12513) - ci: use pull request head commit whenever possible (rust-lang/cargo#12508) - Update hermit-abi (rust-lang/cargo#12504) - Crate checksum lookup query should match on semver build metadata (rust-lang/cargo#11447) r? ghost

rustbot assigned joshtriplett May 23, 2023

rustbot added A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2023

joshtriplett reviewed Jun 13, 2023

View reviewed changes

src/doc/src/reference/semver.md Show resolved Hide resolved

joshtriplett reviewed Jun 13, 2023

View reviewed changes

rustbot added the A-semver Area: semver specifications, version matching, etc. label Jun 15, 2023

ehuss marked this pull request as ready for review June 15, 2023 22:15

ehuss force-pushed the semver-repr branch from d105f9d to 36c6b0a Compare June 15, 2023 22:31

the8472 reviewed Jul 12, 2023

View reviewed changes

ehuss force-pushed the semver-repr branch from 36c6b0a to e347888 Compare July 12, 2023 14:12

obi1kenobi approved these changes Aug 2, 2023

View reviewed changes

obi1kenobi mentioned this pull request Aug 2, 2023

Tracking issue: Additional checks, both semver and non-semver obi1kenobi/cargo-semver-checks#5

Open

ehuss force-pushed the semver-repr branch from ae5cb5f to df9ff37 Compare August 6, 2023 16:08

the8472 reviewed Aug 6, 2023

View reviewed changes

src/doc/src/reference/semver.md Outdated Show resolved Hide resolved

src/doc/src/reference/semver.md Outdated Show resolved Hide resolved

obi1kenobi reviewed Aug 6, 2023

View reviewed changes

arlosi approved these changes Aug 14, 2023

View reviewed changes

obi1kenobi reviewed Aug 16, 2023

View reviewed changes

joshtriplett reviewed Aug 17, 2023

View reviewed changes

src/doc/src/reference/semver.md Outdated Show resolved Hide resolved

joshtriplett reviewed Aug 17, 2023

View reviewed changes

src/doc/src/reference/semver.md Outdated Show resolved Hide resolved

joshtriplett reviewed Aug 17, 2023

View reviewed changes

src/doc/src/reference/semver.md Outdated Show resolved Hide resolved

joshtriplett reviewed Aug 17, 2023

View reviewed changes

ehuss and others added 10 commits August 17, 2023 18:18

Document layout semver compatibility.

3cff9d5

Apply some edits from review to try to clarify things.

661287e

Document the special code block annotations.

4c1d8f1

Support hidden lines.

96da3c4

Split out all layout rules into separate sections with examples.

406f434

Clarify what it means "to document" a transparent type.

99d2aa9

Apply changes from review from obi1kenobi.

9e274d0

Add "reordering" to list of changes.

58aef64

Co-authored-by: the8472 <the8472@users.noreply.github.com>

Updates from review from joshtriplett

a2b36f6

Use header IDs

5dc86dc

ehuss force-pushed the semver-repr branch from 851fd71 to 5dc86dc Compare August 18, 2023 01:22

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2023

ehuss mentioned this pull request Aug 18, 2023

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

Closed

bors merged commit 5633e2a into rust-lang:master Aug 18, 2023
18 checks passed

weihanglo mentioned this pull request Aug 18, 2023

Update cargo rust-lang/rust#114979

Merged

ehuss added this to the 1.73.0 milestone Aug 22, 2023


		It is a breaking change to change the alignment, layout, or size of a type that was previously well-defined.

		In general, nominal types that use the [the default representation] do not have a well-defined alignment, layout, or size.


		Some examples of changes that are not a breaking change are (assuming no other rules in this guide are violated):

		* Adding, removing, or changing fields of a default representation struct, union, or enum.

		#### Major: Changing the value N of `repr(packed(N))` if that changes the alignment or layout

		It is a breaking change to change the value of N of `repr(packed(N))` if that changes the alignment or layout.

		If the value `N` is lowered below the alignment of a public field, then that would break any code that attempts to take a reference of that field.

		Note that some changes to `N` may not change the alignment or layout, for example increasing it when the current value is already equal to the natural alignment of the type.

		* Adding variants to a `repr(C)` enum, if the enum uses `non_exhastive`.
		See [repr-c-enum-variant-new](#repr-c-enum-variant-new).

		* Removing `repr(C)` from a struct, union, or enum.
		See [repr-c-remove](#repr-c-remove).

		* Adding `repr(align)` to a struct, union, or enum.
		See [repr-align-add](#repr-align-add).

		* Changing the order of public fields of a `repr(C)` type.
		See [repr-c-shuffle](#repr-c-shuffle).

Document layout SemVer compatibility. #12169

Document layout SemVer compatibility. #12169

Conversation

ehuss commented May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Jun 15, 2023

ehuss commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 12, 2023

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Aug 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtriplett commented Aug 17, 2023

ehuss commented Aug 18, 2023

bors commented Aug 18, 2023

bors commented Aug 18, 2023

bors commented Aug 18, 2023

ehuss commented May 23, 2023 •

edited

Loading