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

Document that adding #[non_exhaustive] on existing items is breaking. #10877

Merged
merged 7 commits into from
May 1, 2023

Conversation

obi1kenobi
Copy link
Member

What does this PR try to resolve?

Adding #[non_exhaustive] to an existing struct, enum, or variant is almost always a breaking change and requires a major version bump for semver purposes. This PR adds a section to the semver reference page that describes this and provides examples showing how #[non_exhaustive] can break code.

Additional information

Adding #[non_exhaustive] to a unit struct currently has no effect on whether that struct can be constructed in downstream crates. This is inconsistent with the behavior of #[non_exhaustive] on unit enum variants, which may not be constructed outside their own crate. This might be due to a similar underlying cause as: rust-lang/rust#78586

The confusing "variant is private" error messages for non-exhaustive unit and tuple variants are a known issue tracked in: rust-lang/rust#82788

Checking for the struct portion of this semver rule is done in: obi1kenobi/cargo-semver-checks#4

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2022
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment on lines 1125 to 1126
- Pattern matching on non-exhaustive enums always requires
a wildcard (`_`) arm.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be worded a little strongly. Pedantically this isn't correct, as it is possible pattern match without _. For example:

match std::num::IntErrorKind::InvalidDigit {
    // Identifier patterns are exhaustive.
    x => {}
}

match (1, std::num::IntErrorKind::InvalidDigit) {
    // Within another type, other patterns can count for exhaustiveness.
    (x, ..) => {}
}

I would just say something like "Pattern matching on non-exhaustive struct variants requires .. and matching on enums does not count towards exhaustiveness."

using [struct literal] syntax, including [functional update syntax].
- Pattern matching on non-exhaustive enums always requires
a wildcard (`_`) arm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, a new rule is being added in 1.65 that non-exhaustive enums cannot be casted with as (see rust-lang/reference#1249).

I'm reluctant to try to repeat the rules in this document since it will be difficult to keep in sync, or there may be subtle differences. However, this is useful information so it should probably be ok to repeat them here.

@ehuss ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2022
@rustbot rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label May 1, 2023
@ehuss
Copy link
Contributor

ehuss commented May 1, 2023

I pushed a change with the suggestions I had for clarifying what changes when non_exhaustive is added.

Asking for another reviewer just to make sure that the wording here looks good to someone else.

@rustbot ready

r? weihanglo

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels May 1, 2023
@rustbot rustbot assigned weihanglo and unassigned ehuss May 1, 2023
@obi1kenobi
Copy link
Member Author

Thanks for taking this over, sorry for dropping the ball on it! The wording looks great to me.

@weihanglo
Copy link
Member

Looks great to me. Easy to catch up.

@bors r+

@bors
Copy link
Contributor

bors commented May 1, 2023

📌 Commit f18f7f1 has been approved by weihanglo

It is now in the queue for this repository.

@bors 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 May 1, 2023
@bors
Copy link
Contributor

bors commented May 1, 2023

⌛ Testing commit f18f7f1 with merge c455de9...

@bors
Copy link
Contributor

bors commented May 1, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing c455de9 to master...

@bors bors merged commit c455de9 into rust-lang:master May 1, 2023
@obi1kenobi obi1kenobi deleted the adding-non-exhaustive branch May 2, 2023 00:00
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2023
Update cargo

16 commits in 9e586fbd8b931494067144623b76c37d213b1ab6..ac84010322a31f4a581dafe26258aa4ac8dea9cd
2023-04-25 22:09:11 +0000 to 2023-05-02 13:41:16 +0000
- docs(registry): Further specify owner-remove response (rust-lang/cargo#12056) (rust-lang/cargo#12068)
- Remove repeated definite articles (rust-lang/cargo#12067)
- Document that adding `#[non_exhaustive]` on existing items is breaking. (rust-lang/cargo#10877)
- docs(commands): add missed preposition (rust-lang/cargo#12073)
- Fix warning with unused mut (rust-lang/cargo#12065)
- chore: move build-man workflow away from shell (rust-lang/cargo#12048)
- feat: Add `-Zmsrv-policy` feature flag (rust-lang/cargo#12043)
- chore: new xtask to check stale paths in autolabel defintions (rust-lang/cargo#12051)
- cargo-tree: Handle -e no-proc-macro when building the graph (rust-lang/cargo#12044)
- chore: update trigger_files in autolabel (rust-lang/cargo#12052)
- fix broken markdown in docs (rust-lang/cargo#12049)
- home: fix & enhance documentation (rust-lang/cargo#12047)
- chore: Mark unpublished crates as such (rust-lang/cargo#12045)
- Include rust-version in publish request (rust-lang/cargo#12041)
- chore(xtask): Add `cargo xtask unpublished` (rust-lang/cargo#12039)
- docs(ref): Specify 'rust_version' in Index format (rust-lang/cargo#12040)

r? `@ghost`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request May 3, 2023
Update cargo

16 commits in 9e586fbd8b931494067144623b76c37d213b1ab6..ac84010322a31f4a581dafe26258aa4ac8dea9cd
2023-04-25 22:09:11 +0000 to 2023-05-02 13:41:16 +0000
- docs(registry): Further specify owner-remove response (rust-lang/cargo#12056) (rust-lang/cargo#12068)
- Remove repeated definite articles (rust-lang/cargo#12067)
- Document that adding `#[non_exhaustive]` on existing items is breaking. (rust-lang/cargo#10877)
- docs(commands): add missed preposition (rust-lang/cargo#12073)
- Fix warning with unused mut (rust-lang/cargo#12065)
- chore: move build-man workflow away from shell (rust-lang/cargo#12048)
- feat: Add `-Zmsrv-policy` feature flag (rust-lang/cargo#12043)
- chore: new xtask to check stale paths in autolabel defintions (rust-lang/cargo#12051)
- cargo-tree: Handle -e no-proc-macro when building the graph (rust-lang/cargo#12044)
- chore: update trigger_files in autolabel (rust-lang/cargo#12052)
- fix broken markdown in docs (rust-lang/cargo#12049)
- home: fix & enhance documentation (rust-lang/cargo#12047)
- chore: Mark unpublished crates as such (rust-lang/cargo#12045)
- Include rust-version in publish request (rust-lang/cargo#12041)
- chore(xtask): Add `cargo xtask unpublished` (rust-lang/cargo#12039)
- docs(ref): Specify 'rust_version' in Index format (rust-lang/cargo#12040)

r? `@ghost`
@ehuss ehuss added this to the 1.71.0 milestone May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants