Duplicate cases in match statement not producing compiler warnings if grouped

[neildanson]

Writing exhaustive pattern matches is a powerful way to prevent unintended bugs at runtime.

However due to the verbosity and repetitiveness these are often grouped. However when grouped, the exhaustiveness check seems to have an issue spotting repeated cases.

Repro steps

type Foo = 
    | A of int list
    | B of int list
    | C of float list
    | D of float list

let foo = function
| A _ //Note the A Case is repeated
| C _
| D _ -> []
| A l  //and here
| B l -> l

or *any* grouping i.e.

let foo = function
| A _ -> []
| C _ -> []
| D _ -> []
| A l 
| B l -> l

Expected behavior

I'd expect a warning about an unreachable case A (the 2nd one)

Actual behavior

No warning

Known workarounds

Workaround 1: Dont group (at all!)

let foo = function
| A _ -> []
| C _ -> []
| D _ -> []
| A l -> l
| B l -> l

Workaround 2: Wildcard (although the reason for being explicit is lost)

let foo = function
| A l -> l
| B l -> l
| _ -> []

Related information

OCaml reports a unreachable case in this scenario.

F# 4.1 .NET 4.7 Visual Studio 2017 & VS Code

[psfinaki]

Isn't this just a bug actually? @vzarytovskii thoughts?

I find this at least confusing, but actually just weird given how good our DUs and pattern matching are in F#.

[voronoipotato]

Isn't this just a bug actually? @vzarytovskii thoughts?

I find this at least confusing, but actually just weird given how good our DUs and pattern matching are in F#.

IMO it's a bug. If it were up to me "rule will never be matched" would also be an error, I can't think of any reason you would want a rule to never be matched...

[Martin521]

I have spent quite some time on this issue (mostly on understanding pattern match compilation - a hard nut). My conclusion is that there is no way to create the requested warnings without seriously changing the design of the pattern match compilation in the compiler. This is a possibility, of course, but I wonder if it makes sense, for the reasons that I describe further below.

Before I get there, however, I want to clarify this issue. This is NOT a bug in the logic of the RuleNeverMatched warning. In F# pattern matching, there is no such thing as "rule grouping". Take the second example in the original post. It is just a match expression with four rules, where the last one has an OR pattern (parentheses added just for clarity).

let foo = function
| A _ -> []
| C _ -> []
| D _ -> []
| (A l | B l) -> l

So, we have four rules and all can succeed. Therefore, no RuleNeverMatched warning. We are really talking about a different phenomenon, namely RedundantSubpattern. The subpattern A l in the fourth rule is redundant - it can be removed without changing the match outcome. There are obviously different kinds of redundant subpatterns and we would need to discuss which ones we want to warn about (and to which level of nesting we want to investigate this). (And BTW what about expressions? We don't warn about redundant subexpressions like in let a = b + 0 - would we want that also?).

Anyway, we could define a certain set of redundant subpatterns that we want to warn about. And create a new pattern match compiler that can do it. But then? Introducing such warnings is a breaking change. So we can only make it an informational message that can be promoted to a warning by #warn xxxx. Very little impact, very large effort.

My preliminary recommendation is to close this issue as "not planned". How are others thinking about it?

[psfinaki]

Hah it never occurred to me that it's just an OR condition there :)

Thanks for the investigation, @Martin521! Now we just need to understand why OCaml does produce a warning there :D

[Martin521]

OCaml in such cases indeed emits a warning this sub-pattern is unused. I assume that means their pattern match compilation has a different design. (Although I would be happy to hear how it can work with F#'s current frontier-based design.)

The OCaml pattern match compiler has more than 4k LOC, compared to 1.7k of F#. I am not sure if I want to dive into it, given my above mentioned concerns about the (almost zero) impact of what we would do.

[neildanson]

How are others thinking about it?

As the original raiser of the issue (not that that make me an authoritarian), I'm largely indifferent to the fixed-ness of this.

I think the risk of this issue causing a real world problem is relatively low (a case will never be matched) and the risk of a fix is that backwards compatibility could be broken (albeit in a way that forces redundant unexecutable code to be removed) which could be rather annoying.

[brianrourkeboll]

@Martin521

Introducing such warnings is a breaking change. So we can only make it an informational message that can be promoted to a warning by #warn xxxx. Very little impact, very large effort.

I wonder whether putting it behind langversion would be enough?

@neildanson

I think the risk of this issue causing a real world problem is relatively low (a case will never be matched) and the risk of a fix is that backwards compatibility could be broken (albeit in a way that forces redundant unexecutable code to be removed) which could be rather annoying.

I agree that such code is probably fine most of the time in practice, but there are arguably potential bugs lurking in any such code, since what the code looks like it could do is different from what it actually could do.

[Martin521]

I wonder whether putting it behind langversion would be enough?

In that case some more work on it would definitely make sense. The impact would be much larger, especially newcomers would immediately profit from it. @vzarytovskii : Is that an option?