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.

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

Allow async methods to return Task-like types #12518

Merged
merged 1 commit into from Jul 19, 2016

Conversation

Projects
None yet
6 participants
@cston
Contributor

cston commented Jul 14, 2016

Allow async methods to return types other than void, Task, or Task<T>. Specifically allow Task-like types (and AsyncMethodBuilder types) if the types contain the expected set of members.

[Based on Lucian's prototype from features/async-return.]

Ported PR from master: #12434. I will follow up on existing feedback from that PR, making changes here.

@cston

This comment has been minimized.

Show comment
Hide comment
@cston

cston Jul 14, 2016

Contributor
Contributor

cston commented Jul 14, 2016

Show outdated Hide outdated src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs
case SymbolKind.ArrayType:
// PROTOTYPE(tasklike): Use VisitType or similar to cover all cases.
break;
{

This comment has been minimized.

@ljw1004

ljw1004 Jul 14, 2016

Contributor

I think arrays, tuples and generics are the only constructed types. We'll have to make sure it works with Tuples. Whose job is that? (other than that, LGTM)

@ljw1004

ljw1004 Jul 14, 2016

Contributor

I think arrays, tuples and generics are the only constructed types. We'll have to make sure it works with Tuples. Whose job is that? (other than that, LGTM)

@jcouv

This comment has been minimized.

Show comment
Hide comment
@jcouv

jcouv Jul 15, 2016

Member

Starting review. Sorry for the delay.

Member

jcouv commented Jul 15, 2016

Starting review. Sorry for the delay.

@@ -660,6 +660,7 @@ Microsoft.CodeAnalysis.Semantics.UnaryOperationKind.UnsignedPrefixIncrement = 77
abstract Microsoft.CodeAnalysis.Diagnostics.OperationBlockStartAnalysisContext.RegisterOperationAction(System.Action<Microsoft.CodeAnalysis.Diagnostics.OperationAnalysisContext> action, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.OperationKind> operationKinds) -> void
abstract Microsoft.CodeAnalysis.Diagnostics.OperationBlockStartAnalysisContext.RegisterOperationBlockEndAction(System.Action<Microsoft.CodeAnalysis.Diagnostics.OperationBlockAnalysisContext> action) -> void
abstract Microsoft.CodeAnalysis.SemanticModel.GetOperationCore(Microsoft.CodeAnalysis.SyntaxNode node, System.Threading.CancellationToken cancellationToken) -> Microsoft.CodeAnalysis.IOperation
const Microsoft.CodeAnalysis.WellKnownMemberNames.CreateAsyncMethodBuilder = "CreateAsyncMethodBuilder" -> string

This comment has been minimized.

@jcouv

jcouv Jul 15, 2016

Member

This is the only const I can find in shipped.txt and unshipped.txt. Do we really need it public?
Nevermind, I see similar public constants in src\Compilers\Core\Portable\PublicAPI.Shipped.txt ;-)

@jcouv

jcouv Jul 15, 2016

Member

This is the only const I can find in shipped.txt and unshipped.txt. Do we really need it public?
Nevermind, I see similar public constants in src\Compilers\Core\Portable\PublicAPI.Shipped.txt ;-)

internal static bool IsNonGenericTaskType(this TypeSymbol type, CSharpCompilation compilation)
{
var namedType = type as NamedTypeSymbol;

This comment has been minimized.

@jcouv

jcouv Jul 15, 2016

Member

I thought the preference was to do kind checks, instead of type checks. Any reason not to?

@jcouv

jcouv Jul 15, 2016

Member

I thought the preference was to do kind checks, instead of type checks. Any reason not to?

This comment has been minimized.

@cston

cston Jul 18, 2016

Contributor

I'd considered checking Kind but checking as instead avoids an additional check of type != null.

@cston

cston Jul 18, 2016

Contributor

I'd considered checking Kind but checking as instead avoids an additional check of type != null.

Show outdated Hide outdated src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
if ((object)delegateReturnType != null)
{
MethodSymbol createBuilderMethod;
var delegateTaskBuilderType = delegateReturnType.GetAsyncMethodBuilderType(out createBuilderMethod);

This comment has been minimized.

@jcouv

jcouv Jul 15, 2016

Member

Consider assigning directly to taskType.

@jcouv

jcouv Jul 15, 2016

Member

Consider assigning directly to taskType.

This comment has been minimized.

@cston

cston Jul 18, 2016

Contributor

delegateReturnType (rather than delegateTaskBuilderType) is assigned to taskType. I've removed the delegateTaskBuilderType local to avoid confusion.

@cston

cston Jul 18, 2016

Contributor

delegateReturnType (rather than delegateTaskBuilderType) is assigned to taskType. I've removed the delegateTaskBuilderType local to avoid confusion.

Show outdated Hide outdated ...Sharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
Debug.Assert(n1.OriginalDefinition == n2.OriginalDefinition);
// two types, or they are different Task-likes. We don't have a Compilation here to verify
// Task-like however.
//Debug.Assert(n1.OriginalDefinition == n2.OriginalDefinition);

This comment has been minimized.

@jcouv

jcouv Jul 15, 2016

Member

Maybe remove commented-out assert, or add PROTOTYPE marker?

@jcouv

jcouv Jul 15, 2016

Member

Maybe remove commented-out assert, or add PROTOTYPE marker?

This comment has been minimized.

@cston

cston Jul 18, 2016

Contributor

Removed commented out assert and revised the comment.

@cston

cston Jul 18, 2016

Contributor

Removed commented out assert and revised the comment.

Show outdated Hide outdated ...Sharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
if (t2MatchesExactly)
{
// both exactly match expression
return BetterResult.Neither;

This comment has been minimized.

@jcouv

jcouv Jul 15, 2016

Member

The Neither case is gone. That seems surprising. Would you have an example?

@jcouv

jcouv Jul 15, 2016

Member

The Neither case is gone. That seems surprising. Would you have an example?

This comment has been minimized.

@ljw1004

ljw1004 Jul 15, 2016

Contributor

Previously if they both matched exactly then it bypassed "better conversion target" and returned Neither directly.

This feature says that if they both match exactly, then we SHOULD do "better conversion target". An example is

void f(Func<ValueTask> t) {}
void f(Func<Task> t) {}
f(async () => {});

Both of these are exact matches. If we didn't make this change to overload resolution then it would report ambiguity "Neither" in this case.

But with the change to overload resolution it will look into Better Conversion Target. If ValueTask has an implicit conversion to Task then it will prefer ValueTask. And vice versa.

This feature will allow library authors to indicate (via implicit conversions) which of two overloads is the better one to pick.

@ljw1004

ljw1004 Jul 15, 2016

Contributor

Previously if they both matched exactly then it bypassed "better conversion target" and returned Neither directly.

This feature says that if they both match exactly, then we SHOULD do "better conversion target". An example is

void f(Func<ValueTask> t) {}
void f(Func<Task> t) {}
f(async () => {});

Both of these are exact matches. If we didn't make this change to overload resolution then it would report ambiguity "Neither" in this case.

But with the change to overload resolution it will look into Better Conversion Target. If ValueTask has an implicit conversion to Task then it will prefer ValueTask. And vice versa.

This feature will allow library authors to indicate (via implicit conversions) which of two overloads is the better one to pick.

This comment has been minimized.

@jcouv

jcouv Jul 15, 2016

Member

Thanks for the detailed explanation. Makes sense.

@jcouv

jcouv Jul 15, 2016

Member

Thanks for the detailed explanation. Makes sense.

Show outdated Hide outdated src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs
var overwriteDiags = ImmutableInterlocked.InterlockedCompareExchange(ref _diagnostics, newDiags, oldDiags);
if (overwriteDiags == oldDiags)
addTo.AddRange(_lazyParametersAndDiagnostics.Diagnostics);
if (_lazyTypeParameterConstraintsAndDiagnostics != null)

This comment has been minimized.

@jcouv

jcouv Jul 15, 2016

Member

_lazyTypeParameterConstraintsAndDiagnostics [](start = 20, length = 43)

I wasn't clear why we don't also force initialization on this lazy field (but we do for the other two). Could you clarify?

@jcouv

jcouv Jul 15, 2016

Member

_lazyTypeParameterConstraintsAndDiagnostics [](start = 20, length = 43)

I wasn't clear why we don't also force initialization on this lazy field (but we do for the other two). Could you clarify?

This comment has been minimized.

@cston

cston Jul 18, 2016

Contributor

_lazyTypeParameterConstraintsAndDiagnostics is only computed if there are constraints. Added a comment to clarify.

@cston

cston Jul 18, 2016

Contributor

_lazyTypeParameterConstraintsAndDiagnostics is only computed if there are constraints. Added a comment to clarify.

Show outdated Hide outdated src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs
@@ -239,7 +239,7 @@ internal class MemberSignatureComparer : IEqualityComparer<Symbol>
public static readonly MemberSignatureComparer LambdaReturnInferenceCacheComparer = new MemberSignatureComparer(
considerName: false, // valid invoke is always called "Invoke"
considerExplicitlyImplementedInterfaces: false,
considerReturnType: false, // do not care
considerReturnType: true, // to differentiate Task types

This comment has been minimized.

@jcouv

jcouv Jul 15, 2016

Member

This is a bit mysterious to me. How does task-like affect this cache?
Also, the comment above may need refreshing accordingly.

@jcouv

jcouv Jul 15, 2016

Member

This is a bit mysterious to me. How does task-like affect this cache?
Also, the comment above may need refreshing accordingly.

This comment has been minimized.

@ljw1004

ljw1004 Jul 16, 2016

Contributor

In C#6 when you infer the return type of a lambda async (x) => x+1 when matched against a delegate type e.g. Func<int,Task<T>>, it takes into account the parameter types of the delegate (in this case x:int), and infers the type of the body of the lambda (in this case int), and the presence of the async modifier means the inferred return type is Task<int>. It just sticks Task in there blindly for any async lambda.

The compiler caches the result of this computation. So, if we try to infer the return type of the lambda given any delegate type, so long as the new delegate type has the same parameter types as the old one, then C#6 can simply look it up in the cache. The cache was keyed solely off the parameter types.

In C#7, if you inferred the return type of async (x) => x+1 in the context of a target delegate Func<int,Task<T>> then it would infer the return type Task<int>. But if you inferred its return type in the context of a target delegate Func<int,ValueTask<T>> then it would infer the return type ValueTask<int>.

Thus, the new computation depends on the delegate's return type as well as its parameter types.

Therefore the cache must key off return type as well as parameter types.

@ljw1004

ljw1004 Jul 16, 2016

Contributor

In C#6 when you infer the return type of a lambda async (x) => x+1 when matched against a delegate type e.g. Func<int,Task<T>>, it takes into account the parameter types of the delegate (in this case x:int), and infers the type of the body of the lambda (in this case int), and the presence of the async modifier means the inferred return type is Task<int>. It just sticks Task in there blindly for any async lambda.

The compiler caches the result of this computation. So, if we try to infer the return type of the lambda given any delegate type, so long as the new delegate type has the same parameter types as the old one, then C#6 can simply look it up in the cache. The cache was keyed solely off the parameter types.

In C#7, if you inferred the return type of async (x) => x+1 in the context of a target delegate Func<int,Task<T>> then it would infer the return type Task<int>. But if you inferred its return type in the context of a target delegate Func<int,ValueTask<T>> then it would infer the return type ValueTask<int>.

Thus, the new computation depends on the delegate's return type as well as its parameter types.

Therefore the cache must key off return type as well as parameter types.

This comment has been minimized.

@jcouv

jcouv Jul 18, 2016

Member

Thanks for the clarifying examples. LGTM

@jcouv

jcouv Jul 18, 2016

Member

Thanks for the clarifying examples. LGTM

@jcouv

This comment has been minimized.

Show comment
Hide comment
@jcouv

jcouv Jul 15, 2016

Member

LGTM

Member

jcouv commented Jul 15, 2016

LGTM

@cston

This comment has been minimized.

Show comment
Hide comment
@cston

cston Jul 19, 2016

Contributor

@jcouv, @ljw1004 thanks for reviewing. The latest commit should cover all feedback.

@dotnet/roslyn-compiler please provide an additional review.

@MattGertz for approval.

Contributor

cston commented Jul 19, 2016

@jcouv, @ljw1004 thanks for reviewing. The latest commit should cover all feedback.

@dotnet/roslyn-compiler please provide an additional review.

@MattGertz for approval.

@MattGertz

This comment has been minimized.

Show comment
Hide comment
@MattGertz

MattGertz Jul 19, 2016

Contributor

Approved pending the usual signoffs.

Contributor

MattGertz commented Jul 19, 2016

Approved pending the usual signoffs.

Support async methods returning Task-like types
[Based on features/async-return with tests ported from that branch.]

@cston cston merged commit b828f42 into dotnet:dev15-preview-4 Jul 19, 2016

6 checks passed

linux_debug_prtest Build finished.
Details
microbuild_prtest Build finished.
Details
perf_correctness_prtest Build finished.
Details
windows_debug_unit32_prtest Build finished.
Details
windows_debug_unit64_prtest Build finished.
Details
windows_eta_open_prtest Build finished.
Details

@cston cston deleted the cston:tasklike-p4 branch Jul 19, 2016

@jaredpar

This comment has been minimized.

Show comment
Hide comment
@jaredpar

jaredpar Jul 19, 2016

Member

Nice!!! 🎈 🎆

Member

jaredpar commented Jul 19, 2016

Nice!!! 🎈 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment