Skip to content

Fix parsing of for-loops#76476

Merged
CyrusNajmabadi merged 10 commits intodotnet:mainfrom
CyrusNajmabadi:incrementalParsingBug
Dec 17, 2024
Merged

Fix parsing of for-loops#76476
CyrusNajmabadi merged 10 commits intodotnet:mainfrom
CyrusNajmabadi:incrementalParsingBug

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Dec 17, 2024

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.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner December 17, 2024 20:02
@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 17, 2024
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@dotnet/roslyn-compiler @RikkiGibson ptal

@@ -9181,23 +9181,27 @@ private ForStatementSyntax ParseForStatement(SyntaxList<AttributeListSyntax> att
var (variableDeclaration, initializers) = eatVariableDeclarationOrInitializers();

// Pulled out as we need to track this when parsing incrementors to place skipped tokens.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this comment was specific to secondSemicolonToken before, is it still relevant now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes/ will move.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@RikkiGibson this is ready for another pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rename operation corrupts unrelated source code

4 participants