Recover better when a user uses commas in a for-statement instead of semicolons #75632
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
|
@dotnet/roslyn-compiler ptal. |
|
I def run into this myself. ANd the fix was simple. So i'd likek to take this :) |
|
@dotnet/roslyn-compiler ptal |
| @@ -21562,96 +21565,128 @@ static bool TakeOutParam(object y, out bool x) | |||
| } | |||
| "; | |||
| var compilation = CreateCompilationWithMscorlib461(source, options: TestOptions.DebugExe, parseOptions: TestOptions.Regular); | |||
| int[] exclude = new int[] { (int)ErrorCode.ERR_BadVarDecl, | |||
There was a problem hiding this comment.
this test is insane. it's testing out-vars in a series of invalidly written for-loops. specifically, they are written as:
for (bool a, b(
Dummy(TakeOutParam(1, out var x14), In the past, this was parsed terribly to begin with (since you had the varaibles bool a, b and then an error parentheszied expression following.
Now, this is at least seen as bool a as the variable, and b(...) as the condition of the for loop. This ends up changing all the binding.
| @@ -2690,6 +2690,25 @@ static void Main(string[] args) | |||
| } | |||
| } | |||
| } | |||
| "; | |||
| var tree = GetOperationTreeForTest<ForStatementSyntax>(source); | |||
There was a problem hiding this comment.
no tree produced now since hte start/end of hte span is not validly containing the for-statement. it worked before because parsing was so thrown off it through the for loop was terminated.
| ? parseForStatementExpressionList(ref secondSemicolonToken, allowSemicolonAsSeparator: true) | ||
| : default, | ||
| eatUnexpectedTokensAndCloseParenToken(), | ||
| ParseEmbeddedStatement()); |
There was a problem hiding this comment.
broke parsing up into distinct parts for each major chunk of hte for-loop. Teh variables/initializers, the semicolon, the condition, the semicolon, the incrementors, the close paren and the statement.
| @@ -9870,8 +9897,7 @@ private void ParseUsingExpression(ref VariableDeclarationSyntax declaration, ref | |||
|
|
|||
| if (scopedKeyword != null) | |||
| { | |||
| declaration = ParseParenthesizedVariableDeclaration(); | |||
| declaration = declaration.Update(_syntaxFactory.ScopedType(scopedKeyword, declaration.Type), declaration.Variables); | |||
There was a problem hiding this comment.
no need for this strange pattern of getting the decl, grabbing out its type, and replacing it with a newly wrapped 'ScopedType'. We just pass through teh scopedKeyword and make the ScopedType at the appropriate location inside.
|
@RikkiGibson @dotnet/roslyn-compiler ptal :) |
|
@dotnet/roslyn-compiler ptal. |
|
@RikkiGibson ptal :) |
|
@dotnet/roslyn-compiler for another set of eyes. thanks :) |
Fixes #76439 Introduced with #75632. So only in 17.13p2. So fixing this will prevent the regression from making it out. The core issue here was an incorrect understanding i had with order-of-evaluation and storage of values for args when invoking methods. Specifically, i thought that the following: ``` M( x = y(), Z(ref x)); ``` Was evaluated effectively as: ``` xloc = y(); __z_val = Z(ref xloc); M(xloc, __z_val); ``` But it is not. rather, it is: ``` __x_val = x = y(); __z_val = M(ref x); M(__x_val, __z_val); ``` In other words, i thought the call to M woudl see the value of 'x' *after* the call to Z, while it actually evaluates and stores that value prior to the call to Z. This mattered during parsing as we use that value to store skipped tokens onto. So if we use the value prior to updating, we lose those skipped tokens, leading to an invalid incremental parse.
Fixes #66522
I'm doing this for two main reasons:
for'declaration or initializers' was pretty unfun to deal with, as was some ugly code to try to properly handle thescopedkeyword after the fact. As with a prior pr, this aims to make the flow of hte parser much simpler (and more in line with teh grammar productions), eliminating lots of complex variable flow and assignments that are hard to reason about.If these reasons aren't sufficient, i'll understand closing (though i will be sad about it :)).