Allow overriding the AsyncMethodBuilder on methods by jcouv · Pull Request #54033 · dotnet/roslyn

jcouv · 2021-06-11T16:41:11Z

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/async-method-builders.md
Test plan: #51999

Current design choices:

we don't need the return type of async methods with AsyncMethodBuilder to be proper task-like types
we do not care about the accessibility of the builder override
but all members of the builder overrides (factory type and final builder) must have public accessibility
there is no builder factory vs. builder indirection after all
the AsyncMethodBuilder is only allowed on lambdas when their return type is explicit

cston · 2021-06-16T06:57:34Z

+        /// Returns true if the method has a [AsyncMethodBuilder(typeof(B))] attribute. If so it returns the "B".
+        /// Validation of builder type B is left for elsewhere. This method returns B without validation of any kind.
+        /// </summary>
+        internal static bool HasMethodLevelBuilder(this MethodSymbol method, [NotNullWhen(true)] out object? builderArgument)


HasMethodLevelBuilder(this MethodSymbol method

Consider changing this to an extension method on Symbol and call this method from TypeSymbolExtensions.IsCustomTaskType() as well. #Resolved

I'm looking at one usage of IsCustomTaskType in BoundLambda.CalculateReturnType which I need to think more about.

I didn't understand the comment about CalculateReturnType. Could we share this implementation with TypeSymbolExtensions.IsCustomTaskType()?

The comment about CalculateReturnType is no longer relevant now that we're checking the builder from the return type.
Factored logic with IsCustomTaskType.

cston · 2021-06-16T07:07:09Z

        }

+#nullable enable
+        public static bool IsAsyncReturningTaskViaOverride(this MethodSymbol method, [NotNullWhen(true)] out object? builderArgument)


IsAsyncReturningTaskViaOverride

Are IsAsyncReturningGenericTask() and IsAsyncReturningTaskViaOverride() ever called separately? If not, consider combining. Same question for generic task pair. #Closed

cston · 2021-06-16T07:07:15Z

+            var symbol = this.ContainingMemberOrLambda;
+            return symbol?.Kind == SymbolKind.Method && ((MethodSymbol)symbol).IsAsyncReturningTaskViaOverride(out _);
+        }
+


Are these two methods ever called separately? If not, consider combining the methods. Same question for pair of methods below. #Closed

cston · 2021-06-16T07:23:16Z

    public void BuilderFactoryOnMethod_CreateHasParamaeter()

Typo.

In reply to: 862119350

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMethodBuilderOverrideTests.cs:991 in e1f1d41. [](commit_id = e1f1d41, deletion_comment = False)

jcouv · 2021-06-17T20:45:37Z

            if (reachableEndpoint)
            {
-                if (DelegateNeedsReturn(invokeMethod))
+                if (Binder.MethodOrLambdaRequiresValue(lambdaSymbol, this.Binder.Compilation))


📝 Changes to this method are covered by BuilderFactoryOnMethod_OnLambda_NotTaskLikeTypes. With this change the test doesn't report ERR_AnonymousReturnExpected nor ERR_CantConvAsyncAnonFuncReturns below. #Resolved

cston · 2021-06-18T14:32:29Z

+                           ignoredBuilderType, customBuilder: true, out MethodSymbol _) &&
+                        TryGetBuilderMember(F,
+                            isGeneric ? WellKnownMember.System_Runtime_CompilerServices_AsyncTaskMethodBuilder_T__SetStateMachine : WellKnownMember.System_Runtime_CompilerServices_AsyncTaskMethodBuilder__SetStateMachine,
+                            ignoredBuilderType, customBuilder: true, out MethodSymbol _);


Could we use the existing TryCreate() method instead? #Closed

It doesn't make much difference. I did initially ;-) I'll change it back.

cston · 2021-06-19T00:08:54Z


+                if (this.HasMethodLevelBuilder(out _))
+                {
+                    hasErrors = MessageID.IDS_AsyncMethodBuilderOverride.CheckFeatureAvailability(diagnostics, this.DeclaringCompilation, errorLocation);


hasErrors

It looks like this may reset hasErrors that was set above. #Closed

jcouv · 2021-06-21T16:56:10Z

@dotnet/roslyn-compiler for a second review. Let me know if you need a walkthrough. Thanks

This reverts commit 2a3fdf1.

jcouv · 2021-06-22T03:26:11Z

Merged with explicit return types. We'll have to merge #54282 before continuing the review.

In reply to: 865499273

cston · 2021-06-22T04:25:58Z

[AsyncMethodBuilder(typeof(MyTaskMethodBuilder))]

My mistake - the attribute does not affect callers.

In reply to: 865452212

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMethodBuilderOverrideTests.cs:95 in ee207cb. [](commit_id = ee207cb, deletion_comment = False)

jcouv · 2021-06-22T19:24:03Z

@dotnet/roslyn-compiler for review. Thanks

cston · 2021-06-22T19:32:38Z

            // (7,71): error CS8934: The AsyncMethodBuilder attribute is disallowed on anonymous methods without an explicit return type.

CS8935 here and in other locations.

In reply to: 866274208

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMethodBuilderOverrideTests.cs:889 in 3268386. [](commit_id = 3268386, deletion_comment = False)

333fred · 2021-06-23T18:58:58Z

                    method.IsStatic &&
                    method.ParameterCount == 0 &&
                    !method.IsGenericMethod &&
+                    method.RefKind == RefKind.None &&


Is this actually just a bug fix? #Resolved

We need this change for proper behavior of method-level builders (see test BuilderOnMethod_CreateHasRefReturn).
But yes it was a misbehavior of task-like types (see test AsyncMethod_CreateHasRefReturn).

333fred · 2021-06-23T19:00:54Z

        }
+
+        /// <summary>
+        /// Returns true if the method has a [AsyncMethodBuilder(typeof(B))] attribute. If so it returns the "B".


the

Nit: remove the #Closed

or add type

333fred · 2021-06-23T19:04:12Z

+                    && attr.CommonConstructorArguments.Length == 1
+                    && attr.CommonConstructorArguments[0].Kind == TypedConstantKind.Type)
+                {
+                    builderArgument = attr.CommonConstructorArguments[0].ValueInternal!;


ValueInternal

If we know this is a type, why not have the builderArgument be a TypeSymbol? #Closed

Kept in line with existing IsCustomTaskType method.

333fred · 2021-06-23T19:04:43Z

    internal static bool IsCustomTaskType(this NamedTypeSymbol type, [NotNullWhen(true)] out object? builderArgument)

Same question here, I guess.

In reply to: 867086783

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs:1643 in 3268386. [](commit_id = 3268386, deletion_comment = False)

333fred · 2021-06-23T19:05:44Z

+        /// Returns true if the method has a [AsyncMethodBuilder(typeof(B))] attribute. If so it returns the "B".
+        /// Validation of builder type B is left for elsewhere. This method returns B without validation of any kind.
+        /// </summary>
+        internal static bool HasAsyncMethodBuilder(this Symbol symbol, [NotNullWhen(true)] out object? builderArgument)


HasAsyncMethodBuilder

Nit: feels like a better name for this is HasAsyncMethodBuilderAttribute, since it specifically won't return the builder type if the method has a Task-like type with no attribute. #Closed

333fred · 2021-06-23T20:25:28Z

            // (75,19): error CS1983: The return type of an async method must be void, Task, Task<T>, a task-like type, IAsyncEnumerable<T>, or IAsyncEnumerator<T>

These errors have nothing to do with accessibility? Also, several of these have no issues?

In reply to: 867133974

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMethodBuilderOverrideTests.cs:662 in 3268386. [](commit_id = 3268386, deletion_comment = False)

333fred · 2021-06-23T20:35:17Z

[AsyncMethodBuilder(typeof(MyTaskMethodBuilder))] static async MyTask () => {{ System.Console.Write(""Lambda1 ""); await Task.Delay(0); }} // 1

I would have expected this to error?

In reply to: 867139447

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMethodBuilderOverrideTests.cs:918 in 3268386. [](commit_id = 3268386, deletion_comment = False)

333fred · 2021-06-23T20:37:53Z

Done review pass (commit 25)

jcouv · 2021-06-23T21:35:05Z

[AsyncMethodBuilder(typeof(MyTaskMethodBuilder))] static async MyTask () => {{ System.Console.Write(""Lambda1 ""); await Task.Delay(0); }} // 1

Resolved offline

In reply to: 867139447

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMethodBuilderOverrideTests.cs:918 in 3268386. [](commit_id = 3268386, deletion_comment = False)

jcouv · 2021-06-23T21:43:28Z

    public void BuilderOnMethod_WrongAccessibility()

Will fix this test to have builder on method and adjust name.

In reply to: 867181628

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMethodBuilderOverrideTests.cs:632 in 9440d0f. [](commit_id = 9440d0f, deletion_comment = False)

jcouv added the Area-Compilers label Jun 11, 2021

jcouv added this to the C# 10 milestone Jun 11, 2021

jcouv self-assigned this Jun 11, 2021

Allow AsyncMethodBuilder on method

78a89b7

jcouv force-pushed the async-builder branch from fb5c6bb to 78a89b7 Compare June 11, 2021 17:54

Julien Couvreur added 2 commits June 15, 2021 11:58

Allow returning a non-task-like type

f084cb2

Relax accessibility requirements for overrides

e1f1d41

jcouv marked this pull request as ready for review June 15, 2021 19:48

jcouv requested a review from a team as a code owner June 15, 2021 19:48

cston reviewed Jun 16, 2021

View reviewed changes

Julien Couvreur added 4 commits June 17, 2021 11:41

Factor extension methods

a39ee15

Fix nullability scenario

e4ab3c3

Fix lambda scenario with non-task-like return type

72cbc01

Fix test

3e9518b

jcouv commented Jun 17, 2021

View reviewed changes

Julien Couvreur added 2 commits June 17, 2021 15:40

Disallow non-task-like return type after all

2a3fdf1

Validate ignored builder type

869671b

cston reviewed Jun 18, 2021

View reviewed changes

Julien Couvreur added 2 commits June 18, 2021 09:34

use TryCreate

22b5373

Merge remote-tracking branch 'dotnet/main' into async-builder

8ac378a

cston reviewed Jun 19, 2021

View reviewed changes

Julien Couvreur added 3 commits June 20, 2021 22:09

hasErros

2e7ad79

Factor extension method

4375aea

using

43416a2

jcouv marked this pull request as draft June 21, 2021 18:52

Revert "Disallow non-task-like return type after all"

d5c0a27

This reverts commit 2a3fdf1.

jcouv requested a review from a team as a code owner June 22, 2021 03:25

Julien Couvreur added 2 commits June 22, 2021 11:55

Merge remote-tracking branch 'dotnet/main' into async-builder

457aac7

Merge remote-tracking branch 'dotnet/main' into async-builder

3268386

cston approved these changes Jun 22, 2021

View reviewed changes

Adjust error code in tests

9440d0f

333fred reviewed Jun 23, 2021

View reviewed changes

Julien Couvreur added 2 commits June 23, 2021 15:15

Address feedback

a576b8c

typo

36b5a5d

jcouv requested a review from 333fred June 23, 2021 22:23

333fred approved these changes Jun 24, 2021

View reviewed changes

jcouv merged commit b796152 into dotnet:main Jun 24, 2021

ghost modified the milestones: C# 10, Next Jun 24, 2021

jcouv deleted the async-builder branch June 24, 2021 06:57

RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021

sharwell mentioned this pull request Mar 30, 2022

Odd behaviour with async method declaration: wrong types suggestion #60341

Closed

Conversation

jcouv commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Jun 16, 2021 • 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 Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

cston Jun 19, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

cston Jun 16, 2021 • 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 Jun 16, 2021 • 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 commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jun 17, 2021 • 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 Jun 18, 2021 • 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 Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

cston Jun 19, 2021 • 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 Jun 21, 2021

Uh oh!

jcouv commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jun 22, 2021

Uh oh!

jcouv commented Jun 22, 2021

Uh oh!

cston commented Jun 22, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred Jun 23, 2021 • 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 Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

333fred Jun 23, 2021 • 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!

333fred Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

333fred Jun 23, 2021 • 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 Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

333fred commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

jcouv commented Jun 11, 2021 •

edited

Loading

cston Jun 16, 2021 •

edited by jcouv

Loading

cston Jun 16, 2021 •

edited

Loading

cston Jun 16, 2021 •

edited

Loading

cston commented Jun 16, 2021 •

edited

Loading

jcouv Jun 17, 2021 •

edited

Loading

cston Jun 18, 2021 •

edited

Loading

cston Jun 19, 2021 •

edited

Loading

jcouv commented Jun 22, 2021 •

edited

Loading

cston commented Jun 22, 2021 •

edited by jcouv

Loading

333fred Jun 23, 2021 •

edited by jcouv

Loading

333fred Jun 23, 2021 •

edited

Loading

333fred Jun 23, 2021 •

edited

Loading

333fred commented Jun 23, 2021 •

edited

Loading

333fred Jun 23, 2021 •

edited

Loading

333fred commented Jun 23, 2021 •

edited

Loading

333fred commented Jun 23, 2021 •

edited

Loading

jcouv commented Jun 23, 2021 •

edited by 333fred

Loading