"Arbitrary async returns" should use attribute #13405

ljw1004 · 2016-08-26T19:54:54Z

This PR changes "arbitrary async return" to detect tasklikeness from the presence of an [AsyncBuilder] attribute, not from a CreateAsyncMethodBuilder method.

Spec for the change is in two posts:
#10902 (comment)
#10902 (comment)

Bring ljw1004 fork up to date

FI into ljw1004/roslyn

FI from main

FI

FI from main

…ibute, no longer off CreateAsyncMethodBuilder method.

…eanly.

…porting.

ljw1004 · 2016-08-26T20:00:31Z

I also filed one outstanding bug: #13406

(Also, I'm not that good at github... I don't know why all the "Merge..." commits are showing up, don't know if they're a problem, don't know how to get rid of them. Will investigate.)

ljw1004 · 2016-08-26T20:01:02Z

Code review please @cston @jaredpar @gafter

cston · 2016-08-26T22:15:32Z

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

+        /// no more, no less. Validation of builder type B is left for elsewhere. This method returns B
+        /// without validation of any kind.
+        /// </remarks>
+        internal static bool IsCustomTaskType(this NamedTypeSymbol type, out object builderArgument)


Should builderArgument be TypeSymbol rather than object?

I went back and forth on this. If you write [AsyncBuilder("hello")] then it is indeed a tasklike.

Now I said that I'm looking specifically for the AsyncBuilder(System.Type) overload, so if I find it then the above code will be an error. But I figured that in this case the builderArgument would still be a string literal.

So if anyone wants to report errors on what the argument was, then returning it just as object is more useful. If it were TypeSymbol then there'd be the odd case where the method returns true, but it returns a null builderArgument, so there's not much for the caller to go on to know how to report the problem.

If the [AsyncBuilder] argument is not a type, why is the type decorated with that attribute considered task-like?

Why not? :)

I figure it could have gone either way. There is a spectrum of possibilities...

[AsyncBuilder(System.Type)] identifies a tasklike, regardless of what argument is passed, regardless if it is an error (e.g. [AsyncBuilder(NonExistentType)] or [AsyncBuilder("3")]), regardless of anything.

[AsyncBuilder(System.Type)] identifies a tasklike only if the argument is a type

[AsyncBuilder(System.Type)] identifies a tasklike only if the argument is a type with the same generic arity and same accessibility

[AsyncBuilder(System.Type)] identifies a tasklike only if the argument is a type with the same generic arity, same accessibility, and a suitable Create method and a suitable Task property

[AsyncBuilder(System.Type)] identifies a tasklike only if the argument is a type that's 100% suitable as a builder.

I picked the first option because (A) the extremes seem more defensible than the middle grounds, (B) it felt a touch more efficient because places like overload resolution and type inference which might potentially be used a lot only do the minimal checking, (C) I think it's a better user experience while you're developing your custom tasklike if it remains tasklike even while you're in the process of editing+fixing the code of your builder.

We should allow [AsyncBuilder(expr)] if expr is a type expression, even an error type. Are there useful scenarios that are supported if we allow expr to be a string though?

And is returning object rather than TypeSymbol from this method more efficient? At line 1318, we've already determined the argument is a System.Type.

Returning an object and requiring callers to check the type of the result seems unnecessary.

I don't like using extension methods when the target type is defined in the same assembly and namespace. It's just harder to find all the places where applicable extension methods are defined. Especially since this method is not even defined in NamedTypeSymbolExtensions but in TypeSymbolExtensions. By adding extension methods like these the code base is becoming more and more complex to navigate for someone outside the compiler team, who doesn't spend 100% of their time in the compiler code base and doesn't know whether or not it "feels" like non-core functionality.

Not sure what you mean by public API -- NamedTypeSymbol doesn't have any public API since it's internal.
Even if it had there is no enforcement of the fact that it only uses public API of NamedTypeSymbol. It may now but perhaps we refactor something and it won't later.

If the method was implemented using the true public API, e.g. ITypeSymbol then it would be indeed another thing altogether.

I'm not sure where to go with this...

Maybe an extension method public bool ITypeSymbolExtensions.IsTasklike(this ITypeSymbol t), similar to IsAbstractClass, IsAwaitableNonDynamic, ... This concept "IsTasklike" would be used in overload resolution and method declarations and the like. It would be implemented by scanning for the attribute on the type. Meanwhile, the functionality to extract the attribute's argument would be solely used inside AssembleAsyncMethodCollection.cs.

Maybe leave as is, as an extension method in TypeSymbolExtensions

Maybe move to an extension method in NamedTypeSymbolExtensions

Maybe move to an instance method in NamedTypeSymbol.

I vote for an instance method on NamedTypeSymbol, unless we can/want to reuse the code for both C# and VB in which case it would make sense implementing it using ITypeSymbol.

We have very few methods that operate on ISymbol interfaces. It isn't necessary here.

In that case, "leave as is" :)

…tch accessibility of builder" rule.

cston · 2016-08-29T16:07:09Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodBuilderMemberCollection.cs

+                    builderType = builderArgument as NamedTypeSymbol;
+                    if (builderType?.IsErrorType() == true)
+                    {
+                    }


Rather than an empty if block, consider changing to if (builderType?.IsErrorType() != true) { ... }.

The way I'd done cascading conditions was "(1) if an error has been reported, (2) otherwise if it's an error but one hasn't been reported, (3) otherwise success". How about I add a comment to the first branch to indicate this?

Has the error necessarily been reported? What if the task-like type and builder type are from metadata and the builder type is missing? Do we need to report a use-site error here?

Definitely agree, the empty if block here seems off. It's definitely something I'd poke at if I came across this code in the future.

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

Fixed in eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R268

cston · 2016-08-29T17:09:42Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodBuilderMemberCollection.cs

+            NamedTypeSymbol builderType)
+        {
+            // The Create method's return type is expected to be buildderType.
+            // The WellKnownMembers routines aren't able to enforce that, which is this method exists.


which is why ...?

Fixed in eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R400

gafter · 2016-08-29T18:11:51Z

@ljw1004 The merge commits are created each time you update your branch to bring it up-to-date with the master branch. They can be eliminated during the merge by "squashing" the iterations. I can show you how to do that or do it for you.

…meters, and for single parameter of wrong type

… remove noise about "missing Create method" and "missing Task" method.

…t arity/accessibility). Added an error report when builder can't be imported (and added a test for this).

cston · 2016-08-31T15:19:48Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodBuilderMemberCollection.cs

                if (customBuilder)
                {
-                    taskProperty = GetCustomTaskProperty(F, builderType);
+                    builderType = ValidateBuilderType(F, builderArgument, returnType.DeclaredAccessibility, desiredArity:0);
+                    if (builderType != null)


By convention, symbol comparisons with null should cast to (object) to use reference equals: if ((object)builderType != null). Same comment below.

Fixed in 0e30e90#diff-995d1bf33206eef8c980dbd08b8601b0R148
0e30e90#diff-995d1bf33206eef8c980dbd08b8601b0R216

cston · 2016-08-31T21:48:29Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodBuilderMemberCollection.cs

-                    Debug.Assert((object)builderType != null);
-                    builderType = builderType.Construct(resultType);
-                }
+                NamedTypeSymbol builderType = null;


Minor point: initializer is not needed.

Removed in f2cdba8#diff-995d1bf33206eef8c980dbd08b8601b0L207

cston · 2016-08-31T23:28:37Z

Minor comments. Otherwise LGTM.

cston · 2016-08-31T23:49:37Z

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

+                Diagnostic(ErrorCode.ERR_MissingPredefinedMember, "=> await Task.FromResult(1)").WithArguments("N.BN", "Create").WithLocation(22, 22)
+                );
+
+        }


Are we testing task-like interfaces?

[AsyncBuilder(typeof(Builder))] interface ITask { } class Builder { public static ITask Create() { ... } }

Good thought! That's the key advantage of the attribute! I've added tests here: f2cdba8#diff-c55f1eb2556bfc07b602d0647778d699R3641

…Added more unit tests for bad builder, and for interface tasklike.

gafter · 2016-09-06T18:15:49Z

ljw1004 added 12 commits May 25, 2016 13:38

Merge pull request #1 from dotnet/master

b82a3f9

Bring ljw1004 fork up to date

Merge pull request #2 from dotnet/master

1c806ae

FI into ljw1004/roslyn

Merge pull request #3 from dotnet/master

abc0efd

FI from main

Merge pull request #5 from dotnet/master

6ecd515

FI

Merge pull request #6 from dotnet/master

424da4a

FI

Merge pull request #9 from dotnet/master

416cf98

FI from main

Merge pull request #10 from ljw1004/master

1d6580a

FI from main

Altered the unit test source code to be based off [AsyncBuilder] attr…

3d82d1a

…ibute, no longer off CreateAsyncMethodBuilder method.

First stab at making "async returns" be attribute-based

8b8d09d

Fixed the unit-tests for AsyncBuilder attribute. They all now pass cl…

d7076e8

…eanly.

Updated the generic rules for [AsyncBuilder()] argument, and error-re…

62a472f

…porting.

Added tests for the Create method

01eafc2

dnfclas added the cla-already-signed label Aug 26, 2016

Change to enforce same accessibility between tasklike and builder

0ddec05

cston reviewed Aug 26, 2016
View reviewed changes

Update remaining tests for the new "accessibility of tasklike must ma…

bcbd12f

…tch accessibility of builder" rule.

gafter added the Area-Compilers label Aug 26, 2016

gafter self-assigned this Aug 26, 2016

cston reviewed Aug 29, 2016
View reviewed changes

ljw1004 added 3 commits August 29, 2016 14:26

Added tests for AsyncBuilder attribute constructor with multiple para…

33e65fb

…meters, and for single parameter of wrong type

[AsyncBuilder()] tests now mostly have a valid builder type, so as to…

c9633b0

… remove noise about "missing Create method" and "missing Task" method.

Factored out "ValidateAsyncBuilder" tests (that it's a type of correc…

eef0c70

…t arity/accessibility). Added an error report when builder can't be imported (and added a test for this).

ljw1004 mentioned this pull request Aug 31, 2016

Discussion thread for arbitrary async returns #10902

Closed

cston reviewed Aug 31, 2016
View reviewed changes

Followed convention of "(object)t != null" for types

0e30e90

cston reviewed Aug 31, 2016
View reviewed changes

Minor tweaks. Made ValidateBuilderType method a bit simpler+cleaner. …

f2cdba8

…Added more unit tests for bad builder, and for interface tasklike.

gafter merged commit ae20f8b into dotnet:master Sep 6, 2016

ljw1004 mentioned this pull request Sep 15, 2016

Recognize task-like types by the presence of an AsyncBuilderAttribute #13370

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"Arbitrary async returns" should use attribute #13405

"Arbitrary async returns" should use attribute #13405

ljw1004 commented Aug 26, 2016

ljw1004 commented Aug 26, 2016

ljw1004 commented Aug 26, 2016

cston Aug 26, 2016

ljw1004 Aug 26, 2016

cston Aug 26, 2016

ljw1004 Aug 29, 2016

cston Aug 29, 2016

tmat Aug 29, 2016 •

edited

Loading

ljw1004 Aug 29, 2016

tmat Aug 29, 2016 •

edited

Loading

cston Aug 29, 2016

ljw1004 Aug 31, 2016

cston Aug 29, 2016

ljw1004 Aug 29, 2016

cston Aug 29, 2016

jaredpar Aug 29, 2016

ljw1004 Aug 31, 2016

cston Aug 29, 2016

ljw1004 Aug 31, 2016

gafter commented Aug 29, 2016

cston Aug 31, 2016

ljw1004 Aug 31, 2016

cston Aug 31, 2016

ljw1004 Sep 1, 2016

cston commented Aug 31, 2016

cston Aug 31, 2016

ljw1004 Sep 1, 2016

gafter commented Sep 6, 2016

"Arbitrary async returns" should use attribute #13405

"Arbitrary async returns" should use attribute #13405

Conversation

ljw1004 commented Aug 26, 2016

ljw1004 commented Aug 26, 2016

ljw1004 commented Aug 26, 2016

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

tmat Aug 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Aug 29, 2016 • 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

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

gafter commented Aug 29, 2016

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 Aug 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Sep 6, 2016

tmat Aug 29, 2016 •

edited

Loading

tmat Aug 29, 2016 •

edited

Loading