-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Stabilize min_exhaustive_patterns
#122792
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
86b2747
to
3a7fd6c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
98f42a7
to
be31f20
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4c3c0fc
to
ca10919
Compare
This comment has been minimized.
This comment has been minimized.
ca10919
to
b86a6f8
Compare
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in src/tools/clippy cc @rust-lang/clippy The Miri subtree was changed cc @rust-lang/miri |
Finished benchmarking commit (8291d68): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 0.5%, secondary -2.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 1.5%, secondary 2.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 760.23s -> 757.146s (-0.41%) |
@jplatte apologies, it seems I entirely overlooked your comment at the time. The compiler doesn't do the same, I doubt it is even aware of |
Not so much, actually. I think churn is much more likely to happen for libraries that don't declare It's more that I think people who do declare a
What would be the harm in making it allow-by-default in current editions though? It's not that long until the next edition anyways. I think this lint will cause churn for some library writers (in the form of CI breaking, because warnings are set to deny), and I don't think the benefits of slightly shorter code is actually worth it (even though I'm actually pretty excited about this feature shipping!). |
Oh yeah, I didn't think of that.
None indeed. The feature is available to users who want it even without the warning. I'm not sure how to call the shots here though. |
Just to chime in here, I noticed in hyper's CI that this started failing (yes, I deny warnings in CI, it's the best way I notice changes). What's interesting is that the warning seems to be emitted in the same version that it even became possible to omit the pattern in the first place. So, in order to support older rust version (even just current stable, 1.80), I can't appease the warning. I can only turn it off. Would it make sense to promote the lint to a warning a few releases later? For the record, I have code that's matching on a |
I filed issue #129031 to give this matter more visibility. |
@Nadrieril @fee1-dead seems this has a negative impact on the performance of coherence checking which I imagined is expected, right? I'm not sure if there's anything we would want to do given that there's simply more work happening. Thoughts? |
@rylev yeah, at the very least it's calculating inhabitedness of essentially all types involed in patterns, which has unavoidable cost. |
…, 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`
…, 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``
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``
…ble, r=compiler-errors Don't warn empty branches unreachable for now The [stabilization](rust-lang#122792) of `min_exhaustive_patterns` updated the `unreachable_pattern` lint to trigger on empty arms too. This has caused some amount of churn, and imposes an unjoyful `#[allow(unreachable_patterns)]` onto library authors who want to stay backwards-compatible. While I think the lint should eventually cover these cases, for transition's sake I'd prefer to revert linting to what it was prior to stabilization, at least for now. Fixes rust-lang#129031. r? `@compiler-errors`
I think I found either a bug, or a confusing error: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=08d6bead0bfb94e25a784eb88f8d059f let x: Result<(), Infallible> = Ok(());
match *&x {
Ok(()) => { println!("Must be OK"); }
} Errors (it doesn't seem to consider
As far as I know, I would consider that to be matching by value, but it's possible there's some "place vs value" theory that means it's not technically by value. |
Indeed this isn't by-value because the Tbf the line above says "the matched value is of type" when it might be less confusing to say "the matched place is of type". |
…ble, r=compiler-errors Don't warn empty branches unreachable for now The [stabilization](rust-lang#122792) of `min_exhaustive_patterns` updated the `unreachable_pattern` lint to trigger on empty arms too. This has caused some amount of churn, and imposes an unjoyful `#[allow(unreachable_patterns)]` onto library authors who want to stay backwards-compatible. While I think the lint should eventually cover these cases, for transition's sake I'd prefer to revert linting to what it was prior to stabilization, at least for now. Fixes rust-lang#129031. r? `@compiler-errors`
…ble, r=compiler-errors Don't warn empty branches unreachable for now The [stabilization](rust-lang#122792) of `min_exhaustive_patterns` updated the `unreachable_pattern` lint to trigger on empty arms too. This has caused some amount of churn, and imposes an unjoyful `#[allow(unreachable_patterns)]` onto library authors who want to stay backwards-compatible. While I think the lint should eventually cover these cases, for transition's sake I'd prefer to revert linting to what it was prior to stabilization, at least for now. Fixes rust-lang#129031. r? ``@compiler-errors``
Rollup merge of rust-lang#129103 - Nadrieril:dont-warn-empty-unreachable, r=compiler-errors Don't warn empty branches unreachable for now The [stabilization](rust-lang#122792) of `min_exhaustive_patterns` updated the `unreachable_pattern` lint to trigger on empty arms too. This has caused some amount of churn, and imposes an unjoyful `#[allow(unreachable_patterns)]` onto library authors who want to stay backwards-compatible. While I think the lint should eventually cover these cases, for transition's sake I'd prefer to revert linting to what it was prior to stabilization, at least for now. Fixes rust-lang#129031. r? ``@compiler-errors``
Stabilisation report
I propose we stabilize the
min_exhaustive_patterns
language feature.With this feature, patterns of empty types are considered unreachable when matched by-value. This allows:
This is a subset of the long-unstable
exhaustive_patterns
feature. That feature is blocked because omitting empty patterns is tricky when not matched by-value. This PR stabilizes the by-value case, which is not tricky.The not-by-value cases (behind references, pointers, and unions) stay as they are today, e.g.
The consequence on existing code is some extra "unreachable pattern" warnings. This is fully backwards-compatible.
Comparison with today's rust
This proposal only affects match checking of empty types (i.e. types with no valid values). Non-empty types behave the same with or without this feature. Note that everything below is phrased in terms of
match
but applies equallly toif let
and other pattern-matching expressions.To be precise, a visibly empty type is:
!
;#[non_exhaustive]
annotation);[T; N]
withN != 0
andT
visibly empty;(An extra change was proposed below: that we ignore #[non_exhaustive] for structs since adding fields cannot turn an empty struct into a non-empty one)
For normal types, exhaustiveness checking requires that we list all variants (or use a wildcard). For empty types it's more subtle: in some cases we require a
_
pattern even though there are no valid values that can match it. This is where the difference lies regarding this feature.Today's rust
Under today's rust, a
_
is required for all empty types, except specifically: if the matched expression is of type!
(the never type) orEmptyEnum
(whereEmptyEnum
is an enum with no variants), then the_
is not required.After this PR
After this PR, a pattern of an empty type can be omitted if (and only if):
!
orEmptyEnum
(like before);In all other cases, a
_
is required to match on an empty type.Documentation
The reference does not say anything specific about exhaustiveness checking, hence there is nothing to update there. The nomicon does, I opened rust-lang/nomicon#445 to reflect the changes.
Tests
The relevant tests are in
tests/ui/pattern/usefulness/empty-types.rs
.Unresolved Questions
None that I know of.
try-job: dist-aarch64-apple