Irrefutable while-let pattern in log4rs-rolling-file-0.2.0, Rust 1.16

[brson]

UPDATE: The underlying problem has been solved (we believe), but there is not yet a regression test in the test base. Here are some instructions for how to add a regression test.

---

https://github.com/sfackler/log4rs-rolling-file 1ce36b4301c86fa750134d35cb815d146c5836ee

Not on stable/beta.

101 brian@ip-10-145-43-250:~⟫ rustc +nightly -Vv
rustc 1.16.0-nightly (47c8d9fdc 2017-01-08)
binary: rustc
commit-hash: 47c8d9fdcf2e6502cf4ca7d7f059fdc1a2810afa
commit-date: 2017-01-08
host: x86_64-unknown-linux-gnu
release: 1.16.0-nightly
LLVM version: 3.9

brian@ip-10-145-43-250:~/dev/log4rs-rolling-file⟫ cargo +nightly test
   Compiling crossbeam v0.2.10
   Compiling serde v0.7.15
   Compiling libc v0.2.19
   Compiling traitobject v0.0.3
   Compiling unsafe-any v0.4.1
   Compiling typemap v0.3.3
   Compiling winapi-build v0.1.1
   Compiling yaml-rust v0.3.5
   Compiling kernel32-sys v0.2.2
   Compiling num-traits v0.1.36
   Compiling log v0.3.6
   Compiling winapi v0.2.8
   Compiling time v0.1.35
   Compiling num-integer v0.1.32
   Compiling num-iter v0.1.32
   Compiling rand v0.3.15
   Compiling quick-error v1.1.0
   Compiling num v0.1.36
   Compiling chrono v0.2.25
   Compiling humantime v0.1.3
   Compiling serde_yaml v0.2.5
   Compiling ordered-float v0.1.0
   Compiling serde-value v0.2.1
   Compiling antidote v1.0.0
   Compiling tempdir v0.3.5
   Compiling log4rs v0.4.8
   Compiling log4rs-rolling-file v0.2.0 (file:///mnt2/dev/log4rs-rolling-file)
error[E0165]: irrefutable while-let pattern
  --> src/policy/compound/roll/delete/config.rs:81:43
   |
81 |                                 while let Some(key) =
   |                                           ^^^^^^^^^ irrefutable pattern

error: aborting due to previous error

Build failed, waiting for other jobs to finish...
error[E0165]: irrefutable while-let pattern
  --> src/policy/compound/roll/delete/config.rs:81:43
   |
81 |                                 while let Some(key) =
   |                                           ^^^^^^^^^ irrefutable pattern

error: aborting due to previous error

error: Could not compile `log4rs-rolling-file`.

To learn more, run the command again with --verbose.

This case is interesting. The loop is generated by serde and from inspection seems be a degenerate case. I can't tell if it was previously right or wrong.

cc @sfackler @erickt

[sfackler]

Why was this made an error in the first place?

[oli-obk]

cc @mcarton
sorry

cc @dtolnay

[dtolnay]

To save other people from digging through the crate, the situation is:

#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
pub struct Config {
}

The generated code (cleaned up) looks like:

impl Deserialize for Config {
    fn deserialize<D>(deserializer: &mut D) -> Result<Config, D::Error>
        where D: Deserializer
    {
        enum Field {}

        impl Deserialize for Field {
            /* ... */
        }

        struct Visitor<D: Deserializer>(PhantomData<D>);

        impl<D: Deserializer> Visitor for Visitor<D> {
            type Value = Config;
            fn visit_seq<V>(&mut self, mut visitor: V) -> Result<Config, V::Error>
                where V: SeqVisitor
            {
                try!(visitor.end());
                Ok(Config{})
            }
            fn visit_map<V>(&mut self, mut visitor: V) -> Result<Config, V::Error>
                where V: MapVisitor
            {
                // ERROR: irrefutable while-let pattern
                while let Some(key) = try!(visitor.visit_key::<Field>()) {
                    match key {}
                }
                try!(visitor.end());
                Ok(Config{})
            }
        }

        const FIELDS: &'static [&'static str] = &[];
        deserializer.deserialize_struct("Config", FIELDS, Visitor::<D>(PhantomData))
    }
}

This looks like the code for an ordinary braced struct except there are zero fields. So the error is because the line while let Some(key) declares key of type enum Field {} and can't possibly happen.

I filed serde-rs/serde#676 to fix this in Serde and I will leave it to you guys to debate whether this should be an error.

[brson]

Sounds like there's some debate to be had yet about the behavior here. Assigning to niko to make sure something happens.

[nikomatsakis]

cc @canndrew -- @eddyb and I agree this should be a warning. It's not clear what definition of "irrefutable pattern" is being used here, presumably it is checking that the other variants are not reachable, probably so that this works:

#![feature(never_type)]

fn foo(x: Result<u32, !>) {
    let Ok(y) = x;
}

fn main() {
}

[dtolnay]

I love the new behavior and plan to use it in Serde, for example here:

// Before
try!(visitor.visit_key::<__Field>()).map(|impossible| match impossible {});

// After
let None::<__Field> = try!(visitor.visit_key());

But it seems like this should be consistent with how unreachable patterns in match are handled. Either both should be a warning or both should be an error:

enum Void {}

fn f() -> Option<Void> { None }

fn main() {
    match f() {
        None => println!("none"),
        // WARNING: unreachable pattern, #[warn(unreachable_patterns)] on by default
        Some(_) => println!("some"),
    }

    // ERROR: irrefutable if-let pattern
    // (previously okay)
    if let None = f() {
        println!("definitely none");
    }

    // ERROR: irrefutable if-let pattern
    // (previously okay)
    if let Some(_) = f() {
        println!("definitely some");
    }
}

Also "irrefutable pattern" makes sense for if let None but the problem with if let Some(_) is it is 100% refutable, not irrefutable.

[nikomatsakis]

I think the definition is correct, it's just that this should not be an error.

[nikomatsakis]

Should be addressed by #39151, I believe

I meant #39290

[nikomatsakis]

at least for now =)

[nikomatsakis]

@canndrew of course #39290 will paper over the problem, but did you fix this in #39127 ?

[canndrew]

Yeah, this should be fixed properly.

[nikomatsakis]

Looks like #39127 did not include a test for this particular case, so I'm going to add E-needs-test.

[nikomatsakis]

I am marking this issue as E-mentor and E-easy. The task is to add a test case for the while let error which we are observing. One (minor) challenge is that this problem has been fixed in the interim, so to ensure that your test case reflects the true problem, you will have to use the nightly from January 10th or so (17 days ago, the time when this issue was reported).

The other problem is that there is no reduced form of the test. @dtolnay gave a relatively minimized example but it seems like it could be reduced further.

The crux of the problem is that we have an enum Foo with no variants (which can never be instantiated) and a function that returns Option<Foo>. Then you have a while let loop "unpacking" this option. Something like this:

#[deny(warnings)]
enum Foo { }
fn make_foo() -> Option<Foo> { None }
fn main() {
    while let Some(_f) = make_foo() { }
}

This should generate a warning about dead-code, ideally, but it should not generate an error. (At least, not on the current nightly: it should generate an error on the older nightly, if I did the reduction right.)

So the steps to close are:

• Test the test above with the older nightly
• Add it to the testbase in src/test/compile-fail with appropriate annotaitons.
  • there is a description of how the test suite works in forge
• Open a pull request!

[russellmcc]

This seems to still be broken on latest master, actually. The exact test you provide fails with

   |
14 |     while let Some(_f) = make_foo() { }
   |               ^^^^^^^^ irrefutable pattern

[nikomatsakis]

@canndrew see @russellmcc's comment above; that's a bit surprising, no?

[canndrew]

Surprising and wrong 😕, that pattern is definitely refutable. I'll have a look into it...

[canndrew]

Alright, sorry for saying this should be fixed when it was, in fact, not at all fixed. It's fixed now though!

[arielb1]

@canndrew

So with your patch, we end up in a situation where:

enum Foo { }
fn make_foo() -> Option<Foo> { None }
fn main() {
    match make_foo() {
        None => {},
        Some(_) => {}, //~ WARN unreachable pattern
    }
}

but if you remove the arm:

enum Foo { }
fn make_foo() -> Option<Foo> { None }
fn main() {
    match make_foo() { //~ ERROR non-exhaustive patterns
        None => {}
    }
}

If you want to shut it up, you have to use:

enum Foo { }
fn make_foo() -> Option<Foo> { None }
fn main() {
    match make_foo() {
        None => {},
        _ => {}
    }
}

[nikomatsakis]

@arielb1 Heh. That's... kind of amusing. Though to be fair to @canndrew he would have preferred the second example to work, we kind of asked him not to allow it to work just yet. I'm not unhappy with the status quo, in some sense, as long as it's temporary; but it does seem likely to be awfully confusing.

I could imagine a couple of things:

• give a message directing people to an issue where to learn more;
• skip the warning, somehow, that ... probably requires the old code?

I think the TL;DR is that we should discuss and settle some kind of unreachable semantics. But I'm not 100% sure I'm ready to do that yet. I guess at minimum I gotta catch up on the RFC + old conversation.

[canndrew]

@arielb1 Arrgh. The problem is my Hide uninhabitedness checks behind feature gate PR didn't completely hide them.

The changes I made for the uninhabited patterns stuff were:

0. Make it so is_useful doesn't abort early with NotUseful if it gets a pattern matrix with zero rows. This means it will continue to recurse through the matrix columns for an empty enum and notice that the empty pattern set is exhaustive.
1. Remove the special case that made match my_empty_enum {} compile before (because it's made redundant by (0)).
2. Make all_constructors skip constructors that are statically impossible by recursing into them and looking for uninhabited types.

When I tried to hide this stuff I hid (2) but didn't revert (0) and (1) (I guess because they seemed like more of a fix than a major change). I can hide them aswell with a follow-up PR though if you'd like?

[arielb1]

We had some discussion about that yesterday. You can read the notes at
https://public.etherpad-mozilla.org/p/rust-compiler-design-sprint-paris-2017-ucg, and Niko/me will write a writeup.

[nikomatsakis]

@canndrew I think the semantics we ultimately decided that we want kind of fall somewhere in between the older behavior and the newest behavior =)

[SSheldon]

I found another regression from 1.15 to 1.16 that involves empty enums. The following will compile on rust version 1.15:

enum Void { }

fn use_void(v: &Void) -> u32 {
    match v { }
}

fn main() { }

But on rustc 1.16.0-beta.2 (bc15d5281 2017-02-16) and rustc 1.17.0-nightly (306035c21 2017-02-18) it errors with:

error[E0004]: non-exhaustive patterns: `_` not covered

     match v { }
           ^ pattern `_` not covered

Does this seem like the same issue or should I ticket it separately?