dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.92k stars 4.01k forks source link

"pascal_case" on field is only enforced when the first character is not already uppercase #70709

Open LennardF1989 opened 10 months ago

LennardF1989 commented 10 months ago

Similar to #23706.

EDIT: Actually, I may have misread/misunderstood and it might just be a duplicate now that I look at it again... Please close if this is the case. The screenshot in that topic (almost 5 years old!) does not represent the current state of affairs, though.

.editorconfig

[*.*]
dotnet_analyzer_diagnostic.severity = default

[*.{cs,vb}]
# Naming rules
dotnet_naming_rule.fields.severity = error
dotnet_naming_rule.fields.symbols = fields
dotnet_naming_rule.fields.style = pascal_case

# Symbol specifications
dotnet_naming_symbols.fields.applicable_kinds = field
dotnet_naming_symbols.fields.applicable_accessibilities = *
dotnet_naming_symbols.fields.required_modifiers =

# Naming styles
dotnet_naming_style.pascal_case.required_prefix = 
dotnet_naming_style.pascal_case.required_suffix = 
dotnet_naming_style.pascal_case.word_separator = 
dotnet_naming_style.pascal_case.capitalization = pascal_case

[*.cs]

# Analyzer rules
dotnet_diagnostic.IDE1006.severity = error #naming rules

Code

namespace ExampleProject
{
    public class Class1
    {
        public const string TEST_ABC1 = "ABC";
        private const string TEST_ABC2 = "ABC";

        public static readonly string TEST_ABC3 = "ABC";
        private static readonly string TEST_ABC4 = "ABC";

        private string TEST_ABC5 = "ABC";
        private string aTEST_ABC5 = "ABC";
        private string _TEST_ABC5 = "ABC";
    }
}

What happens?

In Visual Studio, only the last two fields will be flagged as a rule violation Resharper is much smarter and will flag all of the them.

What is expected?

None of these actually comply to PascalCase and are rather allupper with a `` seperator.

Potential fix

A normal PascalCase does (should) not have underscores in the name. Similar to #70506, it should not allow underscores anywhere, unless set as a seperator. Optionally, we can also check if there is at least one lowercase character, but that may be biased... Who is to say that CMS is not valid PascalCase? And without separators VSCODE is technically valid too.

Attachments

ExampleProject.zip

CyrusNajmabadi commented 10 months ago

This seems problematic. As it's normal to have both _ and `` as a word separator in codebases. We'd need a way to indicate that this was optional. AFAICT, this is working as expected on our end.

LennardF1989 commented 10 months ago

Yea, I added an additional section going over a potential fix. The problem is... It's totally right. ALL_UPPER is technically PascalCase, since it starts with a capital and there is no telling it's wrong. Other than the underscore, which sometimes is actually valid. For all it knows, these are just two abbreviations...

Maybe the fix is rather a "cannot contain" type of rule to be added to this list: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/naming-rules#naming-style-properties

bzd3y commented 3 months ago

I think I am running into this same issue.

It's totally right. ALL_UPPER is technically PascalCase, since it starts with a capital and there is no telling it's wrong

But it isn't totally right. ALL_UPPER is not PascalCase or any kind of camelCase. It is unambiguously uppercased snake_case (so SCREAMING_SNAKE_CASE). camelCase uses uppercase letters to indicate word boundaries. ALL_UPPER does not do that. It uses an underscore.

@LennardF1989, your original examples above are a little different as there is no underscore. CMS is valid PascalCase, but VSCODE wouldn't be. But, to your point, without ML or a dictionary-lookup or something, it would be tricky to get the compiler to figure that out. But that doesn't mean we should just give up completely. At the very least, when pascal case is specified it should check for underscores that don't prefix the identifier. An identifier with an underscore after a non-underscore would not be valid PascalCase.

Something that might help you as a workaround is https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1707 which enforces that identifiers have no underscores. And this seems to work with the default/earlier rules for private members to be prefixed with an _. So that does solve some of the problem. But, for me at least, there's still the problem of there being uppercased one-word identifiers for constants throughout the code that are not valid PascalCase.