Fsharp: Match statement with a patterns referring to an out of scope identifier give incomplete warning

[jbeeko]

Related #5475.

What

The following code compiles with warnings which could be improved upon.

module DUs =
    type Countries = 
    | CA
    | US
    | GB
let x = DUs.CA

let output = 
    match x with 
    | US -> "US"
    | CA -> "CA"
    | _ -> "XX"
output

There is a warning on | US -> "US" that the value US is unused.

The default pattern has a warning that the rule will never be matched.

Why

If when seeing the warnings you not taking into account that the pattern can be a value or a binding this is confusing in two ways.

1. In the above example is is obvious that US is out of scope. But in larger files this may not be obvious. Moreover normally referencing US without opening DUs would result in "The value or constructor US is not defined..." The message that it will never be used does not really point in the right direction.
2. The warning that the default pattern will never be matched is also confusing. Why not, should they not always match. And if the CA is never used why does the default not match?

And when this code is run it results in "US" which can also be confusing.

Finally because there are only warnings this could result in a bug.

How

The compiler is assuming that | CA is a binding of x to CA not used on the right and emits a message to explain that situation. It should also suggest that CA might be an undefined value. Something like

"Binding CA is not used in this rule or CA refers to a value not defined..."

[jbeeko]

Whoops, I just noticed that when editing completed the match so my point about the default pattern having a warning is bit confusing. You actually get this warning even if the match is not complete. I edit the sample code to make this clear.

[isaacabraham]

The reason why the _ default case (and CA) are never matched is because the US case is acting as the default case here i.e. all values are being bound to the symbol US. You could actually rewrite the first match clause in "full" like this: | _ as US -> "US" which is a more accurate representation of what is going on.

I agree that it's not intuitive though and could probably lead to bugs and runtime. I'm not sure what the solution to this would be though, especially without breaking changes.

[0x53A]

Union cases already have a requirement to have an upper-case first character. Maybe do the reverse with bindings?

I'd propose to add a warning when a binding has an upper-case first character.

```f#
| US -> // warning: bindings in match cases should begin with a lower-case character

```f#
| us -> // no warning

[cartermp]

@0x53A That's a really good idea.

[jbeeko]

@0x53A @cartermp

The existing error message already indicates that bindings should not be upper case. My point it that it assumes the issue is either a misspelled identifier or a contrary to convention an upper case binding name. Our team's experience is that this is often do to a missing open statement and the error message should also suggest that.

See https://github.com/dotnet/fsharp/pull/8936

[abelbraaksma]

@jbeeko is correct, the warning suggested by @0x53A already exists. In VS, you see both warnings at the same time:

But there's a bug with that warning. Consider the following, which doesn't raise FS0049:

Versus:

Which does raise the FS0049 warning:

FS0049: Uppercase variable identifiers should not generally be used in patterns, and may indicate a misspelt pattern name.

In other words, that warning is only triggered if the the uppercase variable binding is three-or-more characters. I think that's a bug, or perhaps there's some historical reason why short names doesn't trigger it.

And I think it would certainly be a nice-to-have to add the "open module XXX" to the suggested quick-fixes: