Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

"Arbitrary async returns" should use attribute #13405

Merged
merged 19 commits into from
Sep 6, 2016
Merged

"Arbitrary async returns" should use attribute #13405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b82a3f9
Merge pull request #1 from dotnet/master
ljw1004 May 25, 2016
1c806ae
Merge pull request #2 from dotnet/master
ljw1004 Jun 1, 2016
abc0efd
Merge pull request #3 from dotnet/master
ljw1004 Jun 17, 2016
6ecd515
Merge pull request #5 from dotnet/master
ljw1004 Jun 20, 2016
424da4a
Merge pull request #6 from dotnet/master
ljw1004 Jul 1, 2016
416cf98
Merge pull request #9 from dotnet/master
ljw1004 Aug 25, 2016
1d6580a
Merge pull request #10 from ljw1004/master
ljw1004 Aug 25, 2016
3d82d1a
Altered the unit test source code to be based off [AsyncBuilder] attr…
ljw1004 Aug 25, 2016
8b8d09d
First stab at making "async returns" be attribute-based
ljw1004 Aug 25, 2016
d7076e8
Fixed the unit-tests for AsyncBuilder attribute. They all now pass cl…
ljw1004 Aug 26, 2016
62a472f
Updated the generic rules for [AsyncBuilder()] argument, and error-re…
ljw1004 Aug 26, 2016
01eafc2
Added tests for the Create method
ljw1004 Aug 26, 2016
0ddec05
Change to enforce same accessibility between tasklike and builder
ljw1004 Aug 26, 2016
bcbd12f
Update remaining tests for the new "accessibility of tasklike must ma…
ljw1004 Aug 26, 2016
33e65fb
Added tests for AsyncBuilder attribute constructor with multiple para…
ljw1004 Aug 29, 2016
c9633b0
[AsyncBuilder()] tests now mostly have a valid builder type, so as to…
ljw1004 Aug 30, 2016
eef0c70
Factored out "ValidateAsyncBuilder" tests (that it's a type of correc…
ljw1004 Aug 30, 2016
0e30e90
Followed convention of "(object)t != null" for types
ljw1004 Aug 31, 2016
f2cdba8
Minor tweaks. Made ValidateBuilderType method a bit simpler+cleaner. …
ljw1004 Sep 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,8 @@ private static TypeSymbol InferReturnType(
var delegateReturnType = delegateType?.GetDelegateType()?.DelegateInvokeMethod?.ReturnType as NamedTypeSymbol;
if ((object)delegateReturnType != null)
{
NamedTypeSymbol builderType;
MethodSymbol createBuilderMethod;
if (delegateReturnType.IsCustomTaskType(out builderType, out createBuilderMethod))
object builderType;
if (delegateReturnType.IsCustomTaskType(out builderType))
{
taskType = delegateReturnType;
}
Expand Down
96 changes: 80 additions & 16 deletions src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodBuilderMemberCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,19 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
{
var returnType = (NamedTypeSymbol)method.ReturnType;
NamedTypeSymbol builderType;
MethodSymbol createBuilderMethod;
PropertySymbol taskProperty;
bool customBuilder = returnType.IsCustomTaskType(out builderType, out createBuilderMethod);
MethodSymbol createBuilderMethod = null;
PropertySymbol taskProperty = null;

object builderArgument;
bool customBuilder = returnType.IsCustomTaskType(out builderArgument);
if (customBuilder)
{
taskProperty = GetCustomTaskProperty(F, builderType);
builderType = ValidateBuilderType(F, builderArgument, returnType.DeclaredAccessibility, isGeneric:false);
if ((object)builderType != null)
{
taskProperty = GetCustomTaskProperty(F, builderType);
createBuilderMethod = GetCustomCreateMethod(F, builderType);
}
}
else
{
Expand All @@ -161,7 +168,8 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
customBuilder,
out taskProperty);
}
if ((object)createBuilderMethod == null ||
if ((object)builderType == null ||
(object)createBuilderMethod == null ||
(object)taskProperty == null)
{
collection = default(AsyncMethodBuilderMemberCollection);
Expand Down Expand Up @@ -197,21 +205,26 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
}
returnType = returnType.ConstructedFrom.Construct(resultType);
NamedTypeSymbol builderType;
MethodSymbol createBuilderMethod;
PropertySymbol taskProperty;
bool customBuilder = returnType.IsCustomTaskType(out builderType, out createBuilderMethod);
if (!customBuilder)
{
builderType = F.WellKnownType(WellKnownType.System_Runtime_CompilerServices_AsyncTaskMethodBuilder_T);
Debug.Assert((object)builderType != null);
builderType = builderType.Construct(resultType);
}
MethodSymbol createBuilderMethod = null;
PropertySymbol taskProperty = null;

object builderArgument;
bool customBuilder = returnType.IsCustomTaskType(out builderArgument);
if (customBuilder)
{
taskProperty = GetCustomTaskProperty(F, builderType);
builderType = ValidateBuilderType(F, builderArgument, returnType.DeclaredAccessibility, isGeneric:true);
if ((object)builderType != null)
{
builderType = builderType.ConstructedFrom.Construct(resultType);
taskProperty = GetCustomTaskProperty(F, builderType);
createBuilderMethod = GetCustomCreateMethod(F, builderType);
}
}
else
{
builderType = F.WellKnownType(WellKnownType.System_Runtime_CompilerServices_AsyncTaskMethodBuilder_T);
Debug.Assert((object)builderType != null);
builderType = builderType.Construct(resultType);
TryGetBuilderMember<MethodSymbol>(
F,
WellKnownMember.System_Runtime_CompilerServices_AsyncTaskMethodBuilder_T__Create,
Expand All @@ -225,7 +238,8 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
customBuilder,
out taskProperty);
}
if ((object)taskProperty == null ||
if ((object)builderType == null ||
(object)taskProperty == null ||
(object)createBuilderMethod == null)
{
collection = default(AsyncMethodBuilderMemberCollection);
Expand All @@ -250,6 +264,28 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
throw ExceptionUtilities.UnexpectedValue(method);
}

private static NamedTypeSymbol ValidateBuilderType(SyntheticBoundNodeFactory F, object builderAttributeArgument, Accessibility desiredAccessibility, bool isGeneric)
{
var builderType = builderAttributeArgument as NamedTypeSymbol;

if ((object)builderType != null &&
!builderType.IsErrorType() &&
builderType.SpecialType != SpecialType.System_Void &&
builderType.DeclaredAccessibility == desiredAccessibility)
{
bool isArityOk = isGeneric
? builderType.IsUnboundGenericType && builderType.ContainingType?.IsGenericType != true
: !builderType.IsGenericType;
if (isArityOk)
{
return builderType;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing desiredArity to bool and remove this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps inline isArityOk checks in the if. It might be slightly harder to read but would require fewer checks of builderType != null.

if ((object)builderType == null ||
    builderType.IsErrorType() == true ||
    ... ||
    (expectingGenericType ?
        (!builderType.IsUnboundGenericType || builderType.ContainingType?.IsGenericType == true) :
        builderType.IsGenericType))
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f2cdba8#diff-995d1bf33206eef8c980dbd08b8601b0R267 - I changed to bool, and streamlined the number of null checks.


F.Diagnostics.Add(ErrorCode.ERR_BadAsyncReturn, F.Syntax.Location);
return null;
}

private static bool TryCreate(
SyntheticBoundNodeFactory F,
bool customBuilder,
Expand Down Expand Up @@ -341,6 +377,34 @@ private static bool TryGetBuilderMember<TSymbol>(
return true;
}

private static MethodSymbol GetCustomCreateMethod(
SyntheticBoundNodeFactory F,
NamedTypeSymbol builderType)
{
// The Create method's return type is expected to be builderType.
// The WellKnownMembers routines aren't able to enforce that, which is why this method exists.
const string methodName = "Create";
var members = builderType.GetMembers(methodName);
foreach (var member in members)
{
if (member.Kind != SymbolKind.Method)
{
continue;
}
var method = (MethodSymbol)member;
if ((method.DeclaredAccessibility == Accessibility.Public) &&
method.IsStatic &&
method.ParameterCount == 0 &&
!method.IsGenericMethod &&
method.ReturnType == builderType)
{
return method;
}
}
F.Diagnostics.Add(ErrorCode.ERR_MissingPredefinedMember, F.Syntax.Location, builderType, methodName);
return null;
}

private static PropertySymbol GetCustomTaskProperty(
SyntheticBoundNodeFactory F,
NamedTypeSymbol builderType)
Expand Down
55 changes: 22 additions & 33 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,9 +1273,8 @@ internal static bool IsNonGenericTaskType(this TypeSymbol type, CSharpCompilatio
{
return false;
}
NamedTypeSymbol builderType;
MethodSymbol createBuilderMethod;
return namedType.IsCustomTaskType(out builderType, out createBuilderMethod);
object builderArgument;
return namedType.IsCustomTaskType(out builderArgument);
}

internal static bool IsGenericTaskType(this TypeSymbol type, CSharpCompilation compilation)
Expand All @@ -1289,51 +1288,42 @@ internal static bool IsGenericTaskType(this TypeSymbol type, CSharpCompilation c
{
return true;
}
NamedTypeSymbol builderType;
MethodSymbol createBuilderMethod;
return namedType.IsCustomTaskType(out builderType, out createBuilderMethod);
object builderArgument;
return namedType.IsCustomTaskType(out builderArgument);
}

/// <summary>
/// Returns true if the type is generic or non-generic task-like type. If so, the async
/// method builder type is returned along with the method to construct that type.
/// Returns true if the type is generic or non-generic custom task-like type due to the
/// [AsyncBuilder(typeof(B))] attribute. It returns the "B".
/// </summary>
internal static bool IsCustomTaskType(this NamedTypeSymbol type, out NamedTypeSymbol builderType, out MethodSymbol createBuilderMethod)
/// <remarks>
/// For the Task types themselves, this method might return true or false depending on mscorlib.
/// The definition of "custom task-like type" is one that has an [AsyncBuilder(typeof(B))] attribute,
/// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should builderArgument be TypeSymbol rather than object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? :)

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

  1. [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.
  2. [AsyncBuilder(System.Type)] identifies a tasklike only if the argument is a type
  3. [AsyncBuilder(System.Type)] identifies a tasklike only if the argument is a type with the same generic arity and same accessibility
  4. [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
  5. [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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tmat tmat Aug 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tmat tmat Aug 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, "leave as is" :)

{
Debug.Assert((object)type != null);
Debug.Assert(type.SpecialType != SpecialType.System_Void);

var arity = type.Arity;
if (arity < 2)
{
// Find the public static CreateAsyncMethodBuilder method.
var members = type.GetMembers(WellKnownMemberNames.CreateAsyncMethodBuilder);
foreach (var member in members)
// Find the AsyncBuilder attribute.
foreach (var attr in type.GetAttributes())
{
if (member.Kind != SymbolKind.Method)
{
continue;
}
var method = (MethodSymbol)member;
if ((method.DeclaredAccessibility == Accessibility.Public) &&
method.IsStatic &&
(method.ParameterCount == 0) &&
!method.IsGenericMethod)
if (attr.IsTargetAttribute(type, AttributeDescription.AsyncBuilderAttribute)
&& attr.CommonConstructorArguments.Length == 1
&& attr.CommonConstructorArguments[0].Kind == TypedConstantKind.Type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we testing constructors with multiple arguments or single argument but not System.Type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added unit tests in 33e65fb

{
var returnType = method.ReturnType as NamedTypeSymbol;
if ((object)returnType == null || returnType.Arity != arity)
{
break;
}
builderType = returnType;
createBuilderMethod = method;
builderArgument = attr.CommonConstructorArguments[0].Value;
return true;
}
}
}

builderType = null;
createBuilderMethod = null;
builderArgument = null;
return false;
}

Expand Down Expand Up @@ -1412,9 +1402,8 @@ private static bool NormalizeTaskTypesInNamedType(CSharpCompilation compilation,
typeArgumentsBuilder.Free();
}

NamedTypeSymbol builderType;
MethodSymbol createBuilderMethod;
if (type.OriginalDefinition.IsCustomTaskType(out builderType, out createBuilderMethod))
object builderArgument;
if (type.OriginalDefinition.IsCustomTaskType(out builderArgument))
{
int arity = type.Arity;
Debug.Assert(arity < 2);
Expand Down
Loading