Skip to content

Fix placement of trivia inside an interpolation#57989

Merged
CyrusNajmabadi merged 42 commits intodotnet:mainfrom
CyrusNajmabadi:simplifyInterpolationPArsing8
Dec 4, 2021
Merged

Fix placement of trivia inside an interpolation#57989
CyrusNajmabadi merged 42 commits intodotnet:mainfrom
CyrusNajmabadi:simplifyInterpolationPArsing8

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Nov 25, 2021

Followup to #57966. Draft until that goes in.

Trivia inside an interpolation was being associated with the wrong token. Foudn during the raw string literal work.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner November 25, 2021 03:12
@ghost ghost added the Area-Compilers label Nov 25, 2021
@CyrusNajmabadi CyrusNajmabadi marked this pull request as draft November 25, 2021 03:12

// First grab any trivia right after the {, it will be trailing trivia for the { token.
var openTokenTrailingTrivia = tempLexer.LexSyntaxTrailingTrivia().Node;
var openTokenText = text[interpolation.OpenBraceRange];
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.

this is the thrust of the change. the initial trivia is actually trailing trivia on the preceding token.

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.

What's the advantage of this?

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.

answered offline. this ensures our tokens follow the syntactic-model invariants of the roslyn tree system.

@@ -963,17 +963,17 @@ public static void Main()
";

ParserErrorMessageTests.ParseAndValidate(test,
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.

these tests changed because they contain trivia that used to improperly be associated with the interpolation expression (as leading trivia). Once the trivia moved, teh location of the error reported changed to a different place.

interpolation.OpenBraceRange.End,
interpolation.HasColon ? interpolation.ColonRange.Start : interpolation.CloseBraceRange.Start)];

// TODO: some of the trivia in the interpolation maybe should be trailing trivia of the openBraceToken
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.

this TODO is what thsi PR fixes. the trivia after the { belongs to it, not the expression we parse between { and }

@CyrusNajmabadi CyrusNajmabadi force-pushed the simplifyInterpolationPArsing8 branch from 625e9ee to 8493bf0 Compare December 1, 2021 22:12

// First grab any trivia right after the {, it will be trailing trivia for the { token.
var openTokenTrailingTrivia = tempLexer.LexSyntaxTrailingTrivia().Node;
var openTokenText = text[interpolation.OpenBraceRange];
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.

What's the advantage of this?


TrySetUnrecoverableError(_lexer.MakeError(_lexer.TextWindow.Position, 1, ErrorCode.ERR_SyntaxError, endingChar.ToString()));
goto default;
case '"' when RecoveringFromRunawayLexing():
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.

I'm pretty sure this would still be emitted as a jump table. Do you have an indication it wouldn't?

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.

talked offline. likely a faulty memory on my part. that said, i prefer the new pattern, and in raw-interpolated branch we extract out this logic for each specific branch into its own method.

@CyrusNajmabadi CyrusNajmabadi force-pushed the simplifyInterpolationPArsing8 branch from 8493bf0 to 625e9ee Compare December 1, 2021 23:12
Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 40)

@CyrusNajmabadi CyrusNajmabadi merged commit 6c4f403 into dotnet:main Dec 4, 2021
@ghost ghost added this to the Next milestone Dec 4, 2021
@Cosifne Cosifne modified the milestones: Next, 17.1.P3 Jan 5, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the simplifyInterpolationPArsing8 branch February 1, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants