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

Tracking Issue for min_exhaustive_patterns #119612

Open
4 of 5 tasks
Nadrieril opened this issue Jan 5, 2024 · 17 comments
Open
4 of 5 tasks

Tracking Issue for min_exhaustive_patterns #119612

Nadrieril opened this issue Jan 5, 2024 · 17 comments
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-exhaustive_patterns `#![feature(exhaustive_patterns)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Nadrieril
Copy link
Member

Nadrieril commented Jan 5, 2024

This is a tracking issue for the experimental "minimal exhaustive patterns" feature (as per the T-lang experimental feature process).
The feature gate for the issue is #![feature(min_exhaustive_patterns)].

Proposal

Pattern-matching on empty types is tricky around unsafe code. For that reason, current stable rust conservatively requires arms for empty types in all but the simplest case. It has long been the intention to allow omitting empty arms when it's safe to do so. The exhaustive_patterns feature allows the omission of all empty arms, but hasn't been stabilized because that was deemed dangerous around unsafe code.

This feature aims to stabilize an uncontroversial subset of exhaustive_patterns. Namely: when we're matching on data by-value, then we allow empty arms to be omitted. E.g.:

let x: Result<T, !> = foo();
match x { // ok
    Ok(y) => ...,
}
let Ok(y) = x; // ok

In other cases (behind references, pointers or unions) we keep stable behavior i.e. we require arms for the empty cases. This is because these cases may be reachable without UB (either now or in the future, until T-opsem decides otherwise).

unsafe {
    let ptr: *const Result<u32, !> = ...;
    match *ptr {
        Ok(x) => { ... }
        Err(_) => { ... } // still required
    }
}
let foo: Result<u32, &!> = ...;
match foo {
    Ok(x) => { ... }
    Err(&_) => { ... } // still required
}

Note: never_patterns will make these remaining cases more convenient to handle. The two features nicely complement each other.

Comparison with stable 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 to if let and other pattern-matching expressions.

To be precise, an empty type is:

  • an enum with no variants;
  • the never type !;
  • a struct with a visible field of empty type (and no #[non_exhaustive] annotation);
  • en enum with all variants empty (and no #[non_exhaustive] annotation).

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 min_exhaustive_patterns.

Stable rust

Under stable rust, a _ is required for all empty types, except specifically: if the matched expression is of type ! (the never type) or EmptyEnum (where EmptyEnum is an enum with no variants), then the _ is not required.

let foo: Result<u32, !> = ...;
match foo {
    Ok(x) => ...,
    Err(_) => ..., // required
}
let foo: Result<u32, &!> = ...;
match foo {
    Ok(x) => ...,
    Err(_) => ..., // required
}
let foo: &! = ...;
match foo {
    _ => ..., // required
}
fn blah(foo: (u32, !)) {
    match foo {
        _ => ..., // required
    }
}
unsafe {
    let ptr: *const ! = ...;
    match *ptr {} // allowed
    let ptr: *const (u32, !) = ...;
    match *ptr {
        (x, _) => { ... } // required
    }
    let ptr: *const Result<u32, !> = ...;
    match *ptr {
        Ok(x) => { ... }
        Err(_) => { ... } // required
    }
}

min_exhaustive_patterns

With min_exhaustive_patterns, a pattern of an empty type can be omitted if (and only if):

  • the match scrutinee expression has type ! or EmptyEnum;
  • or the empty type is matched by value.

In all other cases, a _ is required to match on an empty type.

let foo: Result<u32, !> = ...;
match foo {
    Ok(x) => ..., // `Err` not required
}
let foo: Result<u32, &!> = ...;
match foo {
    Ok(x) => ...,
    Err(_) => ..., // required because `!` is under a dereference
}
let foo: &! = ...;
match foo {
    _ => ..., // required because `!` is under a dereference
}
fn blah(foo: (u32, !)) {
    match foo {} // allowed
}
unsafe {
    let ptr: *const ! = ...;
    match *ptr {} // allowed
    let ptr: *const (u32, !) = ...;
    match *ptr {
        (x, _) => { ... } // required because the matched place is under a (pointer) dereference
    }
    let ptr: *const Result<u32, !> = ...;
    match *ptr {
        Ok(x) => { ... }
        Err(_) => { ... } // required because the matched place is under a (pointer) dereference
    }
}

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

  1. A-exhaustiveness-checking F-exhaustive_patterns S-waiting-on-bors T-compiler
    compiler-errors
  2. F-exhaustive_patterns S-waiting-on-bors T-compiler
    compiler-errors
  3. F-exhaustive_patterns S-waiting-on-bors T-compiler
    compiler-errors
  4. F-exhaustive_patterns S-waiting-on-bors T-libs
    scottmcm
  5. F-exhaustive_patterns S-waiting-on-author T-lang disposition-merge finished-final-comment-period relnotes
    fee1-dead
@Nadrieril Nadrieril added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns F-exhaustive_patterns `#![feature(exhaustive_patterns)]` labels Jan 5, 2024
@Nadrieril
Copy link
Member Author

Nadrieril commented Jan 30, 2024

Hi T-lang! I would like to move forward with this experiment.

Summary of the feature

This feature allows users to omit match arms on empty types when the empty type is matched by-value. This is a guaranteed-sound subset of the exhaustive_patterns feature. This allows:

enum Void {}
let x: Result<T, Void> = foo();
match x { // ok
    Ok(y) => ...,
}
let Ok(y) = x; // ok

If x: &Result<T, Void> or x: Result<T, &Void> or we're matching behind a pointer/union then this changes nothing (i.e. we require a Err(_) arm like on stable rust). In contrast, exhaustive_patterns allows omitting match arms on empty types in all cases, which can cause UB. For more detail see the OP.

Because it's a well-behaved and useful subset, I'd like to move to stabilize it soon. We can think about the rest of exhaustive_patterns later. Never patterns are a likely part of the final plan, and they need more time (and would benefit from this being stabilized).

Proposal

If T-lang approves, I will:

  • mark the feature gate as non-incomplete (that's the part I feel I need approval for);
  • use min_exhaustive_patterns instead of exhaustive_patterns in rustc, std and tests wherever possible;
  • call for the testing of this feature;
  • make a stabilization PR, and ask for T-lang approval again.

@Nadrieril Nadrieril added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Jan 30, 2024
@Nadrieril
Copy link
Member Author

For the triage meeting I prepared step 1 and 2 here: #120742

It appears that the vast majority of in-tree use cases of exhaustive_patterns are covered by min_exhaustive_patterns, as hoped.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

This covers the majority of cases I've encountered myself. I'd be very excited to see this subset move forward. Let's do it!

@rfcbot
Copy link

rfcbot commented Feb 7, 2024

Team member @nikomatsakis 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 Feb 7, 2024
@scottmcm
Copy link
Member

scottmcm commented Feb 8, 2024

This sounds great. By-value-only covers the two places that I immediately think of for this:

  1. let Ok(x) = foo.try_into(); where it's using Error = !, and
  2. The matches that are used for residuals in the try_trait_v2 stuff.

So I'd be quite happy to start with that and worry about the rest afterwards.

@rfcbot reviewed

@scottmcm scottmcm closed this as completed Feb 8, 2024
@scottmcm scottmcm reopened this Feb 8, 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 Feb 8, 2024
@scottmcm
Copy link
Member

scottmcm commented Feb 8, 2024

Sorry, it's too late for me to be looking at things, apparently, if I can't even hit the right button 🤦

Restarting the FCP from #119612 (comment)
@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 8, 2024

Team member @scottmcm 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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 8, 2024
@rfcbot
Copy link

rfcbot commented Feb 13, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@Mark-Simulacrum
Copy link
Member

How do we treat a #[non_exhaustive] tagged enum? That should probably not be treated as empty even if it currently is, for the purposes of this feature. (Checking in playground is hard since it's a cross-crate feature).

@Nadrieril
Copy link
Member Author

Nadrieril commented Feb 13, 2024

Good point: for both #[non_exhaustive] and private fields we pretend the type is inhabited subject to the expected visibility rules. I've updated the OP with a definition of "empty" that specifies this.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

This is going into FCP, so we can unnominate.

Thanks to @Nadrieril for pushing this forward.

@rustbot rustbot removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Feb 14, 2024
@tmandry
Copy link
Member

tmandry commented Feb 17, 2024

@rustbot reviewed

@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 Feb 23, 2024
@rfcbot
Copy link

rfcbot commented Feb 23, 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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 23, 2024
…iler-errors

mark `min_exhaustive_patterns` as complete

This is step 1 and 2 of my [proposal](rust-lang#119612 (comment)) to move `min_exhaustive_patterns` forward. The vast majority of in-tree use cases of `exhaustive_patterns` are covered by `min_exhaustive_patterns`. There are a few cases that still require `exhaustive_patterns` in tests and they're all behind references.

r? `@ghost`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 23, 2024
…iler-errors

mark `min_exhaustive_patterns` as complete

This is step 1 and 2 of my [proposal](rust-lang#119612 (comment)) to move `min_exhaustive_patterns` forward. The vast majority of in-tree use cases of `exhaustive_patterns` are covered by `min_exhaustive_patterns`. There are a few cases that still require `exhaustive_patterns` in tests and they're all behind references.

r? ``@ghost``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 23, 2024
Rollup merge of rust-lang#120742 - Nadrieril:use-min_exh_pats, r=compiler-errors

mark `min_exhaustive_patterns` as complete

This is step 1 and 2 of my [proposal](rust-lang#119612 (comment)) to move `min_exhaustive_patterns` forward. The vast majority of in-tree use cases of `exhaustive_patterns` are covered by `min_exhaustive_patterns`. There are a few cases that still require `exhaustive_patterns` in tests and they're all behind references.

r? ``@ghost``
@Nadrieril
Copy link
Member Author

Nadrieril commented Feb 27, 2024

Call for Testing

Alright! The feature has now been marked as complete. Since #120742 it is also used within the compiler. I'd now like to propose a call for testing.

Testing this feature

To test this feature, enable #![feature(min_exhaustive_patterns)]. This feature makes pattern matching take into account empty types in many situations (for details about which, see the issue description). In practice, this means you can get extra "unreachable pattern" warnings, and code such as the following is now allowed:

enum Void {}

fn foo() -> Result<String, Void> { ... }

fn bar() {
   let Ok(s) = foo();
   // or
   match foo() {
      Ok(s) => ...,
   }
}

If you already use #![feature(exhaustive_patterns)], try to use min_exhaustive_patterns instead. This will be enough unless you use empty types behind references, pointers, or unions. If min_exhaustive_patterns doesn't cover your usecase (and this isn't a bug), you'll have to keep using exhaustive_patterns for now.

Feedback

If you find a bug or the compiler crashes, please file an issue and tag me.

If you the feature is not doing what you think wrt patterns of empty types, have a look at the feature description first, and file an issue if you found a case that doesn't match this description.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 29, 2024
@Nadrieril Nadrieril added the call-for-testing Marks issues that require broader testing from the community, e.g. before stabilization. label Mar 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 13, 2024
…ttmcm

Use `min_exhaustive_patterns` in core & std

[`min_exhaustive_patterns`](rust-lang#119612) provides a subset of the functionality of [`exhaustive_patterns`](rust-lang#51085) which is likely to be stabilized much earlier than the full feature.

The subset covers all the compiler and std use cases. `compiler/` [already uses it](rust-lang@9dd6eda); this PR switches `std` over.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 13, 2024
…ttmcm

Use `min_exhaustive_patterns` in core & std

[`min_exhaustive_patterns`](rust-lang#119612) provides a subset of the functionality of [`exhaustive_patterns`](rust-lang#51085) which is likely to be stabilized much earlier than the full feature.

The subset covers all the compiler and std use cases. `compiler/` [already uses it](rust-lang@9dd6eda); this PR switches `std` over.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 13, 2024
Rollup merge of rust-lang#122255 - Nadrieril:min_exh_pats-libs, r=scottmcm

Use `min_exhaustive_patterns` in core & std

[`min_exhaustive_patterns`](rust-lang#119612) provides a subset of the functionality of [`exhaustive_patterns`](rust-lang#51085) which is likely to be stabilized much earlier than the full feature.

The subset covers all the compiler and std use cases. `compiler/` [already uses it](rust-lang@9dd6eda); this PR switches `std` over.
@U007D
Copy link

U007D commented Mar 14, 2024

I've added this to this week's Call for Testing section in This Week in Rust and have removed the call-for-testing label. Please feel free to re-add this label if you would like this tracking issue to re-appear in a future issue of TWiR.

Edit: looks like I don't have permission to remove the call-for-testing label.

@Nadrieril Nadrieril removed the call-for-testing Marks issues that require broader testing from the community, e.g. before stabilization. label Mar 14, 2024
@Nadrieril
Copy link
Member Author

Thank you! I've removed the label for you.

@Nadrieril
Copy link
Member Author

Stabilization PR: #122792

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 C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-exhaustive_patterns `#![feature(exhaustive_patterns)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants