UseInferredMemberName: use one code style option and share more code by jcouv · Pull Request #23506 · dotnet/roslyn

jcouv · 2017-11-30T23:24:19Z

Customer scenario

We used to expose language-specific code style options for inferred member names:

csharp_prefer_inferred_tuple_names = true:suggestion 
csharp_prefer_inferred_anonymous_type_member_names = true:suggestion 
visual_basic_prefer_inferred_tuple_names = true:suggestion 
visual_basic_prefer_inferred_anonymous_type_member_names = true:suggestion

With this PR, we now use fewer options, which can still be used to achieve the same effect:

[*.cs]
dotnet_style_prefer_inferred_tuple_names = true:suggestion 
dotnet_style_prefer_inferred_anonymous_type_member_names = true:suggestion 

[*.vb]
dotnet_style_prefer_inferred_tuple_names = true:suggestion 
dotnet_style_prefer_inferred_anonymous_type_member_names = true:suggestion

I also noticed that the analyzers and code fixers for those options share a lot of code between C# and VB, so I pulled that code into base classes.

Bugs this fixes

#23313

Risk

Low. The change is limited to one analyzer+code fixer.

Performance impact

None. No added complexity or logic.

Is this a regression from a previous update?

No.

How was the bug found?

Reported by Kasey as she was documenting the code fixers.

jasonmalinowski · 2017-11-30T23:33:35Z

...tures/CSharp/Portable/UseInferredMemberName/CSharpUseInferredMemberNameDiagnosticAnalyzer.cs

            => context.RegisterSyntaxNodeAction(AnalyzeSyntax, SyntaxKind.NameColon, SyntaxKind.NameEquals);

-        private void AnalyzeSyntax(SyntaxNodeAnalysisContext context)
+        override protected void LanguageSpecificAnalyzeSyntax(SyntaxNodeAnalysisContext context, SyntaxTree syntaxTree, OptionSet optionSet)


protected override

jasonmalinowski · 2017-11-30T23:34:55Z

src/Workspaces/Core/Portable/CodeStyle/CodeStyleOptions.cs

+            nameof(PreferInferredTupleNames),
+            defaultValue: TrueWithSuggestionEnforcement,
+            storageLocations: new OptionStorageLocation[] {
+                EditorConfigStorageLocation.ForBoolCodeStyleOption("dotnet_style_prefer_inferred_tuple_names"),


Might be worth calling out somewhere this is an explicit back compat break.

@jinujoseph @kuhlenh Is there a place to document code compat breaks in code style options?

I can add a note to the documentation that it used to be separate but now it is not (and that the previous options are not supported). I never documented the previous version, so we should be ok...

jasonmalinowski · 2017-11-30T23:39:50Z

...isualBasic/Portable/UseInferredMemberName/VisualBasicUseInferredMemberNameCodeFixProvider.vb

+            Select Case node.Kind
+                Case SyntaxKind.NameColonEquals
+                    editor.RemoveNode(node, SyntaxRemoveOptions.KeepExteriorTrivia Or SyntaxRemoveOptions.AddElasticMarker)
+                    Exit Select


This is implied...?

@jasonmalinowski I didn't understand the question

You don't need to Exit Select at the end of a case. This isn't C.

(Exit Select in VB is not mandatory)

jasonmalinowski · 2017-11-30T23:42:25Z

...tures/Core/Portable/UseInferredMemberName/AbstractUseInferredMemberNameDiagnosticAnalyzer.cs

+            var cancellationToken = context.CancellationToken;
+
+            var syntaxTree = context.Node.SyntaxTree;
+            var optionSet = context.Options.GetDocumentOptionSetAsync(syntaxTree, cancellationToken).GetAwaiter().GetResult();


One thing @sharwell recently noticed: this isn't exactly a cheap call, and it's quite wasteful if you're not going to be on a node that you can fix -- i.e. checking kind first is often a better idea. I'll let him speak to whether he's seen this one pop and it's worth fixing now, or not.

@sharwell Any thoughts on Jason's perf question?

➡️ No need to fix it as part of this pull request. The new form is equivalent to the old form, so if it matters it's going to be showing up separately anyway.

jasonmalinowski · 2017-11-30T23:42:35Z

...tures/Core/Portable/UseInferredMemberName/AbstractUseInferredMemberNameDiagnosticAnalyzer.cs

+
+            var syntaxTree = context.Node.SyntaxTree;
+            var optionSet = context.Options.GetDocumentOptionSetAsync(syntaxTree, cancellationToken).GetAwaiter().GetResult();
+            if (optionSet == null)


I know you're preserving existing code, but this can't happen.

jmarolf

jmarolf · 2017-12-01T19:03:05Z

As long as we are OK not handing back compat I am fine with this.

jcouv · 2017-12-08T05:12:25Z

@Pilchie for ask-mode approval for 15.6. Details up top. Thanks

* dotnet/master: (39 commits) Prefer by-val methods over in methods in overload resolution (dotnet#23122) Update build to account for additional Mono.Cecil assemblies Fix build break Enable multicore JIT in the compilers (dotnet#23173) Separate debugging workspace service and EnC service (dotnet#23630) UseInferredMemberName: use one code style option and share more code (dotnet#23506) Add negative test cases Remove unnecessary Mono.Cecil reference Updated formatting of decompiler license notice Use sentence case for the decompiler legal notice Use ConcurrentDictionary to avoid locks in IsGeneratedCode and HasHiddenRegions Recognize condition with logical negation Refactor so that GetSymbolInfo can be called last Change the code fix to find the expression so that we don't need to unwrap the argument Add check for null argument syntax Fix argument names Formatting adjustments Add VB tests for omitted arguments Add WorkItem attributes Pool the Stopwatch instance used in the analyzer driver ...

jcouv · 2017-12-30T07:11:20Z

@kuhlenh Should we document the new/merged option?

jcouv · 2018-01-04T23:02:18Z

Ping @kuhlenh. This fix was merged to 15.6 and can be documented. Thanks

kuhlenh · 2018-01-05T23:19:45Z

@jcouv -- Beautiful! Will get it documented ASAP. Thank you!

UseInferredMemberName: use one code style option and share more code

2e76052

jcouv added the Area-IDE label Nov 30, 2017

jcouv added this to the 15.6 milestone Nov 30, 2017

jcouv self-assigned this Nov 30, 2017

jcouv requested a review from a team as a code owner November 30, 2017 23:24

jasonmalinowski reviewed Nov 30, 2017

View reviewed changes

jasonmalinowski approved these changes Nov 30, 2017

View reviewed changes

jmarolf approved these changes Dec 1, 2017

View reviewed changes

Remove unnecessary Exit Selects

4a09fe4

Pilchie approved these changes Dec 8, 2017

View reviewed changes

Pilchie added the Approved to merge label Dec 8, 2017

jcouv merged commit de637d5 into dotnet:master Dec 8, 2017

jcouv deleted the ide-options branch December 8, 2017 16:43

Conversation

jcouv commented Nov 30, 2017

Customer scenario

Bugs this fixes

Risk

Performance impact

Is this a regression from a previous update?

How was the bug found?

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!

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!

jmarolf left a comment

Choose a reason for hiding this comment

Uh oh!

jmarolf commented Dec 1, 2017

Uh oh!

jcouv commented Dec 8, 2017

Uh oh!

jcouv commented Dec 30, 2017

Uh oh!

jcouv commented Jan 4, 2018

Uh oh!

kuhlenh commented Jan 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants