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

Non-exhaustive structs may be empty #128934

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Aug 10, 2024

This is a follow-up to a discrepancy noticed in #122792: today, the following struct is considered inhabited (non-empty) outside its defining crate:

#[non_exhaustive]
pub struct UninhabitedStruct {
    pub never: !,
    // other fields
}

#[non_exhaustive] on a struct should mean that adding fields to it isn't a breaking change. There is no way that adding fields to this struct could make it non-empty since the never field must stay and is inconstructible. I suspect this was implemented this way due to confusion with #[non_exhaustive] enums, which indeed should be considered non-empty outside their defining crate.

I propose that we consider such a struct uninhabited (empty), just like it would be without the #[non_exhaustive] annotation.

Code that doesn't pass today and will pass after this:

// In a different crate
fn empty_match_on_empty_struct<T>(x: UninhabitedStruct) -> T {
    match x {}
}

This is not a breaking change.

r? @compiler-errors

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 10, 2024
@Nadrieril Nadrieril added I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. F-non_exhaustive `#![feature(non_exhaustive)]` A-patterns Relating to patterns and pattern matching A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns labels Aug 10, 2024
@rust-log-analyzer

This comment has been minimized.

@compiler-errors compiler-errors added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2024
@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 14, 2024
@traviscross traviscross added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 14, 2024
@rust-lang rust-lang deleted a comment from rfcbot Aug 14, 2024
@rust-lang rust-lang deleted a comment from rfcbot Aug 14, 2024
@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 14, 2024
@traviscross
Copy link
Contributor

@rfcbot fcp merge

We discussed this in the triage meeting today. This sounded good to us. Let's do it via FCP since there's a one-way door here.

@pnkfelix
Copy link
Member

Hey @Nadrieril , I skimmed over the PR and noticed that all the test cases touched here that involve never-typed field also had that same field always marked as pub.

Is there a pre-existing test for the case of a non-exhaustive struct with a non-pub field that is empty? (A situation I infer to be a case which we wish to continue erroring on...)

@rust-lang rust-lang deleted a comment from rfcbot Aug 14, 2024
@rust-lang rust-lang deleted a comment from rfcbot Aug 14, 2024
@rfcbot
Copy link

rfcbot commented Aug 14, 2024

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 14, 2024
@Nadrieril
Copy link
Member Author

Hey @Nadrieril , I skimmed over the PR and noticed that all the test cases touched here that involve never-typed field also had that same field always marked as pub.

Is there a pre-existing test for the case of a non-exhaustive struct with a non-pub field that is empty? (A situation I infer to be a case which we wish to continue erroring on...)

There isn't a test for non_exaustive + private empty field anymore. You can see that I changed it because I didn't feel it was testing the interesting case (which is non_exhaustive + public empty field, i.e. what the FCP is about).

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 25, 2024
@rfcbot
Copy link

rfcbot commented Aug 25, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 29, 2024
@Nadrieril
Copy link
Member Author

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2024
@Nadrieril Nadrieril removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Sep 2, 2024
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 3, 2024

📌 Commit 6f6a6bc has been approved by compiler-errors

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 Sep 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 3, 2024
…, r=compiler-errors

Non-exhaustive structs may be empty

This is a follow-up to a discrepancy noticed in rust-lang#122792: today, the following struct is considered inhabited (non-empty) outside its defining crate:
```rust
#[non_exhaustive]
pub struct UninhabitedStruct {
    pub never: !,
    // other fields
}
```

`#[non_exhaustive]` on a struct should mean that adding fields to it isn't a breaking change. There is no way that adding fields to this struct could make it non-empty since the `never` field must stay and is inconstructible. I suspect this was implemented this way due to confusion with `#[non_exhaustive]` enums, which indeed should be considered non-empty outside their defining crate.

I propose that we consider such a struct uninhabited (empty), just like it would be without the `#[non_exhaustive]` annotation.

Code that doesn't pass today and will pass after this:
```rust
// In a different crate
fn empty_match_on_empty_struct<T>(x: UninhabitedStruct) -> T {
    match x {}
}
```

This is not a breaking change.

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
 - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
 - rust-lang#128934 (Non-exhaustive structs may be empty)
 - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
 - rust-lang#129706 (Rename dump of coroutine by-move-body to be more consistent, fix ICE in dump_mir)
 - rust-lang#129896 (do not attempt to prove unknowable goals)
 - rust-lang#129926 (Move `SanityCheck` and `MirPass`)
 - rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
 - rust-lang#129930 (include 1.80.1 release notes on master)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
 - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
 - rust-lang#128934 (Non-exhaustive structs may be empty)
 - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
 - rust-lang#129863 (update comment regarding TargetOptions.features)
 - rust-lang#129896 (do not attempt to prove unknowable goals)
 - rust-lang#129926 (Move `SanityCheck` and `MirPass`)
 - rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
 - rust-lang#129930 (include 1.80.1 release notes on master)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
 - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
 - rust-lang#128934 (Non-exhaustive structs may be empty)
 - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
 - rust-lang#129863 (update comment regarding TargetOptions.features)
 - rust-lang#129896 (do not attempt to prove unknowable goals)
 - rust-lang#129926 (Move `SanityCheck` and `MirPass`)
 - rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
 - rust-lang#129930 (include 1.80.1 release notes on master)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
 - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
 - rust-lang#128934 (Non-exhaustive structs may be empty)
 - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
 - rust-lang#129863 (update comment regarding TargetOptions.features)
 - rust-lang#129896 (do not attempt to prove unknowable goals)
 - rust-lang#129926 (Move `SanityCheck` and `MirPass`)
 - rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
 - rust-lang#129930 (include 1.80.1 release notes on master)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
 - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
 - rust-lang#128934 (Non-exhaustive structs may be empty)
 - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
 - rust-lang#129863 (update comment regarding TargetOptions.features)
 - rust-lang#129896 (do not attempt to prove unknowable goals)
 - rust-lang#129926 (Move `SanityCheck` and `MirPass`)
 - rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
 - rust-lang#129930 (include 1.80.1 release notes on master)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
 - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
 - rust-lang#128934 (Non-exhaustive structs may be empty)
 - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
 - rust-lang#129863 (update comment regarding TargetOptions.features)
 - rust-lang#129896 (do not attempt to prove unknowable goals)
 - rust-lang#129926 (Move `SanityCheck` and `MirPass`)
 - rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
 - rust-lang#129930 (include 1.80.1 release notes on master)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
 - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
 - rust-lang#128934 (Non-exhaustive structs may be empty)
 - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
 - rust-lang#129863 (update comment regarding TargetOptions.features)
 - rust-lang#129896 (do not attempt to prove unknowable goals)
 - rust-lang#129926 (Move `SanityCheck` and `MirPass`)
 - rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
 - rust-lang#129930 (include 1.80.1 release notes on master)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
 - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
 - rust-lang#128934 (Non-exhaustive structs may be empty)
 - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
 - rust-lang#129863 (update comment regarding TargetOptions.features)
 - rust-lang#129896 (do not attempt to prove unknowable goals)
 - rust-lang#129926 (Move `SanityCheck` and `MirPass`)
 - rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
 - rust-lang#129930 (include 1.80.1 release notes on master)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e7504ac into rust-lang:master Sep 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
Rollup merge of rust-lang#128934 - Nadrieril:fix-empty-non-exhaustive, r=compiler-errors

Non-exhaustive structs may be empty

This is a follow-up to a discrepancy noticed in rust-lang#122792: today, the following struct is considered inhabited (non-empty) outside its defining crate:
```rust
#[non_exhaustive]
pub struct UninhabitedStruct {
    pub never: !,
    // other fields
}
```

`#[non_exhaustive]` on a struct should mean that adding fields to it isn't a breaking change. There is no way that adding fields to this struct could make it non-empty since the `never` field must stay and is inconstructible. I suspect this was implemented this way due to confusion with `#[non_exhaustive]` enums, which indeed should be considered non-empty outside their defining crate.

I propose that we consider such a struct uninhabited (empty), just like it would be without the `#[non_exhaustive]` annotation.

Code that doesn't pass today and will pass after this:
```rust
// In a different crate
fn empty_match_on_empty_struct<T>(x: UninhabitedStruct) -> T {
    match x {}
}
```

This is not a breaking change.

r? ``@compiler-errors``
@Nadrieril Nadrieril deleted the fix-empty-non-exhaustive branch September 20, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns A-patterns Relating to patterns and pattern matching disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-non_exhaustive `#![feature(non_exhaustive)]` finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.