Support EnumeratorCancellationAttribute in local functions #40959

RikkiGibson · 2020-01-14T20:13:36Z

Related to #38801

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

jcouv · 2020-01-14T20:36:19Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs

@@ -227,35 +230,39 @@ public override ImmutableArray<CustomModifier> RefCustomModifiers

        private sealed class SynthesizedComplexParameterSymbol : SynthesizedParameterSymbolBase
        {
-            private readonly ImmutableArray<CustomModifier> _refCustomModifiers;
-            private readonly ImmutableArray<CSharpAttributeData> _attributes;
+            private readonly ParameterSymbol _baseParameter;


nit: consider enabling nullability analysis in this file, or asserting that baseParameter is not null in constructor. #Resolved

jcouv

Done with review pass (iteration 1). Thanks!

AlekseyTs · 2020-01-15T22:27:06Z

@RikkiGibson Do you know the reason for CI failures? #Closed

RikkiGibson · 2020-01-15T22:30:12Z

EE was using the old signatures of the internal SynthesizedParameterSymbol.Create methods. Working on how to rationalize the design/hierarchy of the synthesized parameter symbols in light of this. #Resolved

src/Compilers/CSharp/Portable/Symbols/ParameterSymbol.cs

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs

AlekseyTs · 2020-01-15T23:18:18Z

Done with review pass (iteration 1). #Closed

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs

jcouv · 2020-01-17T01:49:04Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs

@@ -833,6 +834,34 @@ internal sealed override void PostDecodeWellKnownAttributes(ImmutableArray<CShar
            base.PostDecodeWellKnownAttributes(boundAttributes, allAttributeSyntaxNodes, diagnostics, symbolPart, decodedData);
        }

+        // Called after all method attributes, return attributes, and parameter attributes have been initialized on this method.
+        protected void PostDecodeAllMethodAttributes(DiagnosticBag diagnostics)


PostDecodeAllMethodAttributes [](start = 23, length = 29)

nit: could be given a more specific name at the moment, since it only checks EnumeratorCancellation attribute #Resolved

jcouv

LGTM Thanks (iteration 4)

src/Compilers/CSharp/Portable/Lowering/SynthesizedMethodBaseSymbol.cs

AlekseyTs · 2020-01-17T18:14:25Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

-                    if (enumeratorCancellationCount == 0 &&
-                        ParameterTypesWithAnnotations.Any(p => p.Type.Equals(cancellationTokenType)))
-                    {
-                        // Warn for CancellationToken parameters in async-iterators with no parameter decorated with [EnumeratorCancellation]


async-iterators [](start = 68, length = 15)

@jcouv I assume you added this code originally, where do we make sure this is an iterator?

Seems like we just check that we have an IAsyncEnumerable return type. Would we want to warn when the method uses that return type and [EnumeratorCancellation] on a parameter, but isn't actually an iterator? @jcouv

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

@AlekseyTs From our discussion, there are three factors to make an async-iterator: (1) async, (2) the return type, and (3) a yield in the body.
But we enforce that you cannot have (1) and (2) without having (3). We produce an error in that case (see test below). So in other places in the code, we can just check for (1) and (2).
I have a vague recollection that there was some technical benefit to this approach, but I'm not longer sure.

@RikkiGibson Would you mind adding a comment in the code that you're moving to capture this? I'd add it in master, but that would only create a conflict for when you'll merge...

[Fact] [WorkItem(31552, "https://github.com/dotnet/roslyn/issues/31552")] public void AsyncIterator_WithThrowOnly() { string source = @" class C { public static async System.Collections.Generic.IAsyncEnumerable<int> M() { throw new System.NotImplementedException(); } }"; var comp = CreateCompilationWithAsyncIterator(source); comp.VerifyDiagnostics( // (4,74): warning CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread. // public static async System.Collections.Generic.IAsyncEnumerable<int> M() Diagnostic(ErrorCode.WRN_AsyncLacksAwaits, "M").WithLocation(4, 74) ); comp.VerifyEmitDiagnostics( // (4,74): warning CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread. // public static async System.Collections.Generic.IAsyncEnumerable<int> M() Diagnostic(ErrorCode.WRN_AsyncLacksAwaits, "M").WithLocation(4, 74), // (4,74): error CS8420: The body of an async-iterator method must contain a 'yield' statement. Consider removing 'async' from the method declaration or adding a 'yield' statement. // public static async System.Collections.Generic.IAsyncEnumerable<int> M() Diagnostic(ErrorCode.ERR_PossibleAsyncIteratorWithoutYieldOrAwait, "M").WithArguments("System.Collections.Generic.IAsyncEnumerable<int>").WithLocation(4, 74) ); var m = comp.SourceModule.GlobalNamespace.GetMember<MethodSymbol>("C.M"); Assert.False(m.IsIterator); Assert.True(m.IsAsync); }

AlekseyTs · 2020-01-17T18:25:07Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

-                }
-            }
-
+            PostDecodeAllMethodAttributes(diagnostics);


PostDecodeAllMethodAttributes [](start = 12, length = 29)

It feels like "AfterReturnTypeBoundAndParametersAreCreated" is more precise name. However, I don't think we need a dedicated method for this operation. In fact, I think that this is not the right place for this kind of checks. We haven't finalized the signature of the method yet and are likely to mutate it below. It feels like this check belongs in LazyAsyncMethodChecks, which is called in the right time. Also, it looks like for local functions the checks performed by LazyAsyncMethodChecks are duplicated. I think it would be good to consolidate that and avoid code duplication. #Closed

AlekseyTs · 2020-01-17T18:31:15Z

src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs

@@ -125,6 +125,8 @@ internal void GetDeclarationDiagnostics(DiagnosticBag addTo)
            GetAttributes();
            GetReturnTypeAttributes();

+            PostDecodeAllMethodAttributes(_declarationDiagnostics);


PostDecodeAllMethodAttributes(_declarationDiagnostics); [](start = 12, length = 55)

Should consolidate with other async-related checks performed for local functions and probably share code with regular methods so that there was one, well defined place where we perform async-related checks. #Closed

AlekseyTs · 2020-01-17T18:40:25Z

Done with review pass (iteration 5) #Closed

RikkiGibson · 2020-01-20T17:52:03Z

@AlekseyTs Please let me know if I have adequately addressed your comments.

AlekseyTs · 2020-01-21T18:59:20Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbol.cs

+        {
+            foreach (var parameter in Parameters)
+            {
+                var loc = parameter.Locations.FirstOrDefault() ?? location;


parameter.Locations.FirstOrDefault() ?? location [](start = 26, length = 48)

Consider calculating this lazily, i.e. only when we have something to report.

AlekseyTs

LGTM (iteration 7)

Support EnumeratorCancellationAttribute in local functions

883cace

RikkiGibson added the Area-Compilers label Jan 14, 2020

RikkiGibson added this to the Compiler.Next milestone Jan 14, 2020

RikkiGibson requested a review from a team as a code owner January 14, 2020 20:13

jcouv reviewed Jan 14, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs Show resolved Hide resolved

jcouv reviewed Jan 14, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs Show resolved Hide resolved

jcouv reviewed Jan 14, 2020

View reviewed changes

jcouv self-assigned this Jan 14, 2020

AlekseyTs reviewed Jan 15, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/ParameterSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jan 15, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jan 15, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jan 15, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs Outdated Show resolved Hide resolved

RikkiGibson added 2 commits January 16, 2020 15:18

Simplify design

29fd9c8

Add tests

b1b70b5

cston reviewed Jan 16, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs Outdated Show resolved Hide resolved

cston reviewed Jan 16, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs Outdated Show resolved Hide resolved

cston approved these changes Jan 16, 2020

View reviewed changes

RikkiGibson requested review from AlekseyTs and jcouv January 17, 2020 00:04

Respond to feedback

6171c43

jcouv reviewed Jan 17, 2020

View reviewed changes

jcouv approved these changes Jan 17, 2020

View reviewed changes

RikkiGibson closed this Jan 17, 2020

RikkiGibson reopened this Jan 17, 2020

AlekseyTs reviewed Jan 17, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/SynthesizedMethodBaseSymbol.cs Outdated Show resolved Hide resolved

Check inheritAttributes

bf2b366

AlekseyTs reviewed Jan 17, 2020

View reviewed changes

RikkiGibson added 2 commits January 17, 2020 13:50

Make NoteAttributesComplete abstract on base

8f8f455

Consolidate async method checks

ccdb14b

cston approved these changes Jan 17, 2020

View reviewed changes

AlekseyTs reviewed Jan 21, 2020

View reviewed changes

AlekseyTs approved these changes Jan 21, 2020

View reviewed changes

Address feedback

ec3da7b

RikkiGibson merged commit 885617c into dotnet:features/local-function-attributes Jan 21, 2020

RikkiGibson deleted the lfa-synthesized-params branch January 21, 2020 22:04

RikkiGibson mentioned this pull request Jan 23, 2020

Test plan for "local function attributes" #38801

Closed

54 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support EnumeratorCancellationAttribute in local functions #40959

Support EnumeratorCancellationAttribute in local functions #40959

RikkiGibson commented Jan 14, 2020

jcouv Jan 14, 2020 •

edited by RikkiGibson

Loading

jcouv left a comment

AlekseyTs commented Jan 15, 2020 •

edited

Loading

RikkiGibson commented Jan 15, 2020 •

edited

Loading

AlekseyTs commented Jan 15, 2020 •

edited

Loading

jcouv Jan 17, 2020 •

edited by RikkiGibson

Loading

jcouv left a comment

AlekseyTs Jan 17, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 20, 2020

jcouv Jan 21, 2020

AlekseyTs Jan 17, 2020 •

edited

Loading

AlekseyTs Jan 17, 2020 •

edited

Loading

AlekseyTs commented Jan 17, 2020 •

edited

Loading

RikkiGibson commented Jan 20, 2020

AlekseyTs Jan 21, 2020

AlekseyTs left a comment

Support EnumeratorCancellationAttribute in local functions #40959

Support EnumeratorCancellationAttribute in local functions #40959

Conversation

RikkiGibson commented Jan 14, 2020

jcouv Jan 14, 2020 • edited by RikkiGibson Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 15, 2020 • edited Loading

RikkiGibson commented Jan 15, 2020 • edited Loading

AlekseyTs commented Jan 15, 2020 • edited Loading

jcouv Jan 17, 2020 • edited by RikkiGibson Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs Jan 17, 2020 • edited by RikkiGibson Loading

Choose a reason for hiding this comment

RikkiGibson Jan 20, 2020

Choose a reason for hiding this comment

jcouv Jan 21, 2020

Choose a reason for hiding this comment

AlekseyTs Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 17, 2020 • edited Loading

RikkiGibson commented Jan 20, 2020

AlekseyTs Jan 21, 2020

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv Jan 14, 2020 •

edited by RikkiGibson

Loading

AlekseyTs commented Jan 15, 2020 •

edited

Loading

RikkiGibson commented Jan 15, 2020 •

edited

Loading

AlekseyTs commented Jan 15, 2020 •

edited

Loading

jcouv Jan 17, 2020 •

edited by RikkiGibson

Loading

AlekseyTs Jan 17, 2020 •

edited by RikkiGibson

Loading

AlekseyTs Jan 17, 2020 •

edited

Loading

AlekseyTs Jan 17, 2020 •

edited

Loading

AlekseyTs commented Jan 17, 2020 •

edited

Loading