-
Notifications
You must be signed in to change notification settings - Fork 4k
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 parser regressions of conditional expressions #75075
Conversation
Thanks for looking into this so quickly. I've double checked all places our solution breaks and it seems to happen for the I've added full replication steps onto the original ticket #75074 |
with
expression in conditional expression
I looked through the list of all contextual keywords and I don't see other keywords, which might be problematic for us. When reviewing please also look through them and point out, if I missed something. Even if the scenario is not a regression, we better add a test or a few for it |
Do you have tests for |
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.
Do we have tests for:
global
yield
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.
Added a test for global qualified name. As of yield
, since it is only valid as a yield return
and yield break
statements, it cannot be a contextual keyword inside an expression. However, this gave me an idea: if we have something like x is X ? yield return 0 ...
we can parse it a lot better we we consider x is X? yield
as one statement and return 0 ...
as another one with missing ;
in between. This is actually true for most keywords that start a statement: if
, while
, for
etc. For now I added 2 yield
tests with the right shape to just swap the errors and parse result in the future, but when main
branch targets 17.13, I'm gnna make a follow-up PR with imrpoved error recovery for such cases
@DoctorKrolic are you going to be able to address feedback, or should we pick this up the rest of the way? |
@333fred PTAL |
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.
@cston for a second review.
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 Thanks (iteration 6)
Thanks @DoctorKrolic ! |
Fixes: #75074