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 syntax disambiguation for declaration expressions and the is operator #16834
Conversation
Modifies the disambiguation in a tuple literal so that a declaration expression must be followed by a `,` or `)` Fixes https://devdiv.visualstudio.com/0bdbc590-a062-4c3f-b0f6-9383f67865ee/_workitems?id=377111
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
Makes me wonder if we add a bunch more lightweight tests on tuples in various contexts and with various expressions.
@jcouv I don't know a "lightweight" way to do it. If I were assigned the adversarial test role for any of these features, I would probably aim to shake these out by writing a test generator (that verifies the parse tree against the shape of the generated syntax). |
I've added a fix for https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=377556 to this PR. I will update the description above shortly. @jcouv Can you please review the changes since your last review, for the new bug fix? |
@@ -6451,7 +6460,8 @@ private enum ParseTypeMode | |||
{ | |||
Normal, | |||
Parameter, | |||
AfterIsOrCase, | |||
AfterIs, | |||
AfterCase, |
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 parse differently after "is" and after "case" ? #Resolved
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.
👍 |
@jcouv Can you please review the changes since your last review, for the new bug fix? I am proposing to get this into RTM. |
I'm on it. Sorry for the delay. |
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
@gafter I've been thinking about how to cover more scenarios with smaller tests. A lightweight verification would just check a few nodes. For instance, I expect one generic type name, one shift operator and no missing nodes. Or in the tuple case, I expect no missing nodes, and one conditional operator. |
Modifies the disambiguation in a tuple literal so that a declaration expression must be followed by a
,
or)
, and modifies the disambiguation in anis
expression so existing code is not affected.Customer scenario
We "broke" code that worked correctly previously. For example, in the statement
We were treating
a<i>
as a generic (incorrect), rather than as a comparison with a shift (correct). This bug breaks existing (pre c#7) code.Also, in the statement
we were incorrectly interpreting
isValid?
as a nullable type, andisValid? Errors
as a declaration expression. The rest of the stuff before the close paren then appeared to be garbage by the parser.This PR adjusts the disambiguation so that we only parse a declaration expression in this context when it is followed by a
,
or)
.Bugs this fixes:
Fixes https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=377111
Fixes https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=377556
Workarounds, if any
It is possible to "fix" customer code broken by these bugs by parenthesizing the
?:
conditional expression (first issue) or parenthesizing the shift operator on the right-hand-side of the comparison (second issue). Unfortunately, some real customer code bases are affected by this, and it is not clear from the compiler error messages what went wrong or how the customer should fix it. Also, this breaks existing customer code that compiled without problem before C# 7, so this could be an adoption blocker.Risk
Low; this fix narrows the disambiguation code so that it only accepts declaration expressions and parses type arguments in more appropriate contexts.
Performance impact
None, as the amount of code executed for typical workloads is the same (it is merely a change to the logic executed).
Is this a regression from a previous update?
Yes.
Root cause analysis:
This was a fallout from a recent complete overhaul for the parsing of patterns and declaration expressions, which are new features in C# 7. We did not have tests for these particular intersections of syntactic features. It is possible that this would have been caught in testing had the compiler team followed through on its plan to assign a team member to adversarial testing of new features.
How was the bug found?
Customer reported (these bugs broke code that worked previously)
@jcouv @VSadov Please review.