Implement subset-based ordering for naming rules by sharwell · Pull Request #36366 · dotnet/roslyn

sharwell · 2019-06-12T15:20:19Z

From my previous investigation:

I reviewed some naming rules and found that many cases will be correct if we follow a best-effort subset ordering. For example, consider the following rules:

Non-private static fields are PascalCase

Non-private readonly fields are PascalCase

Constants are PascalCase

Static fields are s_camelCase

Fields are _camelCase

The potential points of confusion are:

Rule (1) and rule (4). In this case, rule (1) is strictly more specific than rule (4), so it would take precedence

Rule (2) and rule (4). In this case, neither rule is strictly more specific than the other

Rule (2) and rule (5). In this case, rule (2) is strictly more specific than rule (5), so it would take precedence

Rule (4) and rule (5). In this case, rule (4) is strictly more specific than rule (5), so it would take precedence

I did note that whenever an overlap arises that isn’t a strict-subset relation, e.g. rule (2) and rule (4), it’s possible to define a new rule which covers exactly the intersection. In this case it would be a rule for “non-private static readonly fields”. I would propose to order the rules as follows, and allow the user to add a missing rule in the event an incorrect result is produced in the intersection:

If the accessibilities of rule A are a subset of those in rule B, apply rule A first (accessibilities are “match any”)

Otherwise, if the required modifiers of rule A are a superset of those in rule B, apply rule A first (required modifier are “match all”)

Otherwise, if the symbols of rule A are a subset of those in rule B, apply rule A first (symbols are “match any”)

This change implements the above ordering proposal, with a final fallback to rule name order to ensure the order is deterministic for any given configuration.

Update 13 June:

Based on an evaluation of .editorconfig files on GitHub with naming rules, the following changes were made to the ordering:

Ordering by modifiers occurs before ordering by accessibility. This change accounts for the fact that people frequently have ordering for all static members, and separately have ordering for non-private instance fields.
When sorting, the 'const' modifier is treated as always including 'static' and 'readonly' (so 'const' matches a subset of the symbols matched by 'static' and/or 'readonly').
The final sort by names uses the rule name instead of the style name.

…e of truth" Prior to this change, user settings could interact with .editorconfig in unknown ways to produce deviations in code style across a team which could not be controlled by simply updating .editorconfig. This change ensures that .editorconfig, when used, becomes the code style "source of truth" for affected documents.

...oreTest/EditorConfigStorageLocation/NamingStylePreferenceEditorConfigStorageLocationTests.cs

Fixes dotnet#36355

src/Workspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser.cs

jasonmalinowski · 2019-06-12T22:11:31Z

@sharwell Can you include in a commit message (or just this PR) the email you sent internally describing this and the cases you were seeing? It's really helpful to understand the change and I'd hate for other reviewers or people not on the email to not have that context.

sharwell · 2019-06-12T22:21:28Z

@jasonmalinowski added it

jasonmalinowski · 2019-06-12T22:28:18Z

@sharwell Thanks. While looking at the code (doing so right now and but haven't posted yet) I see it might be awesome to stick the comment too in the logic where all the ordering is in case if this ever gets detached from the PR. But otherwise awesome for putting it here because the "why" was otherwise missing.

sharwell · 2019-06-12T22:37:03Z

@jasonmalinowski Once the algorithm stabilizes I'll do that. I'm going to work with @madskristensen to implement an analyzer for the EditorConfig Language Service that detects cases where the ordering above is not the same as the order in which rules appear in the file, and offer to take action to correct it:

Reorder rules in the file to match the order of evaluation (i.e. the compiler is correct and editorconfig should reflect it)
Define a new rule covering the intersection between the conflicting rules, and force it to match the first rule, and then reorder the conflicting rules (i.e. the editorconfig file is correct, but the compiler can't know that so we create a new one that the compiler and file order agree on)

jasonmalinowski

Some general comments but it feels like there's a few tests missing:

Tests asserting that we're case sensitive for rule names if rule names are differing only by case.
A test that confirms that our choice of which rules go first is also correct. The OrderBy/ThenBy makes it clear enough in the code what order is but it seems that I could change that ordering in the code and no test would break. I'm not sure if it's worth having a case added where two rules are applied both with differing accessibility and modifiers, and the right one is still taken. Consider the common cases that got us here in the first place?

src/EditorFeatures/Test/EditorConfig/LegacyEditorConfigDocumentOptionsProviderTests.cs

src/Workspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser.cs

...oreTest/EditorConfigStorageLocation/NamingStylePreferenceEditorConfigStorageLocationTests.cs

jasonmalinowski · 2019-06-12T22:40:18Z

...oreTest/EditorConfigStorageLocation/NamingStylePreferenceEditorConfigStorageLocationTests.cs

+        [Theory]
+        [InlineData("a", "b", "a", "public", "public, private")]
+        [InlineData("b", "a", "a", "public, private", "public")]
+        [InlineData("b", "a", "b", "public", "public, private")]


Consider a comment that this and the next are verifying we aren't accidentally sensitive to textual names?

...oreTest/EditorConfigStorageLocation/NamingStylePreferenceEditorConfigStorageLocationTests.cs

jasonmalinowski · 2019-06-12T23:00:45Z

And otherwise this looks great modulo the nitpicks, but it seems those missing tests make me a bit worried to outright hit signoff.

During modifier ordering, 'const' is explicitly treated as a further restriction on 'static' and 'readonly'.

sharwell · 2019-06-13T23:07:41Z

@jasonmalinowski I'm going to file a follow-up issue to get more tests added

sharwell · 2019-06-13T23:14:53Z

🔗 See madskristensen/EditorConfigLanguage#71 for the new analyzer to help users avoid problems in this space

jasonmalinowski · 2019-06-14T00:10:29Z

@sharwell Do you need a follow-up review or I see you're still tweaking some things?

sharwell · 2019-06-14T13:05:22Z

@jasonmalinowski I'm going to try and find a way to add the name for ordering without changing the serialization everywhere.

This reverts commit a2be1a4.

jasonmalinowski · 2019-06-14T19:43:31Z

...paces/Core/Portable/Options/EditorConfig/NamingStylePreferenceEditorConfigStorageLocation.cs

-                    return (result: combinedNamingStylePreferences, succeeded: true);
-                }
-
                // no existing naming styles were passed so just return the set of styles that were parsed from editorconfig


Comment is now stale.

jasonmalinowski · 2019-06-14T20:21:01Z

src/Workspaces/Core/Portable/NamingStyles/Serialization/SymbolSpecification.cs

            internal static ModifierKind FromXElement(XElement modifierElement)
                => new ModifierKind((ModifierKindEnum)Enum.Parse(typeof(ModifierKindEnum), modifierElement.Value));
+
+            public override bool Equals(object obj)


This is just for the perf benefit of not relying on the slow struct equals?

jasonmalinowski · 2019-06-14T21:01:34Z

src/Workspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser.cs

                    namingStyles.Add(namingStyle);
                    namingRules.Add(serializableNamingRule);
+
+                    var ruleKey = (serializableNamingRule.SymbolSpecificationID, serializableNamingRule.NamingStyleID, serializableNamingRule.EnforcementLevel);


This feels like it needs a lot of comments.

jasonmalinowski · 2019-06-14T21:32:55Z

src/Workspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser.cs

+                    if (modifier.ModifierKindWrapper == SymbolSpecification.ModifierKindEnum.IsStatic
+                        || modifier.ModifierKindWrapper == SymbolSpecification.ModifierKindEnum.IsReadOnly)
+                    {
+                        if (x.SymbolSpecification.RequiredModifierList.Any(x => x.ModifierKindWrapper == SymbolSpecification.ModifierKindEnum.IsConst))


Is it easier just writing RequiredModifierList.Contains(new ModifierKind(ModifierKindEnum.Const)) or something? I'll admit it tripped me up that here we're doing an Any and below we're doing a Contains, but that's largely because this whole wrapping thing is...icky. (And not your fault.)

sharwell requested a review from a team as a code owner June 12, 2019 15:20

sharwell commented Jun 12, 2019

View reviewed changes

...oreTest/EditorConfigStorageLocation/NamingStylePreferenceEditorConfigStorageLocationTests.cs Outdated Show resolved Hide resolved

Implement subset-based ordering for naming rules

86eb4fa

Fixes dotnet#36355

sharwell force-pushed the order-naming-rules branch from 7a1a52a to 86eb4fa Compare June 12, 2019 16:19

sharwell commented Jun 12, 2019

View reviewed changes

src/Workspaces/Core/Portable/NamingStyles/EditorConfig/EditorConfigNamingStyleParser.cs Outdated Show resolved Hide resolved

jasonmalinowski reviewed Jun 12, 2019

View reviewed changes

vatsalyaagrawal added the Area-IDE label Jun 13, 2019

sharwell added 2 commits June 13, 2019 16:16

Order by modifiers before accessibility

fcd7401

During modifier ordering, 'const' is explicitly treated as a further restriction on 'static' and 'readonly'.

Fix the ordering by rule name

a2be1a4

Code cleanup and documentation for editorconfig ordering

f0b12ff

sharwell force-pushed the order-naming-rules branch from fa5fb78 to f0b12ff Compare June 13, 2019 23:14

sharwell added 2 commits June 14, 2019 08:07

Revert "Fix the ordering by rule name"

b90a08e

This reverts commit a2be1a4.

Fix the ordering by rule name

bb6e5fb

jasonmalinowski reviewed Jun 14, 2019

View reviewed changes

jasonmalinowski approved these changes Jun 14, 2019

View reviewed changes

jasonmalinowski reviewed Jun 14, 2019

View reviewed changes

jasonmalinowski requested a review from dpoeschl June 14, 2019 21:33

jinujoseph added the Approved to merge label Jun 19, 2019

jinujoseph added this to the 16.2.P4 milestone Jun 19, 2019

sharwell merged commit e1146e5 into dotnet:master Jun 19, 2019

sharwell deleted the order-naming-rules branch June 19, 2019 10:33

TessenR mentioned this pull request Jun 20, 2019

Roslyn does not provide a way to suppress nullability warnings on the left side of a compound assignments #36617

Open

jmarolf mentioned this pull request Jan 4, 2020

Allow editorconfig Naming Rules/Styles/SymbolSpecs to have uppercase letters #40705

Closed

sharwell mentioned this pull request Jun 21, 2021

[Regression in 16.1] Static field naming rules have started applying to constants #35488

Closed

patricksadowski mentioned this pull request Oct 19, 2022

editorconfig static rule is applying to const #23884

Open

Conversation

sharwell commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasonmalinowski commented Jun 12, 2019

Uh oh!

sharwell commented Jun 12, 2019

Uh oh!

jasonmalinowski commented Jun 12, 2019

Uh oh!

sharwell commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasonmalinowski Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasonmalinowski commented Jun 12, 2019

Uh oh!

sharwell commented Jun 13, 2019

Uh oh!

sharwell commented Jun 13, 2019

Uh oh!

jasonmalinowski commented Jun 14, 2019

Uh oh!

sharwell commented Jun 14, 2019

Uh oh!

jasonmalinowski Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sharwell commented Jun 12, 2019 •

edited

Loading

sharwell commented Jun 12, 2019 •

edited

Loading