-
Notifications
You must be signed in to change notification settings - Fork 507
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.
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
Issue #2801: SA1500 fires for the while clause of do/while statement #3196
Conversation
…hile statement Provide new configuration setting, `layoutRules.allowDoWhileOnClosingBrace` such that when enabled, the following will be allowed: ``` do { Console.WriteLine("test"); } while (false); ```
Codecov Report
@@ Coverage Diff @@
## master #3196 +/- ##
==========================================
- Coverage 93.44% 93.40% -0.04%
==========================================
Files 1029 1029
Lines 111035 111170 +135
Branches 3934 3940 +6
==========================================
+ Hits 103754 103841 +87
- Misses 6266 6268 +2
- Partials 1015 1061 +46 |
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line shows only partial code coverage from the Unit Tests and would be the reason for the decrease in Codecov.
The reason is because there isn't a test in which the braceToken.Parent
is null
. I'm not completely familiar with the flow of execution and what is possible and what isn't, so I wrote this just being safe. I suppose, given the SyntaxTree
, it may be impossible for braceToken.Parent
to be null
, but I just don't know. I tried to write some Unit Tests that had malformed syntax and they would fail with a compiler error.
If someone is sure that braceToken.Parent
can never be null
, then I'd be happy to change it. I know the line of code right above this also uses braceToken.Parent
without the Null-Conditional operator, but I wasn't sure if the fact that it is applicable to a Multi-Dimensional Array Initializer, that the syntax may be different. So I just wanted to be extra safe.
But we can remove it if we want the Codcov percentage to go back up.
documentation/Configuration.md
Outdated
@@ -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 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kevin-Andrew I may still merge this. Let me give it some thought today. |
|
||
if (openBracePreviousToken.IsKind(SyntaxKind.DoKeyword)) | ||
if (openBraceToken.Parent.IsKind(SyntaxKind.Block) | ||
&& openBraceToken.Parent.Parent.IsKind(SyntaxKind.DoStatement)) |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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.
Thanks for looking at this @sharwell. I see that one of the required automation builds is failing. |
I restarted the build, and it should pass this time. I'm guessing our timeout is too short to handle all cases, but I can increase it. |
Provide new configuration setting,
layoutRules.allowDoWhileOnClosingBrace
such that when enabled, the following will be allowed:See Issue #2801.