Improve pattern-checking error message with variable that resembles an enum variant

[Kimundi]

This code:

mod foo {
  pub enum Bars {
    BarFoo,
    BarBar,
    BarBaz
  }
}

fn main() {
    decide(foo::BarBar);
}

fn decide(bar: foo::Bars) -> int {
    match bar {
        BarFoo => 0,
        BarBar => 42,
        BarBaz => 128
    }
}

should fail to compile because it can't resolve the enum variants in the match statement.
But currently it outputs this error message instead:

match_namespace.rs:16:8: 16:17 error: unreachable pattern
match_namespace.rs:16         BarBar => 42,
                              ^~~~~~~~~
match_namespace.rs:17:8: 17:17 error: unreachable pattern
match_namespace.rs:17         BarBaz => 128
                              ^~~~~~~~~

Which is curious because it doesn't complaint about the first item in the match statement, and because it somehow confuses 'Don't know what it is' with 'Can't reach it'.

[jdm]

The problem here is subtle. BarFoo gets treated as the name for any value, meaning that the subsequent statements actually are unreachable (and they are just treated as names for any value as well).

[catamorphism]

This isn't a bug: like @jdm said, because there's no enum variant named BarFoo, resolve assumes you mean to introduce a variable named BarFoo.

There's really no way around this, but I guess we could add some extra logic to the pattern-matching checking that detects this kind of situation and prints out extra diagnostics along with "unreachable pattern" (like, "...because variable pattern BarFoo matches any value"). I'll change the subject line and make this an enhancement issue.

[Kimundi]

Oh, you're right this is fully correct behavior.

I guess the compiler should then warn, if:

• it treats something in camel case as a variable
• the thing it matches on is an enum and the pattern is a valid enum variant in its namespace
• the variable it captures is not used in the match arm

[nikomatsakis]

I think having #3070 (and making it enabled by default) would effectively solve this problem, and in a much easier way.

[catamorphism]

I don't like the idea of just letting lint solve the problem (though I think we should have the lint pass too) since lint happens after match checking. Reading the error messages, it would still be confusing to read the "unreachable pattern" error; you might try to figure out what the error means without continuing to the rest of the errors. I think the error message should be self-contained.

[nikomatsakis]

I agree with your point regarding not having too many error messages. However, there is no reason that lint-style checks have to be done in the lint pass. They could just as easily be done in the exhaustiveness checker. I think we want to do a lint-style check regardless in order to catch things like this:

match foo {
    Bar => { ... } // I thought there was only one variant, but actually I'm getting a variable
}

I was thinking that another useful lint rule would be something like this: if there is ever a variable binding "foo" whose type is an enum E that includes a variant named "foo" (which clearly is not in scope), report a warning.

[catamorphism]

Oh, well if it's a lint check that's not actually part of the lint module, then I guess we're basically agreeing :-)

[nikomatsakis]

I think so. All I really mean is that we don't have to complicate the exhaustiveness checker by having it figure out why some branch is dead, it can instead just look at at the capitalized variable names and give a warning about those.

[catamorphism]

I think #3070 captures the work that needs to be done here.