Warn on uppercase identifiers in patterns.

[edgarfgp]

Fixes #6712, #17878

Don't warn on uppercase identifiers in binding patterns . Removing the historical hack when it was only raise if the identifier was >= 3.

Before

match 1 with 
| US -> "US" // No Warning
| UK -> "UK" // No Warning
| U -> "U" // No Warning

After

match 1 with 
| US -> "US" // Warning 
  ^^
| UK -> "UK" // Warning
  ^^
| U -> "U" // Warning
  ^

Note: We will only raise a warning in the context of True match case clause.

Checklist

• Test cases added
• Release notes entry updated

[vzarytovskii]

I am, honestly, not sure if it's a good idea.

A rule of thumb is - if this will suddently introduce new warnings in existing codebases, it should go under separate version flag or introduce a separate warning, which will be opt-in.

In this particular case - the use cases are country and language codes, which are very common in codebases, and it's not an opt-in warning. I personally thing we should keep it, or make it an additional warning, which will be opt-in.

[edgarfgp]

I was reading the F# spec and I did not find anything about the check >=3. It only stated the following.

I think the main for me is that we only get this warning named pattern that is Uppercase and the length is >= 3 which seems confusing to me. my wishful thinking will expect:

• Dedicated Warning when using named pattern (Pattern matching) Uppercased >= 1
• Dedicated Warning when using function parameters Uppercased >= 1
• This will be under a preview lang flag

Update: Now that I know about this. I cannot unsee this:). Maybe @dsyme can give us some historical context.

[edgarfgp]

A rule of thumb is - if this will suddenly introduce new warnings in existing codebases, it should go under separate version flag or introduce a separate warning, which will be opt-in.

@vzarytovskii Yeah I will definitely add a lang version flag. I wanted to first understand what were the implications of this check

[vzarytovskii]

A rule of thumb is - if this will suddenly introduce new warnings in existing codebases, it should go under separate version flag or introduce a separate warning, which will be opt-in.

@vzarytovskii Yeah I will definitely add a lang version flag. I wanted to first understand what were the implications of this check

I'm still not convinced that language flag is a proper way of doing that. It will cause a lot of new warnings in existing codebases when updating to new language version. And only two options will be either to downgrade a language version, or disable the warning.

I think it will instead be a great use case for analyzers sdk.

[edgarfgp]

I'm still not convinced that language flag is a proper way of doing that. It will cause a lot of new warnings in existing codebases when updating to new language version. And only two options will be either to downgrade a language version, or disable the warning.

I think it will instead be a great use case for analyzers sdk.

Yeah I understand, this check seems to be a terrible hack and dealing with this using an analyser seems a more a bandage. But yeah Happy to close this PR at the very least now I understand the reason of this unexpected compiler behaviour 😓

[psfinaki]

I'm still not convinced that language flag is a proper way of doing that. It will cause a lot of new warnings in existing codebases when updating to new language version.

Well, we have a huge codebase but in the end the amount of code to be fixed was, well, graspable. Also, the original error was for missing the full qualification of DU, which is probably not that common, also judging by the activity in that bug.

Those are just some thoughts, I am sort of on a fence with that. Although at least the collateral changes in this PR are worth merging.

[edgarfgp]

@vzarytovskii @psfinaki Based on the provided feedback, I have reverted to using the original check and:

• Add a comment to the offending code
• Update the error message to include the >=3 check and also that it might require qualified access(Please feel free to suggest a more short/clear message)
• Add more tests.

I think this will, at the very least give more context about this confusing behaviour.

[edgarfgp]

Closing this as there does not seem to be a consensus as to how to better deal with this. Feel free to reopen in case you want to take some of the changes in this PR

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.200.md
LanguageFeatures.fsi	docs/release-notes/.Language/preview.md

[edgarfgp]

@vzarytovskii

In the spirit of making some progress here. I have reverted the breaking changes as I understand that there is not way to have a proper fix for this as it will break people's code or will produce unnecessary noise.

Before

match 1 with 
| US -> "US" // No Warning
| UK -> "UK" // No Warning
| U -> "U" // No Warning

After

match 1 with 
| US -> "US" // Warning 
  ^^
| UK -> "UK" // Warning
  ^^
| U -> "U" // Warning
  ^

• We will only raise a warning in the context of True match case clause. meaning | pat ->
• We will not not longer raise a warning of function, constructor parameters and all of the other cases that are not | pat ->

For more info check the tests.

[vzarytovskii]

I'm personally fine with it, so I'll throw @T-Gro and @KevinRansom under the bus to make the decision.

[edgarfgp]

I'm personally fine with it, so I'll throw @T-Gro and @KevinRansom under the bus to make the decision.

I guess I will have to wait for the bus to arrive to my station for sometime :)