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 13 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
109 changes: 100 additions & 9 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 @@ -133,16 +133,40 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
collection: out collection);
}

// TODO: in both nongeneric and generic forks below, we should unify both "custom tasklike" and "Task/Task<T>"
// down the same custom-tasklike path. (Should mscorlib ever device to put [AsyncBuilder] on Task then that
// will happen anyway).
Copy link
Member

Choose a reason for hiding this comment

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

TODOs should be removed or replaced by links to bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0L136


if (method.IsTaskReturningAsync(F.Compilation))
{
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 = builderArgument as NamedTypeSymbol;
if (builderType?.IsErrorType() == true)
{
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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)

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 eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R268

else if ((object)builderType == null ||
builderType.SpecialType == SpecialType.System_Void ||
builderType.DeclaredAccessibility != returnType.DeclaredAccessibility ||
builderType.IsGenericType)
{
// might be null if type isn't a named type, e.g. [AsyncBuilder(typeof(object[]))]
var diagnostic = new CSDiagnostic(
new CSDiagnosticInfo(ErrorCode.ERR_BadAsyncReturn), F.Syntax.Location);
F.Diagnostics.Add(diagnostic);
Copy link
Member

Choose a reason for hiding this comment

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

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

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 eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R295

}
else
{
taskProperty = GetCustomTaskProperty(F, builderType);
createBuilderMethod = GetCustomCreateMethod(F, builderType);
}
}
else
{
Expand Down Expand Up @@ -196,10 +220,12 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
resultType = typeMap.SubstituteType(resultType).Type;
}
returnType = returnType.ConstructedFrom.Construct(resultType);
NamedTypeSymbol builderType;
MethodSymbol createBuilderMethod;
PropertySymbol taskProperty;
bool customBuilder = returnType.IsCustomTaskType(out builderType, out createBuilderMethod);
NamedTypeSymbol builderType = null;
Copy link
Member

Choose a reason for hiding this comment

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

Minor point: initializer is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in f2cdba8#diff-995d1bf33206eef8c980dbd08b8601b0L207

MethodSymbol createBuilderMethod = null;
PropertySymbol taskProperty = null;

object builderArgument;
bool customBuilder = returnType.IsCustomTaskType(out builderArgument);
if (!customBuilder)
{
builderType = F.WellKnownType(WellKnownType.System_Runtime_CompilerServices_AsyncTaskMethodBuilder_T);
Expand All @@ -208,7 +234,38 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
}
if (customBuilder)
{
taskProperty = GetCustomTaskProperty(F, builderType);
builderType = builderArgument as NamedTypeSymbol;

// builderType must be an open generic of arity 1, and its containing types must be nongeneric
bool isBuilderGenericityOk = ((object)builderType != null &&
!builderType.IsErrorType() &&
builderType.Arity == 1 &&
builderType.TypeArguments[0] is UnboundArgumentErrorTypeSymbol);
Copy link
Member

Choose a reason for hiding this comment

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

builderType.IsUnboundGenericType

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 eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R279


for (var containingType = builderType.ContainingType;
containingType != null && isBuilderGenericityOk;
containingType = containingType.ContainingType)
{
isBuilderGenericityOk &= (containingType.Arity == 0);
}
Copy link
Member

Choose a reason for hiding this comment

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

Loop can be replaced by builder.ContainingType?.IsGenericType != true.

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 eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R279


if (builderType?.IsErrorType() == true)
{
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to revist this. The empty block stands out and looks like a badge merge.

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 eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R220

else if ((object)builderType == null ||
builderType.SpecialType == SpecialType.System_Void ||
builderType.DeclaredAccessibility != returnType.DeclaredAccessibility ||
!isBuilderGenericityOk)
{
Copy link
Member

Choose a reason for hiding this comment

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

Please extract a helper method that does most of these checks, for re-use in generic and non-generic cases.

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 eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R268

F.Diagnostics.Add(new CSDiagnostic(
new CSDiagnosticInfo(ErrorCode.ERR_BadAsyncReturn), F.Syntax.Location));
}
else
{
builderType = builderType.ConstructedFrom.Construct(resultType);
taskProperty = GetCustomTaskProperty(F, builderType);
createBuilderMethod = GetCustomCreateMethod(F, builderType);
}
}
else
{
Expand Down Expand Up @@ -341,6 +398,40 @@ 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 buildderType.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: builderType

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 eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0L405

// The WellKnownMembers routines aren't able to enforce that, which is this method exists.
Copy link
Member

Choose a reason for hiding this comment

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

which is why ...?

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 eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R400

//
// TODO: consider removing WellKnownMember.System_Runtime_CompilerServices_AsyncTaskMethodBuilder*__Create
Copy link
Member

Choose a reason for hiding this comment

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

// TODO: consider [](start = 10, length = 19)

In the master branch TODOs should all have associated bug numbers or be removed from source tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0L136

// and using GetCustomCreateMethod for the Task types as well as for custom tasklikes.
Copy link
Member

Choose a reason for hiding this comment

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

Remove TODO or replace with reference to issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in f2cdba8#diff-995d1bf33206eef8c980dbd08b8601b0L401

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;
}
}
var diagnostic = new CSDiagnostic(
new CSDiagnosticInfo(ErrorCode.ERR_MissingPredefinedMember, builderType, methodName),
F.Syntax.Location);
F.Diagnostics.Add(diagnostic);
Copy link
Member

Choose a reason for hiding this comment

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

F.Diagnostics.Add(ErrorCode.ERR_MissingPredefinedMember, F.Syntax.Location, builderType, methodName);

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 eef0c70#diff-995d1bf33206eef8c980dbd08b8601b0R422

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