-
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
Refine error recovery in the parser around top level statements #65262
Conversation
@dotnet/roslyn-compiler Please review |
3 similar comments
@dotnet/roslyn-compiler Please review |
@dotnet/roslyn-compiler Please review |
@dotnet/roslyn-compiler Please review |
@@ -1704,22 +1704,6 @@ class B { } | |||
VerifyMembers(node, new MemberInfo { Kind = SyntaxKind.ClassDeclaration, Text = "B" }); | |||
} | |||
|
|||
[Fact] | |||
[Trait("Feature", "Directives")] | |||
public void TestNegIfEndifDirectivesWithBadCode() |
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.
Why was this test deleted? #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.
Why was this test deleted?
It looked like a dupe of a same named test in TopLevelStatementsParsingTests.cs, but I now realized that directives are not verified there. I will restore this aspect of the test.
[Fact] | ||
public void ErrorRecovery_05() | ||
{ | ||
var test = @"using aliasY = X.Y;"; |
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.
What's actually erroneous about this syntax? #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.
What's actually erroneous about this syntax?
Nothing, as the test indicates. One of the parts about this change is to avoid recovering from non-error scenarios. That is why some new tests are covering success cases, but it is still an error recovery testing. Negative tests, so to speak.
{ | ||
var test = @" | ||
using X; | ||
using aliasY = X.Y; |
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 one too. I'm not sure why these tests are named ErrorRecovery_X
. #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.
Same response as for ErrorRecovery_05
. The tests are testing code responsible for an error recovery. Both when it should and shouldn't occur.
public void ErrorRecovery_09() | ||
{ | ||
var test = @" | ||
record class Point(int x, int y); |
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.
What's an error about this syntax? #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.
What's an error about this syntax?
Same response as for ErrorRecovery_05
@dotnet/roslyn-compiler For the second review. |
@dotnet/roslyn-compiler For the second review. |
|
||
if (!haveModifiers) | ||
{ | ||
var resetOnFailurePoint = this.GetResetPoint(); |
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.
nit: this method is quite long. Can we extra this new block into a new tryParseStatement
helper?
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 2) with a nit
Fixes #55969.