Async-streams: allow pattern-based disposal in await using and foreach #32731

jcouv · 2019-01-24T06:18:46Z

Customer scenario

Use new BCL helpers WithCancellation and ConfigureAwait on an IAsyncEnumerable.
If you await foreach over the resulting async collection, but break out of the loop before the end of the collection, the enumerator will not be disposed (but it should).

Bugs this fixes

Fixes #32316 (async using and foreach should allow pattern-based disposal)
Fixes #32722 (fix diagnostic message)

Workarounds, if any

None

Risk

Performance impact

Low. We are extending the recently added logic that allows foreach to bind to a pattern-based Dispose method so that await foreach can find a DisposeAsync method.

Is this a regression from a previous update?

No

Root cause analysis

This was supposed to fall out of the feature to bind pattern-based Dispose methods. But that feature was implemented later than expected, and it had some restrictions (only allowed for ref structs, due to last minute LDM decision).

How was the bug found?

While reviewing the BCL API.

Async-streams umbrella: #24037

Filed https://devdiv.visualstudio.com/DevDiv/_workitems/edit/783198 for shiproom

jcouv · 2019-01-25T01:32:41Z

@chsienki @cston @dotnet/roslyn-compiler for review for preview3 tomorrow/Friday. Thanks

jaredpar · 2019-01-25T02:07:10Z

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

@@ -677,7 +677,7 @@ internal MethodSymbol TryFindDisposePatternMethod(BoundExpression expr, SyntaxNo
        {
            Debug.Assert(!(expr is null));
            Debug.Assert(!(expr.Type is null));
-            Debug.Assert(expr.Type.IsValueType && expr.Type.IsRefLikeType); // pattern dispose lookup is only valid on ref structs
+            Debug.Assert((expr.Type.IsValueType && expr.Type.IsRefLikeType) || hasAwait); // pattern dispose lookup is only valid on ref structs or asynchronous usings


Why do we say IsValueType && IsRefLikeType? When can IsRefLikeType be true but IsValueType is false?

jaredpar · 2019-01-25T02:10:43Z

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

+                // Tracked by https://github.com/dotnet/roslyn/issues/32767
+
+                // extension methods do not contribute to pattern-based disposal
+                disposeMethod = null;


The disposeMethod is discarded here but what about any diagnostics that resulted from calling PerformPatternMethodLookup? Won't those still be in diagnostics and hence passed back up to the caller?

In the using scenarios, we keep those diagnostics and add another error (didn't find a proper way of disposing). See UsingPatternScopedExtensionMethodTest, which produces some unnecessary diagnostics. I'm thinking to leave that as-is for now, with tracking issue.

In the foreach scenarios, the caller already uses a separate diagnostic bag. I'll add tests to demonstrate.

jaredpar · 2019-01-25T02:12:02Z

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

+                ? this.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask, diagnostics, this._syntax)
+                : builder.DisposeMethod.ReturnType.TypeSymbol;
+
+            BoundExpression placeholder = new BoundAwaitableValuePlaceholder(_syntax.Expression, awaitableType);


Nit: Why does the local change the type here of the new expression? It's not passed by ref hence couldn't see an obvious reason for this

jaredpar · 2019-01-25T02:15:31Z

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

+            var comp = CreateCompilationWithTasksExtensions(new[] { source, s_IAsyncEnumerable }, options: TestOptions.DebugExe);
+            comp.VerifyDiagnostics();
+            CompileAndVerify(comp, expectedOutput: "MoveNextAsync DisposeAsync Done");
+        }


Please add a test where the enumerator is a ref struct and verify the IL to ensure we don't accidentally box the value. Possibly add one for normal struct as well.

No ref structs allowed in async scenarios (can't be captured into a state machine)

jaredpar · 2019-01-25T02:16:07Z

Done with review pass (iteration 2)

cston · 2019-01-25T05:52:48Z

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

                // We won't need to try and bind a second time if it fails, as async dispose can't be pattern based (ref structs are not allowed in async methods)
-                if (!(type is null) && type.IsValueType && type.IsRefLikeType)
+                bool isRefStruct = !(type is null) && type.IsValueType && type.IsRefLikeType;
+                if (!(type is null) && (isRefStruct || hasAwait))


!(type is null) [](start = 20, length = 15)

For readability, consider avoiding the duplicate !(type is null) check here and above. Perhaps inline isRefStruct in the expression here. Or perhaps use nested if blocks:

if (!(type is null)) { bool isRefStruct = ...; if (isRefStruct || hasAwait)) { ... } } ``` #Resolved

cston · 2019-01-25T06:13:36Z

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

+    {
+        return new MyAsyncEnumerator<T>();
+    }
+    public System.Threading.Tasks.ValueTask DisposeAsync()


DisposeAsync [](start = 44, length = 12)

Should this DisposeAsync be defined on MyAsyncEnumerator<T> rather than here? #Resolved

Good catch. Thanks

cston · 2019-01-25T06:13:57Z

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

+        }
+    }
+}";
+            // DisposeAsync on derived type is ignored, since we don't do runtime check


derived type is ignored [](start = 31, length = 23)

implementing type ...? #Resolved

cston · 2019-01-25T06:15:33Z

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

+}
+public static class Extension
+{
+    public static System.Threading.Tasks.ValueTask DisposeAsync(this C c) => throw null;


C c [](start = 69, length = 3)

Enumerator e #Resolved

Indeed. Thanks

cston · 2019-01-25T06:21:02Z

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

+        {
+            get => throw null;
+        }
+        public Awaitable DisposeAsync()


Awaitable [](start = 15, length = 9)

Consider testing a Task DisposeAsync() as well since that might be a common case. #Resolved

cston · 2019-01-25T06:26:58Z

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

+    }
+    public async System.Threading.Tasks.ValueTask DisposeAsync(params int[] x)
+    {
+        System.Console.Write($""dispose_start "");


dispose_start [](start = 32, length = 13)

Consider including $"x.Length = {x.Length}" so it's clear DisposeAsync was called with an array instance rather than null.

cston · 2019-01-25T06:27:48Z

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

+
+        return 1;
+    }
+    public int DisposeAsync()


int [](start = 11, length = 3)

Consider testing Task DisposeAsync() or Task<bool> DisposeAsync().

The latter is a great case. Currently it is accepted, because you can indeed do await enumerator.DisposeAsync();.
Do you think it should be rejected?

jcouv · 2019-01-25T06:28:16Z

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


-                BoundExpression placeholder = new BoundAwaitableValuePlaceholder(syntax, taskType).MakeCompilerGenerated();
-                awaitOpt = originalBinder.BindAwaitInfo(placeholder, syntax, awaitKeyword.GetLocation(), diagnostics, ref hasErrors);
+                // even if we don't have a proper value to await, we'll still report bad usages of `await`


📝 an IDE test caught a regression. Even if there is no disposable type, we should report bad usage of await. Added TestInRegularMethod to illustrate in compiler tests.

cston · 2019-01-25T06:38:26Z

            Diagnostic(ErrorCode.ERR_AmbigCall, "S1 s = new S1()").WithArguments("C2.Dispose(S1)", "C3.Dispose(S1)").WithLocation(20, 16),

Consider adding comment for #32767 here.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/UsingStatementTests.cs:445 in e3622a4. [](commit_id = e3622a4, deletion_comment = False)

cston · 2019-01-25T06:38:37Z

            Diagnostic(ErrorCode.ERR_AmbigCall, "S1 s = new S1()").WithArguments("C2.Dispose(S1, int)", "C3.Dispose(S1, int)").WithLocation(21, 15),

Consider adding comment for #32767 here.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/UsingStatementTests.cs:679 in e3622a4. [](commit_id = e3622a4, deletion_comment = False)

cston · 2019-01-25T07:06:35Z

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

+
+        [ConditionalFact(typeof(WindowsDesktopOnly))]
+        [WorkItem(32316, "https://github.com/dotnet/roslyn/issues/32316")]
+        public void PatternBasedDisposal_NoExtensions_TwoExtensions()


NoExtensions_TwoExtensions [](start = 41, length = 26)

Is this test necessary? Do we look up extension methods for the collection type?

I think it is useful. See the tracking issue #32767
In this PR, I reject a binding if it is an extension, but we should really do the binding without considering extensions in the first place. So I added tests to show whether this has an impact or not. It does have an impact on using (we'll produce an ambiguity diagnostic), but does not impact foreach (shown here).

The extension methods take the collection type not the enumerator type though, and would not be considered even if extension methods were supported for pattern-based dispose.

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

Oops, sorry, same testing bug as above. Should be extensions on the enumerator

cston · 2019-01-25T07:25:32Z

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

+
+        [ConditionalFact(typeof(WindowsDesktopOnly))]
+        [WorkItem(32316, "https://github.com/dotnet/roslyn/issues/32316")]
+        public void TestPatternBasedDisposal_ReturnsTaskOfInt()


ReturnsTaskOfInt [](start = 45, length = 16)

Testing one of Task<int> or Task<bool> should be sufficient.

Removed the bool one

cston · 2019-01-25T16:01:47Z

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

+    }
+    public sealed class Enumerator
+    {
+        public async System.Threading.Tasks.Task<bool> MoveNextAsync()


System.Threading.Tasks. [](start = 21, length = 23)

It looks like the namespace qualifier can be removed in all these tests.

Removed unnecessary qualifications

jaredpar

jcouv · 2019-01-25T17:42:16Z

Thanks!

dotnet#32731)

#32731) (#32815)

dotnet#32731)

dotnet#32731) (dotnet#32815)

jcouv added Area-Compilers Feature - Async Streams Async Streams labels Jan 24, 2019

jcouv added this to the 16.0.P3 milestone Jan 24, 2019

jcouv self-assigned this Jan 24, 2019

jcouv requested a review from a team as a code owner January 24, 2019 06:18

jcouv force-pushed the pattern-disposal branch from f430844 to ceddc75 Compare January 24, 2019 06:23

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 24, 2019

Async-streams: allow pattern-based disposal in await using and foreach

d772212

jcouv force-pushed the pattern-disposal branch from ceddc75 to d772212 Compare January 24, 2019 21:14

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 25, 2019

jcouv force-pushed the pattern-disposal branch from 321c873 to f0e2f27 Compare January 25, 2019 01:30

jaredpar reviewed Jan 25, 2019

View reviewed changes

Discard found extension methods

e3622a4

jcouv force-pushed the pattern-disposal branch from f0e2f27 to e3622a4 Compare January 25, 2019 03:33

cston reviewed Jan 25, 2019

View reviewed changes

Address PR feedback

5945e59

cston reviewed Jan 25, 2019

View reviewed changes

Address feedback from Chuck

1e16d7e

cston reviewed Jan 25, 2019

View reviewed changes

jcouv commented Jan 25, 2019

View reviewed changes

cston approved these changes Jan 25, 2019

View reviewed changes

cston reviewed Jan 25, 2019

View reviewed changes

More tests

898aa41

jcouv force-pushed the pattern-disposal branch from 3c09cb2 to 898aa41 Compare January 25, 2019 15:42

cston reviewed Jan 25, 2019

View reviewed changes

Tweak tests

e2d996c

jaredpar approved these changes Jan 25, 2019

View reviewed changes

jcouv merged commit 91a119b into dotnet:master Jan 25, 2019

jcouv deleted the pattern-disposal branch January 25, 2019 17:40

JoeRobich pushed a commit to JoeRobich/roslyn that referenced this pull request Jan 26, 2019

Async-streams: allow pattern-based disposal in await using and foreach (

7f71ee9

dotnet#32731)

JoeRobich added a commit that referenced this pull request Jan 28, 2019

Async-streams: allow pattern-based disposal in await using and foreach (

c6d6c04

#32731) (#32815)

jcouv mentioned this pull request Feb 14, 2019

Proposal: Allow using statement to structurally match IDisposable on ref structs (16.3, Core 3) dotnet/csharplang#1623

Open

5 tasks

xoofx pushed a commit to stark-lang/stark-roslyn that referenced this pull request Apr 16, 2019

Async-streams: allow pattern-based disposal in await using and foreach (

befd401

dotnet#32731)

xoofx pushed a commit to stark-lang/stark-roslyn that referenced this pull request Apr 16, 2019

Async-streams: allow pattern-based disposal in await using and foreach (

17fe865

dotnet#32731) (dotnet#32815)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async-streams: allow pattern-based disposal in await using and foreach #32731

Async-streams: allow pattern-based disposal in await using and foreach #32731

jcouv commented Jan 24, 2019 •

edited

Loading

jcouv commented Jan 25, 2019

jaredpar Jan 25, 2019

jaredpar Jan 25, 2019

jcouv Jan 25, 2019

jaredpar Jan 25, 2019

jaredpar Jan 25, 2019

jcouv Jan 25, 2019

jaredpar commented Jan 25, 2019

cston Jan 25, 2019 •

edited

Loading

cston Jan 25, 2019 •

edited

Loading

jcouv Jan 25, 2019

cston Jan 25, 2019 •

edited

Loading

cston Jan 25, 2019 •

edited

Loading

jcouv Jan 25, 2019

cston Jan 25, 2019 •

edited

Loading

cston Jan 25, 2019

cston Jan 25, 2019

jcouv Jan 25, 2019

jcouv Jan 25, 2019

cston commented Jan 25, 2019

cston commented Jan 25, 2019

cston Jan 25, 2019

jcouv Jan 25, 2019

cston Jan 25, 2019

jcouv Jan 25, 2019

cston Jan 25, 2019

jcouv Jan 25, 2019

cston Jan 25, 2019

jcouv Jan 25, 2019

jaredpar left a comment

jcouv commented Jan 25, 2019

Async-streams: allow pattern-based disposal in await using and foreach #32731

Async-streams: allow pattern-based disposal in await using and foreach #32731

Conversation

jcouv commented Jan 24, 2019 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

jcouv commented Jan 25, 2019

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

Choose a reason for hiding this comment

jaredpar commented Jan 25, 2019

cston Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

cston Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

cston Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jan 25, 2019 • 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

cston commented Jan 25, 2019

cston commented Jan 25, 2019

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

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 commented Jan 25, 2019

jcouv commented Jan 24, 2019 •

edited

Loading

cston Jan 25, 2019 •

edited

Loading

cston Jan 25, 2019 •

edited

Loading

cston Jan 25, 2019 •

edited

Loading

cston Jan 25, 2019 •

edited

Loading

cston Jan 25, 2019 •

edited

Loading