Move reporting of ERR_IteratorMustBeAsync to avoid race condition #39990

jcouv · 2019-11-23T00:33:53Z

AlekseyTs · 2019-11-23T01:05:15Z

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

@@ -162,10 +161,6 @@ internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSynta
 }
 elementType = TypeWithAnnotations.Create(CreateErrorType());
 }
- else if (asyncInterface && !_methodSymbol.IsAsync)
- {
- Error(elementTypeDiagnostics, ErrorCode.ERR_IteratorMustBeAsync, errorLocation, _methodSymbol, returnType);


Error(elementTypeDiagnostics, ErrorCode.ERR_IteratorMustBeAsync, errorLocation, _methodSymbol, returnType); [](start = 20, length = 107)

Can we get into similar trouble with errors reported above? #Closed

I think so, yes. #39992 is indeed similar. I'll address in this PR. Thanks! #Resolved

AlekseyTs · 2019-11-23T01:06:23Z

Could you please check if #39992 is the same/similar issue? #Closed

AlekseyTs · 2019-11-25T22:18:16Z

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

@@ -14594,6 +14594,7 @@ public static void Main()
 // public IEnumerator GetEnumerator()
 Diagnostic(ErrorCode.ERR_ReturnExpected, "GetEnumerator").WithArguments("C.GetEnumerator()")
 );
+ comp.GetEmitDiagnostics();


GetEmitDiagnostics [](start = 17, length = 18)

VerifyEmitDiagnostics? #Closed

AlekseyTs · 2019-11-25T22:46:38Z

    internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)

It looks like this parameter is no longer used. #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs:127 in 10d359f. [](commit_id = 10d359f, deletion_comment = False)

AlekseyTs · 2019-11-25T22:55:15Z

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

@@ -1897,6 +1898,7 @@ static System.Collections.Generic.IAsyncEnumerable<int> M()
 // static System.Collections.Generic.IAsyncEnumerable<int> M()
 Diagnostic(ErrorCode.ERR_IteratorMustBeAsync, "M").WithArguments("C.M()", "System.Collections.Generic.IAsyncEnumerable<int>").WithLocation(4, 61)
 );
+ comp.GetEmitDiagnostics();


GetEmitDiagnostics [](start = 17, length = 18)

VerifyEmitDiagnostics? #Closed

AlekseyTs · 2019-11-25T22:56:13Z

src/Compilers/CSharp/Test/Syntax/Diagnostics/DiagnosticTest.cs

@@ -2244,6 +2245,8 @@ public static int Main()
 }
 }";
 var compilation = CreateCompilation(text);
+ compilation.GetDiagnostics();


GetDiagnostics [](start = 24, length = 14)

Verify? #Closed

AlekseyTs · 2019-11-25T22:56:25Z

src/Compilers/CSharp/Test/Syntax/Diagnostics/DiagnosticTest.cs

@@ -2244,6 +2245,8 @@ public static int Main()
 }
 }";
 var compilation = CreateCompilation(text);
+ compilation.GetDiagnostics();
+ compilation.GetEmitDiagnostics();


GetEmitDiagnostics [](start = 24, length = 18)

Verify? #Closed

AlekseyTs · 2019-11-25T22:58:07Z

src/Compilers/CSharp/Test/Syntax/Diagnostics/DiagnosticTest.cs

@@ -2244,6 +2245,8 @@ public static int Main()
 }
 }";
 var compilation = CreateCompilation(text);
+ compilation.GetDiagnostics();


compilation.GetDiagnostics(); [](start = 12, length = 29)

Instead of modifying this particular unit-test, consider creating a new one. It looks like the original test is targeting specific sequence of API calls. #Closed

AlekseyTs · 2019-11-25T23:33:50Z

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

@@ -23,15 +23,13 @@ internal sealed class InMethodBinder : LocalScopeBinder

 private class IteratorInfo


IteratorInfo [](start = 22, length = 12)

Can we use TypeWithAnnotations.Boxed instead of this class now? #Closed

jcouv · 2019-11-25T23:34:10Z

    internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)

It's used in the override in WithLambdaParametersBinder

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

Refers to: src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs:127 in 10d359f. [](commit_id = 10d359f, deletion_comment = False)

AlekseyTs · 2019-11-25T23:42:27Z

    internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)

It's used in the override in WithLambdaParametersBinder

Would it make sense to move reporting of ERR_YieldInAnonMeth to methods that bind YieldStatementSyntax?

In reply to: 558389580 [](ancestors = 558389580,558375087)

Refers to: src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs:127 in 10d359f. [](commit_id = 10d359f, deletion_comment = False)

AlekseyTs · 2019-11-25T23:45:22Z

    internal override TypeWithAnnotations GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)

It feels like we can get rid of this parameter as well. #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs:127 in 10d359f. [](commit_id = 10d359f, deletion_comment = False)

AlekseyTs · 2019-11-26T00:22:20Z

Done with review pass (iteration 3) #Closed

jcouv · 2019-11-26T00:27:02Z

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

@@ -13,27 +13,15 @@ namespace Microsoft.CodeAnalysis.CSharp
 /// <summary>
 /// A binder for a method body, which places the method's parameters in scope
 /// and notes if the method is an iterator method.
+ /// Note: instances of this type can be re-used across different attempts at compiling the same method (caching by binder factory).


Note [](start = 8, length = 4)

📝 I didn't find any other diagnostics stored in this binder.

AlekseyTs · 2019-11-26T00:28:43Z

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

@@ -261,7 +265,8 @@ private BoundStatement BindYieldBreakStatement(YieldStatementSyntax node, Diagno
 Error(diagnostics, ErrorCode.ERR_YieldNotAllowedInScript, node.YieldKeyword);
 }

- GetIteratorElementType(node, diagnostics);
+ ValidateYield(node, diagnostics);
+ GetIteratorElementType();


GetIteratorElementType(); [](start = 12, length = 25)

It looks like this call is not necessary now. #Closed

AlekseyTs

LGTM (iteration 5)

chsienki

…ointers * dotnet/master: (197 commits) Update dependencies from https:/dotnet/arcade build 20191201.1 (dotnet#40084) Update list of C# Next features and reviewers (dotnet#39987) Update dependencies from https:/dotnet/arcade build 20191130.1 (dotnet#40075) [master] Update dependencies from dotnet/arcade (dotnet#40065) Update dependencies from https:/dotnet/arcade build 20191127.4 (dotnet#40057) Added more tests & fixed minor bug Deterministic ordering + added tests back Update dependencies from https:/dotnet/arcade build 20191126.2 (dotnet#40036) removed a test due to random order generation Always restore when running a bootstrap build (dotnet#40025) fixed integration test capitalization for extract method and extract interface Fix tests pt2 DiagnosticIncrementalAnalyzer refactoring (dotnet#39956) Persistence is always available in OOP Update src/Workspaces/Core/Portable/SolutionSize/SolutionSizeTracker.cs Fixed tests Move reporting of iterator diagnostics to avoid race condition (dotnet#39990) Update dependencies from https:/dotnet/arcade build 20191125.7 (dotnet#40020) Fix NGEN priority of csc.exe, vbc.exe, csi.exe (dotnet#40016) Accidentally deleted something ...

jcouv added 2 commits November 22, 2019 15:20

Skip lowering if first binding yielded error diagnostics

3d6644f

Move the fix to ValidateIteratorMethods

e5db947

jcouv added Area-Compilers New Feature - Async Streams Async Streams labels Nov 23, 2019

jcouv requested a review from a team as a code owner November 23, 2019 00:33

jcouv self-assigned this Nov 23, 2019

jcouv added this to the 16.5 milestone Nov 23, 2019

AlekseyTs reviewed Nov 23, 2019

View reviewed changes

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

Fix 39992 too

10d359f

AlekseyTs reviewed Nov 25, 2019

View reviewed changes

jcouv added 2 commits November 25, 2019 16:10

Use VerifyEmitDiagnostics and factor code

a05a32a

Remove IteratorInfo type

7a181ef

jcouv commented Nov 26, 2019

View reviewed changes

AlekseyTs reviewed Nov 26, 2019

View reviewed changes

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

AlekseyTs approved these changes Nov 26, 2019

View reviewed changes

jcouv and others added 2 commits November 25, 2019 16:43

Remove call to GetIteratorElementType

7747b7c

Fix regression with local func inside lambda

7505bef

chsienki approved these changes Nov 26, 2019

View reviewed changes

jcouv merged commit e2f6bf3 into dotnet:master Nov 26, 2019

jcouv deleted the cancellation-token2 branch November 26, 2019 18:21

jcouv modified the milestones: 16.5, 16.5.P1 Nov 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move reporting of ERR_IteratorMustBeAsync to avoid race condition #39990

Move reporting of ERR_IteratorMustBeAsync to avoid race condition #39990

jcouv commented Nov 23, 2019 •

edited

Loading

AlekseyTs Nov 23, 2019 •

edited

Loading

jcouv Nov 25, 2019 •

edited

Loading

AlekseyTs commented Nov 23, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs commented Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

jcouv commented Nov 25, 2019

AlekseyTs commented Nov 25, 2019

AlekseyTs commented Nov 25, 2019 •

edited

Loading

AlekseyTs commented Nov 26, 2019 •

edited

Loading

jcouv Nov 26, 2019

AlekseyTs Nov 26, 2019 •

edited

Loading

AlekseyTs left a comment

chsienki left a comment

		@@ -23,15 +23,13 @@ internal sealed class InMethodBinder : LocalScopeBinder

		private class IteratorInfo

Move reporting of ERR_IteratorMustBeAsync to avoid race condition #39990

Move reporting of ERR_IteratorMustBeAsync to avoid race condition #39990

Conversation

jcouv commented Nov 23, 2019 • edited Loading

AlekseyTs Nov 23, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 23, 2019 • edited Loading

AlekseyTs Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 25, 2019 • edited Loading

AlekseyTs Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv commented Nov 25, 2019

AlekseyTs commented Nov 25, 2019

AlekseyTs commented Nov 25, 2019 • edited Loading

AlekseyTs commented Nov 26, 2019 • edited Loading

jcouv Nov 26, 2019

Choose a reason for hiding this comment

AlekseyTs Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

chsienki left a comment

Choose a reason for hiding this comment

jcouv commented Nov 23, 2019 •

edited

Loading

AlekseyTs Nov 23, 2019 •

edited

Loading

jcouv Nov 25, 2019 •

edited

Loading

AlekseyTs commented Nov 23, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs commented Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs Nov 25, 2019 •

edited

Loading

AlekseyTs commented Nov 25, 2019 •

edited

Loading

AlekseyTs commented Nov 26, 2019 •

edited

Loading

AlekseyTs Nov 26, 2019 •

edited

Loading