Fix issue parsing for-statement initializer chain with commas and no initializers

[CyrusNajmabadi]

Fixes #77160

[CyrusNajmabadi]

@dotnet/roslyn-compiler ptal.

[cston]

                if (flags.HasFlag(VariableFlags.ForStatement) && this.PeekToken(1).Kind != SyntaxKind.SemicolonToken)

Is the intent of this if block to break early if there is an unexpected token, to potentially improve error recovery? If this is the cause of the regression, would it make sense to remove the entire if block?

---

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:5067 in 3d427cd. [](commit_id = 3d427cd, deletion_comment = False)

[CyrusNajmabadi]

If this is the cause of the regression, would it make sense to remove the entire if block?

We def can't remove the entire if-block as it is necessary to know how to process a variable-declarator in a for-statement (where it must be followed by a ;) vs other locations (where it doesn't not have to be, like in a using-statement-block).

I tried removing the PeekToken(1) != Semicolon check as well. but it made the results for TestMultipleDeclaratorsWithInitializers4/TestMultipleDeclaratorsWithInitializers5/TestMultipleDeclaratorsWithInitializers6 worse. So my pref is to keep this.

[cston]

We def can't remove the entire if-block as it is necessary to know how to process a variable-declarator in a for-statement (where it must be followed by a ;) vs other locations (where it doesn't not have to be, like in a using-statement-block).

This if block, if (flags.HasFlag(VariableFlags.ForStatement) && ...) { ... }, was added in #75632, and it appears to break early when the previous variable declarator is followed by , id and then some token other than ;, =, ). It looks like this is for error recovery cases only.

When that block is removed, the only compiler tests named Parsing that fail are TestCommaSeparators* and TestVariableDeclaratorVersusCondition1, also added in #75632, and all seem to fail with reasonable syntax errors.

[CyrusNajmabadi]

Yes. If we remove the error recovery logic tweaks from before, all the code that tests that those error recovery tweaks produce less errors and a nicer tree demonstrate the worse outcome. :)

In the case of the 'comma' tests, they demonstrate that the parser loses control very quickly when encountering the errant <. It also misjudges which sections that follow end up in the initializer/condition/incrementor portion of the for-loop. It also ends up completely losing syntax in the final tree (the < 10 is entirely gone.

With the error recovery the syntax/semantic errors are only:

                    // (5,23): error CS1002: ; expected
                    //         for (int i = 0, i < 10; i++) ;
                    Diagnostic(ErrorCode.ERR_SemicolonExpected, ",").WithLocation(5, 23)

Without the error recovery, the syntax/semantic errors are:

                // (5,25): error CS0128: A local variable or function named 'i' is already defined in this scope
                //         for (int i = 0, i < 10; i++) ;
                Diagnostic(ErrorCode.ERR_LocalDuplicate, "i").WithArguments("i").WithLocation(5, 25),
                // (5,25): warning CS0168: The variable 'i' is declared but never used
                //         for (int i = 0, i < 10; i++) ;
                Diagnostic(ErrorCode.WRN_UnreferencedVar, "i").WithArguments("i").WithLocation(5, 25),
                // (5,27): error CS1002: ; expected
                //         for (int i = 0, i < 10; i++) ;
                Diagnostic(ErrorCode.ERR_SemicolonExpected, "<").WithLocation(5, 27),
                // (5,27): error CS1525: Invalid expression term '<'
                //         for (int i = 0, i < 10; i++) ;
                Diagnostic(ErrorCode.ERR_InvalidExprTerm, "<").WithArguments("<").WithLocation(5, 27)

First, this is much more noise, with lots of strange errors. Why am i being told that 'i' is already declared here? Why am i being told it is never used? Second, the recommended fixes are not great. I'm being told i'm missing a semicolon before the '<' and that < is not a valid expression. These are not good errors to tell a user in terms of fixing the issue.

Compare with the original case where the error is correct and tells the person exactly the right thing to fixup (a comma needs to become a semicolon). There are also no errant cascading errors/warnings that clutter and obfuscate the original issue.

Yes, we can roll back the error recovery improvement here. But i'm loathe to do that as this does genuinely make things better in terms of the UX and for the downstream clients of the syntax tree.

[333fred]

I was being serious about having more comprehensive testing of other possible forms. For example, let's look at something like: for (MyType m = new() { A = 1,; true; m++). We need to have a bunch of test additions here to make sure that we've adequately covered this testing gap.

[CyrusNajmabadi]

sgtm.

[CyrusNajmabadi]

added a lot more tets.

[CyrusNajmabadi]

this is a better result, as this is illegal code, andth e vast majority of the time, this code is hte precursor to int i = 0; i ... so stating that the comma should be a semicolon is a better error.

[CyrusNajmabadi]

this is surprising, and likely a parsing bug (though practically impossible for it to matter in practice). It looks like we see from a and think that that's the start of a variable declarator (with type from and identifier a).

[CyrusNajmabadi]

also surprising, but not really a problem. the strict ang grammar truly does not allow 'ref' here. We could support parsing this, but would have to then ensure binding errors. a s this is extremely unlikely to be written, no need to do extra work here.

[CyrusNajmabadi]

similar issue, parser sees this as a var decl for a with with a as the type and with as the name. As this is vastly unlikely to be ever written, fixing this seems unnecessary.

[cston]

the second 'i'

"consume the second ..."?

[cston]

}

Consider testing:

for (;;) ;
for (int i;;) ;
for (int i, j, k;;) ;
for (int i = 0;;) ;
for (A b;;) ;
for (A b, c, d;;) ;
for (A b = null, c, d = null;;) ;
for (A b = c switch { A => x, _ => y };;) ;

---

Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ForStatementParsingTest.cs:4449 in 310bde0. [](commit_id = 310bde0, deletion_comment = False)

[cston]

        """,

Consider testing:

for (int i =;;) ;
for (int i = 0, j =;;) ;

---

Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ForStatementParsingTest.cs:808 in 310bde0. [](commit_id = 310bde0, deletion_comment = False)

[jaredpar]

/backport release/dev17.13

[jaredpar]

/backport to release/dev17.13

[github-actions]

Started backporting to release/dev17.13: https://github.com/dotnet/roslyn/actions/runs/13549852813