fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

FS0026 "This rule will never be matched" not fired for combined match branches

Open abelbraaksma opened this issue 5 years ago • 5 comments

The following gives FS0026 correctly:

match None with 
| Some _ -> ()
| Some "" -> ()   // FS0026 This rule will never be matched
| None -> ()
| _ -> ()         // FS0026 This rule will never be matched

The following does not:

match None with 
| Some _ 
| Some "" -> ()   // does not raise warning FS0026 This rule will never be matched
| None 
| _ -> ()         // does not raise warning FS0026 This rule will never be matched

Though in either case, the 2nd and 4th branches are redundant. Does the compiler optimize that away, which would make the warning redundant? Either way, I think the warning ought to be raised in the first and second case.

Repro steps

See code above, just copy/paste.

Expected behavior

Both cases should give two warnings.

Actual behavior

Second case does not give a warning.

Known workarounds

Manually check your code for redundant checks or branches. Which is precisely what we're not supposed to have to do (and it is how I found this case in existing code I was reviewing), hence I'm filing it as a bug ;).

Whether either example leads to all branches being compiled, I don't know. I'd hope/guess/suspect that in release builds, the redundant branches are removed, but I didn't verify yet.

abelbraaksma avatar Nov 17 '19 13:11 abelbraaksma

This is a feature improvement to pattern matching by adding more warning checks. Be best to make a RFC on https://github.com/fsharp/fslang-design regarding it. There we should discuss it.

TIHan avatar Nov 26 '19 21:11 TIHan

I've run into the same issue trying to combine two long such match expressions. Has an RFC or a language suggestion been submitted? What is the current status?

I found a faster workaround when using an IDE with support for multiple cursors: Un-combine the cases, deduplicate them, and re-combine them.

LyndonGingerich avatar Nov 28 '22 14:11 LyndonGingerich

Curious, is there a solid reason why this warning can't be an error? Ran into a situation where someone forgot a :? in a try with block, they had a following underscore so they did get warned but because it was only a warning they missed it. This meant the | ArgumentException -> was always matched and no other exceptions could be matched.

voronoipotato avatar Aug 31 '23 19:08 voronoipotato

I too got bitten by this pitfall of disjunctive patterns today. When reading the redundant line of code in isolation, it's easy to get mislead into thinking that it must match... And yet, no error, no warning. (Unrelatedly, there is a typo ("patterm") in 7.5 of the F# spec.)

eldritchconundrum avatar Apr 13 '24 14:04 eldritchconundrum

This is another duplicate of #3448

Martin521 avatar Apr 13 '24 19:04 Martin521