NET-2345 Fix S1116 FP: Empty statement is required after label at end of compound statement

[jilles-sg]

Description

S1116 reports an issue when there is an empty statement after a label at the end of a compound statement. However, this empty statement (semicolon) is required by the syntax.

Repro steps

int[][] a = { new int[] { 1, 2 }, new int[] { 3, 4 } };
foreach (int[] aa in a) {
    foreach (int x in aa) {
        if (x < 2)
            goto next;
    }
next:;
}

Expected behavior

S1116 does not report an issue.

Actual behavior

S1116 complains about an empty statement on the line next:;

Apart from that, S907 correctly complains about the goto statement.

Known workarounds

Disable the rule or mark the issue as false positive.

Related information

• C#/VB.NET Plugins version: csharp version 9.30.0.95878
• Visual Studio version: 2022
• MSBuild / dotnet version: .NET 6.0
• SonarScanner for .NET version (if used): SonarScanner for MSBuild 5.14 (.NET Core) with SonarCloud
• Operating System: Windows 10.0.17763.737

[sebastien-marichal]

Hello @jilles-sg,

I don't think this is a false positive.

We rely on the compiler that categorizes this as an empty statement for this rule.
When you put a label, the compiler expects it to be followed by a statement. That is why you have a syntax error without a ;. (See labeled statements)
Adding a ; without any statement will produce the expected empty statement issue.

In your example, you could replace the goto with a break, and it would produce the same result:

int[][] a = { new int[] { 1, 2 }, new int[] { 3, 4 } };
foreach (int[] aa in a) {
    foreach (int x in aa) {
        if (x < 2)
            break;
    }
}

[jilles-sg]

@sebastien-marichal I simplified the code a bit much, so the rationale for the label was no longer clear. The actual code where I encountered the problem has a foreach loop inside a switch inside a while loop, and wants to leave the switch from the foreach loop. The foreach loop only contains the switch and the labelled empty statement.

My point is not that there isn't an empty statement in the problematic code, but that there isn't an obvious way to avoid the empty statement (different from situations where ;; simply needs to be replaced with ;), other than getting rid of the goto which S907 already complains about.

There are indeed various ways to avoid both the goto statement and the label at the end of a compound statement, such as extracting out a function with the switch statement or the case containing the foreach loop, which can then be returned from.

[sebastien-marichal]

... there isn't an obvious way to avoid the empty statement

I am not sure to understand what you mean by this. You said in you next paragraph there is various ways to avoid it.

If you use goto to go to the end of a block with no statement, you can usually (if not always) replace it with break or return. Then, this rule still make sens to raise on this particular scenario.

In my opinion, introducing an exception to labeled statements for this rule is going against aiming for clean code.

[DaleStan]

What would it take to convince you to reconsider this decision?

As it stands, I can't configure SonarQube with the instruction "I don't care about gotos" unless I also configure it as "I don't care about empty statements."

Is that really the desired behavior?

[Tim-Pohlmann]

@DaleStan Can you give us an example where it would be less obvious on how to avoid the empty statement?

[DaleStan]

I don't think I understand the question. The goto and end-of-block label are staying where they are, because removing the gotos is not part of my current style/quality goals. To tell SonarQube this, I set dotnet_diagnostic.S907.severity = none in .editorconfig.

However, over half of my S1116 messages are on code where I'm doing something like continueing an outer loop or breaking a loop from inside a switch.

One such place is: (Both conditions in the code I copied from are complicated enough not to trigger S3267.)

    foreach (var a in something) {
        if (condition(a)) {
            foreach (var b in somethingElse) {
                if (anotherCondition(b)) {
                    goto continueOuterLoop; // No warning, because I disabled S907
                }
            }
            // Two lines of real work here
        }
continueOuterLoop:; // S1116 that can't be resolved without removing the goto.
    }

In order to actually ignore the goto, SonarQube also has to ignore the empty statement at the end of the block. Do I have to turn off S1116 too, or is there a way to get SonarQube to ignore gotos for real?

[Tim-Pohlmann]

I see your point. Our logic here is that you shouldn't use goto in the first place but without that sentiment this is an FP. Afterall, this rule is about detecting typos and cut-and-paste leftovers, not about restructuring your code. I will reopen this, but I will have to give it a low priority because most people will have S907 enabled (for good reason).
The best way forward for you is to indeed disable S1116 as well.

[github-actions]

Internal ticket NET-2345