Fix binding order for Dispose in async foreach by jcouv · Pull Request #60022 · dotnet/roslyn

jcouv · 2022-03-08T02:40:59Z

The affected scenario is when we enumerate using instance methods (as opposed to through interfaces), but we have a choice for disposal. In that scenario, we'll start preferring the instance AsyncDispose() method.

For scenarios where we enumerate through interfaces, we'll find the AsyncDispose() method on the enumerator type (ie. IAsyncEnumerator), so a conversion is removed (see affected CFG tests).

Clarifying spec in PR dotnet/csharplang#5898.

FYI @bernd5

jcouv · 2022-03-08T06:27:41Z

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

                this.PeekToken(1).Kind == SyntaxKind.ForEachKeyword)
            {
-                return this.ParseForEachStatement(attributes, ParseAwaitKeyword(MessageID.IDS_FeatureAsyncStreams));
+                return this.ParseForEachStatement(attributes, ParseAwaitKeyword(MessageID.None));


📝 I had to move those errors to binding layer, as they were affecting binding logic (which uses BoundNode.HasAnyErrors), but we want to bind the same way regardless of LangVer.

jcouv · 2022-03-08T06:29:58Z

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

-                //         await using (var x = this)
-                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7_3, "await").WithArguments("asynchronous using", "8.0").WithLocation(6, 9)
-                );
+            tree.GetDiagnostics().Verify(); // LangVer error reported in binding


📝 See CodeGenAwaitUsingTests.TestWithCSharp7_3

jcouv · 2022-03-08T06:30:52Z

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

-                //         await foreach (var i in collection)
-                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7_3, "await").WithArguments("async streams", "8.0").WithLocation(6, 9)
-                );
+            tree.GetDiagnostics().Verify(); // LangVer error reported in binding


📝 See CodeGenAwaitForeachTests.TestWithCSharp7_3

jcouv · 2022-03-08T06:32:18Z

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

-                //         await foreach (var (i, j) in collection)
-                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7_3, "await").WithArguments("async streams", "8.0").WithLocation(6, 9)
-                );
+            tree.GetDiagnostics().Verify(); // LangVer error reported in binding


📝 See CodeGenAwaitForeachTests.TestDeconstructionWithCSharp7_3

jcouv · 2022-03-08T06:35:31Z

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

-            var boundNode = GetBoundForEachStatement(text, TestOptions.Regular7_3);
+            var boundNode = GetBoundForEachStatement(text, TestOptions.Regular7_3,
+                // error CS8370: Feature 'using declarations' is not available in C# 7.3. Please use language version 8.0 or greater.
+                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7_3).WithArguments("using declarations", "8.0").WithLocation(1, 1)


📝 Added documentation for breaking change.

jcouv · 2022-03-08T06:39:22Z

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

-                this.Conversions.ClassifyImplicitConversionFromType(enumeratorType,
-                    isAsync ? this.Compilation.GetWellKnownType(WellKnownType.System_IAsyncDisposable) : this.Compilation.GetSpecialType(SpecialType.System_IDisposable),
-                    ref useSiteInfo).IsImplicit)
+            MethodSymbol patternDisposeMethod = null;


📝 The two conditional branches were swapped (pattern-based comes first, then convert to interface) and the LangVer check no longer affects binding result.

cston · 2022-03-09T20:03:07Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md

+
+    struct AsyncEnumerable
+    {
+        AsyncEnumerator<int> GetAsyncEnumerator(CancellationToken token) => new AsyncEnumerator();


Suggested change

AsyncEnumerator<int> GetAsyncEnumerator(CancellationToken token) => new AsyncEnumerator();

public AsyncEnumerator GetAsyncEnumerator(CancellationToken token) => new AsyncEnumerator();

``` #Closed

cston · 2022-03-09T20:04:02Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md

+    }
+    ```
+
+9. <a name="9"></a>Starting with Visual Studio 17.2, a `foreach` using a ref struct enumerator type reports an error if the language version is set to 7.3 or older.


Perhaps "earlier" or "lower".

Suggested change

9. <a name="9"></a>Starting with Visual Studio 17.2, a `foreach` using a ref struct enumerator type reports an error if the language version is set to 7.3 or older.

9. <a name="9"></a>Starting with Visual Studio 17.2, a `foreach` using a ref struct enumerator type reports an error if the language version is set to 7.3 or earlier.

``` #Closed

cston · 2022-03-09T20:07:19Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md

+    {
+    }
+
+    struct AsyncEnumerable


Is AsyncEnumerable required to implement IAsyncEnumerable<int>? #Resolved

No. This sample illustrate pattern-based enumeration and a choice for method of disposal (pattern-based or interface-based).

cston · 2022-03-09T20:07:42Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md

+        AsyncEnumerator<int> GetAsyncEnumerator(CancellationToken token) => new AsyncEnumerator();
+    }
+
+    struct AsyncEnumerator : IAsyncDisposable


Is AsyncEnumerator required to implement IAsyncEnumerator<int>? #Resolved

Answered above

cston · 2022-03-09T21:29:45Z

    private SyntaxToken ParseAwaitKeyword(MessageID feature)

Can the ParseAwaitKeyword(MessageID feature) parameter be removed now?

In reply to: 1063390690

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:7888 in f45129b. [](commit_id = f45129b, deletion_comment = False)

cston · 2022-03-09T21:40:16Z

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

+            var comp = CreateCompilation(source, parseOptions: TestOptions.Regular7_3);
+            comp.VerifyDiagnostics(
+                // error CS8370: Feature 'using declarations' is not available in C# 7.3. Please use language version 8.0 or greater.
+                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7_3).WithArguments("using declarations", "8.0").WithLocation(1, 1)


"using declarations"

This seems incorrect since there are no using declarations. #Closed

The "using declarations" feature bundled two things: using declarations and pattern-based disposal.
I'm fine to split but it snowballs the PR a bit.

I don't think we need to separate "pattern-based disposal" from "using declarations", but it feels like this error message should reference "async streams" rather than "using declarations".

You can hit this with foreach on a ref struct. It's not specific to async streams...

cston · 2022-03-09T21:42:52Z

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

            Assert.Equal(2, op.Info.GetEnumeratorArguments.Length);
            Assert.Equal(3, op.Info.MoveNextArguments.Length);
-            Assert.True(op.Info.DisposeArguments.IsDefault);
-            Assert.Null(op.Info.PatternDisposeMethod);


Assert.Null(op.Info.PatternDisposeMethod);

Are we testing the case where PatternDisposeMethod is null?

cston · 2022-03-09T21:46:15Z

    public void AwaitUsingDeclaration_WithCSharp73()

The behavior should be identical to the C#8 case now. Consider merging with AwaitUsingDeclaration() (and adding aLanguageVersion parameter to the test) so that is clear.

In reply to: 1063404480

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

cston · 2022-03-09T21:46:29Z

    public void AwaitForeach_WithCSharp73()

The behavior should be identical to the C#8 case now. Consider merging with AwaitForeach() (and adding aLanguageVersion parameter to the test) so that is clear.

In reply to: 1063404768

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

cston · 2022-03-09T21:46:52Z

    public void DeconstructionAwaitForeach_WithCSharp73()

The behavior should be identical to the C#8 case now. Consider merging with DeconstructionAwaitForeach() (and adding aLanguageVersion parameter to the test) so that is clear.

In reply to: 1063405174

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

cston · 2022-03-10T01:30:55Z

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

+            var comp = CreateCompilation(source, parseOptions: TestOptions.Regular7_3);
+            comp.VerifyDiagnostics(
+                // error CS8370: Feature 'using declarations' is not available in C# 7.3. Please use language version 8.0 or greater.
+                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7_3).WithArguments("using declarations", "8.0").WithLocation(1, 1)


WithLocation(1, 1)

Should the location be new Enumerable1() to give an indication what caused the error? #Closed

jcouv · 2022-03-10T17:43:51Z

@cston @333fred for review. Thanks

cston · 2022-03-11T04:52:40Z

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

            // If IDisposable is available but its Dispose method is not, then diagnostics will be reported only if the enumerator
            // is potentially disposable.

+            // For async foreach, we don't do the runtime check


// For async foreach, we don't do the runtime check

Consider moving to line 1045, after the following, so it's clear what this refers to.

// if it wasn't pattern-disposable, see if it's directly convertable to IDisposable ``` #Resolved

jcouv · 2022-03-11T08:38:58Z

Tagging @dotnet/docs @BillWagner for new breaking changes.

jcouv · 2022-03-11T17:18:25Z

@dotnet/roslyn-compiler for second review. Thanks

333fred

LGTM (commit 3)

jcouv added the Area-Compilers label Mar 8, 2022

jcouv self-assigned this Mar 8, 2022

jcouv force-pushed the dispose-order branch 3 times, most recently from 6fb6a16 to 0938a6b Compare March 8, 2022 03:59

jcouv mentioned this pull request Mar 8, 2022

Clarify binding rules for await foreach dotnet/csharplang#5898

Merged

jcouv force-pushed the dispose-order branch from 0938a6b to ae17cb4 Compare March 8, 2022 06:21

jcouv commented Mar 8, 2022

View reviewed changes

Fix binding order for Dispose in async foreach

f45129b

jcouv force-pushed the dispose-order branch from ae17cb4 to f45129b Compare March 8, 2022 06:40

jcouv added the Feature - Async Streams Async Streams label Mar 8, 2022

runfoapp bot mentioned this pull request Mar 8, 2022

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

jcouv marked this pull request as ready for review March 8, 2022 17:56

jcouv requested a review from a team as a code owner March 8, 2022 17:56

cston reviewed Mar 9, 2022

View reviewed changes

cston reviewed Mar 10, 2022

View reviewed changes

Address feedback

dc345a9

jcouv requested review from 333fred and cston March 10, 2022 01:54

cston reviewed Mar 11, 2022

View reviewed changes

Move comment

7cfff7f

jcouv requested a review from cston March 11, 2022 08:39

cston approved these changes Mar 11, 2022

View reviewed changes

333fred approved these changes Mar 14, 2022

View reviewed changes

jcouv enabled auto-merge (squash) March 14, 2022 23:29

jcouv merged commit 8192820 into dotnet:main Mar 15, 2022

ghost added this to the Next milestone Mar 15, 2022

allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022

	AsyncEnumerator<int> GetAsyncEnumerator(CancellationToken token) => new AsyncEnumerator();
	public AsyncEnumerator GetAsyncEnumerator(CancellationToken token) => new AsyncEnumerator();
	``` #Closed

	9. <a name="9"></a>Starting with Visual Studio 17.2, a `foreach` using a ref struct enumerator type reports an error if the language version is set to 7.3 or older.
	9. <a name="9"></a>Starting with Visual Studio 17.2, a `foreach` using a ref struct enumerator type reports an error if the language version is set to 7.3 or earlier.
	``` #Closed

Conversation

jcouv commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented Mar 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented Mar 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Mar 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Mar 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 10, 2022

Uh oh!

cston Mar 11, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 11, 2022

Uh oh!

jcouv commented Mar 11, 2022

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

jcouv commented Mar 8, 2022 •

edited

Loading

jcouv Mar 8, 2022 •

edited

Loading

jcouv Mar 8, 2022 •

edited

Loading

cston Mar 9, 2022 •

edited

Loading

cston Mar 9, 2022 •

edited

Loading

cston Mar 9, 2022 •

edited by jcouv

Loading

cston Mar 9, 2022 •

edited by jcouv

Loading

cston commented Mar 9, 2022 •

edited by jcouv

Loading

cston Mar 9, 2022 •

edited

Loading

cston commented Mar 9, 2022 •

edited by jcouv

Loading

cston commented Mar 9, 2022 •

edited by jcouv

Loading

cston commented Mar 9, 2022 •

edited by jcouv

Loading

cston Mar 10, 2022 •

edited

Loading

cston Mar 11, 2022 •

edited by jcouv

Loading