SA1500 fires for the while clause of do/while statement

[smaillet]

The following generates an SA1500 error when it shouldn't:

bool condition = true;
do
{
    condition = DetermineCondition();
} while( condition );

Forcing placement of while onto a separate line is counter to common code style practices. At the very least there should be a configuration option to set how this rule should handle the placement of the while clause.

[vweijsters]

The behaviour is by design. I've marked this needs discussion to discuss if a configuration option is wanted.

[sharwell]

What's the equivalent option in the Visual Studio formatting settings (and the corresponding .editorconfig setting)? If a clear option exists there and we decide to add an option to StyleCop Analyzers, we should use a name that shows a link between the two.

[zahirtezcan-bugs]

👍
When I google "c# do while", results show the tutorials which have similar syntax. That is an indication for the common sense imho.
And there is also Microsoft C# Language Reference for do keyword

Note: I assume that the spacing inside and around condition parantheses are non-intentional in the @smaillet's sample code.

[AJH16]

I'd second this being an odd case. I've always learned that the while should be on the same line as the end of the loop for a do while on the few cases where they are actually useful. I could see the argument that front loaded conditions are on a separate line so why wouldn't the follower, but given how rarely one sees a do/while, the difference in formatting is probably helpful for highlighting the difference.

[werwolfby]

@vweijsters Are we still discussing it?

This issues is really annoying, I want to keep this inspection, but additionally want to keep while on the same line with closing bracket.

I can prepare fix to ignore this for do/while.

Do we need separate inspection, or separate setting in stylecop.json to show hint for do/while?

[zvonimir]

Hi all. Any news on this one? It would be really great to have it fixed since currently because of this I cannot enable SA1500. Thanks!

[vweijsters]

@sharwell there is no config option for this in .editorconfig, but I'm willing to allow someone else to pick this up and make it configurable. What are your thoughts on it?

[Kevin-Andrew]

@sharwell there is no config option for this in .editorconfig, but I'm willing to allow someone else to pick this up and make it configurable. What are your thoughts on it?

@vweijsters, I have implemented a new configuration setting to support this. I called it allowDoWhileOnClosingBrace, as a boolean property of the layoutRules. By default, it is false, which means the default behavior of StyleCop remains the same, requiring the while expression to be on its own line.

When the setting is true, it allows for the following:

do
{
   Console.WriteLine("test");
} while (false);

I believe this is the simplest solution to the issue and would address most people's concerns.

However, when the setting is true, it does not then prohibit the default behavior. That is, when the setting is 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. For example, both the above and the following code snippet would be allowed when the setting is enabled:

do
{
   Console.WriteLine("test");
}
while (false);

I think most people would be okay with this behavior in order to get what they are really after and that is just allowing the while expression to be on the same line as the closing brace.

An alternative that could address the above would be to instead change the new setting such that it has discrete values, similar to the UsingDirectivesPlacement enum. For example, we could have:

internal enum DoWhileLoopPlacement
{
    RequireWhileExpressionOnNewLine,
    RequireWhileExpressionOnSameLIneAsClosingBrace,
    Preserve,
}

However, if this were implemented with respect to SA1500, and someone configures the setting to RequireWhileExpressionOnSameLIneAsClosingBrace then they would get an SA1500 error for having the while expression on a separate line and the error message would be, "Braces for multi-line statements should not share line", which wouldn't make any sense.

So, the last thing I can think of would be to create an entire new layout rule. Unfortunately, I'm not very familiar with how this might be implemented. But I'm guessing we'd have a create a rule called something like SA1521 DoWhileLoopExpressionPlacement, with a default configuration setting value of say requireWhileExpressionOnNewLine. Of course the entire rule can/would be disabled by default.

In the implementation, if rule SA1500 is enabled, then this new rule wouldn't have to do anything by default. But if the rule's configuration setting was set to requireWhileExpressionOnSameLineAsCloseBrace, then the rule would have to mute SA1500 when it encounters a do/while loop (which is basically what my current implementation is doing in the first solution I discussed above). With that configuration setting, the rule would also have to report a diagnostic when the while expression is on its own line. Finally, it would have to include a Code Fix Provider. If we wanted, we could provide a third option, just like the UsingDirectivesPlacement enum has called Preserve. That would allow the while expression to be in either place. I think that would give a user enough fine grained control to get exactly what they want.

As I mentioned, I have code and unit tests completed for the first solution that I described above. Please advise as to the next steps. Would you like me to just submit a Pull Request and continue any discussion there? Or should I create a new Issue first? I've read the page on Contributing, but I'm not sure how much of what is mentioned in the section header "Implementing a diagnostic", is applicable here.

Thanks.

[Kevin-Andrew]

@sharwell , I haven't heard a response to my above post in 10 days. Is anyone around that could review it and if OK with just adding the proposed configuration property, is there someone that would be able to approve a PR if I submit one?