Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C-STRUCT-PRIVATE guideline incomplete #203

Open
gnzlbg opened this issue Sep 25, 2019 · 4 comments
Open

C-STRUCT-PRIVATE guideline incomplete #203

gnzlbg opened this issue Sep 25, 2019 · 4 comments
Labels
accepted An amendment that's been accepted and can be applied amendment Amendments to existing guidelines

Comments

@gnzlbg
Copy link

gnzlbg commented Sep 25, 2019

The C-STRUCT-PRIVATE recommendation talks about a single struct field being public or private.

It does not mention what happens when all fields of a struct are public, e.g., in that particular case, users can exhaustively pattern-match the struct, which means that adding a new field to the struct, even a private one, is a breaking change.

Now that enums are also going to be able to have private fields, I think this guideline should also drop the "struct" part. Maybe the guideline could be replaced with something like this:

AGGREGATE-PRIVATE-FIELD

If an aggregate field is public, then making it private or changing its type or name is a semver breaking change. If an aggregate only contains public fields, then adding any field to it, including private fields, is a semver breaking change. Aggregates can be future-proofed code against these semver breaking changes by adding a private field or by using the #[non_exhaustive] attribute. Note that private fields are not allowed in tuples or arrays, that is, adding a field to a public tuple or changing the length of a public array is always a semver breaking change.

Other changes to aggregates that only contain public fields, like, for example, changing the field order, are also semver breaking changes for some representations like repr(C), which allow users to use the aggregate in, e.g., FFI, where other aspects of the type like its call ABI, field order, etc. are often relied on.

There was a bit of discussion about this here: rust-lang/rust#44109 (comment)

@gnzlbg
Copy link
Author

gnzlbg commented Sep 25, 2019

cc @ibabushkin - this might be relevant for rust-semverver.

@KodrAus KodrAus added amendment Amendments to existing guidelines I-nominated Indicates that an issue has been nominated for discussion during a team meeting. labels Dec 21, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Dec 21, 2020

This seems like a reasonable amendment to me. We should probably include some description of what an aggregate is so users browsing the guidelines know we're talking about enums or structs.

@KodrAus KodrAus added disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed I-nominated Indicates that an issue has been nominated for discussion during a team meeting. labels Dec 22, 2020
@KodrAus

This comment has been minimized.

@KodrAus KodrAus added T-libs Relevant to the libraries subteam, which will review and decide on the PR/issue. accepted An amendment that's been accepted and can be applied and removed disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs Relevant to the libraries subteam, which will review and decide on the PR/issue. labels Dec 22, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Dec 22, 2020

We’re still getting an idea of what kinds of amendments need to run through an FCP or not. On a second reading, this update seems reasonable and uncontentious enough to me to not need one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted An amendment that's been accepted and can be applied amendment Amendments to existing guidelines
Projects
None yet
Development

No branches or pull requests

2 participants