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

#[feature(exhaustive_patterns)] thinks irrefutable pattern is refutable when enum contains struct with private fields. #104034

Closed
BGR360 opened this issue Nov 6, 2022 · 7 comments · Fixed by #111624
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. F-exhaustive_patterns `#![feature(exhaustive_patterns)]` F-never_type `#![feature(never_type)]` requires-nightly This issue requires a nightly compiler in some way. S-bug-has-test Status: This bug is tracked inside the repo by a `known-bug` test. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@BGR360
Copy link
Contributor

BGR360 commented Nov 6, 2022

The following code compiles on nightly (playground):

#![feature(exhaustive_patterns, never_type)]

enum Either<A, B> {
    A(A),
    B(Wrapper<B>),
}

struct Wrapper<T>(T);

fn foo() -> Either<(), !> {
    Either::A(())
}

fn main() {
    let Either::A(()) = foo();
}

But if the wrapper type is moved into a module to make the field private, then it does not: (playground):

#![feature(exhaustive_patterns, never_type)]

mod inner {
    pub struct Wrapper<T>(T);
}

enum Either<A, B> {
    A(A),
    B(inner::Wrapper<B>),
}

fn foo() -> Either<(), !> {
    Either::A(())
}

fn main() {
    let Either::A(()) = foo();
}
error[E0005]: refutable pattern in local binding: `Either::B(_)` not covered
  --> src/main.rs:17:9
   |
17 |     let Either::A(()) = foo();
   |         ^^^^^^^^^^^^^ pattern `Either::B(_)` not covered
   |
   = note: `let` bindings require an "irrefutable pattern", like a `struct` or an `enum` with only one variant
   = note: for more information, visit https://doc.rust-lang.org/book/ch18-02-refutability.html
note: `Either<(), !>` defined here
  --> src/main.rs:9:5
   |
7  | enum Either<A, B> {
   |      ------
8  |     A(A),
9  |     B(inner::Wrapper<B>),
   |     ^ not covered
   = note: the matched value is of type `Either<(), !>`
help: you might want to use `if let` to ignore the variant that isn't matched
   |
17 |     if let Either::A(()) = foo() { todo!() }
   |     ++                           ~~~~~~~~~~~

Making the field pub makes it compile again.

Is this expected? I suppose I could understand if it is; the private fields could change to make Wrapper<!> actually be constructable and therefore make the pattern refutable. But if that's the case, then I think the compiler should point this out, like note: the pattern is currently irrefutable, but the type contains private fields which may change in the future to make the pattern refutable.

If indeed this is expected, is there any way for me as a library author to somehow convince the compiler that I will never change the fields to make the type constructable? I want users of my library to be able to elide match arms when I give them an Either<A, !>.

@rustbot label +T-compiler +F-never_type +D-confusing +requires-nightly +S-bug-has-mcve

@BGR360 BGR360 added the C-bug Category: This is a bug. label Nov 6, 2022
@rustbot rustbot added D-confusing Diagnostics: Confusing error or lint that should be reworked. F-never_type `#![feature(never_type)]` requires-nightly This issue requires a nightly compiler in some way. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 6, 2022
@BGR360
Copy link
Contributor Author

BGR360 commented Nov 7, 2022

@rustbot label +F-exhaustive_patterns

@rustbot rustbot added the F-exhaustive_patterns `#![feature(exhaustive_patterns)]` label Nov 7, 2022
Manishearth added a commit to Manishearth/rust that referenced this issue Nov 14, 2022
…imulacrum

Add a few known-bug tests

The labels of these tests should be changed from `S-bug-has-mcve` to `S-bug-has-test` once this is merged.

cc:
rust-lang#101518
rust-lang#99492
rust-lang#90950
rust-lang#89196
rust-lang#104034
rust-lang#101350
rust-lang#103705
rust-lang#103899

I couldn't reproduce the failures in rust-lang#101962 and rust-lang#100772 (so either these have started passing, or I didn't repro properly), so leaving those out for now.

rust-lang#102065 was a bit more complicated, since it uses `rustc_private` and I didn't want to mess with that.
@jackh726 jackh726 added S-bug-has-test Status: This bug is tracked inside the repo by a `known-bug` test. and removed S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue labels Nov 14, 2022
@BGR360
Copy link
Contributor Author

BGR360 commented Nov 27, 2022

@jackh726 are we sure this is actually a bug? As I said, I could understand how it's not:

Is this expected? I suppose I could understand if it is; the private fields could change to make Wrapper<!> actually be constructable and therefore make the pattern refutable.

@jackh726
Copy link
Member

jackh726 commented Dec 3, 2022

I'm not sure either way - however at the very least, as you point out, it makes sense to have some better diagnostics

@canndrew
Copy link
Contributor

canndrew commented Feb 6, 2023

This is definitely intentional. But yeah, it should have a better error message.

is there any way for me as a library author to somehow convince the compiler that I will never change the fields to make the type constructable?

Not currently I don't think. It would probably require us to add an Uninhabited trait, eg. something like:

trait Uninhabited {
    fn uninhabited(self) -> !;
}

impl<T> Uninhabited for Wrapper<T>
where
    T: Uninhabited,
{
    fn uninhabited(self) -> ! {
        let Wrapper(inner) = self;
        match inner {}
    }
}

This trait would then need to be a lang item so that the exhaustiveness check could make use of it. Adding this trait would require an RFC but I doubt there'll be much enthusiasm for adding another special trait for such a niche use-case.

Alternatively, you could give Wrapper an into_inner or get_ref method to expose the uninhabitedness of the inner value. It's not quite what you're asking for but it's still concise-ish:

let a = match foo() {
    Either::A(a) => a,
    Either::B(wrapper) => match wrapper.get_ref() {},
};

@fmease
Copy link
Member

fmease commented Feb 6, 2023

Is Uninhabited::uninhabited supposed to be “injected” where an otherwise required match arm was omitted? Wouldn't that run into the same discussions project-deref-patterns had about impure code (code with side effects) inside patterns? Their existence would heavily hinder optimizations of match expressions (like translation to jump tables or reordering).

@canndrew
Copy link
Contributor

canndrew commented Feb 9, 2023

If the type implementing Uninhabited really is uninhabited then there won't actually be any impure code in the match. The call to Uninhabited::uninhabited would only need to be injected for types that fake uninhabitedness with an impl that panics/aborts. We could also rule that out be instead making Uninhabited derive-only like Copy.

@Nadrieril Nadrieril added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Apr 10, 2023
@Nadrieril
Copy link
Member

Making Uninhabited a trait isn't straightforward exactly because inhabitedness depends on visibility questions. It would probably have to mean "Uninhabited as far as the outside world is concerned", or it could use a trick like the Context in safe transmute to make it depend on the module.

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-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. F-exhaustive_patterns `#![feature(exhaustive_patterns)]` F-never_type `#![feature(never_type)]` requires-nightly This issue requires a nightly compiler in some way. S-bug-has-test Status: This bug is tracked inside the repo by a `known-bug` test. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants