Skip to content

Parsing for using and foreach await#23866

Merged
jcouv merged 2 commits intodotnet:features/async-streamsfrom
jcouv:async-streams
Dec 27, 2017
Merged

Parsing for using and foreach await#23866
jcouv merged 2 commits intodotnet:features/async-streamsfrom
jcouv:async-streams

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Dec 19, 2017 

using_statement
    : 'using' '(' resource_acquisition ')' embedded_statement
    : 'using' 'await' '(' resource_acquisition ')' embedded_statement // new
    ;

foreach_variable_statement // new
    : 'foreach' '(' declaration_expression 'in' expression ')' embedded_statement
    : 'foreach' 'await' '(' declaration_expression 'in' expression ')' embedded_statement // new
    ;

foreach_statement
    : 'foreach' '(' local_variable_type identifier 'in' expression ')' embedded_statement
    : 'foreach' 'await' '(' local_variable_type identifier 'in' expression ')' embedded_statement // new
    ;

Relative to baseline

@jcouv jcouv added this to the 16.0 milestone Dec 19, 2017
@jcouv jcouv self-assigned this Dec 19, 2017
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Dec 19, 2017

test please

gafter
gafter reviewed Dec 21, 2017
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Errors/MessageID.cs

// C# 7.2 features.
case MessageID.IDS_FeatureAsyncStreams: // PROTOTYPE(async-streams)
case MessageID.IDS_FeatureNonTrailingNamedArguments: // semantic check
Copy link
Copy Markdown
Member

@gafter gafter Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Should be 7.3? #Resolved

Copy link
Copy Markdown
Member Author

@jcouv jcouv Dec 21, 2017 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

It will become 8.0. But since I started work in a branch with 7.2 only, I stuck with 7.2 with a PROTOTYPE. #Resolved

gafter
gafter reviewed Dec 21, 2017
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated
return _syntaxFactory.UsingStatement(@using, awaitToken, openParen, declaration, expression, closeParen, statement);
}

private SyntaxToken ParseAwaitTokenInUsingOrForEach()
Copy link
Copy Markdown
Member

@gafter gafter Dec 21, 2017

Choose a reason for hiding this comment

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

ParseAwaitTokenInUsingOrForEach [](start = 28, length = 31)

Please name it for what it does rather than where it is used, e.g. ParseOptionalAwaitKeywordForAsyncStreams

gafter
gafter reviewed Dec 21, 2017
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Syntax/Parsing/AsyncStreamsParsingTests.cs

protected override SyntaxTree ParseTree(string text, CSharpParseOptions options)
{
return SyntaxFactory.ParseSyntaxTree(text, options: (options ?? TestOptions.Regular).WithLanguageVersion(LanguageVersion.CSharp7_2)); // PROTOTYPE(async-streams)
Copy link
Copy Markdown
Member

@gafter gafter Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

CSharp7_2 [](start = 133, length = 9)

Can be 7.3 now. #Resolved

gafter
gafter reviewed Dec 21, 2017
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Syntax/Parsing/AsyncStreamsParsingTests.cs

protected override CSharpSyntaxNode ParseNode(string text, CSharpParseOptions options = null)
{
return SyntaxFactory.ParseExpression(text, options: (options ?? TestOptions.Regular).WithLanguageVersion(LanguageVersion.CSharp7_2)); // PROTOTYPE(async-streams)
Copy link
Copy Markdown
Member

@gafter gafter Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

CSharp7_2 [](start = 133, length = 9)

Ditto. #Resolved

gafter
gafter reviewed Dec 21, 2017
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Syntax/Parsing/AsyncStreamsParsingTests.cs
// foreach await (var i in collection)
Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7_1, "await").WithArguments("async streams", "7.2").WithLocation(6, 17)
);
}
Copy link
Copy Markdown
Member

@gafter gafter Dec 21, 2017

Choose a reason for hiding this comment

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

Might as well verify nodes as well.

gafter
gafter reviewed Dec 21, 2017
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Syntax/Parsing/AsyncStreamsParsingTests.cs
// foreach await (var (i, j) in collection)
Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7_1, "await").WithArguments("async streams", "7.2").WithLocation(6, 17)
);
}
Copy link
Copy Markdown
Member

@gafter gafter Dec 21, 2017

Choose a reason for hiding this comment

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

Ditto

gafter
gafter suggested changes Dec 21, 2017
View reviewed changes
Copy link
Copy Markdown
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.

Please test the nodes in error cases in the parser tests, as noted.

gafter
gafter approved these changes Dec 26, 2017
View reviewed changes
Copy link
Copy Markdown
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.

:shipit:

@gafter gafter removed their assignment Dec 26, 2017
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Dec 27, 2017

@dotnet/roslyn-compiler for a second review. Thanks

VSadov
VSadov approved these changes Dec 27, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM

@jcouv jcouv merged commit cc7744b into dotnet:features/async-streams Dec 27, 2017
@jcouv jcouv deleted the async-streams branch December 27, 2017 22:57
@jcouv jcouv added the Feature - Async Streams Async Streams label Dec 29, 2017
@jcouv jcouv mentioned this pull request Jan 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@VSadov VSadov VSadov approved these changes

+1 more reviewer

@gafter gafter gafter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Async Streams Async Streams

Projects

None yet

Milestone

16.0

Development

Successfully merging this pull request may close these issues.

3 participants

@jcouv @gafter @VSadov