Merge pull request #3196 from Kevin-Andrew/dowhile

Issue #2801: SA1500 fires for the while clause of do/while statement

Author: sharwell

StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/LayoutRules/SA1500CodeFixProvider.cs

@@ -57,13 +57,13 @@ private static async Task<Document> GetTransformedDocumentAsync(Document documen
 
             var settings = SettingsHelper.GetStyleCopSettings(document.Project.AnalyzerOptions, syntaxRoot.SyntaxTree, cancellationToken);
             var braceToken = syntaxRoot.FindToken(diagnostic.Location.SourceSpan.Start);
-            var tokenReplacements = GenerateBraceFixes(settings.Indentation, ImmutableArray.Create(braceToken));
+            var tokenReplacements = GenerateBraceFixes(settings, ImmutableArray.Create(braceToken));
 
             var newSyntaxRoot = syntaxRoot.ReplaceTokens(tokenReplacements.Keys, (originalToken, rewrittenToken) => tokenReplacements[originalToken]);
             return document.WithSyntaxRoot(newSyntaxRoot);
         }
 
-        private static Dictionary<SyntaxToken, SyntaxToken> GenerateBraceFixes(IndentationSettings indentationSettings, ImmutableArray<SyntaxToken> braceTokens)
+        private static Dictionary<SyntaxToken, SyntaxToken> GenerateBraceFixes(StyleCopSettings settings, ImmutableArray<SyntaxToken> braceTokens)
         {
             var tokenReplacements = new Dictionary<SyntaxToken, SyntaxToken>();
 
@@ -72,7 +72,7 @@ private static Dictionary<SyntaxToken, SyntaxToken> GenerateBraceFixes(Indentati
                 var braceLine = LocationHelpers.GetLineSpan(braceToken).StartLinePosition.Line;
                 var braceReplacementToken = braceToken;
 
-                var indentationSteps = DetermineIndentationSteps(indentationSettings, braceToken);
+                var indentationSteps = DetermineIndentationSteps(settings.Indentation, braceToken);
 
                 var previousToken = braceToken.GetPreviousToken();
 
@@ -102,19 +102,23 @@ private static Dictionary<SyntaxToken, SyntaxToken> GenerateBraceFixes(Indentati
                             AddReplacement(tokenReplacements, previousToken, previousToken.WithTrailingTrivia(previousTokenNewTrailingTrivia));
                         }
 
-                        braceReplacementToken = braceReplacementToken.WithLeadingTrivia(IndentationHelper.GenerateWhitespaceTrivia(indentationSettings, indentationSteps));
+                        braceReplacementToken = braceReplacementToken.WithLeadingTrivia(IndentationHelper.GenerateWhitespaceTrivia(settings.Indentation, indentationSteps));
                     }
 
                     // Check if we need to apply a fix after the brace. No fix is needed when:
                     // - The closing brace is followed by a semi-colon or closing paren
                     // - The closing brace is the last token in the file
+                    // - The closing brace is followed by the while expression of a do/while loop and the
+                    //   allowDoWhileOnClosingBrace setting is enabled.
                     var nextToken = braceToken.GetNextToken();
                     var nextTokenLine = nextToken.IsKind(SyntaxKind.None) ? -1 : LocationHelpers.GetLineSpan(nextToken).StartLinePosition.Line;
                     var isMultiDimensionArrayInitializer = braceToken.IsKind(SyntaxKind.OpenBraceToken) && braceToken.Parent.IsKind(SyntaxKind.ArrayInitializerExpression) && braceToken.Parent.Parent.IsKind(SyntaxKind.ArrayInitializerExpression);
+                    var allowDoWhileOnClosingBrace = settings.LayoutRules.AllowDoWhileOnClosingBrace && nextToken.IsKind(SyntaxKind.WhileKeyword) && (braceToken.Parent?.IsKind(SyntaxKind.Block) ?? false) && (braceToken.Parent.Parent?.IsKind(SyntaxKind.DoStatement) ?? false);
 
                     if ((nextTokenLine == braceLine) &&
                         (!braceToken.IsKind(SyntaxKind.CloseBraceToken) || !IsValidFollowingToken(nextToken)) &&
-                        !isMultiDimensionArrayInitializer)
+                        !isMultiDimensionArrayInitializer &&
+                        !allowDoWhileOnClosingBrace)
                     {
                         var sharedTrivia = nextToken.LeadingTrivia.WithoutTrailingWhitespace();
                         var newTrailingTrivia = braceReplacementToken.TrailingTrivia
@@ -135,7 +139,7 @@ private static Dictionary<SyntaxToken, SyntaxToken> GenerateBraceFixes(Indentati
                                 newIndentationSteps = Math.Max(0, newIndentationSteps - 1);
                             }
 
-                            AddReplacement(tokenReplacements, nextToken, nextToken.WithLeadingTrivia(IndentationHelper.GenerateWhitespaceTrivia(indentationSettings, newIndentationSteps)));
+                            AddReplacement(tokenReplacements, nextToken, nextToken.WithLeadingTrivia(IndentationHelper.GenerateWhitespaceTrivia(settings.Indentation, newIndentationSteps)));
                         }
 
                         braceReplacementToken = braceReplacementToken.WithTrailingTrivia(newTrailingTrivia);
@@ -284,7 +288,7 @@ protected override async Task<SyntaxNode> FixAllInDocumentAsync(FixAllContext fi
 
                 var settings = SettingsHelper.GetStyleCopSettings(document.Project.AnalyzerOptions, syntaxRoot.SyntaxTree, fixAllContext.CancellationToken);
 
-                var tokenReplacements = GenerateBraceFixes(settings.Indentation, tokens);
+                var tokenReplacements = GenerateBraceFixes(settings, tokens);
 
                 return syntaxRoot.ReplaceTokens(tokenReplacements.Keys, (originalToken, rewrittenToken) => tokenReplacements[originalToken]);
             }

StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs

@@ -304,5 +304,164 @@ private void Bar()
 
             await VerifyCSharpFixAsync(testCode, expectedDiagnostics, fixedTestCode, CancellationToken.None).ConfigureAwait(false);
         }
+
+        /// <summary>
+        /// Verifies that no diagnostics are reported for the do/while loop when the <see langword="while"/>
+        /// expression is on the same line as the closing brace and the setting is enabled.
+        /// </summary>
+        /// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
+        [Fact]
+        public async Task TestDoWhileOnClosingBraceWithAllowSettingAsync()
+        {
+            var testSettings = @"
+{
+    ""settings"": {
+        ""layoutRules"": {
+            ""allowDoWhileOnClosingBrace"": true
+        }
+    }
+}";
+
+            var testCode = @"public class Foo
+{
+    private void Bar()
+    {
+        var x = 0;
+
+        do
+        {
+            x = 1;
+        } while (x == 0);
+    }
+}";
+
+            var test = new CSharpTest
+            {
+                TestCode = testCode,
+                Settings = testSettings,
+            };
+
+            await test.RunAsync(CancellationToken.None).ConfigureAwait(false);
+        }
+
+        /// <summary>
+        /// Verifies that diagnostics will be reported for the invalid <see langword="while"/> loop that
+        /// is on the same line as the closing brace which is not part of a <c>do/while</c> loop. This
+        /// ensures that the <c>allowDoWhileOnClosingBrace</c> setting is only applicable to a <c>do/while</c>
+        /// loop and will not mistakenly allow a plain <see langword="while"/> loop after any arbitrary
+        /// closing brace.
+        /// </summary>
+        /// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
+        [Fact]
+        public async Task TestJustWhileLoopOnClosingBraceWithAllowDoWhileOnClosingBraceSettingAsync()
+        {
+            var testSettings = @"
+{
+    ""settings"": {
+        ""layoutRules"": {
+            ""allowDoWhileOnClosingBrace"": true
+        }
+    }
+}";
+
+            var testCode = @"public class Foo
+{
+    private void Bar()
+    {
+        var x = 0;
+
+        while (x == 0)
+        {
+            x = 1;
+        [|}|] while (x == 0)
+        {
+            x = 1;
+        }
+    }
+}";
+
+            var fixedCode = @"public class Foo
+{
+    private void Bar()
+    {
+        var x = 0;
+
+        while (x == 0)
+        {
+            x = 1;
+        }
+        while (x == 0)
+        {
+            x = 1;
+        }
+    }
+}";
+
+            var test = new CSharpTest
+            {
+                TestCode = testCode,
+                FixedCode = fixedCode,
+                Settings = testSettings,
+            };
+
+            await test.RunAsync(CancellationToken.None).ConfigureAwait(false);
+        }
+
+        /// <summary>
+        /// Verifies that no diagnostics are reported for the do/while loop when the <see langword="while"/>
+        /// expression is on the same line as the closing brace and the setting is allowed.
+        /// </summary>
+        /// <remarks>
+        /// <para>The "Invalid do ... while #6" code in the <see cref="TestDoWhileInvalidAsync"/> unit test
+        /// should account for the proper fix when the <c>allowDoWhileOnClosingBrace</c> is <see langword="false"/>,
+        /// which is the default.</para>
+        /// </remarks>
+        /// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
+        [Fact]
+        public async Task TestFixDoWhileOnClosingBraceWithAllowSettingAsync()
+        {
+            var testSettings = @"
+{
+    ""settings"": {
+        ""layoutRules"": {
+            ""allowDoWhileOnClosingBrace"": true
+        }
+    }
+}";
+
+            var testCode = @"public class Foo
+{
+    private void Bar()
+    {
+        var x = 0;
+
+        do
+        {
+            x = 1; [|}|] while (x == 0);
+    }
+}";
+
+            var fixedTestCode = @"public class Foo
+{
+    private void Bar()
+    {
+        var x = 0;
+
+        do
+        {
+            x = 1;
+        } while (x == 0);
+    }
+}";
+
+            var test = new CSharpTest
+            {
+                TestCode = testCode,
+                FixedCode = fixedTestCode,
+                Settings = testSettings,
+            };
+
+            await test.RunAsync(CancellationToken.None).ConfigureAwait(false);
+        }
     }
 }

StyleCop.Analyzers/StyleCop.Analyzers.Test/Settings/SettingsUnitTests.cs

@@ -37,6 +37,8 @@ public void VerifySettingsDefaults()
 
             Assert.NotNull(styleCopSettings.LayoutRules);
             Assert.Equal(OptionSetting.Allow, styleCopSettings.LayoutRules.NewlineAtEndOfFile);
+            Assert.True(styleCopSettings.LayoutRules.AllowConsecutiveUsings);
+            Assert.False(styleCopSettings.LayoutRules.AllowDoWhileOnClosingBrace);
 
             Assert.NotNull(styleCopSettings.SpacingRules);
             Assert.NotNull(styleCopSettings.ReadabilityRules);

StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs

@@ -235,7 +235,7 @@ private static void CheckBraces(SyntaxNodeAnalysisContext context, SyntaxToken o
             CheckBraceToken(context, openBraceToken);
             if (checkCloseBrace)
             {
-                CheckBraceToken(context, closeBraceToken);
+                CheckBraceToken(context, closeBraceToken, openBraceToken);
             }
         }
 
@@ -247,7 +247,7 @@ private static bool InitializerExpressionSharesLine(InitializerExpressionSyntax
             return (index > 0) && (parent.Expressions[index - 1].GetEndLine() == parent.Expressions[index].GetLine());
         }
 
-        private static void CheckBraceToken(SyntaxNodeAnalysisContext context, SyntaxToken token)
+        private static void CheckBraceToken(SyntaxNodeAnalysisContext context, SyntaxToken token, SyntaxToken openBraceToken = default)
         {
             if (token.IsMissing)
             {
@@ -284,6 +284,21 @@ private static void CheckBraceToken(SyntaxNodeAnalysisContext context, SyntaxTok
                     // last token of this file
                     return;
 
+                case SyntaxKind.WhileKeyword:
+                    // Because the default Visual Studio code completion snippet for a do-while loop
+                    // places the while expression on the same line as the closing brace, some users
+                    // may want to allow that and not have SA1500 report it as a style error.
+                    if (context.GetStyleCopSettings(context.CancellationToken).LayoutRules.AllowDoWhileOnClosingBrace)
+                    {
+                        if (openBraceToken.Parent.IsKind(SyntaxKind.Block)
+                            && openBraceToken.Parent.Parent.IsKind(SyntaxKind.DoStatement))
+                        {
+                            return;
+                        }
+                    }
+
+                    break;
+
                 default:
                     break;
                 }

StyleCop.Analyzers/StyleCop.Analyzers/Settings/ObjectModel/LayoutSettings.cs

@@ -18,13 +18,19 @@ internal class LayoutSettings
         /// </summary>
         private readonly bool allowConsecutiveUsings;
 
+        /// <summary>
+        /// This is the backing field of the <see cref="AllowDoWhileOnClosingBrace"/> property.
+        /// </summary>
+        private readonly bool allowDoWhileOnClosingBrace;
+
         /// <summary>
         /// Initializes a new instance of the <see cref="LayoutSettings"/> class.
         /// </summary>
         protected internal LayoutSettings()
         {
             this.newlineAtEndOfFile = OptionSetting.Allow;
             this.allowConsecutiveUsings = true;
+            this.allowDoWhileOnClosingBrace = false;
         }
 
         /// <summary>
@@ -37,6 +43,7 @@ protected internal LayoutSettings(JsonObject layoutSettingsObject, AnalyzerConfi
         {
             OptionSetting? newlineAtEndOfFile = null;
             bool? allowConsecutiveUsings = null;
+            bool? allowDoWhileOnClosingBrace = null;
 
             foreach (var kvp in layoutSettingsObject)
             {
@@ -50,6 +57,10 @@ protected internal LayoutSettings(JsonObject layoutSettingsObject, AnalyzerConfi
                     allowConsecutiveUsings = kvp.ToBooleanValue();
                     break;
 
+                case "allowDoWhileOnClosingBrace":
+                    allowDoWhileOnClosingBrace = kvp.ToBooleanValue();
+                    break;
+
                 default:
                     break;
                 }
@@ -63,15 +74,20 @@ protected internal LayoutSettings(JsonObject layoutSettingsObject, AnalyzerConfi
             };
 
             allowConsecutiveUsings ??= AnalyzerConfigHelper.TryGetBooleanValue(analyzerConfigOptions, "stylecop.layout.allowConsecutiveUsings");
+            allowDoWhileOnClosingBrace ??= AnalyzerConfigHelper.TryGetBooleanValue(analyzerConfigOptions, "stylecop.layout.allowDoWhileOnClosingBrace");
 
             this.newlineAtEndOfFile = newlineAtEndOfFile.GetValueOrDefault(OptionSetting.Allow);
             this.allowConsecutiveUsings = allowConsecutiveUsings.GetValueOrDefault(true);
+            this.allowDoWhileOnClosingBrace = allowDoWhileOnClosingBrace.GetValueOrDefault(false);
         }
 
         public OptionSetting NewlineAtEndOfFile =>
             this.newlineAtEndOfFile;
 
         public bool AllowConsecutiveUsings =>
             this.allowConsecutiveUsings;
+
+        public bool AllowDoWhileOnClosingBrace =>
+            this.allowDoWhileOnClosingBrace;
     }
 }

documentation/Configuration.md

@@ -420,6 +420,7 @@ The following properties are used to configure layout rules in StyleCop Analyzer
 | --- | --- | --- | --- |
 | `newlineAtEndOfFile` | `"allow"` | 1.0.0 | Specifies the handling for newline characters which appear at the end of a file |
 | `allowConsecutiveUsings` | `true` | 1.1.0 | Specifies if SA1519 will allow consecutive using statements without braces |
+| `allowDoWhileOnClosingBrace` | `false` | >1.2.0 | Specifies if SA1500 will allow the `while` expression of a `do`/`while` loop to be on the same line as the closing brace, as is generated by the default code snippet of Visual Studio |
 
 ### Lines at End of File
 
@@ -441,6 +442,13 @@ The `allowConsecutiveUsings` property specifies the behavior:
 This only allows omitting the braces for a using followed by another using statement. A using statement followed by any other type of statement will still
 require braces to used.
 
+### Do-While Loop Placement
+
+The behavior of [SA1500](SA1500.md) can be customized regarding the manner in which the `while` expression of a `do`/`while` loop is allowed to be placed. The `allowDoWhileOnClosingBrace` property specified the behavior:
+
+* `true`: the `while` expression of a `do`/`while` loop may be placed on the same line as the closing brace or on a separate line
+* `false`: the `while` expression of a `do`/`while` loop must be on a separate line from the closing brace
+
 ## Documentation Rules
 
 This section describes the features of documentation rules which can be configured in **stylecop.json**. Each of the described properties are configured in the `documentationRules` object, which is shown in the following sample file.

documentation/SA1500.md

@@ -19,6 +19,8 @@
 
 The opening or closing brace within a C# statement, element, or expression is not placed on its own line.
 
+> :memo: The behavior of this rule can change based on the configuration of the `allowDoWhileOnClosingBrace` property in **stylecop.json**. See [Configuration.md](Configuration.md#Layout-Rules) for more information.
+
 ## Rule description
 
 A violation of this rule occurs when the opening or closing brace within a statement, element, or expression is not placed on its own line. For example: