Await using declarations #31980

chsienki · 2018-12-21T05:57:45Z

Add syntax, binding and lowering for await using declarations
Add tests

- Update syntax nodes - Add parsing tests

- Add awaitable info to bound node - Find and store the awaiter if async - Lower the declaration node correctly - Add tests

chsienki · 2018-12-21T05:58:05Z

@dotnet/roslyn-compiler for review please

jcouv · 2018-12-21T20:02:23Z

    public void AwaitUsingWithExpression_Reversed()

Please add a test for using declaration where await and using are reversed too.

Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/AsyncStreamsParsingTests.cs:254 in bd69f1a. [](commit_id = bd69f1a, deletion_comment = False)

jcouv · 2018-12-21T20:05:03Z

src/Compilers/CSharp/Test/Syntax/Parsing/StatementParsingTests.cs

@@ -2505,6 +2505,132 @@ public void TestUsingVarWithVarDeclarationTree()
            }
        }

+        [Fact]
+        public void TestAwaitUsingVarWithDeclaration()


TestAwaitUsingVarWithDeclaration [](start = 20, length = 32)

nit: I don't know if this test adds much compared to the one below (TestAwaitUsingVarWithDeclarationTree). #Closed

jcouv · 2018-12-21T20:06:21Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

 using Xunit;

 namespace Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen
 {
    public class CodeGenUsingDeclarationTests : EmitMetadataTestBase
    {
+        private const string _asyncDisposable = @"


_asyncDisposable [](start = 29, length = 16)

You could use CSharpTestBase.AsyncStreamsTypes instead. #Closed

Hmm, they haven't been merged in from master yet. I'll make a note to switch it over when they do. (#28892) #Pending

jcouv · 2018-12-21T20:07:29Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

+    public void AwaitOnCompleted<TAwaiter, TStateMachine>(ref TAwaiter awaiter, ref TStateMachine stateMachine) where TAwaiter : INotifyCompletion where TStateMachine : IAsyncStateMachine { }
+    public void AwaitUnsafeOnCompleted<TAwaiter, TStateMachine>(ref TAwaiter awaiter, ref TStateMachine stateMachine) where TAwaiter : ICriticalNotifyCompletion where TStateMachine : IAsyncStateMachine { }
+    public MyTask Task => _task;
+}


nit: extra empty lines below #Closed

jcouv · 2018-12-21T20:10:57Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

+}";
+            var compilation = CreateCompilationWithTasksExtensions(source + _asyncDisposable, options: TestOptions.DebugExe).VerifyDiagnostics();
+
+            CompileAndVerify(compilation, expectedOutput: "Dispose async").VerifyIL("C2.Main", @"


VerifyIL("C2.Main" [](start = 75, length = 18)

I think this is not the method you intended to inspect.
C2.Main is an auto-generated wrapper for the real method. I expect to see a construction of new C1() and a try/finally in the IL. #Closed

jcouv · 2018-12-21T20:12:12Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

+{
+    static async Task Main()
+    {
+        await using C1 c = new C1();


new C1(); [](start = 27, length = 9)

Please add a Write in the constructor for C1, and add a Write below the using. This way the order of evaluation is made completely visible. #Closed

jcouv · 2018-12-21T20:13:16Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

+using System.Threading.Tasks;
+class C1 : IAsyncDisposable
+{
+    public ValueTask DisposeAsync() 


public ValueTask DisposeAsync() [](start = 4, length = 32)

Please add a variant of this test with an explicit implementation of IAsyncDisposable. #Closed

jcouv · 2018-12-21T20:16:56Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

+            var compilation = CreateCompilationWithTasksExtensions(source + _asyncDisposable, options: TestOptions.DebugExe).VerifyDiagnostics();
+
+            CompileAndVerify(compilation, expectedOutput: "Dispose async");
+        }


} [](start = 7, length = 2)

Please add tests where the resource is null (both with interface or with pattern-based instance or extension method).
Can you clarify: will pattern-based using-declaration (with and without await) be restricted to ref structs?

Also, please add tests for await using within a try block, a catch block and a finally block. #Closed

Yes, it will be restricted, its just in the PR after this one (will teach me to queue stuff up :( )

I'll skip the pattern based tests though, as the next PR makes those invalid (can't have ref struct in an async method).

jcouv · 2018-12-21T20:29:02Z

src/Compilers/CSharp/Test/Syntax/Parsing/StatementParsingTests.cs

+                }
+                N(SyntaxKind.SemicolonToken);
+            }
+        }


} [](start = 7, length = 2)

Please also add a parsing test for await var x = b; #Closed

cston · 2018-12-21T22:02:30Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+        /// <summary>
+        /// Binds an awaiter for asynchronous dispose
+        /// </summary>
+        /// <param name="node">The synatx node to bind for</param>


synatx [](start = 35, length = 6)

Typo #Resolved

cston · 2018-12-21T22:02:53Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+        /// <param name="awaitKeyword">The await keyword of the syntax</param>
+        /// <param name="disposeMethodOpt">The dispose method to call, or null to use IAsyncDisposable.DisposeAsync</param>
+        /// <param name="diagnostics">Populated with any errors</param>
+        /// <param name="hasErrors">True if errors occured during binding</param>


occured [](start = 51, length = 7)

Typo #Resolved

cston · 2018-12-21T22:07:24Z

        if (pending.Branch is null)

Consider extracting a local. #Resolved

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs:262 in bd69f1a. [](commit_id = bd69f1a, deletion_comment = False)

cston · 2018-12-21T22:08:53Z

src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs

@@ -1861,6 +1861,10 @@ public override BoundNode VisitMultipleLocalDeclarations(BoundMultipleLocalDecla

        public override BoundNode VisitUsingLocalDeclarations(BoundUsingLocalDeclarations node)
        {
+            if (((LocalDeclarationStatementSyntax)node.Syntax).AwaitKeyword != default)


((LocalDeclarationStatementSyntax)node.Syntax).AwaitKeyword != default [](start = 16, length = 70)

Could this be node.AwaitOpt != null? #Resolved

I seem t o recall there was a reason for doing a syntax check, but I could be wrong.
Looking at VisitForEachStatement, I see it does a syntax check. But VisitUsingStatement does a check on .AwaitOpt...

Note: when you merge recent bits from master into this branch, you'll need to add a check for AwaitUsingAddsPendingBranch (which has since been renamed AwaitUsingAndForeachAddsPendingBranch). I don't remember the scenario to hit this though.
Here's the relevant PR: #31527

In reply to: 243702724 [](ancestors = 243702724)

cston · 2018-12-21T22:13:36Z

using Microsoft.CodeAnalysis.CSharp.Test.Utilities;

Include copyright. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs:1 in bd69f1a. [](commit_id = bd69f1a, deletion_comment = False)

cston · 2018-12-21T22:14:28Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

+        await using C1 c = new C1();
+    }
+}";
+            var compilation = CreateCompilationWithTasksExtensions(source + _asyncDisposable, options: TestOptions.DebugExe).VerifyDiagnostics();


source + _asyncDisposable [](start = 67, length = 25)

Consider using new[] { source, _asyncDisposable } rather than concatenating source files.

jcouv · 2018-12-21T22:14:58Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+        /// <param name="diagnostics">Populated with any errors</param>
+        /// <param name="hasErrors">True if errors occured during binding</param>
+        /// <returns>An <see cref="AwaitableInfo"/> with the bound information</returns>
+        internal AwaitableInfo BindAsyncDisposeAwaiter(SyntaxNode node, SyntaxToken awaitKeyword, MethodSymbol disposeMethodOpt, DiagnosticBag diagnostics, ref bool hasErrors)


internal [](start = 8, length = 8)

protected?
#Closed

jcouv · 2018-12-21T22:16:07Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+        internal AwaitableInfo BindAsyncDisposeAwaiter(SyntaxNode node, SyntaxToken awaitKeyword, MethodSymbol disposeMethodOpt, DiagnosticBag diagnostics, ref bool hasErrors)
+        {
+            TypeSymbol taskType = disposeMethodOpt is null
+                                    ? this.Compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask)


GetWellKnownType [](start = 55, length = 16)

Please add a test where ValueTask is made missing (see MakeTypeMissing test helper).
Also a test where an obsolete or use-site diagnostic is reported. #Closed

jcouv · 2018-12-21T22:42:35Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                        }
+                        else
+                        {
+                            return this.ParseLocalDeclarationStatement(parseAwaitKeywordForAsyncStreams());


return this.ParseLocalDeclarationStatement(parseAwaitKeywordForAsyncStreams()); [](start = 28, length = 79)

maybe good to add a parsing test for this case

We have TestAwaitUsingVarWithVarDeclarationTree, unless I'm misunderstanding you?

In reply to: 243707369 [](ancestors = 243707369)

Ah, didn't realize. Thanks

jcouv · 2018-12-21T22:45:44Z

src/Compilers/CSharp/Portable/Syntax/StackAllocArrayCreationExpressionSyntax.cs

@@ -13,7 +13,7 @@ public StackAllocArrayCreationExpressionSyntax Update(SyntaxToken stackAllocKeyw
    public partial class LocalDeclarationStatementSyntax


LocalDeclarationStatementSyntax [](start = 25, length = 31)

Not caused by this PR: I didn't understand why this type is in this file (for stackalloc). Seems like it should be split into a separate file (for local declaration).

jcouv · 2018-12-21T22:50:10Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

+            var compilation = CreateCompilationWithTasksExtensions(source + _asyncDisposable, options: TestOptions.DebugExe).VerifyDiagnostics();
+
+            CompileAndVerify(compilation, expectedOutput: "Dispose async");
+        }


} [](start = 8, length = 1)

Please test

an async using-declaration with multiple resources.

two async using-declarations in a row.

language version 7.3 (you're missing a diagnostic for this)

When you pull the latest bits from master, it will also be good to do a little bit of cross-validation with async-iterator methods (adding some yield return statements between two async using-declarations, and such. #Closed

jcouv

Done with review pass (iteration 3). Mostly test feedback.

jcouv · 2018-12-22T00:45:05Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                awaitOpt = BindAsyncDisposeAwaiter(node, node.AwaitKeyword, disposeMethod, diagnostics, ref hasErrors);
+            }
+
+            MessageID.IDS_FeatureUsingDeclarations.CheckFeatureAvailability(availableVersion: Compilation.LanguageVersion, diagnostics, node.Declaration.Location);


CheckFeatureAvailability [](start = 51, length = 24)

Feels like this should be a parsing diagnostic rather than a binding diagnostic, unless there is some difficulty I'm missing. #Closed

Nope, i'm just still learning where things go :) will move to parsing. #Closed

jcouv · 2018-12-22T00:51:58Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

+";
+            var compilation = CreateCompilationWithTasksExtensions(source + _asyncDisposable, options: TestOptions.DebugExe).VerifyDiagnostics();
+
+            CompileAndVerify(compilation, expectedOutput: expectedOutput).VerifyIL("C2.<Main>d__0.System.Runtime.CompilerServices.IAsyncStateMachine.MoveNext()", @"


"C2.
d__0.System.Runtime.CompilerServices.IAsyncStateMachine.MoveNext()" [](start = 83, length = 77)

I've given up on IL verification for large async methods... I usually do a CompileAndVerify(...).Dump() verification by hand.

jcouv · 2018-12-22T00:53:28Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingDeclarationTests.cs

+
+            var compilation = CreateCompilationWithTasksExtensions(source + _asyncDisposable, options: TestOptions.DebugExe).VerifyDiagnostics();
+            CompileAndVerify(compilation, expectedOutput: expectedOutput);
+        }


} [](start = 8, length = 1)

nit: extra empty line below

jcouv

Done with review pass (iteration 5)

Cleanup tests

…'s actually present

jcouv

LGTM Thanks (iteration 8)

cston · 2018-12-22T06:18:31Z

src/Compilers/CSharp/Test/Semantic/Semantics/UsingDeclarationTests.cs

+        // c1 is obsolete
+        using C1 c1 = new C1();
+
+        // no warning, we don't warn on dispose being obsolete becuase it comes through interface


becuase [](start = 63, length = 7)

Typo

cston · 2018-12-22T06:25:52Z

src/Compilers/CSharp/Test/Syntax/Parsing/StatementParsingTests.cs

@@ -2354,7 +2354,7 @@ public void TestUsingWithDeclaration()
        public void TestUsingVarWithDeclaration()
        {
            var text = "using T a = b;";
-            var statement = this.ParseStatement(text);
+            var statement = this.ParseStatement(text, options: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp8));


CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp8) [](start = 63, length = 71)

Is there a difference between this and TestOptions.Regular8? (Just curious if there's a reason some tests use one and some use the other.)

I believe they are equivalent. I've switched to TestOptions.Regular8 to be consistent / more concise though.

cston · 2018-12-22T06:27:37Z

src/Compilers/CSharp/Test/Syntax/Parsing/StatementParsingTests.cs

+        }
+
+        [Fact]
+        public void TestAwaitUsingVarWithVarAnNoUsingDeclarationTree()


An [](start = 44, length = 2)

And?

jaredpar

chsienki added 3 commits December 20, 2018 21:48

Add syntax for await using var x = ...

ed3a094

- Update syntax nodes - Add parsing tests

Support binding and lowering for await using var x =...

c26cfa3

- Add awaitable info to bound node - Find and store the awaiter if async - Lower the declaration node correctly - Add tests

Fix parsing for erroneous using declarations

bd69f1a

chsienki added Area-Compilers New Language Feature - enhanced using Using pattern and declaration labels Dec 21, 2018

chsienki requested a review from a team as a code owner December 21, 2018 05:57

jcouv reviewed Dec 21, 2018

View reviewed changes

cston reviewed Dec 21, 2018

View reviewed changes

jcouv reviewed Dec 21, 2018

View reviewed changes

jcouv self-assigned this Dec 21, 2018

PR Feedback

b032aa2

Fix await highlighting + tests

1c6bbf7

chsienki requested a review from a team as a code owner December 22, 2018 00:34

jcouv reviewed Dec 22, 2018

View reviewed changes

chsienki added 3 commits December 21, 2018 17:02

Move langver check to parsing.

aaf79fb

Cleanup tests

Add reversed await/using declaration test

317e403

Only perform feature check on using token in local declarations if it…

abe9796

…'s actually present

jcouv approved these changes Dec 22, 2018

View reviewed changes

Fix parsing tests

ba10243

cston reviewed Dec 22, 2018

View reviewed changes

cston approved these changes Dec 22, 2018

View reviewed changes

PR Feedback

9880351

jaredpar approved these changes Dec 29, 2018

View reviewed changes

chsienki merged commit 2652bea into dotnet:features/enhanced-using Dec 29, 2018

		@@ -13,7 +13,7 @@ public StackAllocArrayCreationExpressionSyntax Update(SyntaxToken stackAllocKeyw
		public partial class LocalDeclarationStatementSyntax

Await using declarations #31980

Await using declarations #31980

Conversation

chsienki commented Dec 21, 2018

chsienki commented Dec 21, 2018

jcouv commented Dec 21, 2018

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

chsienki Dec 21, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

chsienki Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

cston Dec 21, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Dec 21, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston commented Dec 21, 2018 • edited by jcouv Loading

cston Dec 21, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

cston commented Dec 21, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv Dec 22, 2018 • edited Loading

Choose a reason for hiding this comment

chsienki Dec 22, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar left a comment

Choose a reason for hiding this comment

jcouv Dec 21, 2018 •

edited

Loading

jcouv Dec 21, 2018 •

edited

Loading

chsienki Dec 21, 2018 •

edited by jcouv

Loading

jcouv Dec 21, 2018 •

edited

Loading

jcouv Dec 21, 2018 •

edited

Loading

jcouv Dec 21, 2018 •

edited

Loading

jcouv Dec 21, 2018 •

edited

Loading

jcouv Dec 21, 2018 •

edited

Loading

chsienki Dec 21, 2018 •

edited

Loading

jcouv Dec 21, 2018 •

edited

Loading

cston Dec 21, 2018 •

edited by jcouv

Loading

cston Dec 21, 2018 •

edited by jcouv

Loading

cston commented Dec 21, 2018 •

edited by jcouv

Loading

cston Dec 21, 2018 •

edited by jcouv

Loading

jcouv Dec 21, 2018 •

edited

Loading

cston commented Dec 21, 2018 •

edited by jcouv

Loading

jcouv Dec 21, 2018 •

edited

Loading

jcouv Dec 21, 2018 •

edited

Loading

jcouv Dec 21, 2018 •

edited

Loading

jcouv Dec 22, 2018 •

edited

Loading

chsienki Dec 22, 2018 •

edited by jcouv

Loading