Naming style for locals by Neme12 · Pull Request #26336 · dotnet/roslyn

Neme12 · 2018-04-23T15:24:42Z

fixes #22440 (comment) / half of #18121
This PR is separate from #26165 and only contains added support for local variables.

CyrusNajmabadi · 2018-04-23T18:38:07Z

@dpoeschl for insight here. David, i remember you trying to do something to get namign styles working for these cases. But htat work was either shelved or abandoned. Can you recall what the issues were there? Just so we can make sure the same issues dont' apply here.

Thanks!

CyrusNajmabadi · 2018-04-23T18:43:07Z

src/EditorFeatures/Test2/NamingStyles/NamingStylesTests.vb

wait... do we seriously have no VB tests for thsi feature? @dpoeschl ??

there were no tests... I added them in a different file (and project), but only for locals & parameters

CyrusNajmabadi · 2018-04-23T18:44:44Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpNamingStyleDiagnosticAnalyzer.cs

I know your PR should have no problems here, but can we have some tests that these new local-variable features do not affect members inside tuples. Thanks!

added in 3416dc9

… fake SymbolAnalysisContext

Neme12 · 2018-04-23T22:08:30Z

I did some rebasing magic and then did a force push to remove the dependency on the other PR that added local functions. This PR is now completely independent and can be reviewed and merged independently.

CyrusNajmabadi · 2018-04-23T22:14:22Z

src/EditorFeatures/VisualBasicTest/Diagnostics/NamingStyles/NamingStylesTests_OptionSets.vb

+Imports Microsoft.CodeAnalysis.Options
+Imports Microsoft.CodeAnalysis.Simplification
+
+Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.Diagnostics.NamingStyles


can this not be shared with teh C# side?

Apart from the language name, it could be. Where can I put the shared code?

Probaly ServicesTestUtilities/Diagnostics... or thereabouts...

fixed in 543e6f7

CyrusNajmabadi · 2018-04-23T22:15:31Z

src/Features/Core/Portable/Diagnostics/Analyzers/NamingStyleDiagnosticAnalyzerBase.cs

+            {
+                var diagnostic = TryGetDiagnostic(
+                    syntaxContext.Compilation,
+                    syntaxContext.SemanticModel.GetDeclaredSymbol(syntaxContext.Node, syntaxContext.CancellationToken),


this feels strange. one of the kinds you register is SyntaxKind.ForEachStatement... but GetDeclaredSymbol does'nt work on a ForEachStatement does it??

yes it does

it gives the symbol of the loop variable.. note that there isn't any other node you could get the loop variable from - it's not a VariableDeclaration/Declarator, it only has a single token for the variable name. So ForEachStatement is the right node you need to use.

oh wow. that makes sense, and makes me so sad we ever allowed pieces of syntax with "identifier name" in them instead of having hte designator work figured out.

CyrusNajmabadi · 2018-04-23T22:16:08Z

src/Features/Core/Portable/Diagnostics/Analyzers/NamingStyleDiagnosticAnalyzerBase.cs

+
+            return Diagnostic.Create(descriptor, symbol.Locations.First(), builder.ToImmutable());
+
+            // Local functions


CyrusNajmabadi · 2018-04-23T22:17:26Z

src/Features/Core/Portable/Diagnostics/Analyzers/NamingStyleDiagnosticAnalyzerBase.cs

+                    return null;
+                }
+
+                var optionSet = await options.GetDocumentOptionSetAsync(location.SourceTree, cancellationToken).ConfigureAwait(false);


i'm a littler nervous about this. can we check that we have a source tree first?

sure (even though I can't think of a scenario where this would occur, so I can't add a test)

fixed in 926a468

CyrusNajmabadi · 2018-04-23T22:18:20Z

...tures/VisualBasic/Portable/Diagnostics/Analyzers/VisualBasicNamingStyleDiagnosticAnalyzer.vb

+        Inherits NamingStyleDiagnosticAnalyzerBase(Of SyntaxKind)
+
+        Private Shared _supportedSyntaxKinds As ImmutableArray(Of SyntaxKind) =
+            ImmutableArray.Create(SyntaxKind.ModifiedIdentifier)


static should be s_

actually this should just be a readonly auto-property - I forgot VB had them as well

resolved in 926a468

sharwell · 2018-04-24T13:21:35Z

src/Features/Core/Portable/Diagnostics/Analyzers/NamingStyleDiagnosticAnalyzerBase.cs

            SymbolKind.Property,
            SymbolKind.Parameter);

+        // HACK: RegisterSymbolAction doesn't work with locals & local functions


📝 This isn't a hack. It's a normal use of a stable, public API.

fixed in 559efdf

sharwell · 2018-04-24T13:25:36Z

src/Features/CSharp/Portable/Diagnostics/Analyzers/CSharpNamingStyleDiagnosticAnalyzer.cs

+            ImmutableArray.Create(
+                SyntaxKind.VariableDeclarator,
+                SyntaxKind.ForEachStatement,
+                SyntaxKind.SingleVariableDesignation);


❗️ This is missing several cases. See here:
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/1c3c14d47a36735ebac3eea13cf9d6172fad7157/StyleCop.Analyzers/StyleCop.Analyzers/NamingRules/SA1312VariableNamesMustBeginWithLowerCaseLetter.cs#L62-L70

context.RegisterSyntaxNodeAction(VariableDeclarationAction, SyntaxKind.VariableDeclaration);`

This is handled SyntaxKind.VariableDeclarator.

context.RegisterSyntaxNodeAction(CatchDeclarationAction, SyntaxKind.CatchDeclaration);

You're right, I'll fix this, thanks.

context.RegisterSyntaxNodeAction(QueryContinuationAction, SyntaxKind.QueryContinuation); context.RegisterSyntaxNodeAction(FromClauseAction, SyntaxKind.FromClause); context.RegisterSyntaxNodeAction(LetClauseAction, SyntaxKind.LetClause); context.RegisterSyntaxNodeAction(JoinClauseAction, SyntaxKind.JoinClause); context.RegisterSyntaxNodeAction(JoinIntoClauseAction, SyntaxKind.JoinIntoClause);

These are not supported as explained in the tests. They're not actually represented as locals in the symbols. There should later be a design discussion about whether we should pretend that these are in fact locals and do some workarounds to treat IRangeVariableSymbols as ILocalSymbols, or whether we should add yet another option for these. I hope it's not a requirement to sort this out right now.

I personally don't see a reason to have a separate naming style for range variables and don't think other people would need that either. But I've heard in the other PR that there has been some discussion about extending symbol specifications (to allow them to be used for custom classification I think?) and changing them to blur the distinction between 2 kinds of symbols might hinder that (and it's not a very nice solution). So I'm leaving this for the team to decide. Is it OK to keep those unsupported for now?

catch clause added in 09d2e08

sharwell · 2018-04-24T13:28:01Z

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

+{
+    void M()
+    {
+        const int local1 = 0, LOCAL2 = 0;


❓ Where are local constants handled?

Sorry, I don't understand the question. Modifier checks are done by existing code on the actual symbols (not from syntax). I just added a test to confirm that it works for locals as well. So if you specify you want uppercase local constants, but normal locals camel case, that should work and that's the purpose of this test.

It is handled here:

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

Lines 397 to 404 in 70d9902

if (Modifier.IsConst)

{

if ((kind == SymbolKind.Field && ((IFieldSymbol)symbol).IsConst) ||

(kind == SymbolKind.Local && ((ILocalSymbol)symbol).IsConst))

{

return true;

}

}

@Neme12 Thanks, that's exactly the answer I was looking for.

💡 Can you add a new test exercising both constant and non-constant locals in the same test? It will show that we don't have an unintentional overlap in the cases.

Can you add a new test exercising both constant and non-constant locals in the same test?

tests added in 3f32ac5

sharwell

Needs to add missing cases

Neme12 · 2018-04-24T16:19:25Z

src/Features/Core/Portable/Diagnostics/Analyzers/NamingStyleDiagnosticAnalyzerBase.cs

+        // Locals and fields are handled by SupportedSyntaxKinds for now.
        private static readonly ImmutableArray<SymbolKind> _symbolKinds = ImmutableArray.Create(
            SymbolKind.Event,
-            SymbolKind.Field,


This was now redundant because fields are handled by syntax as well. They have the same syntactic form as locals (both use VariableDeclaratorSyntax)

CyrusNajmabadi · 2018-04-24T18:03:26Z

I will review this tonight. If i don't, feel free to ping me.

Neme12 · 2018-05-04T08:22:56Z

@sharwell It looks like you changed GetNamingStylePreferencesAsync to use ValueTask and added a PerformanceSensitive attribute. In this PR, I actually deleted that file and made it into a local function so that it doesn't have to take 4 parameters (it has to work with SyntaxNodeAnalysisContext as well as SymbolAnalysisContext). I cannot add the attribute on a local function. Can you please give me advice what I should do here? I suppose I can just change it into a method and pass all those arguments?

Neme12 · 2018-05-04T08:38:29Z

I ended up just moving the early return outside of GetNamingStylePreferencesAsync and inlining it directly, therefore GetNamingStylePreferencesAsync now always awaits and it should be OK to keep using Task.

Neme12 · 2018-05-04T08:46:13Z

Why is this marked as "NeedWork"? Apart from the merge conflict, I've been waiting for a response. Please tell me what changes I need to do. Thanks

sharwell

Looks good aside from the performance sensitive method and ensuring #20907 reaches a design conclusion (not necessarily implementation) before shipping more changes.

sharwell · 2018-05-04T17:18:25Z

src/Features/Core/Portable/Diagnostics/Analyzers/NamingStyleDiagnosticAnalyzerBase.cs

+
+            // Local functions
+
+            async Task<NamingStylePreferences> GetNamingStylePreferencesAsync()


❗️ Make this a method to preserve the PerformanceSensitive attribute

@sharwell Why is this method still performance sensitive given that the reason for ValueTask was the early return which now happens before this method is called?

@Neme12 The reason for ValueTask is the call to GetDocumentOptionSetAsync often returns synchronously. The early return was not the common case. Also, changes to the behavior documented in [PerformanceSensitive] requires re-running the scenario in the linked URI to gather before/after data proving that a regression on the indicated metrics did not occur.

I see, that makes sense. I didn't think of that. Thanks.

fixed in df39892

sharwell · 2018-05-04T18:28:54Z

@Neme12 We should reach a conclusion on the related design issue #20907 Monday 7 April which would allow this to proceed.

…method

sharwell · 2018-05-09T15:34:57Z

@jinujoseph For approval to merge. This pull request does not implement the accessibility modification described in #20907, but I do not see any reason why that should block progress of this incremental improvement.

jinujoseph · 2018-05-09T17:05:46Z

Approved to merged for 15.8.preview2

Neme12 requested a review from a team as a code owner April 23, 2018 15:24

Neme12 mentioned this pull request Apr 23, 2018

Naming style for local functions #26165

Merged

CyrusNajmabadi reviewed Apr 23, 2018

View reviewed changes

Neme12 added 14 commits April 23, 2018 22:47

Adding workaround to analyze local functions by syntax action

336f3af

Using separate SymbolAction and SyntaxNodeAction rather than creating…

cead672

… fake SymbolAnalysisContext

Addressing Cyrus' feedback

0605703

Details

3dc47c7

Addressing nits

2e712e8

Adding C# locals

8dd9a43

Adding VB locals

da5167f

Adding EditorConfig option

1356d2f

Adding VS option

ab0c075

Moving SyntaxNodeAction to shared code

4ef792d

Sharing all the code

c4a5fe3

Adding tests with const locals & fields

7261663

Adding new VB test

7e306d0

Removing trace of local functions

18659c7

Neme12 force-pushed the localNamingStyle branch from 2d0d78c to 18659c7 Compare April 23, 2018 22:01

CyrusNajmabadi reviewed Apr 23, 2018

View reviewed changes

jcouv added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 23, 2018

jcouv added this to the 15.8 milestone Apr 23, 2018

Tweaks

926a468

Neme12 added 2 commits April 24, 2018 13:44

Extracting shared test helper for option sets

543e6f7

Adding negative tests with tuples

3416dc9

sharwell reviewed Apr 24, 2018

View reviewed changes

sharwell suggested changes Apr 24, 2018

View reviewed changes

Neme12 added 4 commits April 24, 2018 15:51

Removing "HACK" comment

559efdf

Adding support for catch clause variables

09d2e08

Fixing NRE

be3845f

Removing redundant SymbolKind.Field

f43475b

Neme12 commented Apr 24, 2018

View reviewed changes

Adding tests with options for both locals & const locals

3f32ac5

zaytsev-victor mentioned this pull request Apr 26, 2018

Some refactoring doesn't respect naming rules #26417

Closed

Moving early return outside of GetNamingStylePreferencesAsync

8a46736

sharwell suggested changes May 4, 2018

View reviewed changes

Changing GetNamingStylePreferencesAsync into a performance sensitive …

df39892

…method

sharwell approved these changes May 7, 2018

View reviewed changes

sharwell self-assigned this May 7, 2018

jinujoseph added the Approved to merge label May 9, 2018

sharwell merged commit dfe02b6 into dotnet:master May 9, 2018

Neme12 deleted the localNamingStyle branch May 9, 2018 17:14


		return Diagnostic.Create(descriptor, symbol.Locations.First(), builder.ToImmutable());

		// Local functions

	if (Modifier.IsConst)
	{
	if ((kind == SymbolKind.Field && ((IFieldSymbol)symbol).IsConst) \|\|
	(kind == SymbolKind.Local && ((ILocalSymbol)symbol).IsConst))
	{
	return true;
	}
	}


		// Local functions

		async Task<NamingStylePreferences> GetNamingStylePreferencesAsync()

Conversation

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

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 23, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 23, 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!

CyrusNajmabadi Apr 23, 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!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 23, 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!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 24, 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!

Neme12 Apr 24, 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!

Choose a reason for hiding this comment

Uh oh!

sharwell Apr 24, 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!

Neme12 Apr 24, 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!

Neme12 Apr 24, 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

Neme12 commented Apr 23, 2018 •

edited

Loading

Neme12 Apr 23, 2018 •

edited

Loading

Neme12 commented Apr 23, 2018 •

edited

Loading

Neme12 Apr 23, 2018 •

edited

Loading

CyrusNajmabadi Apr 23, 2018 •

edited

Loading

Neme12 Apr 23, 2018 •

edited

Loading

Neme12 Apr 24, 2018 •

edited

Loading

Neme12 Apr 24, 2018 •

edited

Loading

sharwell Apr 24, 2018 •

edited

Loading

Neme12 Apr 24, 2018 •

edited

Loading

Neme12 Apr 24, 2018 •

edited

Loading

Neme12 Apr 24, 2018 •

edited

Loading

Neme12 Apr 24, 2018 •

edited

Loading

Neme12 Apr 24, 2018 •

edited

Loading

Neme12 Apr 24, 2018 •

edited

Loading

Neme12 commented May 4, 2018 •

edited

Loading

Neme12 May 4, 2018 •

edited

Loading

sharwell May 4, 2018 •

edited

Loading