Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve error tolerance when parsing invalid implicit arrays. #24706

Merged
merged 2 commits into from
Feb 9, 2018
Merged

Improve error tolerance when parsing invalid implicit arrays. #24706

merged 2 commits into from
Feb 9, 2018

Conversation

CyrusNajmabadi
Copy link
Member

Fixes #24701

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 8, 2018 08:47
@CyrusNajmabadi
Copy link
Member Author

tagging @gafter @dotnet/roslyn-compiler

alrz
alrz reviewed Feb 8, 2018
View reviewed changes
src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
commas.Add(this.EatToken());
if (this.IsPossibleExpression())
{
var size = this.AddError(this.ParseExpressionCore(), ErrorCode.ERR_InvalidArray);
Copy link
Contributor

@alrz alrz Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 If you don't mind I incorporate this line in the stackalloc inits PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would mind that greatly. roslyn is built on non-cooperation, and it should stay that way.

CyrusNajmabadi
CyrusNajmabadi commented Feb 8, 2018
View reviewed changes
src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_ArrayCreationAndInitializer.cs
@@ -435,27 +435,26 @@ public void F(int dimension)
}
";
string expectedOperationTree = @"
IArrayCreationOperation (OperationKind.ArrayCreation, Type: System.Int32[], IsInvalid) (Syntax: 'new[2]/*</bind>*/')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this used to be the case because we'd parse "new [2]" as "new [] { 2 }" (with lots of missing tokens). The new parse is "new [] { }" (with '2' as skipped text), so we get semantic changes as well.

@CyrusNajmabadi
Copy link
Member Author

@jcouv Could i get a review? Also, i'm not sure, but i think Neal said he doesn't see these unless he's tagged as a reviewer. But i can't add reviewers. Could you do that for me?

Thanks!

@jcouv jcouv added this to the 15.7 milestone Feb 9, 2018
@alrz
Copy link
Contributor

alrz commented Feb 9, 2018

I wonder what would be the expected behavior for stackalloc[], should we parse commas? should we give "expected ]" in all cases?

@jcouv
Copy link
Member

jcouv commented Feb 9, 2018

Could target dev15.7.x (master branch currently means 15.8 release).

@gafter
Copy link
Member

gafter commented Feb 9, 2018

@CyrusNajmabadi I'm subscribed to the whole Roslyn repo, so I'm drinking from the firehose now.

jcouv
jcouv reviewed Feb 9, 2018
View reviewed changes
src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated
@@ -10600,13 +10600,31 @@ private ImplicitArrayCreationExpressionSyntax ParseImplicitlyTypedArrayCreation(
var commas = _pool.Allocate();
try
{
while (this.CurrentToken.Kind == SyntaxKind.CommaToken)
while (true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had some parsing bugs with "while (true)" before.
Consider the new pattern and utility method:

                        int tokenProgress = -1;
                        while(IsMakingProgress(ref tokenProgress))
                        {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do. thanks!

jcouv
jcouv approved these changes Feb 9, 2018
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks

gafter
gafter approved these changes Feb 9, 2018
View reviewed changes
Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@CyrusNajmabadi
Copy link
Member Author

@CyrusNajmabadi I'm subscribed to the whole Roslyn repo, so I'm drinking from the firehose now.

Gotcha. No worries then. I'll just tag you in the future.

@CyrusNajmabadi
Copy link
Member Author

Please don't merge. I'd lke to make th change @jcouv suggested.

@gafter
Copy link
Member

gafter commented Feb 9, 2018

@CyrusNajmabadi Can you please retarget to dev15.7.x ?

@gafter gafter changed the base branch from master to dev15.7.x February 9, 2018 21:06
@gafter gafter changed the base branch from dev15.7.x to master February 9, 2018 21:07
@gafter
Copy link
Member

gafter commented Feb 9, 2018

(I can't do it myself because your branch includes things not in dev15.7.x)

@gafter gafter added Area-Compilers Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. labels Feb 9, 2018
@CyrusNajmabadi
Copy link
Member Author

yup. will do.

@CyrusNajmabadi
Improve error tolerance when parsing invalid implicit arrays.
44e0387 
Provide helper for common pattern.

Update test.
@CyrusNajmabadi CyrusNajmabadi changed the base branch from master to dev15.7.x February 9, 2018 21:22
@CyrusNajmabadi
Copy link
Member Author

Ok. Ready for final review/merge @jcouv

@jcouv jcouv merged commit 5c9cd2a into dotnet:dev15.7.x Feb 9, 2018
@jcouv
Copy link
Member

jcouv commented Feb 9, 2018

Thanks!

@jcouv
Copy link
Member

jcouv commented Feb 9, 2018

Tagging @jaredpar FYI

@gafter gafter removed their assignment Feb 10, 2018
@gafter
Copy link
Member

gafter commented Feb 10, 2018

@CyrusNajmabadi Thank you so much! You are a real... llama?

@CyrusNajmabadi
Copy link
Member Author

image

@CyrusNajmabadi CyrusNajmabadi deleted the implicitArrayParsing branch February 10, 2018 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alrz alrz alrz left review comments

@gafter gafter gafter approved these changes

@jcouv jcouv jcouv approved these changes

Assignees
No one assigned
Labels
Area-Compilers Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings.
Projects
None yet
Milestone
15.7
Development

Successfully merging this pull request may close these issues.
4 participants
@CyrusNajmabadi @alrz @jcouv @gafter