Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 19, 2021
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bfbb4ca
Issue #2801: SA1500 fires for the while clause of do/while statement
Kevin-Andrew Sep 6, 2020
a965593
Merge remote-tracking branch 'DotNetAnalyzers/master' into dowhile
sharwell May 19, 2021
3163cdf
Simplify testing and cover code fixes
sharwell May 19, 2021
036d8e9
Simplify syntax checks by using Parent instead of GetPreviousToken
sharwell May 19, 2021
c58307c
Clarify documentation
sharwell May 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions StyleCop.Analyzers/StyleCop.Analyzers.Test/LayoutRules/SA1500/SA1500UnitTests.DoWhiles.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -383,10 +400,7 @@ private void Bar()
var test = new CSharpTest
{
TestCode = testCode,
ExpectedDiagnostics =
{
Diagnostic().WithLocation(10, 9),
},
FixedCode = fixedCode,
Settings = testSettings,
};

Expand Down Expand Up @@ -423,7 +437,7 @@ private void Bar()

do
{
x = 1; } while (x == 0);
x = 1; [|}|] while (x == 0);
}
}";

Expand All @@ -443,10 +457,6 @@ private void Bar()
var test = new CSharpTest
{
TestCode = testCode,
ExpectedDiagnostics =
{
Diagnostic().WithLocation(9, 20),
},
FixedCode = fixedTestCode,
Settings = testSettings,
};
Expand Down
7 changes: 3 additions & 4 deletions ...zers/StyleCop.Analyzers/LayoutRules/SA1500BracesForMultiLineStatementsMustNotShareLine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,10 @@ private static void CheckBraceToken(SyntaxNodeAnalysisContext context, SyntaxTok
// 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)
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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I don't know the syntax tree well enough, I just want to make sure, as I had similar code and this same question elsewhere, "Is using .Parent.Parent safe here?" We know that openBraceToken.Parent is not null and is of kind Block, but does that guarantee that .Parent.Parent will never be null? For example what if I have code like the following that uses some extra curly braces for scoping:

{ // Some braces for scoping
   var x = 0;
   do
   {
      x = 1;
   } while (x == 0);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsKind allows null, and openBraceToken.Parent.IsKind(SyntaxKind.Block) is only true if openBraceToken.Parent is not null.

{
return;
}
Expand Down
5 changes: 4 additions & 1 deletion StyleCop.Analyzers/StyleCop.Analyzers/Settings/ObjectModel/LayoutSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ protected internal LayoutSettings(JsonObject layoutSettingsObject, AnalyzerConfi
{
OptionSetting? newlineAtEndOfFile = null;
bool? allowConsecutiveUsings = null;
bool? allowDoWhileOnClosingBrace = null;

foreach (var kvp in layoutSettingsObject)
{
Expand All @@ -57,7 +58,7 @@ protected internal LayoutSettings(JsonObject layoutSettingsObject, AnalyzerConfi
break;

case "allowDoWhileOnClosingBrace":
this.allowDoWhileOnClosingBrace = kvp.ToBooleanValue();
allowDoWhileOnClosingBrace = kvp.ToBooleanValue();
break;

default:
Expand All @@ -73,9 +74,11 @@ 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 =>
Expand Down