Async-streams: minimal test for IOperation and CFG. Improve diagnostics #30363

jcouv · 2018-10-07T18:13:57Z

Added a couple of tests to verify that IOperation/CFG doesn't crash on async foreach, async using or async-iterator method. Opened follow-up issue for proper design/implementation (Finalize design and implementation of IOperation and CFG for async-streams feature #30362)
Refined diagnostic for wrong "async-ness" to help users switch between regular using and async using

jcouv · 2018-10-12T15:40:55Z

@dotnet/roslyn-compiler for review. Adding some tests to capture current IOperation/CFG behavior, and improving a diagnostic. Thanks #Closed

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

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IForEachLoopStatement.cs

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

AlekseyTs

LGTM (iteration 4)

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

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

jcouv · 2018-11-08T00:55:38Z

Re-targeted to preview2 branch. #Closed

333fred · 2018-11-08T18:15:55Z

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

-                TypeSymbol declType = declarations[0].DeclaredType.Type;
-
-                if (declType.IsDynamic())
+                if (declarationTypeOpt.IsDynamic())


declarationTypeOpt [](start = 20, length = 18)

If this is optional, consider ?. instead. If it is not optional, consider renaming. #Resolved

It's optional in the method, but it's always set in this branch, so I don't see a great way. I could have an extra local for the branch, but that seems overkill :-(

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

You could assert, but I'm ok with this.

In reply to: 232067572 [](ancestors = 232067572,232006228)

333fred · 2018-11-08T18:20:08Z

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

+            {
+                HashSet<DiagnosticInfo> useSiteDiagnostics = null;
+
+                iDisposableConversion = fromExpression ?


Nit: could consider extracting this expression into a local function. #Closed

333fred · 2018-11-08T18:22:20Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -2959,9 +2959,15 @@ A catch() block after a catch (System.Exception e) block can catch non-CLS excep
  <data name="ERR_NoConvToIDisp" xml:space="preserve">
    <value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable'</value>
  </data>
+  <data name="ERR_NoConvToIDispWrongAsync" xml:space="preserve">
+    <value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable'. Did you mean 'await using'?</value>


Nit: use the same end to the error message for both of these. #Closed

333fred · 2018-11-08T18:26:50Z

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IForEachLoopStatement.cs

+    }
+}
+";
+            // https://github.com/dotnet/roslyn/issues/30362 how do we flag `await`?


https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/Operations/IAwaitOperation.cs #Resolved

Thanks
We'll have to follow-up with IOperation design team. IAwaitOperation is currently meant for expressions.
I'm not sure that we want to represent an await foreach ... as an IAwaitOperation containing an IForEachLoopOperation. It is a bit odd to apply IAwaitOperation to a statement and have it modify the semantics of that statement.
Rather, I expect we would have a new node (IAwaitForEachLoopOperation?), or maybe a flag on IForEachLoopOperation.

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

333fred · 2018-11-08T18:28:04Z

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IForEachLoopStatement.cs

+            VerifyFlowGraphAndDiagnosticsForTest<BlockSyntax>(source + s_IAsyncEnumerable + s_ValueTask, expectedFlowGraph, expectedDiagnostics);
+        }
+
+        private static string s_ValueTask = @"


Is this something we should consider putting in CSharpTestBase? #Resolved

I hesitate to do that because it is a non-functional implementation.

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

333fred · 2018-11-08T18:29:15Z

Done review pass (commit 8) #Closed

jcouv · 2018-11-09T01:25:36Z

test this please

jcouv · 2018-11-09T02:32:54Z

@333fred I addressed your feedback (either with a fix or a reply). Let me know if good with you now.

@jaredpar Please approve for preview2

333fred

LGTM (commit 9)

jaredpar · 2018-11-09T18:46:21Z

approved

…out-if-error-message * origin/master: (174 commits) Add Compilers filter for Roslyn (dotnet#30880) Remove NonNullTypes context and other unnecessary information stored in TypeSymbolWithAnnotations. (dotnet#30913) Update BoundCall method based on receiver nullability (dotnet#31000) Make nullabiulity inference associative and commutative (dotnet#30990) Async-streams: minimal test for IOperation and CFG. Improve diagnostics (dotnet#30363) Add src. Add test. Remove dead code Update the build status table Script for generating our build status tables Add parsing tests to compiler benchmarks (dotnet#31033) Fix Edit and Continue in CPS projects Add comment Sorting. Save work Address PR feedback only produce optprof data on an official build Add bunch of exclusions to dead code analysis for special methods Disable WinRT tests on Linux (dotnet#31026) Change prerelease version to beta2 (dotnet#31017) ... # Conflicts: # src/Compilers/CSharp/Portable/CSharpResources.resx # src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

jcouv added Area-Compilers New Feature - Async Streams Async Streams labels Oct 7, 2018

jcouv added this to the 16.0 milestone Oct 7, 2018

jcouv self-assigned this Oct 7, 2018

jcouv requested a review from a team as a code owner October 7, 2018 18:13

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 7, 2018

jcouv force-pushed the async-blockers branch 2 times, most recently from 8b787ef to a315820 Compare October 8, 2018 16:50

jcouv mentioned this pull request Oct 9, 2018

[Umbrella] Work items and test plan for async streams #24037

Closed

jcouv force-pushed the async-blockers branch 3 times, most recently from 366c16c to d993252 Compare October 12, 2018 15:38

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 12, 2018

jcouv added the Feature - IOperation IOperation label Oct 12, 2018

jcouv mentioned this pull request Oct 12, 2018

Implement fixer for wrong async on foreach and using #30477

Closed

AlekseyTs reviewed Oct 12, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/UsingStatementBinder.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Oct 12, 2018

View reviewed changes

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IForEachLoopStatement.cs Show resolved Hide resolved

AlekseyTs reviewed Oct 12, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/UsingStatementBinder.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Oct 12, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/UsingStatementBinder.cs Outdated Show resolved Hide resolved

AlekseyTs approved these changes Oct 13, 2018

View reviewed changes

jcouv commented Oct 13, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs Show resolved Hide resolved

jcouv commented Oct 13, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/UsingStatementBinder.cs Show resolved Hide resolved

jcouv force-pushed the async-blockers branch from 8e3b20f to fb8aeb2 Compare October 16, 2018 22:18

jcouv requested review from a team as code owners October 16, 2018 22:18

jcouv changed the base branch from features/async-streams to master October 16, 2018 22:19

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 16, 2018

jcouv changed the base branch from master to dev16.0.x October 16, 2018 22:20

jcouv force-pushed the async-blockers branch from fb8aeb2 to 699a1f1 Compare November 6, 2018 01:03

jcouv and others added 7 commits November 7, 2018 16:50

Give specific error for wrong async

1ea14d9

fixup

5d9e170

Simplify logic for wrong async in using

e0f72d9

Unconditionally free arguments

9e3c291

Further simplification

ada5fad

Integrate with syntax change

0b1f6c8

Update XLF files

a89c4a6

jcouv force-pushed the async-blockers branch from 4f10a16 to a89c4a6 Compare November 8, 2018 00:55

333fred reviewed Nov 8, 2018

View reviewed changes

Address PR feedback from Fred

fb0982d

jcouv closed this Nov 9, 2018

jcouv reopened this Nov 9, 2018

jcouv modified the milestones: 16.0, 16.0.P2 Nov 9, 2018

333fred approved these changes Nov 9, 2018

View reviewed changes

jaredpar added the Approved to merge label Nov 9, 2018

jcouv merged commit d90b55e into dotnet:dev16.0-preview2 Nov 9, 2018

jcouv deleted the async-blockers branch November 9, 2018 18:48

jcouv mentioned this pull request Nov 16, 2018

Implement async pattern dispose #31201

Merged

agocke mentioned this pull request Dec 5, 2018

Follow up issues for #28587 #28892

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async-streams: minimal test for IOperation and CFG. Improve diagnostics #30363

Async-streams: minimal test for IOperation and CFG. Improve diagnostics #30363

jcouv commented Oct 7, 2018 •

edited

Loading

jcouv commented Oct 12, 2018 •

edited

Loading

AlekseyTs left a comment

jcouv commented Nov 8, 2018 •

edited

Loading

333fred Nov 8, 2018 •

edited

Loading

jcouv Nov 8, 2018

333fred Nov 9, 2018

333fred Nov 8, 2018 •

edited

Loading

333fred Nov 8, 2018 •

edited

Loading

333fred Nov 8, 2018 •

edited by jcouv

Loading

jcouv Nov 8, 2018

333fred Nov 8, 2018 •

edited by jcouv

Loading

jcouv Nov 8, 2018

333fred commented Nov 8, 2018 •

edited by jcouv

Loading

jcouv commented Nov 9, 2018

jcouv commented Nov 9, 2018

333fred left a comment

jaredpar commented Nov 9, 2018

Async-streams: minimal test for IOperation and CFG. Improve diagnostics #30363

Async-streams: minimal test for IOperation and CFG. Improve diagnostics #30363

Conversation

jcouv commented Oct 7, 2018 • edited Loading

jcouv commented Oct 12, 2018 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Nov 8, 2018 • edited Loading

333fred Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 8, 2018

Choose a reason for hiding this comment

333fred Nov 9, 2018

Choose a reason for hiding this comment

333fred Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Nov 8, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 8, 2018

Choose a reason for hiding this comment

333fred Nov 8, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 8, 2018

Choose a reason for hiding this comment

333fred commented Nov 8, 2018 • edited by jcouv Loading

jcouv commented Nov 9, 2018

jcouv commented Nov 9, 2018

333fred left a comment

Choose a reason for hiding this comment

jaredpar commented Nov 9, 2018

jcouv commented Oct 7, 2018 •

edited

Loading

jcouv commented Oct 12, 2018 •

edited

Loading

jcouv commented Nov 8, 2018 •

edited

Loading

333fred Nov 8, 2018 •

edited

Loading

333fred Nov 8, 2018 •

edited

Loading

333fred Nov 8, 2018 •

edited

Loading

333fred Nov 8, 2018 •

edited by jcouv

Loading

333fred Nov 8, 2018 •

edited by jcouv

Loading

333fred commented Nov 8, 2018 •

edited by jcouv

Loading