Fix handling of unrecognized naming style values by sharwell · Pull Request #27331 · dotnet/roslyn

sharwell · 2018-06-01T14:55:58Z

The first commit implements Editorconfig backwards compatibility dilemma #20907 (comment)
The second commit fixes Cannot create naming rules for type parameters and local variables #18121, fixes EditorConfig Naming : type_parameter is typed to namespace? #23336, fixes Cannot refer to "private protected" in "applicable_accessibilities" #23709, and closes .editorconfig some values aren't recognizable/supported by VS #24248
The third commit implements Editorconfig backwards compatibility dilemma #20907 (comment), which closes Editorconfig backwards compatibility dilemma #20907

Screenshots

Baseline (15.8 Preview 2)

Baseline (current master branch `e3dcf8c`)

After second commit

After third commit

Customer scenario

A user attempts to create a naming rule in .editorconfig for a symbol type added in a recent release of Roslyn. Old versions treat the rule as apply to all symbols when it should apply to no symbols. A similar situation applies for accessibilities.

In addition, some of our documented member kinds (namespace and type_parameter) do not work as documented.

Bugs this fixes

#18121
#20907
#23336
#23709
#24248

Workarounds, if any

Don't use the new feature.

Risk

This change is relatively low risk for Preview 3. It builds on other new feature work in Preview 3, and helps ensure that a consistent experience is provided for the new feature now and in future releases.

Performance impact

Low. Existing working patterns were used for the new analysis.

Is this a regression from a previous update?

No.

Root cause analysis

This is a supporting change for feature work already in Preview 3. It was intended for inclusion in the "mop-up" on Monday, but infrastructure and test problems delayed the work. Now that we see the tests working again with this pull request we are ready to submit it.

How was the bug found?

Customer and internal customer reported.

Test documentation updated?

No, but many new tests are added.

Related to dotnet#20907

Fixes dotnet#18121 Fixes dotnet#23336 Closes dotnet#24248

Closes dotnet#20907

Neme12 · 2018-06-01T20:28:57Z

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

+                case SymbolKind.TypeParameter:
+                    continue;
+
+                case SymbolKind.Method when ((IMethodSymbol)currentSymbol).MethodKind == MethodKind.LocalFunction:


What about parameters of lambdas & anonymous methods?
They should be local, so this is likely missing another case for MethodKind.AnonymousFunction.

I added the following tests to TestPascalCaseSymbol_ExpectedSymbolAndAccessibility and fixed the condition here.

[InlineData("void Outer() { void Inner(int [|m|]) {} }", "void Outer() { void Inner(int M) {} }", SymbolKind.Parameter, Accessibility.NotApplicable)] [InlineData("void Outer() { System.Action<int> action = [|m|] => {} }", "void Outer() { System.Action<int> action = M => {} }", SymbolKind.Parameter, Accessibility.NotApplicable)] [InlineData("void Outer() { System.Action<int> action = ([|m|]) => {} }", "void Outer() { System.Action<int> action = (M) => {} }", SymbolKind.Parameter, Accessibility.NotApplicable)] [InlineData("void Outer() { System.Action<int> action = (int [|m|]) => {} }", "void Outer() { System.Action<int> action = (int M) => {} }", SymbolKind.Parameter, Accessibility.NotApplicable)] [InlineData("void Outer() { System.Action<int> action = delegate (int [|m|]) {} }", "void Outer() { System.Action<int> action = delegate (int M) {} }", SymbolKind.Parameter, Accessibility.NotApplicable)]

I'll wait to see if there are other tests I need to add before pushing this update.

➡️ Fixed in 40ded3c

heejaechang · 2018-06-01T23:59:56Z

src/EditorFeatures/CSharpTest/Diagnostics/NamingStyles/NamingStylesTests.cs

+        [WorkItem(20907, "https://github.com/dotnet/roslyn/issues/20907")]
+        public async Task TestPascalCaseSymbol_ExpectedSymbolAndAccessibility(string camelCaseSymbol, string pascalCaseSymbol, object symbolKind, Accessibility accessibility)
+        {
+            var alternateSymbolKind = TypeKind.Class.Equals(symbolKind) ? TypeKind.Interface : TypeKind.Class;


I am fine with this InlineData. but with this many inlineData, how one debug it? should one go through every inlinedata until right inline data is finally used for the test?

seems it is just different way to do stuff than our usual test just calling VerifyXXXX(code, epected result);

with this many inlineData, how one debug it?

You can right click the specific case in Test Explorer and Debug Selected Tests.

dpoeschl

Nice test coverage. 😄

dpoeschl · 2018-06-06T19:36:32Z

src/EditorFeatures/CSharpTest/Diagnostics/NamingStyles/EditorConfigNamingStyleParserTests.cs

+        {
+            switch (symbolOrTypeKind)
+            {
+            case TypeKind typeKind:


Looks like your indentation settings might be off (I think it's usually one deeper)

Same comment as #26589 (comment)

I'll update as a separate PR so we don't have to retest this one for just indentation.

➡️ Had to rerun tests for retargeting, so fixed in 32e150c

DustinCampbell · 2018-06-06T19:49:38Z

src/EditorFeatures/CSharpTest/Diagnostics/NamingStyles/EditorConfigNamingStyleParserTests.cs

+        [InlineData(null, new object[] { SymbolKind.Namespace, TypeKind.Class, TypeKind.Struct, TypeKind.Interface, TypeKind.Enum, SymbolKind.Property, MethodKind.Ordinary, MethodKind.LocalFunction, SymbolKind.Field, SymbolKind.Event, TypeKind.Delegate, SymbolKind.Parameter, SymbolKind.TypeParameter, SymbolKind.Local })]
+        [InlineData("property,method,invalid", new object[] { SymbolKind.Property, MethodKind.Ordinary })]
+        [InlineData("invalid", new object[] { })]
+        [InlineData("", new object[] { })]


DustinCampbell · 2018-06-06T19:49:48Z

src/EditorFeatures/CSharpTest/Diagnostics/NamingStyles/EditorConfigNamingStyleParserTests.cs

+        [InlineData(null, new[] { Accessibility.NotApplicable, Accessibility.Public, Accessibility.Internal, Accessibility.Private, Accessibility.Protected, Accessibility.ProtectedAndInternal, Accessibility.ProtectedOrInternal })]
+        [InlineData("internal,protected,invalid", new[] { Accessibility.Internal, Accessibility.Protected })]
+        [InlineData("invalid", new Accessibility[] { })]
+        [InlineData("", new Accessibility[] { })]


nice again!

jinujoseph · 2018-06-16T02:45:06Z

Approved to merge for 15.8.Preview4 (Once tests are green)

…lists

sharwell · 2018-06-19T12:06:35Z

Both integration test failures are the issue from #27933

sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 1, 2018

sharwell requested a review from a team as a code owner June 1, 2018 14:55

sharwell force-pushed the editorconfig-empty-lists branch from 64c8cd1 to 96975e1 Compare June 1, 2018 14:58

Fix handling of unrecognized naming style values

b0147a1

Related to dotnet#20907

sharwell force-pushed the editorconfig-empty-lists branch from 96975e1 to b0147a1 Compare June 1, 2018 18:16

sharwell added 2 commits June 1, 2018 14:04

Implement naming styles support for 'namespace' and 'type_parameter'

10d8f1a

Fixes dotnet#18121 Fixes dotnet#23336 Closes dotnet#24248

Implement support for 'local' and 'private protected' accessibility

2bec98e

Closes dotnet#20907

sharwell removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 1, 2018

sharwell added this to the 15.8 milestone Jun 1, 2018

Neme12 reviewed Jun 1, 2018

View reviewed changes

Fix tests for declaration name completion and parameter name analysis

40ded3c

jcouv added the Area-IDE label Jun 1, 2018

heejaechang reviewed Jun 1, 2018

View reviewed changes

dpoeschl approved these changes Jun 6, 2018

View reviewed changes

DustinCampbell approved these changes Jun 6, 2018

View reviewed changes

sharwell changed the base branch from master to dev15.8-preview3 June 6, 2018 20:27

Fix indentation of switch cases

32e150c

jinujoseph added the Needs Shiproom Approval label Jun 7, 2018

jinujoseph assigned sharwell Jun 8, 2018

Pilchie added Approved to merge and removed Needs Shiproom Approval labels Jun 10, 2018

Merge branch 'dev15.8-preview3' into editorconfig-empty-lists

8f72383

jinujoseph added Needs Shiproom Approval and removed Approved to merge labels Jun 15, 2018

sharwell changed the base branch from dev15.8-preview3 to master June 15, 2018 21:12

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels Jun 16, 2018

Merge remote-tracking branch 'dotnet/master' into editorconfig-empty-…

b97d896

…lists

sharwell merged commit a8c9afc into dotnet:master Jun 19, 2018

sharwell deleted the editorconfig-empty-lists branch June 19, 2018 12:06

allisonchou mentioned this pull request Aug 28, 2018

EditorConfig interpretted incorrectly if a rule references a symbol group that doesn't exist. #28826

Closed

sharwell mentioned this pull request Apr 8, 2019

.editorconfig naming convention for private protected #34846

Closed

Conversation

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

Uh oh!

Screenshots

Baseline (15.8 Preview 2)

Baseline (current master branch e3dcf8c)

After second commit

After third commit

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

Neme12 Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpoeschl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinujoseph commented Jun 16, 2018

Uh oh!

sharwell commented Jun 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sharwell commented Jun 1, 2018 •

edited

Loading

Baseline (current master branch `e3dcf8c`)

Neme12 Jun 1, 2018 •

edited

Loading

sharwell Jun 1, 2018 •

edited

Loading

sharwell Jun 2, 2018 •

edited

Loading