-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix await keyword recognition in interpolated strings with parentheses #80763
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
Conversation
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| using var _ = new FieldKeywordContext(tempParser, isInFieldKeywordContext); | ||
| using var __ = new AsyncContext(tempParser, isInAsync); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot rename these to _1 and _2 as that is the normal patten we use when having multiple unnamed temps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 8c24cdc - renamed to _1 and _2.
| public void AwaitInInterpolatedString_WithParentheses() | ||
| { | ||
| UsingTree(@" | ||
| class C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please use a raw string literals for all the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 8c24cdc - using raw string literals with triple quotes.
| [Fact] | ||
| // Tests that await keyword is recognized in interpolated strings with parentheses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please remove superflous comment. And please add an appropriate [WorkItem("url")] attribute to al the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 8c24cdc - removed comments and added [WorkItem("https://github.com/dotnet/roslyn/issues/39149")].
| { | ||
| async System.Threading.Tasks.Task M() | ||
| { | ||
| var z = $""{await (System.Threading.Tasks.Task.FromResult(string.Empty))}""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please simplify the test as it is only testing syntax. First, you an use UsingStatement to just test the var v = $"..."; line. second, the code inside the interpolation can be simpler, without such a complex expression inside the parenthesized region. This applies to both tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 8c24cdc - simplified to minimal async method with just the interpolated string statement. Used UsingTree with simplified expression await (M()) and await M() inside the interpolation.
- Rename _ and __ to _1 and _2 per naming convention - Use raw string literals for test code - Add WorkItem attributes with issue URL - Simplify tests to minimal async method with interpolated string Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
|
@dotnet/roslyn-compiler ptal. |
|
@dotnet/roslyn-compiler ptal. |
|
@jjonescz can you ptal? |
|
@RikkiGibson could you ptal? |
| using var tempParser = new LanguageParser(tempLexer, oldTree: null, changes: null); | ||
| using var _ = new FieldKeywordContext(tempParser, isInFieldKeywordContext); | ||
| using var _1 = new FieldKeywordContext(tempParser, isInFieldKeywordContext); | ||
| using var _2 = new AsyncContext(tempParser, isInAsync); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this does address the current bug, but, I am wondering if a more "holistic" way of forwarding state from a parent to child parser instance is warranted here. Or, at least something which forces the developer to make an explicit decision on whether/how to forward the state here, when state fields used by the parser are added/changed.
It seems likely that any expression whose parsing depends on fields in SyntaxFactoryContext, may behave incorrectly inside interpolations, if it is not forwarded here.
RikkiGibson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think it will be good to follow up and fix parsing state handling for interpolations more categorically.
Agreed. Will try an immediate followup. |
awaitkeyword not recognized in interpolated strings with parentheses (e.g.,$"{await (Task.FromResult(string.Empty))}")IsInAsynccontext is not passed to temp parser for interpolation expression parsingIsInAsyncto ParseInterpolationSummary
Fixed the parser to properly recognize the
awaitkeyword inside interpolated string expressions when the awaited expression is wrapped in parentheses. The fix involves:IsInAsynccontext from the outer parser to the temp parser created for interpolation expressionsAsyncContextstruct that follows the same pattern asFieldKeywordContextto manage the async stateFiles Changed
src/Compilers/CSharp/Portable/Parser/LanguageParser.cs: AddedAsyncContextstructsrc/Compilers/CSharp/Portable/Parser/LanguageParser_InterpolatedString.cs: Updated to pass and useIsInAsyncsrc/Compilers/CSharp/Test/Syntax/Parsing/AsyncParsingTests.cs: Added two simplified tests using raw string literalsFixes #39149
Original prompt
Fixes #39149
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.