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?

[Kevin-Andrew]

@zvonimir , @werwolfby , @AJH16 @zahirtezcan-bugs, @smaillet

Any one interested in reviewing my above comments and the Pull Request that I opened? Do you think it would be sufficient and satisfy your needs here?

[pejta2207]

Is there any follow up on this issue? Having the closing while keywoard in a separate line seems unintuitive so having a configuration switch for this seems like a good idea

[sharwell]

I don't believe there are any plans to make this configurable.

[Kevin-Andrew]

@sharwell, can you elaborate on what you mean by:

...especially since it sounds like even the more relaxed formatter in Visual Studio doesn't support this.

Are you referring to the settings in the following:

[sharwell]

It looks like the Visual Studio formatter silently accepts both of these, without exposing a preference via a user option:

do
{
} while (true);

do
{
}
while (true);

With that said, there are many places where StyleCop Analyzers is more opinionated than Visual Studio about specific formatting rules, with this being one of the cases. Since each added option incurs substantial long-term maintenance and testing considerations, I don't believe we need to add this specific case.

Note that teams could either implement a separate analyzer for brace placement, or could implement a DiagnosticSuppressor that prevents the warning at this location from appearing. Neither of these approaches would require a change to StyleCop Analyzers itself.

[Kevin-Andrew]

@sharwell, thanks. That's too bad since there was already a PR #3196 for it. So I suppose you'd want to close that as well.

[sharwell]

I'm looking to include this because it takes the simpler approach of "ignoring" this location, as opposed to requiring the while to go on the same line as }. This behavior aligns with all configurations of the Visual Studio formatter so it's easy to see how it would make things a bit easier for people.

[Kevin-Andrew]

I think I understand you. The proposed PR introduces a new setting, allowDoWhileOnClosingBrace which defaults to false, meaning no change in current behavior and an SA1500 error will be reported if you specify the while on the same line as the }.

But if you enable this new setting, it will allow either syntax to be used throughout the code. So correct, in that it does not force you to place the while on the same line as the }. Both:

// See unit test TestDoWhileOnClosingBraceWithAllowSettingAsync().
var x = 0;
do
{
    x = 1;
} while (x == 0);

and

var x = 0;
do
{
    i++;
}
while (i < 10);

will be allowed. Perhaps we should add an additional Unit Test for the latter in which the setting is explicitly enabled.