Add analyzer/fixer for informal IDE brace style with statements.

[CyrusNajmabadi]

Adds a new analyzer/fixer for the informal IDE style that we shouldn't bunch of statements too closely. Specifically, if we have a multiline block, and that block is followed by a statement, then there should be a blank line between the block and the statement. In other words, we prefer:

while (...)
{
   // ...
}

if (...)
{
}

return;

over

while (...)
{
   // ...
}
if (...)
{
}
return;

[CyrusNajmabadi]

Could someone remind me how to run this against Roslyn itself? I want to see if i missed any cases. Thanks!

[sharwell]

Here's the test cases for this rule:

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1513UnitTests.cs
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/LayoutRules/SA1513CSharp7UnitTests.cs

[CyrusNajmabadi]

Here's the test cases for this rule:

Added the cases. There are two differences between SA1513 and my code currently. Specifically:

1. My code (currently) doesn't touch curlies outside of block-statements. I feel like code to adjust how members are spaced should be different, and out of scope of this analyzer/fixer.
2. My code does not touch a block-statement followed by a case-clause. This is up in the air for me if we would insist this be fixed or not.

Note that in both of these cases we simply do not care what the code is. So that leaves us in the state we are in today. IMO, we can decide to be more stringent about the coding here later if we want to.

Thoughts @sharwell ?

[CyrusNajmabadi]

@sharwell #4647 (comment) ^ plz :)

[CyrusNajmabadi]

Can see results of this here: dotnet/roslyn#50201

[sharwell]

@CyrusNajmabadi thanks for detailing the differences. No objections.

[CyrusNajmabadi]

@sharwell i have no idea why this particular test is failing :)

[CyrusNajmabadi]

Specifically:

Microsoft.CodeQuality.Analyzers.QualityGuidelines.UnitTests.DoNotInitializeUnnecessarilyTests.Diagnostics_InitializerRemoved

Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException : Context: Fix all in solution\r\nExpected '2' iterations but found '1' iterations.\r\nAssert.Equal() Failure\r\nExpected: 2\r\nActual:   1

[Evangelink]

I am really surprised that dotnet/roslyn#50195 got closed and this analyzer pops-up as a Roslyn specific analyzer.

[Youssef1313]

@Evangelink Didn't you fix this before (the empty line before the table)? or I'm mis-remembering?

[Evangelink]

I don't recall doing the fix :)

[CyrusNajmabadi]

I am really surprised that dotnet/roslyn#50195 got closed and this analyzer pops-up as a Roslyn specific analyzer.

Why? As I said in that issue:

Closing out. Such an analyzer can already be written today. There is no need for it to be in roslyn.

This pr follows that opinion. I don't think this should be part of roslyn. It can just be something written externally.

This was also something Manish asked if I would do when I did my last set of analyzers :-)

[Youssef1313]

await Verify.VerifyCodeFixAsync(code, code); is shorter.

[Evangelink]

I don't want to pollute your PR too much so I am just going to ask one last question. Aren't all rules described here: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/formatting-rules#c-formatting-rules part of the roslyn repo? If they are what makes the new line after brace different from all the other new line rules?

[CyrusNajmabadi]

If they are what makes the new line after brace different from all the other new line rules?

I believe the set in roslyn is the core set we want to support (esp. for legacy reasons). I don't want to add new ad-hoc rules like this. :-)

A core reason for this is just how ad-hoc this is and how little I want to be beholden to the rules here. As a repo rule, this can be tweaked to our hearts content. As an official rule, our hands are much more tied with what we can do here.

[Evangelink]

I don't recall doing the fix :)

[Evangelink]

We have some code-fixes doing a loop over the Diagnostics and some other doing .First() what is the right way?

[Youssef1313]

dotnet/roslyn#44824 (comment)

[Evangelink]

It might be worth doing an anlyzer for this then.

[CyrusNajmabadi]

I think this logic is correct. If there were multiple diagnostics for the same token, we'd only want to process one of them anyways. If we looked at all diagnostics, we'd have to dedupe. So this has the same effect.

[Evangelink]

I would not test the FixedCode when we don't expect a diagnostic to be raised. If you go with this change, it applies to some other tests.

[CyrusNajmabadi]

I disagree... i'm trying to doc expected behavior here about what is/isn't controlled by this analyzer. this is very normal IMO for full testing of these types.

[Youssef1313]

You'll need to run msbuild /t:pack at repo root to update auto-generated files.

[CyrusNajmabadi]

You'll need to run msbuild /t:pack at repo root to update auto-generated files.

Thanks!

[codecov]

Codecov Report

Merging #4647 (85c8abc) into master (abb5bf7) will increase coverage by 0.00%.
The diff coverage is 99.90%.

@@            Coverage Diff            @@
##           master    #4647     +/-   ##
=========================================
  Coverage   95.77%   95.77%             
=========================================
  Files        1193     1198      +5     
  Lines      269067   270078   +1011     
  Branches    16245    16273     +28     
=========================================
+ Hits       257706   258675    +969     
- Misses       9278     9282      +4     
- Partials     2083     2121     +38     

[mavasani]

I am really surprised that dotnet/roslyn#50195 got closed and this analyzer pops-up as a Roslyn specific analyzer.

Personally, I completely agree. There is nothing Roslyn.sln specific about all these formatting analyzers that we are adding them to Roslyn.Diagnostics.Analyzers, our private package. These are extremely useful rules, which should either ship as part of IDE0055: Fix formatting OR a new IDExxxx diagnostic for experimental formatting rules. Either way - the current approach is causing us to block shipping these useful formatting rules to VS customers, which is very unfortunate.

[mavasani]

This pr follows that opinion. I don't think this should be part of roslyn. It can just be something written externally.
This was also something Manish asked if I would do when I did my last set of analyzers :-)

Cyrus, this was done as part of a temporary experiment to enable dogfooding these rules on Roslyn.sln. I had presented my pushback on this approach even back then, but agreed contingent on the plan that these analyzers will be deleted in 16.8 and appropriately moved into Roslyn repo as part of existing or new formatting diagnostic ID. This unfortunately did not happen.

I have started an email thread around this and proposed we start with deleting these temporary formatting rules from Roslyn.Diagnostics.Analyzers, self-inflict some pain on Roslyn.sln devs and help fast track shipping these as part of VS/CodeStyle NuGet for external customers.

[mavasani]

Marking this as blocked until we finalize the long term ship strategy for these new, useful formatting rules to external customers. I have no pushback on the analyzer/fixer as such.

[CyrusNajmabadi]

Sure. I can move back to roslyn.

[CyrusNajmabadi]

This merged into roslyn itself.