-
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 parsing certain postfix expressions with top level collection literals #68787
Fix parsing certain postfix expressions with top level collection literals #68787
Conversation
@333fred @RikkiGibson can you take a look since Chuck is on vacation? |
""", | ||
// (5,14): error CS1002: ; expected | ||
// [A]++i; | ||
Diagnostic(ErrorCode.ERR_SemicolonExpected, "i").WithLocation(5, 14)); |
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.
this was an existing error recovery case that i added a few years back (recovering when attributes were put in incorrect places). It's still an error after this pr, just one that views the above as an invalid operation on a collection-expr, not an invalid attribute on an expression statement. Trying to detect this case and keep the old parsing isn't worth it IMO.
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.
Agreed, I was just about to comment and say that this needed to be updated.
// Check the next token to see if it indicates the `[...]` sequence we have is a term or not. | ||
var isCollectionExpression = this.CurrentToken.Kind is SyntaxKind.DotToken or SyntaxKind.QuestionToken or SyntaxKind.ExclamationToken; | ||
// Check the next token to see if it indicates the `[...]` sequence we have is a term or not. This is the | ||
// same set of tokens that ParsePostFixExpression looks 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.
are we going to remember to make a corresponding change here if another postfix operator is ever added to the language? :)
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.
I'll add comment
@@ -10609,6 +10609,289 @@ public void MemberAccess22A() | |||
EOF(); | |||
} | |||
|
|||
[Fact] | |||
public void MemberAccess23() |
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.
consider naming these tests something like PostFixOperator
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.
Will fix in next pr
Partial fix for #68766. More changes incoming in future PRs.