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 reachable patterns lint #112

Closed
DevinR528 opened this issue Jul 21, 2021 · 6 comments
Closed

Non exhaustive reachable patterns lint #112

DevinR528 opened this issue Jul 21, 2021 · 6 comments
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change Major change proposal T-lang

Comments

@DevinR528
Copy link

DevinR528 commented Jul 21, 2021

Proposal

Add a new lint that checks for missing patterns in matches and destructuring assignment of non_exhaustive enums and structs.
This was suggested in the original non_exhaustive PR rust-lang/rust#45394 and there is an open issue rust-lang/rust#84332 for this lint as well as a very early PR rust-lang/rust#86809.

If the attribute is put on the wildcard pattern there is this issue with .. rust-lang/rust#81282.

Example

// crate x
#[non_exhaustive]
pub struct Foo {
    a: u8,
    b: usize,
    c: String,
}

#[non_exhaustive]
pub enum Bar {
    A,
    B,
    C,
}

// crate y
match Bar::A {
    Bar::A => {},
    Bar::B => {},
    #[deny(reachable)] // attribute goes here or on the expresion
    _ => {} // triggers lint "missing Bar::C..."
}

let Foo { 
    a
    b,
    #[warn(reachable)]
    .. // triggers lint "missing field `c`..."
} = structure;

Summary and problem statement

Since the non_exhaustive attribute was introduced it allows crate authors to make normally breaking API changes in a non-breaking way. This lint will warn users of non_exhaustive types when the type changes and they are no longer handling all cases, using the _/.. for more than the NonExhaustive variant/field.

Motivation, use-cases, and solution sketches

Implement this as a lint inside of rustc_lint or implement the lint as part of the usefulness checker here

Links and related work

WIP PR: rust-lang/rust#86809
lint in usefulness checker commit: DevinR528/rust@df377ca
compiler-team MCP: rust-lang/compiler-team#445
zulip compiler/major changes: https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Non.20exhaustive.20reachable.20patterns.20lint.20compiler-team.23445

Initial people involved

I have started a WIP PR and plan to finish this as part of my GSoC project.

  • Owner, if known: ?
  • Liaison

What happens now?

This issue is part of the lang-team initiative process. Once this issue is filed, a Zulip topic will be opened for discussion, and the lang-team will review open proposals in its weekly triage meetings. You should receive feedback within a week or two.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@DevinR528 DevinR528 added major-change Major change proposal T-lang labels Jul 21, 2021
@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2021

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@rustbot rustbot added the to-announce Not yet announced MCP proposals label Jul 21, 2021
@scottmcm
Copy link
Member

👍 to an opt-in lint for this.

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jul 21, 2021
@nikomatsakis
Copy link
Contributor

I approve.

@Mark-Simulacrum Mark-Simulacrum removed the to-announce Not yet announced MCP proposals label Aug 17, 2021
@DevinR528
Copy link
Author

Is it important that the initial comment reflect the merged state of this lint? It was renamed to non_exhaustive_omitted_patterns and the attribute can go either on the wildcard for enum matches or on the expression which works for structs and enums in pattern position.

@nikomatsakis
Copy link
Contributor

Discussed in meeting today: We are on board with this lint but don't feel there is a need for an initiative. The implementation work has been ongoing. Please cc @rust-lang/lang on the PR itself (we look forward to it). Thanks!

@jplatte
Copy link

jplatte commented Sep 28, 2021

The PR has been open for quite a while and was merged a week ago: rust-lang/rust#86809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change Major change proposal T-lang
Projects
None yet
Development

No branches or pull requests

6 participants