Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parsing of for-loops #76476

Merged
merged 10 commits into from
Dec 17, 2024
Merged

Conversation

CyrusNajmabadi
Copy link
Member

@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
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 17, 2024
@CyrusNajmabadi
Copy link
Member 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
Contributor

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
Member 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
Member Author

@RikkiGibson this is ready for another pass.

@CyrusNajmabadi CyrusNajmabadi merged commit 37d00bf into dotnet:main Dec 17, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Dec 17, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the incrementalParsingBug branch December 18, 2024 15:12
@dibarbet dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025
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