From bfbb4ca4a80280f49e08df51423ea8db9dbc4222 Mon Sep 17 00:00:00 2001 From: Kevin-Andrew Date: Sat, 5 Sep 2020 21:34:48 -0700 Subject: [PATCH 1/4] Issue #2801: SA1500 fires for the while clause of do/while statement Provide new configuration setting, `layoutRules.allowDoWhileOnClosingBrace` such that when enabled, the following will be allowed: ``` do { Console.WriteLine("test"); } while (false); ``` --- .../LayoutRules/SA1500CodeFixProvider.cs | 18 ++- .../SA1500/SA1500UnitTests.DoWhiles.cs | 149 ++++++++++++++++++ .../Settings/SettingsUnitTests.cs | 2 + ...sForMultiLineStatementsMustNotShareLine.cs | 20 ++- .../Settings/ObjectModel/LayoutSettings.cs | 13 ++ documentation/Configuration.md | 8 + documentation/SA1500.md | 2 + 7 files changed, 203 insertions(+), 9 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/LayoutRules/SA1500CodeFixProvider.cs b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/LayoutRules/SA1500CodeFixProvider.cs index 7c0f544f9..05c39b36c 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/LayoutRules/SA1500CodeFixProvider.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/LayoutRules/SA1500CodeFixProvider.cs @@ -57,13 +57,13 @@ private static async Task GetTransformedDocumentAsync(Document documen var settings = SettingsHelper.GetStyleCopSettings(document.Project.AnalyzerOptions, 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 GenerateBraceFixes(IndentationSettings indentationSettings, ImmutableArray braceTokens) + private static Dictionary GenerateBraceFixes(StyleCopSettings settings, ImmutableArray braceTokens) { var tokenReplacements = new Dictionary(); @@ -72,7 +72,7 @@ private static Dictionary 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 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 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 FixAllInDocumentAsync(FixAllContext fi var settings = SettingsHelper.GetStyleCopSettings(document.Project.AnalyzerOptions, fixAllContext.CancellationToken); - var tokenReplacements = GenerateBraceFixes(settings.Indentation, tokens); + var tokenReplacements = GenerateBraceFixes(settings, tokens); return syntaxRoot.ReplaceTokens(tokenReplacements.Keys, (originalToken, rewrittenToken) => tokenReplacements[originalToken]); } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs index 4bb6ce55c..5ce882b31 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs @@ -305,5 +305,154 @@ private void Bar() await VerifyCSharpFixAsync(testCode, expectedDiagnostics, fixedTestCode, CancellationToken.None).ConfigureAwait(false); } + + /// + /// Verifies that no diagnostics are reported for the do/while loop when the + /// expression is on the same line as the closing brace and the setting is enabled. + /// + /// A representing the asynchronous unit test. + [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); + } + + /// + /// Verifies that diagnostics will be reported for the invalid loop that + /// is on the same line as the closing brace which is not part of a do/while loop. This + /// ensures that the allowDoWhileOnClosingBrace setting is only applicable to a do/while + /// loop and will not mistakenly allow a plain loop after any arbitrary + /// closing brace. + /// + /// A representing the asynchronous unit test. + [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 test = new CSharpTest + { + TestCode = testCode, + ExpectedDiagnostics = + { + Diagnostic().WithLocation(10, 9), + }, + Settings = testSettings, + }; + + await test.RunAsync(CancellationToken.None).ConfigureAwait(false); + } + + /// + /// Verifies that no diagnostics are reported for the do/while loop when the + /// expression is on the same line as the closing brace and the setting is allowed. + /// + /// + /// The "Invalid do ... while #6" code in the unit test + /// should account for the proper fix when the allowDoWhileOnClosingBrace is , + /// which is the default. + /// + /// A representing the asynchronous unit test. + [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, + ExpectedDiagnostics = + { + Diagnostic().WithLocation(9, 20), + }, + FixedCode = fixedTestCode, + Settings = testSettings, + }; + + await test.RunAsync(CancellationToken.None).ConfigureAwait(false); + } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/Settings/SettingsUnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/Settings/SettingsUnitTests.cs index 77eb222e5..a5e69077e 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/Settings/SettingsUnitTests.cs +++ b/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); diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs b/StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs index c17f25ec2..5e1f04f53 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs @@ -230,7 +230,7 @@ private static void CheckBraces(SyntaxNodeAnalysisContext context, SyntaxToken o CheckBraceToken(context, openBraceToken); if (checkCloseBrace) { - CheckBraceToken(context, closeBraceToken); + CheckBraceToken(context, closeBraceToken, openBraceToken); } } @@ -242,7 +242,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) { @@ -279,6 +279,22 @@ 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.Options.GetStyleCopSettings(context.CancellationToken).LayoutRules.AllowDoWhileOnClosingBrace) + { + var openBracePreviousToken = openBraceToken.GetPreviousToken(includeZeroWidth: true); + + if (openBracePreviousToken.IsKind(SyntaxKind.DoKeyword)) + { + return; + } + } + + break; + default: break; } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Settings/ObjectModel/LayoutSettings.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Settings/ObjectModel/LayoutSettings.cs index 85f3e0fe9..6970657f2 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/Settings/ObjectModel/LayoutSettings.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Settings/ObjectModel/LayoutSettings.cs @@ -17,6 +17,11 @@ internal class LayoutSettings /// private readonly bool allowConsecutiveUsings; + /// + /// This is the backing field of the property. + /// + private readonly bool allowDoWhileOnClosingBrace; + /// /// Initializes a new instance of the class. /// @@ -24,6 +29,7 @@ protected internal LayoutSettings() { this.newlineAtEndOfFile = OptionSetting.Allow; this.allowConsecutiveUsings = true; + this.allowDoWhileOnClosingBrace = false; } /// @@ -45,6 +51,10 @@ protected internal LayoutSettings(JsonObject layoutSettingsObject) this.allowConsecutiveUsings = kvp.ToBooleanValue(); break; + case "allowDoWhileOnClosingBrace": + this.allowDoWhileOnClosingBrace = kvp.ToBooleanValue(); + break; + default: break; } @@ -56,5 +66,8 @@ protected internal LayoutSettings(JsonObject layoutSettingsObject) public bool AllowConsecutiveUsings => this.allowConsecutiveUsings; + + public bool AllowDoWhileOnClosingBrace => + this.allowDoWhileOnClosingBrace; } } diff --git a/documentation/Configuration.md b/documentation/Configuration.md index 420b3add4..9b7af7f3e 100644 --- a/documentation/Configuration.md +++ b/documentation/Configuration.md @@ -418,6 +418,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 @@ -439,6 +440,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 +* `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. diff --git a/documentation/SA1500.md b/documentation/SA1500.md index eaa039f65..652728001 100644 --- a/documentation/SA1500.md +++ b/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: From 3163cdf3a4309e0c4cca295b264a405b2c33b2bd Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Wed, 19 May 2021 14:42:01 -0700 Subject: [PATCH 2/4] Simplify testing and cover code fixes --- .../SA1500/SA1500UnitTests.DoWhiles.cs | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs index 65653a4b8..b0a617a91 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs @@ -373,7 +373,24 @@ private void Bar() while (x == 0) { x = 1; - } while (x == 0) + [|}|] 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; } @@ -383,10 +400,7 @@ private void Bar() var test = new CSharpTest { TestCode = testCode, - ExpectedDiagnostics = - { - Diagnostic().WithLocation(10, 9), - }, + FixedCode = fixedCode, Settings = testSettings, }; @@ -423,7 +437,7 @@ private void Bar() do { - x = 1; } while (x == 0); + x = 1; [|}|] while (x == 0); } }"; @@ -443,10 +457,6 @@ private void Bar() var test = new CSharpTest { TestCode = testCode, - ExpectedDiagnostics = - { - Diagnostic().WithLocation(9, 20), - }, FixedCode = fixedTestCode, Settings = testSettings, }; From 036d8e9760b18cd8a7db59f88b2e3bb5413081d8 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Wed, 19 May 2021 14:45:55 -0700 Subject: [PATCH 3/4] Simplify syntax checks by using Parent instead of GetPreviousToken --- .../SA1500BracesForMultiLineStatementsMustNotShareLine.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs b/StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs index d1b641f95..101612fcd 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs @@ -290,9 +290,8 @@ private static void CheckBraceToken(SyntaxNodeAnalysisContext context, SyntaxTok // may want to allow that and not have SA1500 report it as a style error. if (context.GetStyleCopSettings(context.CancellationToken).LayoutRules.AllowDoWhileOnClosingBrace) { - var openBracePreviousToken = openBraceToken.GetPreviousToken(includeZeroWidth: true); - - if (openBracePreviousToken.IsKind(SyntaxKind.DoKeyword)) + if (openBraceToken.Parent.IsKind(SyntaxKind.Block) + && openBraceToken.Parent.Parent.IsKind(SyntaxKind.DoStatement)) { return; } From c58307c75a03b1bc377d2bf99ff82edb75eda5f2 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Wed, 19 May 2021 14:50:18 -0700 Subject: [PATCH 4/4] Clarify documentation --- documentation/Configuration.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/documentation/Configuration.md b/documentation/Configuration.md index 7ef4f4407..8f0ce75b0 100644 --- a/documentation/Configuration.md +++ b/documentation/Configuration.md @@ -420,7 +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 | +| `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 @@ -444,10 +444,10 @@ 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: +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 -* `false`: the `while` expression of a `do-while` loop must be on a separate line from the closing brace +* `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